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

[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?

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.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat