Re: [PATCH v2 3/5] drm/simpledrm: Store xfrm buffer in device instance

[Jocelyn Falempe <jfalempe@redhat.com>]

On 20/09/2023 16:24, Thomas Zimmermann wrote:
> Store and instance of struct drm_xfrm_buf in struct simpledrm_device
> and keep the allocated memory allocated across display updates. Avoid
> possibly reallocating temporary memory on each display update. Instead
> preallocate temporary memory during initialization. Releasing the DRM
> device also releases the xfrm buffer.
> 
> v2:
> 	* reserve storage during probe
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/tiny/simpledrm.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 8aceb7d378dea..a3d8a956a4c4e 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -232,6 +232,7 @@ struct simpledrm_device {
>   	struct drm_display_mode mode;
>   	const struct drm_format_info *format;
>   	unsigned int pitch;
> +	struct drm_xfrm_buf xfrm;
>   
>   	/* memory management */
>   	struct iosys_map screen_base;
> @@ -486,7 +487,6 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>   	struct drm_framebuffer *fb = plane_state->fb;
>   	struct drm_device *dev = plane->dev;
>   	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
> -	struct drm_xfrm_buf xfrm = DRM_XFRM_BUF_INIT;
>   	struct drm_atomic_helper_damage_iter iter;
>   	struct drm_rect damage;
>   	int ret, idx;
> @@ -508,13 +508,12 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>   
>   		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
>   		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data,
> -			    fb, &damage, &xfrm);
> +			    fb, &damage, &sdev->xfrm);
>   	}
>   
>   	drm_dev_exit(idx);
>   out_drm_gem_fb_end_cpu_access:
>   	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> -	drm_xfrm_buf_release(&xfrm);
>   }
>   
>   static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
> @@ -637,6 +636,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	struct drm_connector *connector;
>   	unsigned long max_width, max_height;
>   	size_t nformats;
> +	void *buf;
>   	int ret;
>   
>   	sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simpledrm_device, dev);
> @@ -718,6 +718,13 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
>   		&format->format, width, height, stride);
>   
> +	ret = drmm_xfrm_buf_init(dev, &sdev->xfrm);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	buf = drm_xfrm_buf_reserve(&sdev->xfrm, sdev->pitch, GFP_KERNEL);
> +	if (!buf)
> +		return ERR_PTR(-ENOMEM);
> +

I think it would be nice to have a "init_and_reserve()" function, to 
simplify the callers ?

>   	/*
>   	 * Memory management
>   	 */