[RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback
Thomas Zimmermann
tzimmermann at suse.de
Fri Sep 1 06:53:45 UTC 2023
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 at linux-m68k.org>
> Signed-off-by: Javier Martinez Canillas <javierm at 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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230901/353f9a25/attachment.sig>
More information about the dri-devel
mailing list