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

[Thomas Zimmermann]

Hi Javier

Am 09.10.23 um 10:58 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann at suse.de> writes:
> 
> Hello Thomas,
> 
>> Hi Javier
>>
>> Am 05.10.23 um 15:38 schrieb Javier Martinez Canillas:
>>> Thomas Zimmermann <tzimmermann at suse.de> writes:
> 
> [...]
> 
>>>> +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]
>>
> 
> The argument then is that drivers should open code *exactly* the same code
> that the helper function already has just because they implement their own
> .atomic_check callback?
> 
> And that the helper should only be used when is the .atomic_check callback
> but not as a helper function?

My point (and I think that's what Christian was also referring to) is 
that drm_plane_helper_atomic_check() does little more than pick a few 
default values for the parameters. It doesn't do anything in terms of 
algorithms. Hence there's no saving here that outweighs the cost of 
using this helper.

> 
> I don't understand that rationale to be honest, but if there is one then
> it should be very clear in the kernel-doc what functions are supposed to
> be used only as callbacks and what functions can also be used as helpers.

There's no clear rule AFAIK. We have to decide case by case. TBH I don't 
mind re-evaluating cases from time to time. At least, I'm going to 
revert the open-coded helper in ssd130x, as you asked me to.

Best regards
Thomas


> 
>> Best regards
>> Thomas
>>
> 

-- 
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/f0aada26/attachment.sig>