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

[Javier Martinez Canillas <javierm@redhat.com>]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Store and instance of struct drm_xfrm_buf in struct ssd130x_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>
> ---

[...]

> @@ -1084,6 +1081,8 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
>  	struct ssd130x_device *ssd130x;
>  	struct backlight_device *bl;
>  	struct drm_device *drm;
> +	const struct drm_format_info *fi;
> +	void *buf;
>  	int ret;
>  
>  	ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
> @@ -1117,6 +1116,18 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
>  	bl->props.max_brightness = MAX_CONTRAST;
>  	ssd130x->bl_dev = bl;
>  
> +	ret = drmm_xfrm_buf_init(drm, &ssd130x->xfrm);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	fi = drm_format_info(DRM_FORMAT_R1);
> +	if (!fi)
> +		return ERR_PTR(-EINVAL);
> +	buf = drm_xfrm_buf_reserve(&ssd130x->xfrm,
> +				   drm_format_info_min_pitch(fi, 0, ssd130x->width),
> +				   GFP_KERNEL);
> +	if (!buf)
> +		return ERR_PTR(-ENOMEM);
> +
  
I think this is OK but then I wonder if we should not just allocate all
the buffers in the probe function. Right now, what the driver does is to
have two structures to keep the driver-private atomic state:

1) struct ssd130x_crtc_state that has a .data_array to store the pixels
   in the HW format (e.g: R1) and written to the panel.

2) struct ssd130x_plane_state that has a .buffer to store the pixels that
   are converted from the emulated XRGB8888 used by the shadow-plane, to
   the HW pixel format.

The (2) will be optional once Geert's R1 support lands. Now we are adding
a third buffer so I wonder if should be part of one of these private state
or not.

I said that should be a field of struct ssd130x_plane_state in a previous
email, but on a second thought it makes more sense if is a field of the
struct ssd130x_crtc_state.

That way the allocation will only be in ssd130x_crtc_atomic_check() and
the release in the ssd130x_crtc_destroy_state(). If you do that on patch
#2, then this patch #5 could be dropped.

The reason why I added those private state structures is twofold: because
the buffers are tied to the CRTC and planes and to show how a driver can
maintain their own private atomic state.

After all, one of my goals of this driver is to be used for educational
purposes and provide a simple driver that people can use as a reference.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat