LinuxLists.cc - [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Hi

Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> The driver uses a naming convention where functions for struct drm_*_funcs
> callbacks are named ssd130x_$object_$operation, while the callbacks for
> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
>
> The idea is that this helper_ prefix in the function names denote that are
> for struct drm_*_helper_funcs callbacks. This convention was copied from
> other drivers, when ssd130x was written but Maxime pointed out that is the
> exception rather than the norm.

I guess you found this in my code. I want to point out that I use the
_helper infix to signal that these are callback for
drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
naming is intentional.

Best regards
Thomas

>
> So let's get rid of the _helper prefixes from the function handlers names.
>
> Suggested-by: Maxime Ripard <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---
>
> drivers/gpu/drm/solomon/ssd130x.c | 46 +++++++++++++++----------------
> 1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 8ab02724f65f..245e4ba1c041 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -630,8 +630,8 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb,
> return ret;
> }
>
> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> - struct drm_atomic_state *state)
> +static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_state *state)
> {
> struct drm_device *drm = plane->dev;
> struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> @@ -667,8 +667,8 @@ static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> return 0;
> }
>
> -static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
> - struct drm_atomic_state *state)
> +static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_state *state)
> {
> struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> @@ -701,8 +701,8 @@ static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
> drm_dev_exit(idx);
> }
>
> -static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
> - struct drm_atomic_state *state)
> +static void ssd130x_primary_plane_atomic_disable(struct drm_plane *plane,
> + struct drm_atomic_state *state)
> {
> struct drm_device *drm = plane->dev;
> struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> @@ -777,9 +777,9 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>
> static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
> DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> - .atomic_check = ssd130x_primary_plane_helper_atomic_check,
> - .atomic_update = ssd130x_primary_plane_helper_atomic_update,
> - .atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
> + .atomic_check = ssd130x_primary_plane_atomic_check,
> + .atomic_update = ssd130x_primary_plane_atomic_update,
> + .atomic_disable = ssd130x_primary_plane_atomic_disable,
> };
>
> static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
> @@ -791,8 +791,8 @@ static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
> .destroy = drm_plane_cleanup,
> };
>
> -static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc,
> - const struct drm_display_mode *mode)
> +static enum drm_mode_status ssd130x_crtc_mode_valid(struct drm_crtc *crtc,
> + const struct drm_display_mode *mode)
> {
> struct ssd130x_device *ssd130x = drm_to_ssd130x(crtc->dev);
>
> @@ -807,8 +807,8 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
> return MODE_OK;
> }
>
> -static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
> - struct drm_atomic_state *state)
> +static int ssd130x_crtc_atomic_check(struct drm_crtc *crtc,
> + struct drm_atomic_state *state)
> {
> struct drm_device *drm = crtc->dev;
> struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> @@ -882,8 +882,8 @@ static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
> * the screen in the primary plane's atomic_disable function.
> */
> static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
> - .mode_valid = ssd130x_crtc_helper_mode_valid,
> - .atomic_check = ssd130x_crtc_helper_atomic_check,
> + .mode_valid = ssd130x_crtc_mode_valid,
> + .atomic_check = ssd130x_crtc_atomic_check,
> };
>
> static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> @@ -895,8 +895,8 @@ static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> .atomic_destroy_state = ssd130x_crtc_destroy_state,
> };
>
> -static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
> - struct drm_atomic_state *state)
> +static void ssd130x_encoder_atomic_enable(struct drm_encoder *encoder,
> + struct drm_atomic_state *state)
> {
> struct drm_device *drm = encoder->dev;
> struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> @@ -921,8 +921,8 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
> return;
> }
>
> -static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder,
> - struct drm_atomic_state *state)
> +static void ssd130x_encoder_atomic_disable(struct drm_encoder *encoder,
> + struct drm_atomic_state *state)
> {
> struct drm_device *drm = encoder->dev;
> struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> @@ -935,15 +935,15 @@ static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder,
> }
>
> static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs = {
> - .atomic_enable = ssd130x_encoder_helper_atomic_enable,
> - .atomic_disable = ssd130x_encoder_helper_atomic_disable,
> + .atomic_enable = ssd130x_encoder_atomic_enable,
> + .atomic_disable = ssd130x_encoder_atomic_disable,
> };
>
> static const struct drm_encoder_funcs ssd130x_encoder_funcs = {
> .destroy = drm_encoder_cleanup,
> };
>
> -static int ssd130x_connector_helper_get_modes(struct drm_connector *connector)
> +static int ssd130x_connector_get_modes(struct drm_connector *connector)
> {
> struct ssd130x_device *ssd130x = drm_to_ssd130x(connector->dev);
> struct drm_display_mode *mode;
> @@ -963,7 +963,7 @@ static int ssd130x_connector_helper_get_modes(struct drm_connector *connector)
> }
>
> static const struct drm_connector_helper_funcs ssd130x_connector_helper_funcs = {
> - .get_modes = ssd130x_connector_helper_get_modes,
> + .get_modes = ssd130x_connector_get_modes,
> };
>
> static const struct drm_connector_funcs ssd130x_connector_funcs = {

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

Attachments:

OpenPGP_signature.asc (855.00 B)
OpenPGP digital signature

2023-09-18 18:14:43

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Thomas Zimmermann <[email protected]> writes:

> Hi
>
> Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
>> The driver uses a naming convention where functions for struct drm_*_funcs
>> callbacks are named ssd130x_$object_$operation, while the callbacks for
>> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
>>
>> The idea is that this helper_ prefix in the function names denote that are
>> for struct drm_*_helper_funcs callbacks. This convention was copied from
>> other drivers, when ssd130x was written but Maxime pointed out that is the
>> exception rather than the norm.
>
> I guess you found this in my code. I want to point out that I use the
> _helper infix to signal that these are callback for
> drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
> naming is intentional.
>

Yes, that's what tried to say in the commit message and indeed I got the
convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
function names are since first iteration of the driver, when was meant to
be a tiny driver.

According to Maxime it's the exception rather than the rule and suggested
to change it, I don't really have a strong opinion on either naming TBH.

> Best regards
> Thomas
>

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

2023-09-21 17:17:42

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Hi,

On Thu, Sep 21, 2023 at 10:52:14AM +0200, Thomas Zimmermann wrote:
> Am 21.09.23 um 09:44 schrieb Maxime Ripard:
> > On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
> > > Thomas Zimmermann <[email protected]> writes:
> > >
> > > > Hi
> > > >
> > > > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> > > > > The driver uses a naming convention where functions for struct drm_*_funcs
> > > > > callbacks are named ssd130x_$object_$operation, while the callbacks for
> > > > > struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
> > > > >
> > > > > The idea is that this helper_ prefix in the function names denote that are
> > > > > for struct drm_*_helper_funcs callbacks. This convention was copied from
> > > > > other drivers, when ssd130x was written but Maxime pointed out that is the
> > > > > exception rather than the norm.
> > > >
> > > > I guess you found this in my code. I want to point out that I use the
> > > > _helper infix to signal that these are callback for
> > > > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
> > > > naming is intentional.
> > > >
> > >
> > > Yes, that's what tried to say in the commit message and indeed I got the
> > > convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
> > > function names are since first iteration of the driver, when was meant to
> > > be a tiny driver.
> > >
> > > According to Maxime it's the exception rather than the rule and suggested
> > > to change it, I don't really have a strong opinion on either naming TBH.
> >
> > Maybe that's just me, but the helper in the name indeed throws me off. In my
> > mind, it's supposed to be used only for helpers, not functions implementing the
> > helpers hooks.
>
> Tying the function name to its _funcs structure makes perfect sense to me,
> as it helps to structure the driver code. So I always use the _helper_
> infix.
>
> In contrast, the DRM helpers that implement certain functionality does not
> seem to follow any naming scheme. For example drm_atomic_helper_check()
> implements struct drm_mode_config_funcs.atomic_check. To me, it's not
> obvious that these two belong together.

Right, but then we end up with functions that have helpers in their name
and are indeed helpers, and then we have functions that have helpers in
their name but are not meant to help anyone or be reused anywhere else.

> And in the same structure, there's fb_create, which is provided by
> drm_gem_fb_create_with_dirty(). This one doesn't even mention that
> it's a helper.

We should fix that then I guess?

Anyway, we're way past bikeshedding there :)

Maxime

Attachments:

(No filename) (2.67 kB)
signature.asc (235.00 B)
Download all attachments

2023-09-21 18:04:07

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Hi Maxime,

On Thu, Sep 21, 2023 at 9:44 AM Maxime Ripard <[email protected]> wrote:
> On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
> > Thomas Zimmermann <[email protected]> writes:
> > > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> > >> The driver uses a naming convention where functions for struct drm_*_funcs
> > >> callbacks are named ssd130x_$object_$operation, while the callbacks for
> > >> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
> > >>
> > >> The idea is that this helper_ prefix in the function names denote that are
> > >> for struct drm_*_helper_funcs callbacks. This convention was copied from
> > >> other drivers, when ssd130x was written but Maxime pointed out that is the
> > >> exception rather than the norm.
> > >
> > > I guess you found this in my code. I want to point out that I use the
> > > _helper infix to signal that these are callback for
> > > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
> > > naming is intentional.
> >
> > Yes, that's what tried to say in the commit message and indeed I got the
> > convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
> > function names are since first iteration of the driver, when was meant to
> > be a tiny driver.
> >
> > According to Maxime it's the exception rather than the rule and suggested
> > to change it, I don't really have a strong opinion on either naming TBH.
>
> Maybe that's just me, but the helper in the name indeed throws me off. In my
> mind, it's supposed to be used only for helpers, not functions implementing the
> helpers hooks.

With several callbacks using the same (field) name, it is very helpful
to name the actual implementation by combining the struct type name
and the field name. Anything else confuses the casual reader.
Perhaps the real question is whether the structures should have "helper"
in their name in the first place?

Just my 2€c as a DRM novice...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-09-21 19:12:09

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Hi Maxime,

On Thu, Sep 21, 2023 at 10:12 AM Maxime Ripard <[email protected]> wrote:
> On Thu, Sep 21, 2023 at 09:57:22AM +0200, Geert Uytterhoeven wrote:
> > On Thu, Sep 21, 2023 at 9:44 AM Maxime Ripard <[email protected]> wrote:
> > > On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
> > > > Thomas Zimmermann <[email protected]> writes:
> > > > > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> > > > >> The driver uses a naming convention where functions for struct drm_*_funcs
> > > > >> callbacks are named ssd130x_$object_$operation, while the callbacks for
> > > > >> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
> > > > >>
> > > > >> The idea is that this helper_ prefix in the function names denote that are
> > > > >> for struct drm_*_helper_funcs callbacks. This convention was copied from
> > > > >> other drivers, when ssd130x was written but Maxime pointed out that is the
> > > > >> exception rather than the norm.
> > > > >
> > > > > I guess you found this in my code. I want to point out that I use the
> > > > > _helper infix to signal that these are callback for
> > > > > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
> > > > > naming is intentional.
> > > >
> > > > Yes, that's what tried to say in the commit message and indeed I got the
> > > > convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
> > > > function names are since first iteration of the driver, when was meant to
> > > > be a tiny driver.
> > > >
> > > > According to Maxime it's the exception rather than the rule and suggested
> > > > to change it, I don't really have a strong opinion on either naming TBH.
> > >
> > > Maybe that's just me, but the helper in the name indeed throws me off. In my
> > > mind, it's supposed to be used only for helpers, not functions implementing the
> > > helpers hooks.
> >
> > With several callbacks using the same (field) name, it is very helpful
> > to name the actual implementation by combining the struct type name
> > and the field name.
>
> I can't think of any (at least for a given object). Which one do you have in
> mind?

E.g. atomic_check():

drm_crtc_helper_funcs.atomic_check()
drm_encoder_helper_funcs.atomic_check()
drm_connector_helper_funcs.atomic_check()
drm_plane_helper_funcs.atomic_check()

Interestingly, drm_mode_config_helper_funcs does not have an
atomic_check() callback, but drm_mode_config_funcs has.

> > Anything else confuses the casual reader. Perhaps the real question is whether
> > the structures should have "helper" in their name in the first place?
>
> Those structures are meant for functions used by the helpers, they are not
> helper functions.

That might be how they started, but to me it looks like all these helpers
are no longer helpers, but part of the core...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-09-21 20:45:09

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Hi,

On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
> Thomas Zimmermann <[email protected]> writes:
>
> > Hi
> >
> > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> >> The driver uses a naming convention where functions for struct drm_*_funcs
> >> callbacks are named ssd130x_$object_$operation, while the callbacks for
> >> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
> >>
> >> The idea is that this helper_ prefix in the function names denote that are
> >> for struct drm_*_helper_funcs callbacks. This convention was copied from
> >> other drivers, when ssd130x was written but Maxime pointed out that is the
> >> exception rather than the norm.
> >
> > I guess you found this in my code. I want to point out that I use the
> > _helper infix to signal that these are callback for
> > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
> > naming is intentional.
> >
>
> Yes, that's what tried to say in the commit message and indeed I got the
> convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
> function names are since first iteration of the driver, when was meant to
> be a tiny driver.
>
> According to Maxime it's the exception rather than the rule and suggested
> to change it, I don't really have a strong opinion on either naming TBH.

Maybe that's just me, but the helper in the name indeed throws me off. In my
mind, it's supposed to be used only for helpers, not functions implementing the
helpers hooks.

Maxime

Attachments:

(No filename) (1.54 kB)
signature.asc (235.00 B)
Download all attachments

2023-09-21 21:08:59

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

On Thu, 14 Sep 2023 21:51:24 +0200, Javier Martinez Canillas wrote:
> The driver uses a naming convention where functions for struct drm_*_funcs
> callbacks are named ssd130x_$object_$operation, while the callbacks for
> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
>
> The idea is that this helper_ prefix in the function names denote that are
>
> [ ... ]

Reviewed-by: Maxime Ripard <[email protected]>

Thanks!
Maxime

2023-09-21 21:34:51

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Hi

Am 21.09.23 um 10:46 schrieb Geert Uytterhoeven:
[...]
>
>>> Anything else confuses the casual reader. Perhaps the real question is whether
>>> the structures should have "helper" in their name in the first place?
>>
>> Those structures are meant for functions used by the helpers, they are not
>> helper functions.
>
> That might be how they started, but to me it looks like all these helpers
> are no longer helpers, but part of the core...

They are in library modules. You can write a DRM driver without
_helper_funcs, see i915. It's just a really hard sell to upstream nowadays.

Best regards
Thomas

>
> Gr{oetje,eeting}s,
>
> Geert
>

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

Attachments:

OpenPGP_signature.asc (855.00 B)
OpenPGP digital signature

2023-09-21 22:17:05

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Maxime Ripard <[email protected]> writes:

> On Thu, Sep 21, 2023 at 09:57:22AM +0200, Geert Uytterhoeven wrote:

[...]

>> Anything else confuses the casual reader. Perhaps the real question is whether
>> the structures should have "helper" in their name in the first place?
>
> Those structures are meant for functions used by the helpers, they are not
> helper functions.
>

Ah, that makes more sense. I wasn't aware of that.

If that's the case, then the split makes more sense to me now.

> Maxime

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

2023-09-21 22:34:35

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Hi Maxime,

On Thu, Sep 21, 2023 at 10:51 AM Maxime Ripard <[email protected]> wrote:
> On Thu, Sep 21, 2023 at 10:46:05AM +0200, Geert Uytterhoeven wrote:
> > On Thu, Sep 21, 2023 at 10:12 AM Maxime Ripard <[email protected]> wrote:
> > > On Thu, Sep 21, 2023 at 09:57:22AM +0200, Geert Uytterhoeven wrote:
> > > > On Thu, Sep 21, 2023 at 9:44 AM Maxime Ripard <[email protected]> wrote:
> > > > > On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
> > > > > > Thomas Zimmermann <[email protected]> writes:
> > > > > > > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> > > > > > >> The driver uses a naming convention where functions for struct drm_*_funcs
> > > > > > >> callbacks are named ssd130x_$object_$operation, while the callbacks for
> > > > > > >> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
> > > > > > >>
> > > > > > >> The idea is that this helper_ prefix in the function names denote that are
> > > > > > >> for struct drm_*_helper_funcs callbacks. This convention was copied from
> > > > > > >> other drivers, when ssd130x was written but Maxime pointed out that is the
> > > > > > >> exception rather than the norm.
> > > > > > >
> > > > > > > I guess you found this in my code. I want to point out that I use the
> > > > > > > _helper infix to signal that these are callback for
> > > > > > > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
> > > > > > > naming is intentional.
> > > > > >
> > > > > > Yes, that's what tried to say in the commit message and indeed I got the
> > > > > > convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
> > > > > > function names are since first iteration of the driver, when was meant to
> > > > > > be a tiny driver.
> > > > > >
> > > > > > According to Maxime it's the exception rather than the rule and suggested
> > > > > > to change it, I don't really have a strong opinion on either naming TBH.
> > > > >
> > > > > Maybe that's just me, but the helper in the name indeed throws me off. In my
> > > > > mind, it's supposed to be used only for helpers, not functions implementing the
> > > > > helpers hooks.
> > > >
> > > > With several callbacks using the same (field) name, it is very helpful
> > > > to name the actual implementation by combining the struct type name
> > > > and the field name.
> > >
> > > I can't think of any (at least for a given object). Which one do you have in
> > > mind?
> >
> > E.g. atomic_check():
> >
> > drm_crtc_helper_funcs.atomic_check()
> > drm_encoder_helper_funcs.atomic_check()
> > drm_connector_helper_funcs.atomic_check()
> > drm_plane_helper_funcs.atomic_check()
>
> Right, but that's between objects, not between drm_$OBJECT_funcs and
> drm_$OBJECT_helper_funcs. So conflicts for a single given driver is unlikely,
> and can be solved by using, say, $DRIVER_crtc_atomic_check and
> $DRIVER_plane_atomic_check.

IC. There are indeed no such conflicts (except between
drm_encoder_slave_funcs and drm_encoder_helper_funcs, which I guess
doesn't count).

Thanks, this helps a lot to explain why there is no need to have
"helper" in the name of the callbacks.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-09-21 23:29:11

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

On Thu, Sep 21, 2023 at 09:57:22AM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
>
> On Thu, Sep 21, 2023 at 9:44 AM Maxime Ripard <[email protected]> wrote:
> > On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
> > > Thomas Zimmermann <[email protected]> writes:
> > > > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> > > >> The driver uses a naming convention where functions for struct drm_*_funcs
> > > >> callbacks are named ssd130x_$object_$operation, while the callbacks for
> > > >> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
> > > >>
> > > >> The idea is that this helper_ prefix in the function names denote that are
> > > >> for struct drm_*_helper_funcs callbacks. This convention was copied from
> > > >> other drivers, when ssd130x was written but Maxime pointed out that is the
> > > >> exception rather than the norm.
> > > >
> > > > I guess you found this in my code. I want to point out that I use the
> > > > _helper infix to signal that these are callback for
> > > > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
> > > > naming is intentional.
> > >
> > > Yes, that's what tried to say in the commit message and indeed I got the
> > > convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
> > > function names are since first iteration of the driver, when was meant to
> > > be a tiny driver.
> > >
> > > According to Maxime it's the exception rather than the rule and suggested
> > > to change it, I don't really have a strong opinion on either naming TBH.
> >
> > Maybe that's just me, but the helper in the name indeed throws me off. In my
> > mind, it's supposed to be used only for helpers, not functions implementing the
> > helpers hooks.
>
> With several callbacks using the same (field) name, it is very helpful
> to name the actual implementation by combining the struct type name
> and the field name.

I can't think of any (at least for a given object). Which one do you have in
mind?

> Anything else confuses the casual reader. Perhaps the real question is whether
> the structures should have "helper" in their name in the first place?

Those structures are meant for functions used by the helpers, they are not
helper functions.

Maxime

Attachments:

(No filename) (2.28 kB)
signature.asc (235.00 B)
Download all attachments

2023-09-21 23:56:41

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

On Thu, Sep 21, 2023 at 10:46:05AM +0200, Geert Uytterhoeven wrote:
> On Thu, Sep 21, 2023 at 10:12 AM Maxime Ripard <[email protected]> wrote:
> > On Thu, Sep 21, 2023 at 09:57:22AM +0200, Geert Uytterhoeven wrote:
> > > On Thu, Sep 21, 2023 at 9:44 AM Maxime Ripard <[email protected]> wrote:
> > > > On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
> > > > > Thomas Zimmermann <[email protected]> writes:
> > > > > > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> > > > > >> The driver uses a naming convention where functions for struct drm_*_funcs
> > > > > >> callbacks are named ssd130x_$object_$operation, while the callbacks for
> > > > > >> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
> > > > > >>
> > > > > >> The idea is that this helper_ prefix in the function names denote that are
> > > > > >> for struct drm_*_helper_funcs callbacks. This convention was copied from
> > > > > >> other drivers, when ssd130x was written but Maxime pointed out that is the
> > > > > >> exception rather than the norm.
> > > > > >
> > > > > > I guess you found this in my code. I want to point out that I use the
> > > > > > _helper infix to signal that these are callback for
> > > > > > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
> > > > > > naming is intentional.
> > > > >
> > > > > Yes, that's what tried to say in the commit message and indeed I got the
> > > > > convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
> > > > > function names are since first iteration of the driver, when was meant to
> > > > > be a tiny driver.
> > > > >
> > > > > According to Maxime it's the exception rather than the rule and suggested
> > > > > to change it, I don't really have a strong opinion on either naming TBH.
> > > >
> > > > Maybe that's just me, but the helper in the name indeed throws me off. In my
> > > > mind, it's supposed to be used only for helpers, not functions implementing the
> > > > helpers hooks.
> > >
> > > With several callbacks using the same (field) name, it is very helpful
> > > to name the actual implementation by combining the struct type name
> > > and the field name.
> >
> > I can't think of any (at least for a given object). Which one do you have in
> > mind?
>
> E.g. atomic_check():
>
> drm_crtc_helper_funcs.atomic_check()
> drm_encoder_helper_funcs.atomic_check()
> drm_connector_helper_funcs.atomic_check()
> drm_plane_helper_funcs.atomic_check()

Right, but that's between objects, not between drm_$OBJECT_funcs and
drm_$OBJECT_helper_funcs. So conflicts for a single given driver is unlikely,
and can be solved by using, say, $DRIVER_crtc_atomic_check and
$DRIVER_plane_atomic_check.

> Interestingly, drm_mode_config_helper_funcs does not have an
> atomic_check() callback, but drm_mode_config_funcs has.
>
> > > Anything else confuses the casual reader. Perhaps the real question is whether
> > > the structures should have "helper" in their name in the first place?
> >
> > Those structures are meant for functions used by the helpers, they are not
> > helper functions.
>
> That might be how they started, but to me it looks like all these helpers
> are no longer helpers, but part of the core...

They are part of the core, but very much optional still. i915 doesn't use a lot
of helpers, vc4 (used to?) rolls its own atomic_commit implementation, etc. It's
really not uncommon.

Maxime

Attachments:

(No filename) (3.45 kB)
signature.asc (235.00 B)
Download all attachments

2023-09-22 00:53:24

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Hi

Am 21.09.23 um 10:23 schrieb Javier Martinez Canillas:
[...]
>
> Both options have cons and pros (e.g: quickly figuring out to what struct
> callback is associated as you said), but the reason I posted this patch is
> to attempt making the driver more consistent with the rest of the drivers.
>
>> Perhaps the real question is whether the structures should have "helper"
>> in their name in the first place?
>>
>
> Indeed. I never fully understood why the DRM/KMS objects callbacks are
> split in drm_$object_funcs and drm_$object_helper_funcs structs. AFAIU
> is because the former is the minimum required and the latter is to add
> additional custom behavior ?

The drm_<object>_funcs is an interface that is being called from DRM
userspace/clients/ioctls. It's the interface that we present to the
outside world. Implement them in each hardware's driver.

But most graphics hardware is similar to each other. The differences are
in the way how things are done, but not so much what is being done.
Hence, a good number of drm_$object_funcs can be provided in
hardware-independent helpers. drm_object_helper_funcs are callback for
these helpers. In the places where the helpers need the driver to do
something with the hardware, they refer to _helper_funcs.

IIRC, there are a few outliers, but that's the overall idea.

Best regards
Thomas

>
> I read this section of the documentation but still isn't clear to me:
>
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html
>
>> Just my 2€c as a DRM novice...
>>
>

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

Attachments:

OpenPGP_signature.asc (855.00 B)
OpenPGP digital signature

2023-09-22 04:11:10

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Hi

Am 21.09.23 um 09:44 schrieb Maxime Ripard:
> Hi,
>
> On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
>> Thomas Zimmermann <[email protected]> writes:
>>
>>> Hi
>>>
>>> Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
>>>> The driver uses a naming convention where functions for struct drm_*_funcs
>>>> callbacks are named ssd130x_$object_$operation, while the callbacks for
>>>> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
>>>>
>>>> The idea is that this helper_ prefix in the function names denote that are
>>>> for struct drm_*_helper_funcs callbacks. This convention was copied from
>>>> other drivers, when ssd130x was written but Maxime pointed out that is the
>>>> exception rather than the norm.
>>>
>>> I guess you found this in my code. I want to point out that I use the
>>> _helper infix to signal that these are callback for
>>> drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
>>> naming is intentional.
>>>
>>
>> Yes, that's what tried to say in the commit message and indeed I got the
>> convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
>> function names are since first iteration of the driver, when was meant to
>> be a tiny driver.
>>
>> According to Maxime it's the exception rather than the rule and suggested
>> to change it, I don't really have a strong opinion on either naming TBH.
>
> Maybe that's just me, but the helper in the name indeed throws me off. In my
> mind, it's supposed to be used only for helpers, not functions implementing the
> helpers hooks.

Tying the function name to its _funcs structure makes perfect sense to
me, as it helps to structure the driver code. So I always use the
_helper_ infix.

In contrast, the DRM helpers that implement certain functionality does
not seem to follow any naming scheme. For example
drm_atomic_helper_check() implements struct
drm_mode_config_funcs.atomic_check. To me, it's not obvious that these
two belong together. And in the same structure, there's fb_create,
which is provided by drm_gem_fb_create_with_dirty(). This one doesn't
even mention that it's a helper.

Best regards
Thomas

>
> Maxime

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

Attachments:

OpenPGP_signature.asc (855.00 B)
OpenPGP digital signature

2023-09-22 08:25:59

[permalink] [raw]

Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper_funcs callbacks

Geert Uytterhoeven <[email protected]> writes:

Hello Geert and Maxime,

> Hi Maxime,
>
> On Thu, Sep 21, 2023 at 9:44 AM Maxime Ripard <[email protected]> wrote:
>> On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
>> > Thomas Zimmermann <[email protected]> writes:
>> > > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
>> > >> The driver uses a naming convention where functions for struct drm_*_funcs
>> > >> callbacks are named ssd130x_$object_$operation, while the callbacks for
>> > >> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
>> > >>
>> > >> The idea is that this helper_ prefix in the function names denote that are
>> > >> for struct drm_*_helper_funcs callbacks. This convention was copied from
>> > >> other drivers, when ssd130x was written but Maxime pointed out that is the
>> > >> exception rather than the norm.
>> > >
>> > > I guess you found this in my code. I want to point out that I use the
>> > > _helper infix to signal that these are callback for
>> > > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
>> > > naming is intentional.
>> >
>> > Yes, that's what tried to say in the commit message and indeed I got the
>> > convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
>> > function names are since first iteration of the driver, when was meant to
>> > be a tiny driver.
>> >
>> > According to Maxime it's the exception rather than the rule and suggested
>> > to change it, I don't really have a strong opinion on either naming TBH.
>>
>> Maybe that's just me, but the helper in the name indeed throws me off. In my
>> mind, it's supposed to be used only for helpers, not functions implementing the
>> helpers hooks.

I agree with your definition of helpers. But more importantly for me is
what you mentioned, that most DRM drivers aren't using this convention
of concatenating the driver name + struct type name + callback name.

>
> With several callbacks using the same (field) name, it is very helpful
> to name the actual implementation by combining the struct type name
> and the field name. Anything else confuses the casual reader.

Both options have cons and pros (e.g: quickly figuring out to what struct
callback is associated as you said), but the reason I posted this patch is
to attempt making the driver more consistent with the rest of the drivers.

> Perhaps the real question is whether the structures should have "helper"
> in their name in the first place?
>

Indeed. I never fully understood why the DRM/KMS objects callbacks are
split in drm_$object_funcs and drm_$object_helper_funcs structs. AFAIU
is because the former is the minimum required and the latter is to add
additional custom behavior ?

I read this section of the documentation but still isn't clear to me:

https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html

> Just my 2€c as a DRM novice...
>

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

2023-09-22 15:27:57