Subject: Re: [PATCH 4/6] drm/cma-helper: Support DRM_MODE_DUMB_KERNEL_MAP flag

From: Laurent Pinchart <[email-masked]>
Date: Tue, 22 Oct 2019 14:30:01 +0300

Hi Rob,

Thank you for the patch.

On Mon, Oct 21, 2019 at 04:45:48PM -0500, Rob Herring wrote:
> Add support in CMA helpers to handle callers specifying
> DRM_MODE_DUMB_KERNEL_MAP flag. Existing behavior is maintained with this
> change. drm_gem_cma_dumb_create() always creates a kernel mapping as
> before. drm_gem_cma_dumb_create_internal() lets the caller set the flags
> as desired. Therefore, update all the existing callers of
> drm_gem_cma_dumb_create_internal() to also set the
> DRM_MODE_DUMB_KERNEL_MAP flag.
> 
> Cc: Maarten Lankhorst <[email-masked]>
> Cc: Maxime Ripard <[email-masked]>
> Cc: Sean Paul <[email-masked]>
> Cc: David Airlie <[email-masked]>
> Cc: Daniel Vetter <[email-masked]>
> Cc: "James (Qian) Wang" <[email-masked]>
> Cc: Liviu Dudau <[email-masked]>
> Cc: Brian Starkey <[email-masked]>
> Cc: Neil Armstrong <[email-masked]>
> Cc: Kevin Hilman <[email-masked]>
> Cc: Laurent Pinchart <[email-masked]>
> Cc: Kieran Bingham <[email-masked]>
> Cc: Sandy Huang <[email-masked]>
> Cc: "Heiko Stübner" <[email-masked]>
> Cc: Yannick Fertre <[email-masked]>
> Cc: Philippe Cornu <[email-masked]>
> Cc: Benjamin Gaignard <[email-masked]>
> Cc: Vincent Abriou <[email-masked]>
> Cc: Maxime Coquelin <[email-masked]>
> Cc: Alexandre Torgue <[email-masked]>
> Cc: Chen-Yu Tsai <[email-masked]>
> Cc: [email-masked]
> Cc: [email-masked]
> Cc: [email-masked]
> Cc: [email-masked]
> Cc: [email-masked]
> Signed-off-by: Rob Herring <[email-masked]>
> ---
>  .../gpu/drm/arm/display/komeda/komeda_kms.c   |  1 +
>  drivers/gpu/drm/arm/malidp_drv.c              |  1 +
>  drivers/gpu/drm/drm_gem_cma_helper.c          | 48 +++++++++++--------
>  drivers/gpu/drm/meson/meson_drv.c             |  1 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c         |  1 +
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c   |  1 +
>  drivers/gpu/drm/stm/drv.c                     |  1 +
>  drivers/gpu/drm/sun4i/sun4i_drv.c             |  1 +
>  8 files changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index d49772de93e0..7cf0dc4cbfc1 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -31,6 +31,7 @@ static int komeda_gem_cma_dumb_create(struct drm_file *file,
>  	u32 pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>  
>  	args->pitch = ALIGN(pitch, mdev->chip.bus_width);
> +	args->flags = DRM_MODE_DUMB_KERNEL_MAP;
>  
>  	return drm_gem_cma_dumb_create_internal(file, dev, args);
>  }
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index 8a76315aaa0f..aeb1a779ecc1 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -465,6 +465,7 @@ static int malidp_dumb_create(struct drm_file *file_priv,
>  	u8 alignment = malidp_hw_get_pitch_align(malidp->dev, 1);
>  
>  	args->pitch = ALIGN(DIV_ROUND_UP(args->width * args->bpp, 8), alignment);
> +	args->flags = DRM_MODE_DUMB_KERNEL_MAP;
>  
>  	return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
>  }
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 4cebfe01e6ea..f91e9e8adeaf 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -78,21 +78,8 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size)
>  	return ERR_PTR(ret);
>  }
>  
> -/**
> - * drm_gem_cma_create - allocate an object with the given size
> - * @drm: DRM device
> - * @size: size of the object to allocate
> - *
> - * This function creates a CMA GEM object and allocates a contiguous chunk of
> - * memory as backing store. The backing memory has the writecombine attribute
> - * set.
> - *
> - * Returns:
> - * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
> - * error code on failure.
> - */
> -struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> -					      size_t size)
> +static struct drm_gem_cma_object *
> +drm_gem_cma_create_flags(struct drm_device *drm, size_t size, u32 flags)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	int ret;
> @@ -103,6 +90,9 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>  	if (IS_ERR(cma_obj))
>  		return cma_obj;
>  
> +	if (!(flags & DRM_MODE_DUMB_KERNEL_MAP))
> +		cma_obj->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
> +
>  	cma_obj->vaddr = dma_alloc_attrs(drm->dev, size, &cma_obj->paddr,
>  					 GFP_KERNEL | __GFP_NOWARN,
>  					 cma_obj->dma_attrs);
> @@ -119,6 +109,25 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>  	drm_gem_object_put_unlocked(&cma_obj->base);
>  	return ERR_PTR(ret);
>  }
> +
> +/**
> + * drm_gem_cma_create - allocate an object with the given size
> + * @drm: DRM device
> + * @size: size of the object to allocate
> + *
> + * This function creates a CMA GEM object and allocates a contiguous chunk of
> + * memory as backing store. The backing memory has the writecombine attribute
> + * set.
> + *

Shouldn't you mention here that the function always creates a kernel
mapping, and that callers that don't need the mapping should use
drm_gem_cma_dumb_create_internal() instead ?
drm_gem_cma_dumb_create_internal() operates at a different level though,
and drm_gem_cma_create() is only exported for a single driver. There's
no equivalent to drm_gem_cma_create() that can skip the kernel mapping.

Apart from this,

Reviewed-by: Laurent Pinchart <[email-masked]>

> + * Returns:
> + * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
> + * error code on failure.
> + */
> +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> +					      size_t size)
> +{
> +	return drm_gem_cma_create_flags(drm, size, DRM_MODE_DUMB_KERNEL_MAP);
> +}
>  EXPORT_SYMBOL_GPL(drm_gem_cma_create);
>  
>  /**
> @@ -139,14 +148,14 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_create);
>   */
>  static struct drm_gem_cma_object *
>  drm_gem_cma_create_with_handle(struct drm_file *file_priv,
> -			       struct drm_device *drm, size_t size,
> +			       struct drm_device *drm, size_t size, u32 flags,
>  			       uint32_t *handle)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	struct drm_gem_object *gem_obj;
>  	int ret;
>  
> -	cma_obj = drm_gem_cma_create(drm, size);
> +	cma_obj = drm_gem_cma_create_flags(drm, size, flags);
>  	if (IS_ERR(cma_obj))
>  		return cma_obj;
>  
> @@ -225,7 +234,7 @@ int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
>  		args->size = args->pitch * args->height;
>  
>  	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
> -						 &args->handle);
> +						 args->flags, &args->handle);
>  	return PTR_ERR_OR_ZERO(cma_obj);
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create_internal);
> @@ -256,9 +265,10 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>  
>  	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>  	args->size = args->pitch * args->height;
> +	args->flags = DRM_MODE_DUMB_KERNEL_MAP;
>  
>  	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
> -						 &args->handle);
> +						 args->flags, &args->handle);
>  	return PTR_ERR_OR_ZERO(cma_obj);
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
> diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
> index 397c33182f4f..1593518dcbe4 100644
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -81,6 +81,7 @@ static int meson_dumb_create(struct drm_file *file, struct drm_device *dev,
>  	 */
>  	args->pitch = ALIGN(DIV_ROUND_UP(args->width * args->bpp, 8), SZ_64);
>  	args->size = PAGE_ALIGN(args->pitch * args->height);
> +	args->flags = DRM_MODE_DUMB_KERNEL_MAP;
>  
>  	return drm_gem_cma_dumb_create_internal(file, dev, args);
>  }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 2dc9caee8767..c9b1f298ce7e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -299,6 +299,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
>  		align = 16 * args->bpp / 8;
>  
>  	args->pitch = roundup(min_pitch, align);
> +	args->flags = DRM_MODE_DUMB_KERNEL_MAP;
>  
>  	return drm_gem_cma_dumb_create_internal(file, dev, args);
>  }
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index 7582d0e6a60a..f09b9a035376 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -419,6 +419,7 @@ int rockchip_gem_dumb_create(struct drm_file *file_priv,
>  	 * align to 64 bytes since Mali requires it.
>  	 */
>  	args->pitch = ALIGN(min_pitch, 64);
> +	args->flags = DRM_MODE_DUMB_KERNEL_MAP;
>  	args->size = args->pitch * args->height;

My OCD gets triggered by flags appearing in the middle here while it is
at the end in other drivers :-)

>  
>  	rk_obj = rockchip_gem_create_with_handle(file_priv, dev, args->size,
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index 5a9f9aca8bc2..0f76a4ac95b3 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -47,6 +47,7 @@ static int stm_gem_cma_dumb_create(struct drm_file *file,
>  	 */
>  	args->pitch = roundup(min_pitch, 128);
>  	args->height = roundup(args->height, 4);
> +	args->flags = DRM_MODE_DUMB_KERNEL_MAP;
>  
>  	return drm_gem_cma_dumb_create_internal(file, dev, args);
>  }
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index a5757b11b730..f653a5d1e2d6 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -34,6 +34,7 @@ static int drm_sun4i_gem_dumb_create(struct drm_file *file_priv,
>  {
>  	/* The hardware only allows even pitches for YUV buffers. */
>  	args->pitch = ALIGN(DIV_ROUND_UP(args->width * args->bpp, 8), 2);
> +	args->flags = DRM_MODE_DUMB_KERNEL_MAP;
>  
>  	return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
>  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel