Messages in this thread | | | Date | Fri, 1 Sep 2023 08:53:45 +0200 | Subject | Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback | From | Thomas Zimmermann <> |
| |
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) [unhandled content-type:application/pgp-signature] | |