Message-ID: <6a43a18a-bdef-5595-e9f4-38f2d1eac12e@suse.de>
Date:   Wed, 30 Aug 2023 09:08:49 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        linux-kernel@...r.kernel.org
Cc:     Maxime Ripard <mripard@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's
 .atomic_check() callback

Hi Javier

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

Double 'move the'

And maybe buffer's rather than buffers'

> 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@...ux-m68k.org>
> Signed-off-by: Javier Martinez Canillas <javierm@...hat.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;

Using 'primary' is deprecated. You can only refer from plane to crtc 
easily. The other direction is complicated.

> +	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);

I'd store these pointers in the CRTC state and fetch them from within 
ssd130x_primary_plane_helper_atomic_update() during the display update. 
This guarantees that the pointer addresses are always legal.

Besides the pointers, the CRTC state can also store the primary plane 
format, which you update from the plane's atomic check. By doing so, you 
wont need to refer to the plane state from the CRTC's atomic_check. The 
plane's atomic_check runs before the CRTC's atomic_check. [1]

The struct ssd130x_plane_state can then be replaced by stardard DRM 
shadow-plane state. It also solves the problem with 'crtc->primary'.

[1] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L974

> +	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);

Do you really need kcalloc here? kmalloc seems good enough.

> +	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);

Same question about kcalloc.

> +		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);

This cross references among state-helpers has the potential to blow up. 
It's not really clear if there even is a plane state here.

Also see my comment on the allocation of these buffers., which would 
solve this problem as well.

Best regards
Thomas

> +}
> +
>   /*
>    * 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)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists