| Message ID | 1610092442-36168-1-git-send-email-tiantao6@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/connector: Set the default callback function for drm_connector_funcs | expand |
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 >
On Fri, 08 Jan 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Drivers cannot legally declare the funcs instance as static const. > Having static const allows for write protected pages. This. I've done quite a bit of refactoring all over the place to be ablet to move to the complete opposite direction. We want to keep all callback structs static const. If the idea here was good (on which I'm inclined to side with Thomas and Daniel that it isn't), the way to go would be to add a small wrapper for calling ->fill_modes(), with a different path for when it's NULL. BR, Jani.
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, 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; + + 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,
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. 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(-)