Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 9000 bytes --]

Hi Javier,

another idea about this patch: why not just keep the allocation in the 
plane's atomic check, but store the temporary buffers in a plane struct. 
You'd only grow the arrays length in atomic_check and later fetch the 
pointers in atomic_update. It needs some locking, but nothing complicated.

Best regards
Thomas

Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
> 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 primary plane's
> .atomic_check callback.
> 
> This was suggested by Maxime Ripard because drivers aren't allowed to fail
> after drm_atomic_helper_swap_state() has been called, and the encoder's
> .atomic_enable happens after the new atomic state has been swapped.
> 
> But that change caused a performance regression in very slow platforms,
> since now the allocation happens for every plane's atomic state commit.
> For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> softcore (RISC-V CPU implementation on an FPGA).
> 
> To prevent that, move the move the buffers' allocation and free to the
> CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
> happens on a modeset. Since the intermediate buffer is only needed when
> not using the controller native format (R1), doing the buffer allocation
> at that CRTC's .atomic_check time would be enough.
> 
> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> Hello,
> 
> This is a RFC because I'm not sure if there is a net benefit after this
> change. I find the currect code much cleaner and less error prone, even
> when Geert reports that performs worse on his (very slow) platform.
> 
> But I'm still posting it to see what others think. I've tested the patch
> on an I2C ssd1306 OLED and found no regressions.
> 
> The patch is on top on Geert's latest patch-set that adds support for the
> DRM_FORMAT_R1 to the ssd130x driver:
> 
> https://lore.kernel.org/dri-devel/cover.1692888745.git.geert@linux-m68k.org
> 
> Best regards,
> Javier
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 106 ++++++++++++++++--------------
>   1 file changed, 56 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 0d2b36ba4011..60536cd0c42d 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -650,46 +650,6 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
>   	return ret;
>   }
>   
> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						     struct drm_atomic_state *state)
> -{
> -	struct drm_device *drm = plane->dev;
> -	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;
> -
> -	ret = drm_plane_helper_atomic_check(plane, state);
> -	if (ret)
> -		return ret;
> -
> -	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> -	if (!ssd130x_state->data_array)
> -		return -ENOMEM;
> -
> -	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> -		fi = drm_format_info(DRM_FORMAT_R1);
> -		if (!fi)
> -			return -EINVAL;
> -
> -		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> -
> -		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> -		if (!ssd130x_state->buffer) {
> -			kfree(ssd130x_state->data_array);
> -			/* Set to prevent a double free in .atomic_destroy_state() */
> -			ssd130x_state->data_array = NULL;
> -			return -ENOMEM;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   						       struct drm_atomic_state *state)
>   {
> @@ -762,10 +722,6 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
>   	if (!ssd130x_state)
>   		return NULL;
>   
> -	/* The buffers are not duplicated and are allocated in .atomic_check */
> -	ssd130x_state->buffer = NULL;
> -	ssd130x_state->data_array = NULL;
> -
>   	new_shadow_plane_state = &ssd130x_state->base;
>   
>   	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
> @@ -778,9 +734,6 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   {
>   	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
>   
> -	kfree(ssd130x_state->data_array);
> -	kfree(ssd130x_state->buffer);
> -
>   	__drm_gem_destroy_shadow_plane_state(&ssd130x_state->base);
>   
>   	kfree(ssd130x_state);
> @@ -788,7 +741,7 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   
>   static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>   	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>   	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>   };
> @@ -818,6 +771,59 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>   	return MODE_OK;
>   }
>   
> +int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	struct drm_plane *plane = crtc->primary;
> +	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;
> +
> +	ret = drm_crtc_helper_atomic_check(crtc, state);
> +	if (ret)
> +		return ret;
> +
> +	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> +	if (!ssd130x_state->data_array)
> +		return -ENOMEM;
> +
> +	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> +		fi = drm_format_info(DRM_FORMAT_R1);
> +		if (!fi)
> +			return -EINVAL;
> +
> +		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> +
> +		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> +		if (!ssd130x_state->buffer) {
> +			kfree(ssd130x_state->data_array);
> +			/* Set to prevent a double free in .atomic_destroy_state() */
> +			ssd130x_state->data_array = NULL;
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
> +				       struct drm_crtc_state *state)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state->state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> +
> +	drm_atomic_helper_crtc_destroy_state(crtc, state);
> +
> +	kfree(ssd130x_state->data_array);
> +	kfree(ssd130x_state->buffer);
> +}
> +
>   /*
>    * The CRTC is always enabled. Screen updates are performed by
>    * the primary plane's atomic_update function. Disabling clears
> @@ -825,7 +831,7 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>    */
>   static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
>   	.mode_valid = ssd130x_crtc_helper_mode_valid,
> -	.atomic_check = drm_crtc_helper_atomic_check,
> +	.atomic_check = ssd130x_crtc_helper_atomic_check,
>   };
>   
>   static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> @@ -834,7 +840,7 @@ static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
>   	.set_config = drm_atomic_helper_set_config,
>   	.page_flip = drm_atomic_helper_page_flip,
>   	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_destroy_state = ssd130x_crtc_destroy_state,
>   };
>   
>   static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]