Re: [RFC] drm/connector: Set the default callback function for drm_connector_funcs

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jan 8, 2021 at 9:12 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 08.01.21 um 08:54 schrieb Tian Tao:
> > The member functions of drm_connector_funcs are not specific to each
> > manufacturer's driver, so drm_connector_funcs is allowed to use default
> > values, which prevents all drivers from setting the same member
> > functions for drm_connector_funcs.
>
> I don't think that's a good idea.

Yeah this breaks the helper/core separation. We have occasionally move
functionality across that border, but this is the one place where
we're ok with a pile of duplication, since it helps in making sure
that the helper library (like the probe function you want to make the
default here) really stays an optional helper library and doesn't
become some kind of midlayer spaghetti mess.
-Daniel

>
> >
> > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > ---
> >   drivers/gpu/drm/drm_connector.c                  | 7 ++++++-
> >   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 1 -
> >   include/drm/drm_connector.h                      | 2 +-
> >   3 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 98b6ec4..356d8a3 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -25,6 +25,7 @@
> >   #include <drm/drm_encoder.h>
> >   #include <drm/drm_utils.h>
> >   #include <drm/drm_print.h>
> > +#include <drm/drm_probe_helper.h>
> >   #include <drm/drm_drv.h>
> >   #include <drm/drm_file.h>
> >   #include <drm/drm_sysfs.h>
> > @@ -216,7 +217,7 @@ void drm_connector_free_work_fn(struct work_struct *work)
> >    */
> >   int drm_connector_init(struct drm_device *dev,
> >                      struct drm_connector *connector,
> > -                    const struct drm_connector_funcs *funcs,
> > +                    struct drm_connector_funcs *funcs,
>
> Drivers cannot legally declare the funcs instance as static const.
> Having static const allows for write protected pages.
>
> >                      int connector_type)
> >   {
> >       struct drm_mode_config *config = &dev->mode_config;
> > @@ -228,6 +229,10 @@ int drm_connector_init(struct drm_device *dev,
> >               (!funcs->atomic_destroy_state ||
> >                !funcs->atomic_duplicate_state));
> >
> > +     if (!funcs->fill_modes)
> > +             funcs->fill_modes = &drm_helper_probe_single_connector_modes;
>
> It's not clear that this is really the correct function for this driver.
>
> Best regards
> Thomas
>
> > +
> > +
> >       ret = __drm_mode_object_add(dev, &connector->base,
> >                                   DRM_MODE_OBJECT_CONNECTOR,
> >                                   false, drm_connector_free);
> > diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> > index c76f996..7d3b662 100644
> > --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> > +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> > @@ -64,7 +64,6 @@ static const struct drm_connector_helper_funcs
> >   };
> >
> >   static const struct drm_connector_funcs hibmc_connector_funcs = {
> > -     .fill_modes = drm_helper_probe_single_connector_modes,
> >       .destroy = hibmc_connector_destroy,
> >       .reset = drm_atomic_helper_connector_reset,
> >       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 1922b27..4810583 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -1561,7 +1561,7 @@ struct drm_connector {
> >
> >   int drm_connector_init(struct drm_device *dev,
> >                      struct drm_connector *connector,
> > -                    const struct drm_connector_funcs *funcs,
> > +                    struct drm_connector_funcs *funcs,
> >                      int connector_type);
> >   int drm_connector_init_with_ddc(struct drm_device *dev,
> >                               struct drm_connector *connector,
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel