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