Hi Javier Am 05.10.23 um 15:38 schrieb Javier Martinez Canillas:
Thomas Zimmermann <tzimmermann@xxxxxxx> writes: Hello Thomas,Preallocate the format-conversion state's storage in the plane's atomic_check function if a format conversion is necessary. Allows the update to fail if no memory is available. Avoids the same allocation within atomic_update, which may not fail. Also inline drm_plane_helper_atomic_check() into the driver and thus return early for invisible planes. Avoids memory allocation entirely in this case. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> --- drivers/gpu/drm/tiny/simpledrm.c | 41 +++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index 6788f465995b3..d2280c81cbaae 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -19,6 +19,7 @@ #include <drm/drm_drv.h> #include <drm/drm_fbdev_generic.h> #include <drm/drm_format_helper.h> +#include <drm/drm_framebuffer.h> #include <drm/drm_gem_atomic_helper.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> @@ -579,6 +580,44 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = { DRM_FORMAT_MOD_INVALID };+static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,+ struct drm_atomic_state *state) +{ + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); + struct drm_shadow_plane_state *new_shadow_plane_state = + to_drm_shadow_plane_state(new_plane_state); + struct drm_framebuffer *new_fb = new_plane_state->fb; + struct drm_crtc *new_crtc = new_plane_state->crtc; + struct drm_crtc_state *new_crtc_state = NULL; + struct drm_device *dev = plane->dev; + struct simpledrm_device *sdev = simpledrm_device_of_dev(dev); + int ret; + + if (new_crtc) + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc); + + ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state, + DRM_PLANE_NO_SCALING, + DRM_PLANE_NO_SCALING, + false, false);Same comment that with the ssd130x driver. I think that we should use the drm_plane_helper_atomic_check() helper instead of open coding it in each
I'm going to replace the call in simpledrm. drm_plane_helper_atomic_check() is useful to remove the entire atomic_check function from the driver; it does nothing apart from that. I've been called out before for such do-nothing helpers; deservedly so. [1]
Best regards Thomas[1] https://lore.kernel.org/dri-devel/aa3c4ad6-f99e-de48-e797-0748c9706e9e@xxxxxxx/
driver. But regardless of what's decided on that, the change looks good: Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
-- 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:
OpenPGP_signature.asc
Description: OpenPGP digital signature
