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,
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);
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);-- 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.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jani Nikula -
Thomas Zimmermann -
Tian Tao