Re: [PATCH 1/2] drm/omap: add omap_gem_put_paddr_locked()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Monday 08 May 2017 11:51:21 Tomi Valkeinen wrote:
> Add omap_gem_put_paddr_locked() which is a version of
> omap_gem_put_paddr() that expects the caller to hold the struct_mutex.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_gem.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index 68a75b829b71..5d73dccc1383
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -873,12 +873,12 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
>  /* Release physical address, when DMA is no longer being performed.. this
>   * could potentially unpin and unmap buffers from TILER
>   */
> -void omap_gem_put_paddr(struct drm_gem_object *obj)
> +
> +static void omap_gem_put_paddr_locked(struct drm_gem_object *obj)
>  {
>  	struct omap_gem_object *omap_obj = to_omap_bo(obj);
>  	int ret;
> 
> -	mutex_lock(&obj->dev->struct_mutex);
>  	if (omap_obj->paddr_cnt > 0) {
>  		omap_obj->paddr_cnt--;
>  		if (omap_obj->paddr_cnt == 0) {

You could now decrease the indentation level in this function by adding a few 
returns. This doesn't call for a v2, but as you'll have to rebase anyway due 
to the function name change, you can throw that change in at the same time :-)

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On a side note, it might be worth it changing the count field to a refcount_t, 
this would give us a WARN_ON() if the pin/unpin calls are unbalanced.

> @@ -896,7 +896,12 @@ void omap_gem_put_paddr(struct drm_gem_object *obj)
>  			omap_obj->block = NULL;
>  		}
>  	}
> +}
> 
> +void omap_gem_put_paddr(struct drm_gem_object *obj)
> +{
> +	mutex_lock(&obj->dev->struct_mutex);
> +	omap_gem_put_paddr_locked(obj);
>  	mutex_unlock(&obj->dev->struct_mutex);
>  }

-- 
Regards,

Laurent Pinchart

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