Re: [PATCH 2/2] drm/radeon: fix deadlock when bo is associated to different handle

["Christian König" <deathsimple@vodafone.de>]

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);
>   }