On 27.11.2012 19:07, j.glisse at gmail.com wrote:
> From: Jerome Glisse <jglisse at 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 at 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);
>   }

Reply via email to