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

[Javier Martinez Canillas <javierm@redhat.com>]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi Javier
>
> Am 09.10.23 um 10:58 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>> Hello Thomas,
>> 
>>> Hi Javier
>>>
>>> Am 05.10.23 um 15:38 schrieb Javier Martinez Canillas:
>>>> Thomas Zimmermann <tzimmermann@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.
>

Got it.

>> 
>> 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.
>

No, that's OK. If you are going to revert also in simpledrm and the only
user will be a driver that has it as a callback, then I'm fine with your
original patch to ssd130x that open codes it in its .atomic_check as well.

> Best regards
> Thomas
>
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat