[PATCH v4 5/7] drm/simpledrm: Preallocate format-conversion buffer in atomic_check

[Thomas Zimmermann]

Hi Javier

Am 05.10.23 um 15:38 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann at suse.de> 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 at suse.de>
>> ---
>>   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@amd.com/

> driver. But regardless of what's decided on that, the change looks good:
> 
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
> 

-- 
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.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231009/3da0c239/attachment.sig>