[PATCH v5 1/8] drm/omap: use refcount API to track the number of users of dma_addr

[Tomi Valkeinen]

Hi JJ,

On 10/10/2019 14:59, Jean-Jacques Hiblot wrote:
> This would give us a WARN_ON() if the pin/unpin calls are unbalanced.
> 
> Proposed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
> ---
>   drivers/gpu/drm/omapdrm/omap_gem.c | 44 +++++++++++++++---------------
>   1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index 08f539efddfb..61caa7a1a24b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -67,7 +67,7 @@ struct omap_gem_object {
>   	/**
>   	 * # of users of dma_addr
>   	 */
> -	u32 dma_addr_cnt;
> +	refcount_t dma_addr_cnt;
>   
>   	/**
>   	 * If the buffer has been imported from a dmabuf the OMAP_DB_DMABUF flag
> @@ -773,13 +773,15 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr)
>   	mutex_lock(&omap_obj->lock);
>   
>   	if (!omap_gem_is_contiguous(omap_obj) && priv->has_dmm) {
> -		if (omap_obj->dma_addr_cnt == 0) {
> +		if (refcount_read(&omap_obj->dma_addr_cnt) == 0) {
>   			u32 npages = obj->size >> PAGE_SHIFT;
>   			enum tiler_fmt fmt = gem2fmt(omap_obj->flags);
>   			struct tiler_block *block;
>   
>   			BUG_ON(omap_obj->block);
>   
> +			refcount_set(&omap_obj->dma_addr_cnt, 1);
> +
>   			ret = omap_gem_attach_pages(obj);
>   			if (ret)
>   				goto fail;
> @@ -813,10 +815,10 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr)
>   			omap_obj->block = block;
>   
>   			DBG("got dma address: %pad", &omap_obj->dma_addr);
> +		} else {
> +			refcount_inc(&omap_obj->dma_addr_cnt);
>   		}
>   
> -		omap_obj->dma_addr_cnt++;
> -
>   		*dma_addr = omap_obj->dma_addr;
>   	} else if (omap_gem_is_contiguous(omap_obj)) {
>   		*dma_addr = omap_obj->dma_addr;
> @@ -846,22 +848,19 @@ void omap_gem_unpin(struct drm_gem_object *obj)
>   
>   	mutex_lock(&omap_obj->lock);
>   
> -	if (omap_obj->dma_addr_cnt > 0) {
> -		omap_obj->dma_addr_cnt--;
> -		if (omap_obj->dma_addr_cnt == 0) {
> -			ret = tiler_unpin(omap_obj->block);
> -			if (ret) {
> -				dev_err(obj->dev->dev,
> -					"could not unpin pages: %d\n", ret);
> -			}
> -			ret = tiler_release(omap_obj->block);
> -			if (ret) {
> -				dev_err(obj->dev->dev,
> -					"could not release unmap: %d\n", ret);
> -			}
> -			omap_obj->dma_addr = 0;
> -			omap_obj->block = NULL;
> +	if (refcount_dec_and_test(&omap_obj->dma_addr_cnt)) {
> +		ret = tiler_unpin(omap_obj->block);
> +		if (ret) {
> +			dev_err(obj->dev->dev,
> +				"could not unpin pages: %d\n", ret);
> +		}
> +		ret = tiler_release(omap_obj->block);
> +		if (ret) {
> +			dev_err(obj->dev->dev,
> +				"could not release unmap: %d\n", ret);
>   		}
> +		omap_obj->dma_addr = 0;
> +		omap_obj->block = NULL;

This is not correct, and causes problems on devices without DMM (or, I 
think, also on devices with DMM if you disable it manually).

The old code was tracking dma_addr_cnt only for DMM buffers, and in 
omap_gem_unpin() the code checks if dma_addr_cnt > 0, and if so, it goes 
and decrements and possibly frees the DMM buffer. So for non-DMM 
buffers, nothing was done there. But this version always decrements the 
dma_addr_cnt, which then breaks down.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki