From: Jerome Glisse jglisse@redhat.com
This fix black screen on resume issue that some people are experiencing. There is a bug in the atombios code regarding pll/crtc mapping. The atombios code reverse the logic for the pll and crtc mapping.
v2: DCE3 or DCE2 only have 2 crtc
Signed-off-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/atombios_crtc.c | 48 ++++++++++------------------------ 1 file changed, 14 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c index 3bce029..24d932f 100644 --- a/drivers/gpu/drm/radeon/atombios_crtc.c +++ b/drivers/gpu/drm/radeon/atombios_crtc.c @@ -1696,42 +1696,22 @@ static int radeon_atom_pick_pll(struct drm_crtc *crtc) return ATOM_PPLL2; DRM_ERROR("unable to allocate a PPLL\n"); return ATOM_PPLL_INVALID; - } else if (ASIC_IS_AVIVO(rdev)) { - /* in DP mode, the DP ref clock can come from either PPLL - * depending on the asic: - * DCE3: PPLL1 or PPLL2 - */ - if (ENCODER_MODE_IS_DP(atombios_get_encoder_mode(radeon_crtc->encoder))) { - /* use the same PPLL for all DP monitors */ - pll = radeon_get_shared_dp_ppll(crtc); - if (pll != ATOM_PPLL_INVALID) - return pll; - } else { - /* use the same PPLL for all monitors with the same clock */ - pll = radeon_get_shared_nondp_ppll(crtc); - if (pll != ATOM_PPLL_INVALID) - return pll; - } - /* all other cases */ - pll_in_use = radeon_get_pll_use_mask(crtc); - /* the order shouldn't matter here, but we probably - * need this until we have atomic modeset - */ - if (rdev->flags & RADEON_IS_IGP) { - if (!(pll_in_use & (1 << ATOM_PPLL1))) - return ATOM_PPLL1; - if (!(pll_in_use & (1 << ATOM_PPLL2))) - return ATOM_PPLL2; - } else { - if (!(pll_in_use & (1 << ATOM_PPLL2))) - return ATOM_PPLL2; - if (!(pll_in_use & (1 << ATOM_PPLL1))) - return ATOM_PPLL1; - } - DRM_ERROR("unable to allocate a PPLL\n"); - return ATOM_PPLL_INVALID; } else { /* on pre-R5xx asics, the crtc to pll mapping is hardcoded */ + /* some atombios (observed in some DCE2/DCE3) code have a bug, + * the matching btw pll and crtc is done through + * PCLK_CRTC[1|2]_CNTL (0x480/0x484) but atombios code use the + * pll (1 or 2) to select which register to write. ie if using + * pll1 it will use PCLK_CRTC1_CNTL (0x480) and if using pll2 + * it will use PCLK_CRTC2_CNTL (0x484), it then use crtc id to + * choose which value to write. Which is reverse order from + * register logic. So only case that works is when pllid is + * same as crtcid or when both pll and crtc are enabled and + * both use same clock. + * + * So just return crtc id as if crtc and pll were hard linked + * together even if they aren't + */ return radeon_crtc->crtc_id; } }
From: Jerome Glisse jglisse@redhat.com
There is a rare case, that seems to only happen accross suspend/resume cycle, where a bo is associated with several different handle. This lead to a deadlock in ttm buffer reservation path. This could only happen with flinked(globaly exported) object. Userspace should not reopen multiple time a globaly exported object.
However the kernel should handle gracefully this corner case and not keep rejecting the userspace command stream. This is the object of this patch.
Fix suspend/resume issue where user see following message : [drm:radeon_cs_ioctl] *ERROR* Failed to parse relocation -35!
Signed-off-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon_cs.c | 53 ++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 41672cc..064e64d 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -54,39 +54,48 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) return -ENOMEM; } for (i = 0; i < p->nrelocs; i++) { - struct drm_radeon_cs_reloc *r; - + struct drm_radeon_cs_reloc *reloc; + + /* One bo could be associated with several different handle. + * Only happen for flinked bo that are open several time. + * + * FIXME: + * Maybe we should consider an alternative to idr for gem + * object to insure a 1:1 uniq mapping btw handle and gem + * object. + */ duplicate = false; - r = (struct drm_radeon_cs_reloc *)&chunk->kdata[i*4]; + reloc = (struct drm_radeon_cs_reloc *)&chunk->kdata[i*4]; + p->relocs[i].handle = 0; + p->relocs[i].flags = reloc->flags; + p->relocs[i].gobj = drm_gem_object_lookup(ddev, + p->filp, + reloc->handle); + if (p->relocs[i].gobj == NULL) { + DRM_ERROR("gem object lookup failed 0x%x\n", + reloc->handle); + return -ENOENT; + } + p->relocs[i].robj = gem_to_radeon_bo(p->relocs[i].gobj); + p->relocs[i].lobj.bo = p->relocs[i].robj; + p->relocs[i].lobj.wdomain = reloc->write_domain; + p->relocs[i].lobj.rdomain = reloc->read_domains; + p->relocs[i].lobj.tv.bo = &p->relocs[i].robj->tbo; + for (j = 0; j < i; j++) { - if (r->handle == p->relocs[j].handle) { + if (p->relocs[i].lobj.bo == p->relocs[j].lobj.bo) { p->relocs_ptr[i] = &p->relocs[j]; duplicate = true; break; } } + if (!duplicate) { - p->relocs[i].gobj = drm_gem_object_lookup(ddev, - p->filp, - r->handle); - if (p->relocs[i].gobj == NULL) { - DRM_ERROR("gem object lookup failed 0x%x\n", - r->handle); - return -ENOENT; - } p->relocs_ptr[i] = &p->relocs[i]; - p->relocs[i].robj = gem_to_radeon_bo(p->relocs[i].gobj); - p->relocs[i].lobj.bo = p->relocs[i].robj; - p->relocs[i].lobj.wdomain = r->write_domain; - p->relocs[i].lobj.rdomain = r->read_domains; - p->relocs[i].lobj.tv.bo = &p->relocs[i].robj->tbo; - p->relocs[i].handle = r->handle; - p->relocs[i].flags = r->flags; + p->relocs[i].handle = reloc->handle; radeon_bo_list_add_object(&p->relocs[i].lobj, &p->validated); - - } else - p->relocs[i].handle = 0; + } } return radeon_bo_list_validate(&p->validated); }
On 27.11.2012 19:07, j.glisse@gmail.com wrote:
From: Jerome Glisse jglisse@redhat.com
There is a rare case, that seems to only happen accross suspend/resume cycle, where a bo is associated with several different handle. This lead to a deadlock in ttm buffer reservation path. This could only happen with flinked(globaly exported) object. Userspace should not reopen multiple time a globaly exported object.
However the kernel should handle gracefully this corner case and not keep rejecting the userspace command stream. This is the object of this patch.
Fix suspend/resume issue where user see following message : [drm:radeon_cs_ioctl] *ERROR* Failed to parse relocation -35!
Signed-off-by: Jerome Glisse jglisse@redhat.com
See comment below.
drivers/gpu/drm/radeon/radeon_cs.c | 53 ++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 41672cc..064e64d 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -54,39 +54,48 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) return -ENOMEM; } for (i = 0; i < p->nrelocs; i++) {
struct drm_radeon_cs_reloc *r;
struct drm_radeon_cs_reloc *reloc;/* One bo could be associated with several different handle.* Only happen for flinked bo that are open several time.** FIXME:* Maybe we should consider an alternative to idr for gem* object to insure a 1:1 uniq mapping btw handle and gem* object.*/
r = (struct drm_radeon_cs_reloc *)&chunk->kdata[i*4];
reloc = (struct drm_radeon_cs_reloc *)&chunk->kdata[i*4];p->relocs[i].handle = 0;p->relocs[i].flags = reloc->flags;p->relocs[i].gobj = drm_gem_object_lookup(ddev,p->filp,reloc->handle);if (p->relocs[i].gobj == NULL) {DRM_ERROR("gem object lookup failed 0x%x\n",reloc->handle);return -ENOENT;}p->relocs[i].robj = gem_to_radeon_bo(p->relocs[i].gobj);p->relocs[i].lobj.bo = p->relocs[i].robj;p->relocs[i].lobj.wdomain = reloc->write_domain;p->relocs[i].lobj.rdomain = reloc->read_domains;p->relocs[i].lobj.tv.bo = &p->relocs[i].robj->tbo;- for (j = 0; j < i; j++) {
if (r->handle == p->relocs[j].handle) {
if (p->relocs[i].lobj.bo == p->relocs[j].lobj.bo) { p->relocs_ptr[i] = &p->relocs[j]; duplicate = true; break; }- if (!duplicate) {
p->relocs[i].gobj = drm_gem_object_lookup(ddev,p->filp,r->handle);if (p->relocs[i].gobj == NULL) {DRM_ERROR("gem object lookup failed 0x%x\n",r->handle);return -ENOENT;} p->relocs_ptr[i] = &p->relocs[i];p->relocs[i].robj = gem_to_radeon_bo(p->relocs[i].gobj);p->relocs[i].lobj.bo = p->relocs[i].robj;p->relocs[i].lobj.wdomain = r->write_domain;p->relocs[i].lobj.rdomain = r->read_domains;p->relocs[i].lobj.tv.bo = &p->relocs[i].robj->tbo;p->relocs[i].handle = r->handle;p->relocs[i].flags = r->flags;
p->relocs[i].handle = reloc->handle; radeon_bo_list_add_object(&p->relocs[i].lobj, &p->validated);
} elsep->relocs[i].handle = 0;
I'm not sure if the memory p->relocs is pointing to is zero initialized, so we should at least initialize whatever member we use to find the duplicates. Also I think we don't need the handle in this structure any more if we don't use it for comparison (but not 100% sure without testing it).
}
On Wed, Nov 28, 2012 at 11:27:27AM +0100, Christian König wrote:
On 27.11.2012 19:07, j.glisse@gmail.com wrote:
From: Jerome Glisse jglisse@redhat.com
There is a rare case, that seems to only happen accross suspend/resume cycle, where a bo is associated with several different handle. This lead to a deadlock in ttm buffer reservation path. This could only happen with flinked(globaly exported) object. Userspace should not reopen multiple time a globaly exported object.
However the kernel should handle gracefully this corner case and not keep rejecting the userspace command stream. This is the object of this patch.
Fix suspend/resume issue where user see following message : [drm:radeon_cs_ioctl] *ERROR* Failed to parse relocation -35!
Signed-off-by: Jerome Glisse jglisse@redhat.com
See comment below.
drivers/gpu/drm/radeon/radeon_cs.c | 53 ++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 41672cc..064e64d 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -54,39 +54,48 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) return -ENOMEM; } for (i = 0; i < p->nrelocs; i++) {
struct drm_radeon_cs_reloc *r;
struct drm_radeon_cs_reloc *reloc;/* One bo could be associated with several different handle.* Only happen for flinked bo that are open several time.** FIXME:* Maybe we should consider an alternative to idr for gem* object to insure a 1:1 uniq mapping btw handle and gem* object.*/
r = (struct drm_radeon_cs_reloc *)&chunk->kdata[i*4];
reloc = (struct drm_radeon_cs_reloc *)&chunk->kdata[i*4];p->relocs[i].handle = 0;p->relocs[i].flags = reloc->flags;p->relocs[i].gobj = drm_gem_object_lookup(ddev,p->filp,reloc->handle);if (p->relocs[i].gobj == NULL) {DRM_ERROR("gem object lookup failed 0x%x\n",reloc->handle);return -ENOENT;}p->relocs[i].robj = gem_to_radeon_bo(p->relocs[i].gobj);p->relocs[i].lobj.bo = p->relocs[i].robj;p->relocs[i].lobj.wdomain = reloc->write_domain;p->relocs[i].lobj.rdomain = reloc->read_domains;p->relocs[i].lobj.tv.bo = &p->relocs[i].robj->tbo;- for (j = 0; j < i; j++) {
if (r->handle == p->relocs[j].handle) {
if (p->relocs[i].lobj.bo == p->relocs[j].lobj.bo) { p->relocs_ptr[i] = &p->relocs[j]; duplicate = true; break; }- if (!duplicate) {
p->relocs[i].gobj = drm_gem_object_lookup(ddev,p->filp,r->handle);if (p->relocs[i].gobj == NULL) {DRM_ERROR("gem object lookup failed 0x%x\n",r->handle);return -ENOENT;} p->relocs_ptr[i] = &p->relocs[i];p->relocs[i].robj = gem_to_radeon_bo(p->relocs[i].gobj);p->relocs[i].lobj.bo = p->relocs[i].robj;p->relocs[i].lobj.wdomain = r->write_domain;p->relocs[i].lobj.rdomain = r->read_domains;p->relocs[i].lobj.tv.bo = &p->relocs[i].robj->tbo;p->relocs[i].handle = r->handle;p->relocs[i].flags = r->flags;
p->relocs[i].handle = reloc->handle; radeon_bo_list_add_object(&p->relocs[i].lobj, &p->validated);
} elsep->relocs[i].handle = 0;I'm not sure if the memory p->relocs is pointing to is zero initialized, so we should at least initialize whatever member we use to find the duplicates. Also I think we don't need the handle in this structure any more if we don't use it for comparison (but not 100% sure without testing it).
No need to initialize p->relocs[i].lobj.bo which is the one use to find duplicate. When a duplicate is found p->relocs_ptr[i] points to first relocation with the duplicate bo. p->relocs[i].lobj.bo is always initialized before looking for duplicate.
I kept the handle around because its usefull for debuging. But it could as well be removed and just added back whenever someone is doing debugging.
Cheers, Jerome
}}
dri-devel@lists.freedesktop.org
-
Christian König -
j.glisse＠gmail.com -
Jerome Glisse