Re: [PATCH 1/5] drm/omap: return error if dma_alloc_writecombine fails

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

Hi Tomi,

On Tuesday 23 June 2015 11:18:16 Tomi Valkeinen wrote:
> On 18/06/15 17:27, Laurent Pinchart wrote:
> > On Thursday 18 June 2015 13:10:35 Tomi Valkeinen wrote:
> >> On a platform with no TILER (e.g. omap3, am43xx), when the user wants to
> >> allocate buffer with flag OMAP_BO_SCANOUT, the buffer needs to be
> >> allocated with dma_alloc_writecombine. For some reason the driver does
> >> not return an error if that alloc fails, instead it continues without
> >> backing memory. This leads to errors later when the user tries to use
> >> the buffer.
> >> 
> >> This patch makes the driver return an error if dma_alloc_writecombine
> >> fails.
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_gem.c | 12 ++++++++++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 2ab77801cf5f..51c5635aff62
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> >> @@ -1392,9 +1392,17 @@ struct drm_gem_object *omap_gem_new(struct
> >> drm_device *dev, */
> >>  		omap_obj->vaddr =  dma_alloc_writecombine(dev->dev, size,
> >>  				&omap_obj->paddr, GFP_KERNEL);
> >> 
> >> -		if (omap_obj->vaddr)
> >> -			flags |= OMAP_BO_DMA;
> >> +		if (!omap_obj->vaddr) {
> >> +			spin_lock(&priv->list_lock);
> >> +			list_del(&omap_obj->mm_list);
> >> +			spin_unlock(&priv->list_lock);
> > 
> > Wouldn't it be simpler to move the list_add after the "if ((flags &
> > OMAP_BO_SCANOUT) && !priv->has_dmm)" block ? You could then avoid the
> > list_del.
> 
> Thanks, it's cleaner that way:
> 
> commit 70a7c80ae9f787031fa3eebc70df024deadde09f (HEAD)
> Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date:   Tue Mar 17 15:31:11 2015 +0200
> 
>     drm/omap: return error if dma_alloc_writecombine fails
> 
>     On a platform with no TILER (e.g. omap3, am43xx), when the user wants to
>     allocate buffer with flag OMAP_BO_SCANOUT, the buffer needs to be
>     allocated with dma_alloc_writecombine. For some reason the driver does
>     not return an error if that alloc fails, instead it continues without
>     backing memory. This leads to errors later when the user tries to use
>     the buffer.
> 
>     This patch makes the driver return an error if dma_alloc_writecombine
>     fails.
> 
>     Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index 2ab77801cf5f..874eb6dcf94b
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -1378,11 +1378,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device
> *dev,
> 
>  	omap_obj = kzalloc(sizeof(*omap_obj), GFP_KERNEL);
>  	if (!omap_obj)
> -		goto fail;
> -
> -	spin_lock(&priv->list_lock);
> -	list_add(&omap_obj->mm_list, &priv->obj_list);
> -	spin_unlock(&priv->list_lock);
> +		return NULL;
> 
>  	obj = &omap_obj->base;
> 
> @@ -1392,11 +1388,19 @@ struct drm_gem_object *omap_gem_new(struct
> drm_device *dev, */
>  		omap_obj->vaddr =  dma_alloc_writecombine(dev->dev, size,
>  				&omap_obj->paddr, GFP_KERNEL);
> -		if (omap_obj->vaddr)
> -			flags |= OMAP_BO_DMA;
> +		if (!omap_obj->vaddr) {
> +			kfree(omap_obj);
> 
> +			return NULL;
> +		}
> +
> +		flags |= OMAP_BO_DMA;
>  	}
> 
> +	spin_lock(&priv->list_lock);
> +	list_add(&omap_obj->mm_list, &priv->obj_list);
> +	spin_unlock(&priv->list_lock);
> +
>  	omap_obj->flags = flags;
> 
>  	if (flags & OMAP_BO_TILED) {

-- 
Regards,

Laurent Pinchart

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