Re: [PATCH v2 4/6] drm/omap: gem: Replace struct_mutex usage with omap_obj private lock

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: dri-devel@lists.freedesktop.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>
Subject: Re: [PATCH v2 4/6] drm/omap: gem: Replace struct_mutex usage with omap_obj private lock
Date: Sat, 26 May 2018 20:10:32 +0300	[thread overview]
Message-ID: <1816854.LCQKW1FNGA@avalon> (raw)
In-Reply-To: <20180525163925.10736-5-laurent.pinchart@ideasonboard.com>

Hello,

On Friday, 25 May 2018 19:39:23 EEST Laurent Pinchart wrote:
> The DRM device struct_mutex is used to protect against concurrent GEM
> object operations that deal with memory allocation and pinning. All
> those operations are local to a GEM object and don't need to be
> serialized across different GEM objects. Replace the struct_mutex with
> a local omap_obj.lock or drop it altogether where not needed.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> Changes since v1:
> 
> - Use lockdep_assert_held() to verify locking correctness
> ---
>  drivers/gpu/drm/omapdrm/omap_debugfs.c |   7 --
>  drivers/gpu/drm/omapdrm/omap_fbdev.c   |   8 +--
>  drivers/gpu/drm/omapdrm/omap_gem.c     | 122 ++++++++++++++++++++----------
>  3 files changed, 81 insertions(+), 56 deletions(-)

[snip]

> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index 0a9ba1f711e6..39cbc7273b29
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c

[snip]

> -/** release backing pages */
> +/* Release backing pages. Must be called with the omap_obj.lock held. */
>  static void omap_gem_detach_pages(struct drm_gem_object *obj)
>  {
>  	struct omap_gem_object *omap_obj = to_omap_bo(obj);
>  	unsigned int npages = obj->size >> PAGE_SHIFT;
>  	unsigned int i;
> 
> +	lockdep_assert_held(&omap_obj->lock);
> +
>  	for (i = 0; i < npages; i++) {
>  		if (omap_obj->dma_addrs[i])
>  			dma_unmap_page(obj->dev->dev, omap_obj->dma_addrs[i],

[snip]

> @@ -1056,15 +1091,16 @@ void omap_gem_free_object(struct drm_gem_object
> *obj)
> 
>  	omap_gem_evict(obj);
> 
> -	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
>  	spin_lock(&priv->list_lock);
>  	list_del(&omap_obj->mm_list);
>  	spin_unlock(&priv->list_lock);
> 
> -	/* this means the object is still pinned.. which really should
> -	 * not happen.  I think..
> +	/*
> +	 * No need to take the omap_obj.lock as at this point we own the sole
> +	 * reference to the object.
>  	 */

And this of course causes a lockdep warning, as the function calls 
omap_gem_detach_pages(), and the lockdep_assert_held() call there doesn't know 
we're the sole owner. I've sent a v2.1 to fix this.

> +
> +	/* The object should not be pinned. */
>  	WARN_ON(omap_obj->dma_addr_cnt > 0);
> 
>  	if (omap_obj->pages) {

[snip]

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel