* [PATCH 1/2] radeon: fix pll/ctrc mapping on dce2 and dce3 hardware v2
@ 2012-11-27 18:07 j.glisse
2012-11-27 18:07 ` [PATCH 2/2] drm/radeon: fix deadlock when bo is associated to different handle j.glisse
0 siblings, 1 reply; 4+ messages in thread
From: j.glisse @ 2012-11-27 18:07 UTC (permalink / raw)
To: dri-devel; +Cc: Jerome Glisse
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;
}
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] drm/radeon: fix deadlock when bo is associated to different handle
2012-11-27 18:07 [PATCH 1/2] radeon: fix pll/ctrc mapping on dce2 and dce3 hardware v2 j.glisse
@ 2012-11-27 18:07 ` j.glisse
2012-11-28 10:27 ` Christian König
0 siblings, 1 reply; 4+ messages in thread
From: j.glisse @ 2012-11-27 18:07 UTC (permalink / raw)
To: dri-devel; +Cc: Jerome Glisse
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);
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] drm/radeon: fix deadlock when bo is associated to different handle
2012-11-27 18:07 ` [PATCH 2/2] drm/radeon: fix deadlock when bo is associated to different handle j.glisse
@ 2012-11-28 10:27 ` Christian König
2012-11-28 15:38 ` Jerome Glisse
0 siblings, 1 reply; 4+ messages in thread
From: Christian König @ 2012-11-28 10:27 UTC (permalink / raw)
To: j.glisse; +Cc: Jerome Glisse, dri-devel
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.
> + */
> 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;
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).
> + }
> }
> return radeon_bo_list_validate(&p->validated);
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] drm/radeon: fix deadlock when bo is associated to different handle
2012-11-28 10:27 ` Christian König
@ 2012-11-28 15:38 ` Jerome Glisse
0 siblings, 0 replies; 4+ messages in thread
From: Jerome Glisse @ 2012-11-28 15:38 UTC (permalink / raw)
To: Christian König; +Cc: Jerome Glisse, dri-devel
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.
> >+ */
> > 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;
>
> 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
>
> >+ }
> > }
> > return radeon_bo_list_validate(&p->validated);
> > }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-11-28 15:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-27 18:07 [PATCH 1/2] radeon: fix pll/ctrc mapping on dce2 and dce3 hardware v2 j.glisse
2012-11-27 18:07 ` [PATCH 2/2] drm/radeon: fix deadlock when bo is associated to different handle j.glisse
2012-11-28 10:27 ` Christian König
2012-11-28 15:38 ` Jerome Glisse
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).