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 eachI'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)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
