Re: [PATCH v2] drm/ssd130x: Store the HW buffer in the driver-private CRTC state

[Maxime Ripard]

Hi,

On Sun, Sep 10, 2023 at 11:40:28AM +0200, Javier Martinez Canillas wrote:
> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> .atomic_check() callback") moved the allocation of the intermediate and
> HW buffers from the encoder's .atomic_enable callback, to the plane's
> .atomic_check callback.
> 
> This was suggested by Maxime Ripard, because drivers aren't allowed to
> fail after the drm_atomic_helper_swap_state() function has been called.
> 
> And the encoder's .atomic_enable happens after the new atomic state has
> been swapped, so allocations (that can fail) shouldn't be done there.
> 
> But the HW buffer isn't really tied to the plane's state. It has a fixed
> size that only depends on the (also fixed) display resolution defined in
> the Device Tree Blob.
> 
> That buffer can be considered part of the CRTC state, and for this reason
> makes more sense to do its allocation in the CRTC .atomic_check callback.
> 
> The other allocated buffer (used to store a conversion from the emulated
> XR24 format to the native R1 format) is part of the plane's state, since
> it will be optional once the driver supports R1 and allows user-space to
> set that pixel format.
> 
> So let's keep the allocation for it in the plane's .atomic_check callback,
> this can't be moved to the CRTC's .atomic_check because changing a format
> does not trigger a CRTC mode set.
> 
> Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/dri-devel/CAMuHMdWv_QSatDgihr8=2SXHhvp=icNxumZcZOPwT9Q_QiogNQ@xxxxxxxxxxxxxx/
> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> ---
> 
> Changes in v2:
> - Drop RFC prefix.
> - Fix typo in commit message (Thomas Zimmermann).
> - Store the HW buffer in the driver's private CRTC state (Thomas Zimmermann).
> - Just use kmalloc() kcalloc() when allocating buffers (Thomas Zimmermann).
> - Keep the allocation of the intermediate buffer in the plane's .atomic_check
> 
>  drivers/gpu/drm/solomon/ssd130x.c | 143 ++++++++++++++++++++++--------
>  1 file changed, 108 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 3b4dde09538a..c866dd40d163 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -141,14 +141,23 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
>  };
>  EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
>  
> +struct ssd130x_crtc_state {
> +	struct drm_crtc_state base;
> +	/* Buffer to store pixels in HW format and written to the panel */
> +	u8 *data_array;
> +};
> +
>  struct ssd130x_plane_state {
>  	struct drm_shadow_plane_state base;
>  	/* Intermediate buffer to convert pixels from XRGB8888 to HW format */
>  	u8 *buffer;
> -	/* Buffer to store pixels in HW format and written to the panel */
> -	u8 *data_array;
>  };
>  
> +static inline struct ssd130x_crtc_state *to_ssd130x_crtc_state(struct drm_crtc_state *state)
> +{
> +	return container_of(state, struct ssd130x_crtc_state, base);
> +}
> +
>  static inline struct ssd130x_plane_state *to_ssd130x_plane_state(struct drm_plane_state *state)
>  {
>  	return container_of(state, struct ssd130x_plane_state, base.base);
> @@ -448,13 +457,11 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
>  }
>  
>  static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
> -			       struct ssd130x_plane_state *ssd130x_state,
> -			       struct drm_rect *rect)
> +			       struct drm_rect *rect, u8 *buf,
> +			       u8 *data_array)
>  {
>  	unsigned int x = rect->x1;
>  	unsigned int y = rect->y1;
> -	u8 *buf = ssd130x_state->buffer;
> -	u8 *data_array = ssd130x_state->data_array;

That's just a matter of taste I guess, but I would pass the crtc_state
pointer there instead of an opaque byte array (without any boundary).

>  	unsigned int width = drm_rect_width(rect);
>  	unsigned int height = drm_rect_height(rect);
>  	unsigned int line_length = DIV_ROUND_UP(width, 8);
> @@ -550,12 +557,10 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
>  	return ret;
>  }
>  
> -static void ssd130x_clear_screen(struct ssd130x_device *ssd130x,
> -				 struct ssd130x_plane_state *ssd130x_state)
> +static void ssd130x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
>  {
>  	unsigned int page_height = ssd130x->device_info->page_height;
>  	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> -	u8 *data_array = ssd130x_state->data_array;
>  	unsigned int width = ssd130x->width;
>  	int ret, i;
>  
> @@ -594,15 +599,13 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x,
>  	}
>  }
>  
> -static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
> +static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb,
>  				const struct iosys_map *vmap,
> -				struct drm_rect *rect)
> +				struct drm_rect *rect,
> +				u8 *buf, u8 *data_array)
>  {
> -	struct drm_framebuffer *fb = state->fb;
>  	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>  	unsigned int page_height = ssd130x->device_info->page_height;
> -	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
> -	u8 *buf = ssd130x_state->buffer;
>  	struct iosys_map dst;
>  	unsigned int dst_pitch;
>  	int ret = 0;
> @@ -622,7 +625,7 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
>  
>  	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>  
> -	ssd130x_update_rect(ssd130x, ssd130x_state, rect);
> +	ssd130x_update_rect(ssd130x, rect, buf, data_array);
>  
>  	return ret;
>  }
> @@ -634,8 +637,6 @@ static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
>  	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
>  	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>  	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> -	unsigned int page_height = ssd130x->device_info->page_height;
> -	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
>  	const struct drm_format_info *fi;
>  	unsigned int pitch;
>  	int ret;
> @@ -654,14 +655,6 @@ static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
>  	if (!ssd130x_state->buffer)
>  		return -ENOMEM;
>  
> -	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> -	if (!ssd130x_state->data_array) {
> -		kfree(ssd130x_state->buffer);
> -		/* Set to prevent a double free in .atomic_destroy_state() */
> -		ssd130x_state->buffer = NULL;
> -		return -ENOMEM;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -671,6 +664,10 @@ static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>  	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>  	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>  	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);

You can have CRTC-less commits if you only modify a property of the
plane for example. drm_atomic_get_new_crtc_state will retrieve the CRTC
state in the global state passed as an argument, but will not make any
effort to retrieve the current CRTC state if it's not part of that commit.

You should add a call to drm_atomic_get_crtc_state in your atomic_check
hook to pull the CRTC state if it's not there so you can rely on it
here.

Maxime

Attachment: signature.asc
Description: PGP signature