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

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

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

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,
 		       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,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3541 bytes --]

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.

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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

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

[Jani Nikula]

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.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel