* [Intel-gfx] [PATCH v3 1/7] drm: Include the encoder itself in possible_clones
2020-02-11 16:22 [Intel-gfx] [PATCH v3 0/7] drm: Try to fix encoder possible_clones/crtc Ville Syrjala
@ 2020-02-11 16:22 ` Ville Syrjala
2020-02-11 16:58 ` Daniel Vetter
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 2/7] drm/gma500: Sanitize possible_clones Ville Syrjala
` (6 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Ville Syrjala @ 2020-02-11 16:22 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Thomas Zimmermann
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
The docs say possible_clones should always include the encoder itself.
Since most drivers don't want to deal with the complexities of cloning
let's allow them to set possible_clones=0 and instead we'll fix that
up in the core.
We can't put this special case into drm_encoder_init() because drivers
will have to fill up possible_clones after adding all the relevant
encoders. Otherwise they wouldn't know the proper encoder indexes to
use. So we'll just do it just before registering the device.
v2: Don't set the bit if possible_clones!=0 so that the
validation (coming soon) will WARN (Thomas)
Fix up the docs to allow possible_clones==0 (Daniel)
.late_register() is too late, introduce drm_mode_config_validate()
which gets called _before_ we register the char device (Daniel)
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/drm_crtc_internal.h | 1 +
drivers/gpu/drm/drm_drv.c | 3 +++
drivers/gpu/drm/drm_mode_config.c | 19 +++++++++++++++++++
include/drm/drm_encoder.h | 4 +++-
4 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 16f2413403aa..ace363b4f82b 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -82,6 +82,7 @@ int drm_mode_setcrtc(struct drm_device *dev,
/* drm_mode_config.c */
int drm_modeset_register_all(struct drm_device *dev);
void drm_modeset_unregister_all(struct drm_device *dev);
+void drm_mode_config_validate(struct drm_device *dev);
/* drm_modes.c */
const char *drm_get_mode_status_name(enum drm_mode_status status);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7b1a628d1f6e..65a0acb79323 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -946,6 +946,9 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
struct drm_driver *driver = dev->driver;
int ret;
+ if (!driver->load)
+ drm_mode_config_validate(dev);
+
if (drm_dev_needs_global_mutex(dev))
mutex_lock(&drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 08e6eff6a179..75e357c7e84d 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -532,3 +532,22 @@ void drm_mode_config_cleanup(struct drm_device *dev)
drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
}
EXPORT_SYMBOL(drm_mode_config_cleanup);
+
+/*
+ * For some reason we want the encoder itself included in
+ * possible_clones. Make life easy for drivers by allowing them
+ * to leave possible_clones unset if no cloning is possible.
+ */
+static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
+{
+ if (encoder->possible_clones == 0)
+ encoder->possible_clones = drm_encoder_mask(encoder);
+}
+
+void drm_mode_config_validate(struct drm_device *dev)
+{
+ struct drm_encoder *encoder;
+
+ drm_for_each_encoder(encoder, dev)
+ fixup_encoder_possible_clones(encoder);
+}
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 5623994b6e9e..22d6cdf729f1 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -159,7 +159,9 @@ struct drm_encoder {
* encoders can be used in a cloned configuration, they both should have
* each another bits set.
*
- * In reality almost every driver gets this wrong.
+ * As an exception to the above rule if the driver doesn't implement
+ * any cloning it can leave @possible_clones set to 0. The core will
+ * automagically fix this up by setting the bit for the encoder itself.
*
* Note that since encoder objects can't be hotplugged the assigned indices
* are stable and hence known before registering all objects.
--
2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 1/7] drm: Include the encoder itself in possible_clones
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 1/7] drm: Include the encoder itself in possible_clones Ville Syrjala
@ 2020-02-11 16:58 ` Daniel Vetter
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2020-02-11 16:58 UTC (permalink / raw)
To: Ville Syrjala; +Cc: intel-gfx, Thomas Zimmermann, dri-devel
On Tue, Feb 11, 2020 at 06:22:02PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The docs say possible_clones should always include the encoder itself.
> Since most drivers don't want to deal with the complexities of cloning
> let's allow them to set possible_clones=0 and instead we'll fix that
> up in the core.
>
> We can't put this special case into drm_encoder_init() because drivers
> will have to fill up possible_clones after adding all the relevant
> encoders. Otherwise they wouldn't know the proper encoder indexes to
> use. So we'll just do it just before registering the device.
>
> v2: Don't set the bit if possible_clones!=0 so that the
> validation (coming soon) will WARN (Thomas)
> Fix up the docs to allow possible_clones==0 (Daniel)
> .late_register() is too late, introduce drm_mode_config_validate()
> which gets called _before_ we register the char device (Daniel)
Looks solid.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/drm_crtc_internal.h | 1 +
> drivers/gpu/drm/drm_drv.c | 3 +++
> drivers/gpu/drm/drm_mode_config.c | 19 +++++++++++++++++++
> include/drm/drm_encoder.h | 4 +++-
> 4 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 16f2413403aa..ace363b4f82b 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -82,6 +82,7 @@ int drm_mode_setcrtc(struct drm_device *dev,
> /* drm_mode_config.c */
> int drm_modeset_register_all(struct drm_device *dev);
> void drm_modeset_unregister_all(struct drm_device *dev);
> +void drm_mode_config_validate(struct drm_device *dev);
>
> /* drm_modes.c */
> const char *drm_get_mode_status_name(enum drm_mode_status status);
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7b1a628d1f6e..65a0acb79323 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -946,6 +946,9 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> struct drm_driver *driver = dev->driver;
> int ret;
>
> + if (!driver->load)
> + drm_mode_config_validate(dev);
> +
> if (drm_dev_needs_global_mutex(dev))
> mutex_lock(&drm_global_mutex);
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 08e6eff6a179..75e357c7e84d 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -532,3 +532,22 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
> }
> EXPORT_SYMBOL(drm_mode_config_cleanup);
> +
> +/*
> + * For some reason we want the encoder itself included in
> + * possible_clones. Make life easy for drivers by allowing them
> + * to leave possible_clones unset if no cloning is possible.
> + */
> +static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
> +{
> + if (encoder->possible_clones == 0)
> + encoder->possible_clones = drm_encoder_mask(encoder);
> +}
> +
> +void drm_mode_config_validate(struct drm_device *dev)
> +{
> + struct drm_encoder *encoder;
> +
> + drm_for_each_encoder(encoder, dev)
> + fixup_encoder_possible_clones(encoder);
> +}
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 5623994b6e9e..22d6cdf729f1 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -159,7 +159,9 @@ struct drm_encoder {
> * encoders can be used in a cloned configuration, they both should have
> * each another bits set.
> *
> - * In reality almost every driver gets this wrong.
> + * As an exception to the above rule if the driver doesn't implement
> + * any cloning it can leave @possible_clones set to 0. The core will
> + * automagically fix this up by setting the bit for the encoder itself.
> *
> * Note that since encoder objects can't be hotplugged the assigned indices
> * are stable and hence known before registering all objects.
> --
> 2.24.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Intel-gfx] [PATCH v3 2/7] drm/gma500: Sanitize possible_clones
2020-02-11 16:22 [Intel-gfx] [PATCH v3 0/7] drm: Try to fix encoder possible_clones/crtc Ville Syrjala
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 1/7] drm: Include the encoder itself in possible_clones Ville Syrjala
@ 2020-02-11 16:22 ` Ville Syrjala
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 3/7] drm/exynos: Use drm_encoder_mask() Ville Syrjala
` (5 subsequent siblings)
7 siblings, 0 replies; 28+ messages in thread
From: Ville Syrjala @ 2020-02-11 16:22 UTC (permalink / raw)
To: dri-devel; +Cc: Daniel Vetter, intel-gfx
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
I doubt the DP+DP and SDVO+SDVO cloning works for this driver.
i915 at least doesn't do those. Truthfully there could be some very
specific circumstances where some of them would do doable, but
genereally it's too much pain to deal with so we've chose not to
bother. Let's use the same approach for gma500.
Also the LVDS+LVDS and DSI+DSI cases probably don't really exist as
there is one of each at most.
This does mean we'll now leave possible_clones at 0 for these encoder
types whereas previosuly we included the encoder itself in the bitmask.
But that's fine as the core now treaks 0 as a special case and adds
the encoder itself into the final bitmask reported to userspace.
Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/gma500/framebuffer.c | 16 ++++++++--------
drivers/gpu/drm/gma500/mdfld_dsi_dpi.c | 4 ++--
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 1459076d1980..6ca4e6ded96c 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -581,31 +581,31 @@ static void psb_setup_outputs(struct drm_device *dev)
break;
case INTEL_OUTPUT_SDVO:
crtc_mask = dev_priv->ops->sdvo_mask;
- clone_mask = (1 << INTEL_OUTPUT_SDVO);
+ clone_mask = 0;
break;
case INTEL_OUTPUT_LVDS:
- crtc_mask = dev_priv->ops->lvds_mask;
- clone_mask = (1 << INTEL_OUTPUT_LVDS);
+ crtc_mask = dev_priv->ops->lvds_mask;
+ clone_mask = 0;
break;
case INTEL_OUTPUT_MIPI:
crtc_mask = (1 << 0);
- clone_mask = (1 << INTEL_OUTPUT_MIPI);
+ clone_mask = 0;
break;
case INTEL_OUTPUT_MIPI2:
crtc_mask = (1 << 2);
- clone_mask = (1 << INTEL_OUTPUT_MIPI2);
+ clone_mask = 0;
break;
case INTEL_OUTPUT_HDMI:
- crtc_mask = dev_priv->ops->hdmi_mask;
+ crtc_mask = dev_priv->ops->hdmi_mask;
clone_mask = (1 << INTEL_OUTPUT_HDMI);
break;
case INTEL_OUTPUT_DISPLAYPORT:
crtc_mask = (1 << 0) | (1 << 1);
- clone_mask = (1 << INTEL_OUTPUT_DISPLAYPORT);
+ clone_mask = 0;
break;
case INTEL_OUTPUT_EDP:
crtc_mask = (1 << 1);
- clone_mask = (1 << INTEL_OUTPUT_EDP);
+ clone_mask = 0;
}
encoder->possible_crtcs = crtc_mask;
encoder->possible_clones =
diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
index d4c65f268922..187817e0c004 100644
--- a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
+++ b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
@@ -1006,10 +1006,10 @@ struct mdfld_dsi_encoder *mdfld_dsi_dpi_init(struct drm_device *dev,
/*set possible crtcs and clones*/
if (dsi_connector->pipe) {
encoder->possible_crtcs = (1 << 2);
- encoder->possible_clones = (1 << 1);
+ encoder->possible_clones = 0;
} else {
encoder->possible_crtcs = (1 << 0);
- encoder->possible_clones = (1 << 0);
+ encoder->possible_clones = 0;
}
dsi_connector->base.encoder = &dpi_output->base.base;
--
2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Intel-gfx] [PATCH v3 3/7] drm/exynos: Use drm_encoder_mask()
2020-02-11 16:22 [Intel-gfx] [PATCH v3 0/7] drm: Try to fix encoder possible_clones/crtc Ville Syrjala
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 1/7] drm: Include the encoder itself in possible_clones Ville Syrjala
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 2/7] drm/gma500: Sanitize possible_clones Ville Syrjala
@ 2020-02-11 16:22 ` Ville Syrjala
2020-02-17 2:27 ` Inki Dae
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 4/7] drm/imx: Remove the bogus possible_clones setup Ville Syrjala
` (4 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Ville Syrjala @ 2020-02-11 16:22 UTC (permalink / raw)
To: dri-devel
Cc: Joonyoung Shim, intel-gfx, Seung-Woo Kim, Inki Dae,
Kyungmin Park, Thomas Zimmermann
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Replace the hand rolled encoder bitmask thing with drm_encoder_mask()
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index ba0f868b2477..57defeb44522 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -270,7 +270,7 @@ static int exynos_drm_bind(struct device *dev)
struct drm_encoder *encoder;
struct drm_device *drm;
unsigned int clone_mask;
- int cnt, ret;
+ int ret;
drm = drm_dev_alloc(&exynos_drm_driver, dev);
if (IS_ERR(drm))
@@ -293,10 +293,9 @@ static int exynos_drm_bind(struct device *dev)
exynos_drm_mode_config_init(drm);
/* setup possible_clones. */
- cnt = 0;
clone_mask = 0;
list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
- clone_mask |= (1 << (cnt++));
+ clone_mask |= drm_encoder_mask(encoder);
list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
encoder->possible_clones = clone_mask;
--
2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 3/7] drm/exynos: Use drm_encoder_mask()
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 3/7] drm/exynos: Use drm_encoder_mask() Ville Syrjala
@ 2020-02-17 2:27 ` Inki Dae
0 siblings, 0 replies; 28+ messages in thread
From: Inki Dae @ 2020-02-17 2:27 UTC (permalink / raw)
To: Ville Syrjala, dri-devel
Cc: Kyungmin Park, intel-gfx, Seung-Woo Kim, Joonyoung Shim,
Thomas Zimmermann
20. 2. 12. 오전 1:22에 Ville Syrjala 이(가) 쓴 글:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the hand rolled encoder bitmask thing with drm_encoder_mask()
>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
THanks,
Inki Dae
> ---
> drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index ba0f868b2477..57defeb44522 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -270,7 +270,7 @@ static int exynos_drm_bind(struct device *dev)
> struct drm_encoder *encoder;
> struct drm_device *drm;
> unsigned int clone_mask;
> - int cnt, ret;
> + int ret;
>
> drm = drm_dev_alloc(&exynos_drm_driver, dev);
> if (IS_ERR(drm))
> @@ -293,10 +293,9 @@ static int exynos_drm_bind(struct device *dev)
> exynos_drm_mode_config_init(drm);
>
> /* setup possible_clones. */
> - cnt = 0;
> clone_mask = 0;
> list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
> - clone_mask |= (1 << (cnt++));
> + clone_mask |= drm_encoder_mask(encoder);
>
> list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
> encoder->possible_clones = clone_mask;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Intel-gfx] [PATCH v3 4/7] drm/imx: Remove the bogus possible_clones setup
2020-02-11 16:22 [Intel-gfx] [PATCH v3 0/7] drm: Try to fix encoder possible_clones/crtc Ville Syrjala
` (2 preceding siblings ...)
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 3/7] drm/exynos: Use drm_encoder_mask() Ville Syrjala
@ 2020-02-11 16:22 ` Ville Syrjala
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 5/7] drm: Validate encoder->possible_clones Ville Syrjala
` (3 subsequent siblings)
7 siblings, 0 replies; 28+ messages in thread
From: Ville Syrjala @ 2020-02-11 16:22 UTC (permalink / raw)
To: dri-devel; +Cc: Daniel Vetter, intel-gfx, Philipp Zabel
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
It's not at all clear what cloning options this driver supports.
So let's just clear possible_clones instead of setting it to some
bogus value.
v2: Adjust the FIXME (Daniel)
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index da87c70e413b..9e8e0b703498 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -139,8 +139,8 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
encoder->possible_crtcs = crtc_mask;
- /* FIXME: this is the mask of outputs which can clone this output. */
- encoder->possible_clones = ~0;
+ /* FIXME: cloning support not clear, disable it all for now */
+ encoder->possible_clones = 0;
return 0;
}
--
2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Intel-gfx] [PATCH v3 5/7] drm: Validate encoder->possible_clones
2020-02-11 16:22 [Intel-gfx] [PATCH v3 0/7] drm: Try to fix encoder possible_clones/crtc Ville Syrjala
` (3 preceding siblings ...)
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 4/7] drm/imx: Remove the bogus possible_clones setup Ville Syrjala
@ 2020-02-11 16:22 ` Ville Syrjala
2020-02-11 17:02 ` Daniel Vetter
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs Ville Syrjala
` (2 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Ville Syrjala @ 2020-02-11 16:22 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Thomas Zimmermann
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Many drivers are populating encoder->possible_clones wrong. Let's
persuade them to get it right by adding some loud WARNs.
We'll cross check the bits between any two encoders. So either
both encoders can clone with the other, or neither can.
We'll also complain about effectively empty possible_clones, and
possible_clones containing bits for encoders that don't exist.
v2: encoder->possible_clones now includes the encoder itelf
v3: Move to drm_mode_config_validate() (Daniel)
Document that you get a WARN when this is wrong (Daniel)
Extract full_encoder_mask()
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/drm_mode_config.c | 40 +++++++++++++++++++++++++++++++
include/drm/drm_encoder.h | 2 ++
2 files changed, 42 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 75e357c7e84d..afc91447293a 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -533,6 +533,17 @@ void drm_mode_config_cleanup(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_mode_config_cleanup);
+static u32 full_encoder_mask(struct drm_device *dev)
+{
+ struct drm_encoder *encoder;
+ u32 encoder_mask = 0;
+
+ drm_for_each_encoder(encoder, dev)
+ encoder_mask |= drm_encoder_mask(encoder);
+
+ return encoder_mask;
+}
+
/*
* For some reason we want the encoder itself included in
* possible_clones. Make life easy for drivers by allowing them
@@ -544,10 +555,39 @@ static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
encoder->possible_clones = drm_encoder_mask(encoder);
}
+static void validate_encoder_possible_clones(struct drm_encoder *encoder)
+{
+ struct drm_device *dev = encoder->dev;
+ u32 encoder_mask = full_encoder_mask(dev);
+ struct drm_encoder *other;
+
+ drm_for_each_encoder(other, dev) {
+ WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
+ !(other->possible_clones & drm_encoder_mask(encoder)),
+ "possible_clones mismatch: "
+ "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
+ "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
+ encoder->base.id, encoder->name,
+ drm_encoder_mask(encoder), encoder->possible_clones,
+ other->base.id, other->name,
+ drm_encoder_mask(other), other->possible_clones);
+ }
+
+ WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
+ (encoder->possible_clones & ~encoder_mask) != 0,
+ "Bogus possible_clones: "
+ "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
+ encoder->base.id, encoder->name,
+ encoder->possible_clones, encoder_mask);
+}
+
void drm_mode_config_validate(struct drm_device *dev)
{
struct drm_encoder *encoder;
drm_for_each_encoder(encoder, dev)
fixup_encoder_possible_clones(encoder);
+
+ drm_for_each_encoder(encoder, dev)
+ validate_encoder_possible_clones(encoder);
}
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 22d6cdf729f1..3741963b9587 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -163,6 +163,8 @@ struct drm_encoder {
* any cloning it can leave @possible_clones set to 0. The core will
* automagically fix this up by setting the bit for the encoder itself.
*
+ * You will get a WARN if you get this wrong in the driver.
+ *
* Note that since encoder objects can't be hotplugged the assigned indices
* are stable and hence known before registering all objects.
*/
--
2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 5/7] drm: Validate encoder->possible_clones
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 5/7] drm: Validate encoder->possible_clones Ville Syrjala
@ 2020-02-11 17:02 ` Daniel Vetter
2020-02-11 17:13 ` Ville Syrjälä
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2020-02-11 17:02 UTC (permalink / raw)
To: Ville Syrjala; +Cc: intel-gfx, Thomas Zimmermann, dri-devel
On Tue, Feb 11, 2020 at 06:22:06PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Many drivers are populating encoder->possible_clones wrong. Let's
> persuade them to get it right by adding some loud WARNs.
>
> We'll cross check the bits between any two encoders. So either
> both encoders can clone with the other, or neither can.
>
> We'll also complain about effectively empty possible_clones, and
> possible_clones containing bits for encoders that don't exist.
>
> v2: encoder->possible_clones now includes the encoder itelf
> v3: Move to drm_mode_config_validate() (Daniel)
> Document that you get a WARN when this is wrong (Daniel)
> Extract full_encoder_mask()
>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
I wonder whether we should start to have some unit tests for stuff like
this, like set up broken driver, make sure we have a WARN in dmesg. But
ideally we'd do that with the mocking stuff Kunit hopefully has soon.
</idle musings>
> ---
> drivers/gpu/drm/drm_mode_config.c | 40 +++++++++++++++++++++++++++++++
> include/drm/drm_encoder.h | 2 ++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 75e357c7e84d..afc91447293a 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -533,6 +533,17 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_mode_config_cleanup);
>
> +static u32 full_encoder_mask(struct drm_device *dev)
> +{
> + struct drm_encoder *encoder;
> + u32 encoder_mask = 0;
> +
> + drm_for_each_encoder(encoder, dev)
> + encoder_mask |= drm_encoder_mask(encoder);
> +
> + return encoder_mask;
> +}
> +
> /*
> * For some reason we want the encoder itself included in
> * possible_clones. Make life easy for drivers by allowing them
> @@ -544,10 +555,39 @@ static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
> encoder->possible_clones = drm_encoder_mask(encoder);
> }
>
> +static void validate_encoder_possible_clones(struct drm_encoder *encoder)
> +{
> + struct drm_device *dev = encoder->dev;
> + u32 encoder_mask = full_encoder_mask(dev);
> + struct drm_encoder *other;
> +
> + drm_for_each_encoder(other, dev) {
> + WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
> + !(other->possible_clones & drm_encoder_mask(encoder)),
Bikeshed: !! as canonical "make this a bool value" might be slightly
clearer, but whatever.
> + "possible_clones mismatch: "
> + "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
> + "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
> + encoder->base.id, encoder->name,
> + drm_encoder_mask(encoder), encoder->possible_clones,
> + other->base.id, other->name,
> + drm_encoder_mask(other), other->possible_clones);
> + }
> +
> + WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
> + (encoder->possible_clones & ~encoder_mask) != 0,
> + "Bogus possible_clones: "
> + "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
> + encoder->base.id, encoder->name,
> + encoder->possible_clones, encoder_mask);
> +}
Since it's next to each another double-checking that the fixup did add the
self-clone is probably too much :-)
> +
> void drm_mode_config_validate(struct drm_device *dev)
> {
> struct drm_encoder *encoder;
>
> drm_for_each_encoder(encoder, dev)
> fixup_encoder_possible_clones(encoder);
> +
> + drm_for_each_encoder(encoder, dev)
> + validate_encoder_possible_clones(encoder);
> }
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 22d6cdf729f1..3741963b9587 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -163,6 +163,8 @@ struct drm_encoder {
> * any cloning it can leave @possible_clones set to 0. The core will
> * automagically fix this up by setting the bit for the encoder itself.
> *
> + * You will get a WARN if you get this wrong in the driver.
Nice.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> + *
> * Note that since encoder objects can't be hotplugged the assigned indices
> * are stable and hence known before registering all objects.
> */
> --
> 2.24.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 5/7] drm: Validate encoder->possible_clones
2020-02-11 17:02 ` Daniel Vetter
@ 2020-02-11 17:13 ` Ville Syrjälä
2020-02-12 8:56 ` Daniel Vetter
0 siblings, 1 reply; 28+ messages in thread
From: Ville Syrjälä @ 2020-02-11 17:13 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Thomas Zimmermann, dri-devel
On Tue, Feb 11, 2020 at 06:02:33PM +0100, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 06:22:06PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Many drivers are populating encoder->possible_clones wrong. Let's
> > persuade them to get it right by adding some loud WARNs.
> >
> > We'll cross check the bits between any two encoders. So either
> > both encoders can clone with the other, or neither can.
> >
> > We'll also complain about effectively empty possible_clones, and
> > possible_clones containing bits for encoders that don't exist.
> >
> > v2: encoder->possible_clones now includes the encoder itelf
> > v3: Move to drm_mode_config_validate() (Daniel)
> > Document that you get a WARN when this is wrong (Daniel)
> > Extract full_encoder_mask()
> >
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> I wonder whether we should start to have some unit tests for stuff like
> this, like set up broken driver, make sure we have a WARN in dmesg. But
> ideally we'd do that with the mocking stuff Kunit hopefully has soon.
>
> </idle musings>
>
>
> > ---
> > drivers/gpu/drm/drm_mode_config.c | 40 +++++++++++++++++++++++++++++++
> > include/drm/drm_encoder.h | 2 ++
> > 2 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 75e357c7e84d..afc91447293a 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -533,6 +533,17 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> > }
> > EXPORT_SYMBOL(drm_mode_config_cleanup);
> >
> > +static u32 full_encoder_mask(struct drm_device *dev)
> > +{
> > + struct drm_encoder *encoder;
> > + u32 encoder_mask = 0;
> > +
> > + drm_for_each_encoder(encoder, dev)
> > + encoder_mask |= drm_encoder_mask(encoder);
> > +
> > + return encoder_mask;
> > +}
> > +
> > /*
> > * For some reason we want the encoder itself included in
> > * possible_clones. Make life easy for drivers by allowing them
> > @@ -544,10 +555,39 @@ static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
> > encoder->possible_clones = drm_encoder_mask(encoder);
> > }
> >
> > +static void validate_encoder_possible_clones(struct drm_encoder *encoder)
> > +{
> > + struct drm_device *dev = encoder->dev;
> > + u32 encoder_mask = full_encoder_mask(dev);
> > + struct drm_encoder *other;
> > +
> > + drm_for_each_encoder(other, dev) {
> > + WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
> > + !(other->possible_clones & drm_encoder_mask(encoder)),
>
> Bikeshed: !! as canonical "make this a bool value" might be slightly
> clearer, but whatever.
Can repaint.
>
> > + "possible_clones mismatch: "
> > + "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
> > + "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
> > + encoder->base.id, encoder->name,
> > + drm_encoder_mask(encoder), encoder->possible_clones,
> > + other->base.id, other->name,
> > + drm_encoder_mask(other), other->possible_clones);
> > + }
> > +
> > + WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
> > + (encoder->possible_clones & ~encoder_mask) != 0,
> > + "Bogus possible_clones: "
> > + "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
> > + encoder->base.id, encoder->name,
> > + encoder->possible_clones, encoder_mask);
> > +}
>
> Since it's next to each another double-checking that the fixup did add the
> self-clone is probably too much :-)
I changed the fixup to be just
if (possible_clones == 0)
possible_clones = drm_encoder_mask();
So if the driver tries to set it up but fails and forgets the
encoder itself this WARN will still trip.
>
> > +
> > void drm_mode_config_validate(struct drm_device *dev)
> > {
> > struct drm_encoder *encoder;
> >
> > drm_for_each_encoder(encoder, dev)
> > fixup_encoder_possible_clones(encoder);
> > +
> > + drm_for_each_encoder(encoder, dev)
> > + validate_encoder_possible_clones(encoder);
>
> > }
> > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > index 22d6cdf729f1..3741963b9587 100644
> > --- a/include/drm/drm_encoder.h
> > +++ b/include/drm/drm_encoder.h
> > @@ -163,6 +163,8 @@ struct drm_encoder {
> > * any cloning it can leave @possible_clones set to 0. The core will
> > * automagically fix this up by setting the bit for the encoder itself.
> > *
> > + * You will get a WARN if you get this wrong in the driver.
>
> Nice.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> > + *
> > * Note that since encoder objects can't be hotplugged the assigned indices
> > * are stable and hence known before registering all objects.
> > */
> > --
> > 2.24.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 5/7] drm: Validate encoder->possible_clones
2020-02-11 17:13 ` Ville Syrjälä
@ 2020-02-12 8:56 ` Daniel Vetter
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2020-02-12 8:56 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Thomas Zimmermann, intel-gfx, dri-devel
On Tue, Feb 11, 2020 at 07:13:31PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2020 at 06:02:33PM +0100, Daniel Vetter wrote:
> > On Tue, Feb 11, 2020 at 06:22:06PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Many drivers are populating encoder->possible_clones wrong. Let's
> > > persuade them to get it right by adding some loud WARNs.
> > >
> > > We'll cross check the bits between any two encoders. So either
> > > both encoders can clone with the other, or neither can.
> > >
> > > We'll also complain about effectively empty possible_clones, and
> > > possible_clones containing bits for encoders that don't exist.
> > >
> > > v2: encoder->possible_clones now includes the encoder itelf
> > > v3: Move to drm_mode_config_validate() (Daniel)
> > > Document that you get a WARN when this is wrong (Daniel)
> > > Extract full_encoder_mask()
> > >
> > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > I wonder whether we should start to have some unit tests for stuff like
> > this, like set up broken driver, make sure we have a WARN in dmesg. But
> > ideally we'd do that with the mocking stuff Kunit hopefully has soon.
> >
> > </idle musings>
> >
> >
> > > ---
> > > drivers/gpu/drm/drm_mode_config.c | 40 +++++++++++++++++++++++++++++++
> > > include/drm/drm_encoder.h | 2 ++
> > > 2 files changed, 42 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index 75e357c7e84d..afc91447293a 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -533,6 +533,17 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> > > }
> > > EXPORT_SYMBOL(drm_mode_config_cleanup);
> > >
> > > +static u32 full_encoder_mask(struct drm_device *dev)
> > > +{
> > > + struct drm_encoder *encoder;
> > > + u32 encoder_mask = 0;
> > > +
> > > + drm_for_each_encoder(encoder, dev)
> > > + encoder_mask |= drm_encoder_mask(encoder);
> > > +
> > > + return encoder_mask;
> > > +}
> > > +
> > > /*
> > > * For some reason we want the encoder itself included in
> > > * possible_clones. Make life easy for drivers by allowing them
> > > @@ -544,10 +555,39 @@ static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
> > > encoder->possible_clones = drm_encoder_mask(encoder);
> > > }
> > >
> > > +static void validate_encoder_possible_clones(struct drm_encoder *encoder)
> > > +{
> > > + struct drm_device *dev = encoder->dev;
> > > + u32 encoder_mask = full_encoder_mask(dev);
> > > + struct drm_encoder *other;
> > > +
> > > + drm_for_each_encoder(other, dev) {
> > > + WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
> > > + !(other->possible_clones & drm_encoder_mask(encoder)),
> >
> > Bikeshed: !! as canonical "make this a bool value" might be slightly
> > clearer, but whatever.
>
> Can repaint.
>
> >
> > > + "possible_clones mismatch: "
> > > + "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
> > > + "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
> > > + encoder->base.id, encoder->name,
> > > + drm_encoder_mask(encoder), encoder->possible_clones,
> > > + other->base.id, other->name,
> > > + drm_encoder_mask(other), other->possible_clones);
> > > + }
> > > +
> > > + WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
> > > + (encoder->possible_clones & ~encoder_mask) != 0,
> > > + "Bogus possible_clones: "
> > > + "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
> > > + encoder->base.id, encoder->name,
> > > + encoder->possible_clones, encoder_mask);
> > > +}
> >
> > Since it's next to each another double-checking that the fixup did add the
> > self-clone is probably too much :-)
>
> I changed the fixup to be just
> if (possible_clones == 0)
> possible_clones = drm_encoder_mask();
>
> So if the driver tries to set it up but fails and forgets the
> encoder itself this WARN will still trip.
Duh, I was just blind, everything is looking good.
-Daniel
>
> >
> > > +
> > > void drm_mode_config_validate(struct drm_device *dev)
> > > {
> > > struct drm_encoder *encoder;
> > >
> > > drm_for_each_encoder(encoder, dev)
> > > fixup_encoder_possible_clones(encoder);
> > > +
> > > + drm_for_each_encoder(encoder, dev)
> > > + validate_encoder_possible_clones(encoder);
> >
> > > }
> > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > > index 22d6cdf729f1..3741963b9587 100644
> > > --- a/include/drm/drm_encoder.h
> > > +++ b/include/drm/drm_encoder.h
> > > @@ -163,6 +163,8 @@ struct drm_encoder {
> > > * any cloning it can leave @possible_clones set to 0. The core will
> > > * automagically fix this up by setting the bit for the encoder itself.
> > > *
> > > + * You will get a WARN if you get this wrong in the driver.
> >
> > Nice.
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > > + *
> > > * Note that since encoder objects can't be hotplugged the assigned indices
> > > * are stable and hence known before registering all objects.
> > > */
> > > --
> > > 2.24.1
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-02-11 16:22 [Intel-gfx] [PATCH v3 0/7] drm: Try to fix encoder possible_clones/crtc Ville Syrjala
` (4 preceding siblings ...)
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 5/7] drm: Validate encoder->possible_clones Ville Syrjala
@ 2020-02-11 16:22 ` Ville Syrjala
2020-02-11 17:04 ` Daniel Vetter
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0 Ville Syrjala
2020-12-03 22:16 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm: Try to fix encoder possible_clones/crtc (rev4) Patchwork
7 siblings, 1 reply; 28+ messages in thread
From: Ville Syrjala @ 2020-02-11 16:22 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Thomas Zimmermann
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
WARN if the encoder possible_crtcs is effectively empty or contains
bits for non-existing crtcs.
v2: Move to drm_mode_config_validate() (Daniel)
Make the docs say we WARN when this is wrong (Daniel)
Extract full_crtc_mask()
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/drm_mode_config.c | 27 ++++++++++++++++++++++++++-
include/drm/drm_encoder.h | 2 +-
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index afc91447293a..4c1b350ddb95 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -581,6 +581,29 @@ static void validate_encoder_possible_clones(struct drm_encoder *encoder)
encoder->possible_clones, encoder_mask);
}
+static u32 full_crtc_mask(struct drm_device *dev)
+{
+ struct drm_crtc *crtc;
+ u32 crtc_mask = 0;
+
+ drm_for_each_crtc(crtc, dev)
+ crtc_mask |= drm_crtc_mask(crtc);
+
+ return crtc_mask;
+}
+
+static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
+{
+ u32 crtc_mask = full_crtc_mask(encoder->dev);
+
+ WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
+ (encoder->possible_crtcs & ~crtc_mask) != 0,
+ "Bogus possible_crtcs: "
+ "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
+ encoder->base.id, encoder->name,
+ encoder->possible_crtcs, crtc_mask);
+}
+
void drm_mode_config_validate(struct drm_device *dev)
{
struct drm_encoder *encoder;
@@ -588,6 +611,8 @@ void drm_mode_config_validate(struct drm_device *dev)
drm_for_each_encoder(encoder, dev)
fixup_encoder_possible_clones(encoder);
- drm_for_each_encoder(encoder, dev)
+ drm_for_each_encoder(encoder, dev) {
validate_encoder_possible_clones(encoder);
+ validate_encoder_possible_crtcs(encoder);
+ }
}
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 3741963b9587..b236269f41ac 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -142,7 +142,7 @@ struct drm_encoder {
* the bits for all &drm_crtc objects this encoder can be connected to
* before calling drm_dev_register().
*
- * In reality almost every driver gets this wrong.
+ * You will get a WARN if you get this wrong in the driver.
*
* Note that since CRTC objects can't be hotplugged the assigned indices
* are stable and hence known before registering all objects.
--
2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs Ville Syrjala
@ 2020-02-11 17:04 ` Daniel Vetter
2020-09-06 11:19 ` Jan Kiszka
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2020-02-11 17:04 UTC (permalink / raw)
To: Ville Syrjala; +Cc: intel-gfx, Thomas Zimmermann, dri-devel
On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> WARN if the encoder possible_crtcs is effectively empty or contains
> bits for non-existing crtcs.
>
> v2: Move to drm_mode_config_validate() (Daniel)
> Make the docs say we WARN when this is wrong (Daniel)
> Extract full_crtc_mask()
>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
When pushing the fixup needs to be applied before the validation patch
here, because we don't want to anger the bisect gods.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I think with the fixup we should be good enough with the existing nonsense
in drivers. Fingers crossed.
-Daniel
> ---
> drivers/gpu/drm/drm_mode_config.c | 27 ++++++++++++++++++++++++++-
> include/drm/drm_encoder.h | 2 +-
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index afc91447293a..4c1b350ddb95 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -581,6 +581,29 @@ static void validate_encoder_possible_clones(struct drm_encoder *encoder)
> encoder->possible_clones, encoder_mask);
> }
>
> +static u32 full_crtc_mask(struct drm_device *dev)
> +{
> + struct drm_crtc *crtc;
> + u32 crtc_mask = 0;
> +
> + drm_for_each_crtc(crtc, dev)
> + crtc_mask |= drm_crtc_mask(crtc);
> +
> + return crtc_mask;
> +}
> +
> +static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
> +{
> + u32 crtc_mask = full_crtc_mask(encoder->dev);
> +
> + WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> + (encoder->possible_crtcs & ~crtc_mask) != 0,
> + "Bogus possible_crtcs: "
> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> + encoder->base.id, encoder->name,
> + encoder->possible_crtcs, crtc_mask);
> +}
> +
> void drm_mode_config_validate(struct drm_device *dev)
> {
> struct drm_encoder *encoder;
> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct drm_device *dev)
> drm_for_each_encoder(encoder, dev)
> fixup_encoder_possible_clones(encoder);
>
> - drm_for_each_encoder(encoder, dev)
> + drm_for_each_encoder(encoder, dev) {
> validate_encoder_possible_clones(encoder);
> + validate_encoder_possible_crtcs(encoder);
> + }
> }
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 3741963b9587..b236269f41ac 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -142,7 +142,7 @@ struct drm_encoder {
> * the bits for all &drm_crtc objects this encoder can be connected to
> * before calling drm_dev_register().
> *
> - * In reality almost every driver gets this wrong.
> + * You will get a WARN if you get this wrong in the driver.
> *
> * Note that since CRTC objects can't be hotplugged the assigned indices
> * are stable and hence known before registering all objects.
> --
> 2.24.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-02-11 17:04 ` Daniel Vetter
@ 2020-09-06 11:19 ` Jan Kiszka
2020-09-07 7:14 ` Daniel Vetter
0 siblings, 1 reply; 28+ messages in thread
From: Jan Kiszka @ 2020-09-06 11:19 UTC (permalink / raw)
To: Daniel Vetter, Ville Syrjala; +Cc: intel-gfx, dri-devel, Thomas Zimmermann
On 11.02.20 18:04, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> WARN if the encoder possible_crtcs is effectively empty or contains
>> bits for non-existing crtcs.
>>
>> v2: Move to drm_mode_config_validate() (Daniel)
>> Make the docs say we WARN when this is wrong (Daniel)
>> Extract full_crtc_mask()
>>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When pushing the fixup needs to be applied before the validation patch
> here, because we don't want to anger the bisect gods.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> I think with the fixup we should be good enough with the existing nonsense
> in drivers. Fingers crossed.
> -Daniel
>
>
>> ---
>> drivers/gpu/drm/drm_mode_config.c | 27 ++++++++++++++++++++++++++-
>> include/drm/drm_encoder.h | 2 +-
>> 2 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>> index afc91447293a..4c1b350ddb95 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -581,6 +581,29 @@ static void validate_encoder_possible_clones(struct drm_encoder *encoder)
>> encoder->possible_clones, encoder_mask);
>> }
>>
>> +static u32 full_crtc_mask(struct drm_device *dev)
>> +{
>> + struct drm_crtc *crtc;
>> + u32 crtc_mask = 0;
>> +
>> + drm_for_each_crtc(crtc, dev)
>> + crtc_mask |= drm_crtc_mask(crtc);
>> +
>> + return crtc_mask;
>> +}
>> +
>> +static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
>> +{
>> + u32 crtc_mask = full_crtc_mask(encoder->dev);
>> +
>> + WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
>> + (encoder->possible_crtcs & ~crtc_mask) != 0,
>> + "Bogus possible_crtcs: "
>> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
>> + encoder->base.id, encoder->name,
>> + encoder->possible_crtcs, crtc_mask);
>> +}
>> +
>> void drm_mode_config_validate(struct drm_device *dev)
>> {
>> struct drm_encoder *encoder;
>> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct drm_device *dev)
>> drm_for_each_encoder(encoder, dev)
>> fixup_encoder_possible_clones(encoder);
>>
>> - drm_for_each_encoder(encoder, dev)
>> + drm_for_each_encoder(encoder, dev) {
>> validate_encoder_possible_clones(encoder);
>> + validate_encoder_possible_crtcs(encoder);
>> + }
>> }
>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
>> index 3741963b9587..b236269f41ac 100644
>> --- a/include/drm/drm_encoder.h
>> +++ b/include/drm/drm_encoder.h
>> @@ -142,7 +142,7 @@ struct drm_encoder {
>> * the bits for all &drm_crtc objects this encoder can be connected to
>> * before calling drm_dev_register().
>> *
>> - * In reality almost every driver gets this wrong.
>> + * You will get a WARN if you get this wrong in the driver.
>> *
>> * Note that since CRTC objects can't be hotplugged the assigned indices
>> * are stable and hence known before registering all objects.
>> --
>> 2.24.1
>>
>
Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
[ 14.033246] ------------[ cut here ]------------
[ 14.033248] Bogus possible_crtcs: [ENCODER:65:TMDS-65] possible_crtcs=0xf (full crtc mask=0x7)
[ 14.033279] WARNING: CPU: 0 PID: 282 at ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 [drm]
[ 14.033279] Modules linked in: amdgpu(E+) mfd_core(E) snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
[ 14.033306] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
[ 14.033310] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G E 5.9.0-rc3+ #2
[ 14.033311] Hardware name: Default string Default string/Default string, BIOS 5.0.1.4 02/14/2020
[ 14.033324] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
[ 14.033327] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d
[ 14.033328] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282
[ 14.033329] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX: 0000000000000027
[ 14.033330] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff9c6910a18ac8
[ 14.033331] RBP: 0000000000000001 R08: 0000000000000064 R09: ffffffff9784a724
[ 14.033331] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c690bf8b800
[ 14.033332] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15: 000000000000003f
[ 14.033333] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000) knlGS:0000000000000000
[ 14.033334] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 14.033335] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4: 00000000003506f0
[ 14.033335] Call Trace:
[ 14.033350] drm_dev_register+0x117/0x1e0 [drm]
[ 14.033423] amdgpu_pci_probe+0x134/0x200 [amdgpu]
[ 14.033428] local_pci_probe+0x42/0x90
[ 14.033430] pci_device_probe+0x108/0x1c0
[ 14.033433] really_probe+0xef/0x4a0
[ 14.033435] driver_probe_device+0xde/0x150
[ 14.033436] device_driver_attach+0x4f/0x60
[ 14.033438] __driver_attach+0x9a/0x140
[ 14.033439] ? device_driver_attach+0x60/0x60
[ 14.033441] bus_for_each_dev+0x76/0xc0
[ 14.033443] ? klist_add_tail+0x3b/0x70
[ 14.033445] bus_add_driver+0x144/0x220
[ 14.033446] ? 0xffffffffc0949000
[ 14.033447] driver_register+0x5b/0xf0
[ 14.033448] ? 0xffffffffc0949000
[ 14.033451] do_one_initcall+0x46/0x1f4
[ 14.033454] ? _cond_resched+0x15/0x40
[ 14.033456] ? kmem_cache_alloc_trace+0x40/0x440
[ 14.033459] ? do_init_module+0x22/0x213
[ 14.033460] do_init_module+0x5b/0x213
[ 14.033462] load_module+0x258c/0x2d30
[ 14.033465] ? __kernel_read+0xf5/0x160
[ 14.033467] ? __do_sys_finit_module+0xe9/0x110
[ 14.033468] __do_sys_finit_module+0xe9/0x110
[ 14.033471] do_syscall_64+0x33/0x80
[ 14.033473] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 14.033474] RIP: 0033:0x7feacf1bef59
[ 14.033477] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
[ 14.033477] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[ 14.033478] RAX: ffffffffffffffda RBX: 0000565489749410 RCX: 00007feacf1bef59
[ 14.033479] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI: 0000000000000015
[ 14.033479] RBP: 00007feacf0c3cad R08: 0000000000000000 R09: 0000000000000000
[ 14.033480] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000
[ 14.033480] R13: 00005654897472c0 R14: 0000000000020000 R15: 0000565489749410
[ 14.033482] ---[ end trace 16aeaa08847a13d8 ]---
[ 14.033483] ------------[ cut here ]------------
[ 14.033484] Bogus possible_crtcs: [ENCODER:69:TMDS-69] possible_crtcs=0xf (full crtc mask=0x7)
[ 14.033507] WARNING: CPU: 0 PID: 282 at ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 [drm]
[ 14.033507] Modules linked in: amdgpu(E+) mfd_core(E) snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
[ 14.033522] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
[ 14.033524] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G W E 5.9.0-rc3+ #2
[ 14.033525] Hardware name: Default string Default string/Default string, BIOS 5.0.1.4 02/14/2020
[ 14.033538] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
[ 14.033539] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d
[ 14.033540] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282
[ 14.033540] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX: 0000000000000027
[ 14.033541] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff9c6910a18ac8
[ 14.033542] RBP: 0000000000000001 R08: 0000000000000064 R09: ffffffff9784a724
[ 14.033542] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c690bf89800
[ 14.033543] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15: 000000000000003f
[ 14.033544] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000) knlGS:0000000000000000
[ 14.033544] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 14.033545] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4: 00000000003506f0
[ 14.033545] Call Trace:
[ 14.033557] drm_dev_register+0x117/0x1e0 [drm]
[ 14.033625] amdgpu_pci_probe+0x134/0x200 [amdgpu]
[ 14.033627] local_pci_probe+0x42/0x90
[ 14.033629] pci_device_probe+0x108/0x1c0
[ 14.033630] really_probe+0xef/0x4a0
[ 14.033632] driver_probe_device+0xde/0x150
[ 14.033633] device_driver_attach+0x4f/0x60
[ 14.033634] __driver_attach+0x9a/0x140
[ 14.033635] ? device_driver_attach+0x60/0x60
[ 14.033636] bus_for_each_dev+0x76/0xc0
[ 14.033638] ? klist_add_tail+0x3b/0x70
[ 14.033639] bus_add_driver+0x144/0x220
[ 14.033640] ? 0xffffffffc0949000
[ 14.033641] driver_register+0x5b/0xf0
[ 14.033642] ? 0xffffffffc0949000
[ 14.033643] do_one_initcall+0x46/0x1f4
[ 14.033645] ? _cond_resched+0x15/0x40
[ 14.033646] ? kmem_cache_alloc_trace+0x40/0x440
[ 14.033648] ? do_init_module+0x22/0x213
[ 14.033649] do_init_module+0x5b/0x213
[ 14.033650] load_module+0x258c/0x2d30
[ 14.033652] ? __kernel_read+0xf5/0x160
[ 14.033654] ? __do_sys_finit_module+0xe9/0x110
[ 14.033655] __do_sys_finit_module+0xe9/0x110
[ 14.033657] do_syscall_64+0x33/0x80
[ 14.033659] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 14.033660] RIP: 0033:0x7feacf1bef59
[ 14.033661] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
[ 14.033662] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[ 14.033663] RAX: ffffffffffffffda RBX: 0000565489749410 RCX: 00007feacf1bef59
[ 14.033663] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI: 0000000000000015
[ 14.033664] RBP: 00007feacf0c3cad R08: 0000000000000000 R09: 0000000000000000
[ 14.033664] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000
[ 14.033665] R13: 00005654897472c0 R14: 0000000000020000 R15: 0000565489749410
[ 14.033666] ---[ end trace 16aeaa08847a13d9 ]---
[ 14.033667] ------------[ cut here ]------------
[ 14.033668] Bogus possible_crtcs: [ENCODER:73:TMDS-73] possible_crtcs=0xf (full crtc mask=0x7)
[ 14.033690] WARNING: CPU: 0 PID: 282 at ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 [drm]
[ 14.033690] Modules linked in: amdgpu(E+) mfd_core(E) snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
[ 14.033704] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
[ 14.033706] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G W E 5.9.0-rc3+ #2
[ 14.033707] Hardware name: Default string Default string/Default string, BIOS 5.0.1.4 02/14/2020
[ 14.033719] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
[ 14.033721] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d
[ 14.033721] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282
[ 14.033722] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX: 0000000000000027
[ 14.033723] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff9c6910a18ac8
[ 14.033723] RBP: 0000000000000001 R08: 0000000000000064 R09: ffffffff9784a724
[ 14.033724] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c690bf7c000
[ 14.033724] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15: 000000000000003f
[ 14.033725] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000) knlGS:0000000000000000
[ 14.033726] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 14.033726] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4: 00000000003506f0
[ 14.033727] Call Trace:
[ 14.033739] drm_dev_register+0x117/0x1e0 [drm]
[ 14.033806] amdgpu_pci_probe+0x134/0x200 [amdgpu]
[ 14.033808] local_pci_probe+0x42/0x90
[ 14.033810] pci_device_probe+0x108/0x1c0
[ 14.033811] really_probe+0xef/0x4a0
[ 14.033813] driver_probe_device+0xde/0x150
[ 14.033814] device_driver_attach+0x4f/0x60
[ 14.033815] __driver_attach+0x9a/0x140
[ 14.033816] ? device_driver_attach+0x60/0x60
[ 14.033817] bus_for_each_dev+0x76/0xc0
[ 14.033818] ? klist_add_tail+0x3b/0x70
[ 14.033820] bus_add_driver+0x144/0x220
[ 14.033821] ? 0xffffffffc0949000
[ 14.033822] driver_register+0x5b/0xf0
[ 14.033823] ? 0xffffffffc0949000
[ 14.033824] do_one_initcall+0x46/0x1f4
[ 14.033825] ? _cond_resched+0x15/0x40
[ 14.033827] ? kmem_cache_alloc_trace+0x40/0x440
[ 14.033828] ? do_init_module+0x22/0x213
[ 14.033829] do_init_module+0x5b/0x213
[ 14.033831] load_module+0x258c/0x2d30
[ 14.033833] ? __kernel_read+0xf5/0x160
[ 14.033834] ? __do_sys_finit_module+0xe9/0x110
[ 14.033835] __do_sys_finit_module+0xe9/0x110
[ 14.033838] do_syscall_64+0x33/0x80
[ 14.033839] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 14.033840] RIP: 0033:0x7feacf1bef59
[ 14.033841] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
[ 14.033842] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[ 14.033843] RAX: ffffffffffffffda RBX: 0000565489749410 RCX: 00007feacf1bef59
[ 14.033843] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI: 0000000000000015
[ 14.033844] RBP: 00007feacf0c3cad R08: 0000000000000000 R09: 0000000000000000
[ 14.033844] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000
[ 14.033845] R13: 00005654897472c0 R14: 0000000000020000 R15: 0000565489749410
[ 14.033846] ---[ end trace 16aeaa08847a13da ]---
Probably the same issue as in
https://bugzilla.kernel.org/show_bug.cgi?id=209123. What does it
practically mean? Can/should it be silenced in this setup?
Jan
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-09-06 11:19 ` Jan Kiszka
@ 2020-09-07 7:14 ` Daniel Vetter
2020-09-10 18:18 ` Deucher, Alexander
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2020-09-07 7:14 UTC (permalink / raw)
To: Jan Kiszka, amd-gfx list, Wentland, Harry, Kazlauskas, Nicholas
Cc: dri-devel, intel-gfx, Thomas Zimmermann
On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka <jan.kiszka@web.de> wrote:
>
> On 11.02.20 18:04, Daniel Vetter wrote:
> > On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> WARN if the encoder possible_crtcs is effectively empty or contains
> >> bits for non-existing crtcs.
> >>
> >> v2: Move to drm_mode_config_validate() (Daniel)
> >> Make the docs say we WARN when this is wrong (Daniel)
> >> Extract full_crtc_mask()
> >>
> >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > When pushing the fixup needs to be applied before the validation patch
> > here, because we don't want to anger the bisect gods.
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > I think with the fixup we should be good enough with the existing nonsense
> > in drivers. Fingers crossed.
> > -Daniel
> >
> >
> >> ---
> >> drivers/gpu/drm/drm_mode_config.c | 27 ++++++++++++++++++++++++++-
> >> include/drm/drm_encoder.h | 2 +-
> >> 2 files changed, 27 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> >> index afc91447293a..4c1b350ddb95 100644
> >> --- a/drivers/gpu/drm/drm_mode_config.c
> >> +++ b/drivers/gpu/drm/drm_mode_config.c
> >> @@ -581,6 +581,29 @@ static void validate_encoder_possible_clones(struct drm_encoder *encoder)
> >> encoder->possible_clones, encoder_mask);
> >> }
> >>
> >> +static u32 full_crtc_mask(struct drm_device *dev)
> >> +{
> >> + struct drm_crtc *crtc;
> >> + u32 crtc_mask = 0;
> >> +
> >> + drm_for_each_crtc(crtc, dev)
> >> + crtc_mask |= drm_crtc_mask(crtc);
> >> +
> >> + return crtc_mask;
> >> +}
> >> +
> >> +static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
> >> +{
> >> + u32 crtc_mask = full_crtc_mask(encoder->dev);
> >> +
> >> + WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> >> + (encoder->possible_crtcs & ~crtc_mask) != 0,
> >> + "Bogus possible_crtcs: "
> >> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> >> + encoder->base.id, encoder->name,
> >> + encoder->possible_crtcs, crtc_mask);
> >> +}
> >> +
> >> void drm_mode_config_validate(struct drm_device *dev)
> >> {
> >> struct drm_encoder *encoder;
> >> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct drm_device *dev)
> >> drm_for_each_encoder(encoder, dev)
> >> fixup_encoder_possible_clones(encoder);
> >>
> >> - drm_for_each_encoder(encoder, dev)
> >> + drm_for_each_encoder(encoder, dev) {
> >> validate_encoder_possible_clones(encoder);
> >> + validate_encoder_possible_crtcs(encoder);
> >> + }
> >> }
> >> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> >> index 3741963b9587..b236269f41ac 100644
> >> --- a/include/drm/drm_encoder.h
> >> +++ b/include/drm/drm_encoder.h
> >> @@ -142,7 +142,7 @@ struct drm_encoder {
> >> * the bits for all &drm_crtc objects this encoder can be connected to
> >> * before calling drm_dev_register().
> >> *
> >> - * In reality almost every driver gets this wrong.
> >> + * You will get a WARN if you get this wrong in the driver.
> >> *
> >> * Note that since CRTC objects can't be hotplugged the assigned indices
> >> * are stable and hence known before registering all objects.
> >> --
> >> 2.24.1
> >>
> >
>
> Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
Adding amdgpu display folks.
-Daniel
>
> [ 14.033246] ------------[ cut here ]------------
> [ 14.033248] Bogus possible_crtcs: [ENCODER:65:TMDS-65] possible_crtcs=0xf (full crtc mask=0x7)
> [ 14.033279] WARNING: CPU: 0 PID: 282 at ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 [drm]
> [ 14.033279] Modules linked in: amdgpu(E+) mfd_core(E) snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
> [ 14.033306] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
> [ 14.033310] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G E 5.9.0-rc3+ #2
> [ 14.033311] Hardware name: Default string Default string/Default string, BIOS 5.0.1.4 02/14/2020
> [ 14.033324] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
> [ 14.033327] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d
> [ 14.033328] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282
> [ 14.033329] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX: 0000000000000027
> [ 14.033330] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff9c6910a18ac8
> [ 14.033331] RBP: 0000000000000001 R08: 0000000000000064 R09: ffffffff9784a724
> [ 14.033331] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c690bf8b800
> [ 14.033332] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15: 000000000000003f
> [ 14.033333] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000) knlGS:0000000000000000
> [ 14.033334] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 14.033335] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4: 00000000003506f0
> [ 14.033335] Call Trace:
> [ 14.033350] drm_dev_register+0x117/0x1e0 [drm]
> [ 14.033423] amdgpu_pci_probe+0x134/0x200 [amdgpu]
> [ 14.033428] local_pci_probe+0x42/0x90
> [ 14.033430] pci_device_probe+0x108/0x1c0
> [ 14.033433] really_probe+0xef/0x4a0
> [ 14.033435] driver_probe_device+0xde/0x150
> [ 14.033436] device_driver_attach+0x4f/0x60
> [ 14.033438] __driver_attach+0x9a/0x140
> [ 14.033439] ? device_driver_attach+0x60/0x60
> [ 14.033441] bus_for_each_dev+0x76/0xc0
> [ 14.033443] ? klist_add_tail+0x3b/0x70
> [ 14.033445] bus_add_driver+0x144/0x220
> [ 14.033446] ? 0xffffffffc0949000
> [ 14.033447] driver_register+0x5b/0xf0
> [ 14.033448] ? 0xffffffffc0949000
> [ 14.033451] do_one_initcall+0x46/0x1f4
> [ 14.033454] ? _cond_resched+0x15/0x40
> [ 14.033456] ? kmem_cache_alloc_trace+0x40/0x440
> [ 14.033459] ? do_init_module+0x22/0x213
> [ 14.033460] do_init_module+0x5b/0x213
> [ 14.033462] load_module+0x258c/0x2d30
> [ 14.033465] ? __kernel_read+0xf5/0x160
> [ 14.033467] ? __do_sys_finit_module+0xe9/0x110
> [ 14.033468] __do_sys_finit_module+0xe9/0x110
> [ 14.033471] do_syscall_64+0x33/0x80
> [ 14.033473] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 14.033474] RIP: 0033:0x7feacf1bef59
> [ 14.033477] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
> [ 14.033477] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [ 14.033478] RAX: ffffffffffffffda RBX: 0000565489749410 RCX: 00007feacf1bef59
> [ 14.033479] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI: 0000000000000015
> [ 14.033479] RBP: 00007feacf0c3cad R08: 0000000000000000 R09: 0000000000000000
> [ 14.033480] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000
> [ 14.033480] R13: 00005654897472c0 R14: 0000000000020000 R15: 0000565489749410
> [ 14.033482] ---[ end trace 16aeaa08847a13d8 ]---
> [ 14.033483] ------------[ cut here ]------------
> [ 14.033484] Bogus possible_crtcs: [ENCODER:69:TMDS-69] possible_crtcs=0xf (full crtc mask=0x7)
> [ 14.033507] WARNING: CPU: 0 PID: 282 at ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 [drm]
> [ 14.033507] Modules linked in: amdgpu(E+) mfd_core(E) snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
> [ 14.033522] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
> [ 14.033524] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G W E 5.9.0-rc3+ #2
> [ 14.033525] Hardware name: Default string Default string/Default string, BIOS 5.0.1.4 02/14/2020
> [ 14.033538] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
> [ 14.033539] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d
> [ 14.033540] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282
> [ 14.033540] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX: 0000000000000027
> [ 14.033541] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff9c6910a18ac8
> [ 14.033542] RBP: 0000000000000001 R08: 0000000000000064 R09: ffffffff9784a724
> [ 14.033542] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c690bf89800
> [ 14.033543] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15: 000000000000003f
> [ 14.033544] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000) knlGS:0000000000000000
> [ 14.033544] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 14.033545] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4: 00000000003506f0
> [ 14.033545] Call Trace:
> [ 14.033557] drm_dev_register+0x117/0x1e0 [drm]
> [ 14.033625] amdgpu_pci_probe+0x134/0x200 [amdgpu]
> [ 14.033627] local_pci_probe+0x42/0x90
> [ 14.033629] pci_device_probe+0x108/0x1c0
> [ 14.033630] really_probe+0xef/0x4a0
> [ 14.033632] driver_probe_device+0xde/0x150
> [ 14.033633] device_driver_attach+0x4f/0x60
> [ 14.033634] __driver_attach+0x9a/0x140
> [ 14.033635] ? device_driver_attach+0x60/0x60
> [ 14.033636] bus_for_each_dev+0x76/0xc0
> [ 14.033638] ? klist_add_tail+0x3b/0x70
> [ 14.033639] bus_add_driver+0x144/0x220
> [ 14.033640] ? 0xffffffffc0949000
> [ 14.033641] driver_register+0x5b/0xf0
> [ 14.033642] ? 0xffffffffc0949000
> [ 14.033643] do_one_initcall+0x46/0x1f4
> [ 14.033645] ? _cond_resched+0x15/0x40
> [ 14.033646] ? kmem_cache_alloc_trace+0x40/0x440
> [ 14.033648] ? do_init_module+0x22/0x213
> [ 14.033649] do_init_module+0x5b/0x213
> [ 14.033650] load_module+0x258c/0x2d30
> [ 14.033652] ? __kernel_read+0xf5/0x160
> [ 14.033654] ? __do_sys_finit_module+0xe9/0x110
> [ 14.033655] __do_sys_finit_module+0xe9/0x110
> [ 14.033657] do_syscall_64+0x33/0x80
> [ 14.033659] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 14.033660] RIP: 0033:0x7feacf1bef59
> [ 14.033661] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
> [ 14.033662] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [ 14.033663] RAX: ffffffffffffffda RBX: 0000565489749410 RCX: 00007feacf1bef59
> [ 14.033663] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI: 0000000000000015
> [ 14.033664] RBP: 00007feacf0c3cad R08: 0000000000000000 R09: 0000000000000000
> [ 14.033664] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000
> [ 14.033665] R13: 00005654897472c0 R14: 0000000000020000 R15: 0000565489749410
> [ 14.033666] ---[ end trace 16aeaa08847a13d9 ]---
> [ 14.033667] ------------[ cut here ]------------
> [ 14.033668] Bogus possible_crtcs: [ENCODER:73:TMDS-73] possible_crtcs=0xf (full crtc mask=0x7)
> [ 14.033690] WARNING: CPU: 0 PID: 282 at ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 [drm]
> [ 14.033690] Modules linked in: amdgpu(E+) mfd_core(E) snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
> [ 14.033704] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
> [ 14.033706] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G W E 5.9.0-rc3+ #2
> [ 14.033707] Hardware name: Default string Default string/Default string, BIOS 5.0.1.4 02/14/2020
> [ 14.033719] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
> [ 14.033721] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d
> [ 14.033721] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282
> [ 14.033722] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX: 0000000000000027
> [ 14.033723] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff9c6910a18ac8
> [ 14.033723] RBP: 0000000000000001 R08: 0000000000000064 R09: ffffffff9784a724
> [ 14.033724] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c690bf7c000
> [ 14.033724] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15: 000000000000003f
> [ 14.033725] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000) knlGS:0000000000000000
> [ 14.033726] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 14.033726] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4: 00000000003506f0
> [ 14.033727] Call Trace:
> [ 14.033739] drm_dev_register+0x117/0x1e0 [drm]
> [ 14.033806] amdgpu_pci_probe+0x134/0x200 [amdgpu]
> [ 14.033808] local_pci_probe+0x42/0x90
> [ 14.033810] pci_device_probe+0x108/0x1c0
> [ 14.033811] really_probe+0xef/0x4a0
> [ 14.033813] driver_probe_device+0xde/0x150
> [ 14.033814] device_driver_attach+0x4f/0x60
> [ 14.033815] __driver_attach+0x9a/0x140
> [ 14.033816] ? device_driver_attach+0x60/0x60
> [ 14.033817] bus_for_each_dev+0x76/0xc0
> [ 14.033818] ? klist_add_tail+0x3b/0x70
> [ 14.033820] bus_add_driver+0x144/0x220
> [ 14.033821] ? 0xffffffffc0949000
> [ 14.033822] driver_register+0x5b/0xf0
> [ 14.033823] ? 0xffffffffc0949000
> [ 14.033824] do_one_initcall+0x46/0x1f4
> [ 14.033825] ? _cond_resched+0x15/0x40
> [ 14.033827] ? kmem_cache_alloc_trace+0x40/0x440
> [ 14.033828] ? do_init_module+0x22/0x213
> [ 14.033829] do_init_module+0x5b/0x213
> [ 14.033831] load_module+0x258c/0x2d30
> [ 14.033833] ? __kernel_read+0xf5/0x160
> [ 14.033834] ? __do_sys_finit_module+0xe9/0x110
> [ 14.033835] __do_sys_finit_module+0xe9/0x110
> [ 14.033838] do_syscall_64+0x33/0x80
> [ 14.033839] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 14.033840] RIP: 0033:0x7feacf1bef59
> [ 14.033841] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
> [ 14.033842] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [ 14.033843] RAX: ffffffffffffffda RBX: 0000565489749410 RCX: 00007feacf1bef59
> [ 14.033843] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI: 0000000000000015
> [ 14.033844] RBP: 00007feacf0c3cad R08: 0000000000000000 R09: 0000000000000000
> [ 14.033844] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000
> [ 14.033845] R13: 00005654897472c0 R14: 0000000000020000 R15: 0000565489749410
> [ 14.033846] ---[ end trace 16aeaa08847a13da ]---
>
> Probably the same issue as in
> https://bugzilla.kernel.org/show_bug.cgi?id=209123. What does it
> practically mean? Can/should it be silenced in this setup?
>
> Jan
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-09-07 7:14 ` Daniel Vetter
@ 2020-09-10 18:18 ` Deucher, Alexander
2020-09-29 9:36 ` Jan Kiszka
0 siblings, 1 reply; 28+ messages in thread
From: Deucher, Alexander @ 2020-09-10 18:18 UTC (permalink / raw)
To: Daniel Vetter, Jan Kiszka, amd-gfx list, Wentland, Harry,
Kazlauskas, Nicholas
Cc: intel-gfx, Thomas Zimmermann, dri-devel
[AMD Public Use]
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Daniel Vetter
> Sent: Monday, September 7, 2020 3:15 AM
> To: Jan Kiszka <jan.kiszka@web.de>; amd-gfx list <amd-
> gfx@lists.freedesktop.org>; Wentland, Harry <Harry.Wentland@amd.com>;
> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
> Cc: dri-devel <dri-devel@lists.freedesktop.org>; intel-gfx <intel-
> gfx@lists.freedesktop.org>; Thomas Zimmermann
> <tzimmermann@suse.de>; Ville Syrjala <ville.syrjala@linux.intel.com>
> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
>
> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka <jan.kiszka@web.de> wrote:
> >
> > On 11.02.20 18:04, Daniel Vetter wrote:
> > > On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
> > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>
> > >> WARN if the encoder possible_crtcs is effectively empty or contains
> > >> bits for non-existing crtcs.
> > >>
> > >> v2: Move to drm_mode_config_validate() (Daniel)
> > >> Make the docs say we WARN when this is wrong (Daniel)
> > >> Extract full_crtc_mask()
> > >>
> > >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > >> Cc: Daniel Vetter <daniel@ffwll.ch>
> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > When pushing the fixup needs to be applied before the validation
> > > patch here, because we don't want to anger the bisect gods.
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > I think with the fixup we should be good enough with the existing
> > > nonsense in drivers. Fingers crossed.
> > > -Daniel
> > >
> > >
> > >> ---
> > >> drivers/gpu/drm/drm_mode_config.c | 27
> ++++++++++++++++++++++++++-
> > >> include/drm/drm_encoder.h | 2 +-
> > >> 2 files changed, 27 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_mode_config.c
> > >> b/drivers/gpu/drm/drm_mode_config.c
> > >> index afc91447293a..4c1b350ddb95 100644
> > >> --- a/drivers/gpu/drm/drm_mode_config.c
> > >> +++ b/drivers/gpu/drm/drm_mode_config.c
> > >> @@ -581,6 +581,29 @@ static void
> validate_encoder_possible_clones(struct drm_encoder *encoder)
> > >> encoder->possible_clones, encoder_mask); }
> > >>
> > >> +static u32 full_crtc_mask(struct drm_device *dev) {
> > >> + struct drm_crtc *crtc;
> > >> + u32 crtc_mask = 0;
> > >> +
> > >> + drm_for_each_crtc(crtc, dev)
> > >> + crtc_mask |= drm_crtc_mask(crtc);
> > >> +
> > >> + return crtc_mask;
> > >> +}
> > >> +
> > >> +static void validate_encoder_possible_crtcs(struct drm_encoder
> > >> +*encoder) {
> > >> + u32 crtc_mask = full_crtc_mask(encoder->dev);
> > >> +
> > >> + WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> > >> + (encoder->possible_crtcs & ~crtc_mask) != 0,
> > >> + "Bogus possible_crtcs: "
> > >> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> > >> + encoder->base.id, encoder->name,
> > >> + encoder->possible_crtcs, crtc_mask); }
> > >> +
> > >> void drm_mode_config_validate(struct drm_device *dev) {
> > >> struct drm_encoder *encoder;
> > >> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct
> drm_device *dev)
> > >> drm_for_each_encoder(encoder, dev)
> > >> fixup_encoder_possible_clones(encoder);
> > >>
> > >> - drm_for_each_encoder(encoder, dev)
> > >> + drm_for_each_encoder(encoder, dev) {
> > >> validate_encoder_possible_clones(encoder);
> > >> + validate_encoder_possible_crtcs(encoder);
> > >> + }
> > >> }
> > >> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > >> index 3741963b9587..b236269f41ac 100644
> > >> --- a/include/drm/drm_encoder.h
> > >> +++ b/include/drm/drm_encoder.h
> > >> @@ -142,7 +142,7 @@ struct drm_encoder {
> > >> * the bits for all &drm_crtc objects this encoder can be connected to
> > >> * before calling drm_dev_register().
> > >> *
> > >> - * In reality almost every driver gets this wrong.
> > >> + * You will get a WARN if you get this wrong in the driver.
> > >> *
> > >> * Note that since CRTC objects can't be hotplugged the assigned
> indices
> > >> * are stable and hence known before registering all objects.
> > >> --
> > >> 2.24.1
> > >>
> > >
> >
> > Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
>
> Adding amdgpu display folks.
I took a quick look at this and it looks like we limit the number of crtcs later in the mode init process if the number of physical displays can't actually use more crtcs. E.g., the physical board configuration would only allow for 3 active displays even if the hardware technically supports 4 crtcs. I presume that way we can just leave the additional hardware power gated all the time.
Alex
> -Daniel
>
> >
> > [ 14.033246] ------------[ cut here ]------------
> > [ 14.033248] Bogus possible_crtcs: [ENCODER:65:TMDS-65]
> possible_crtcs=0xf (full crtc mask=0x7)
> > [ 14.033279] WARNING: CPU: 0 PID: 282 at
> ../drivers/gpu/drm/drm_mode_config.c:622
> drm_mode_config_validate+0x17d/0x200 [drm]
> > [ 14.033279] Modules linked in: amdgpu(E+) mfd_core(E)
> snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E)
> snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E)
> drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E)
> snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E)
> snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E)
> snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E)
> glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E)
> sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E)
> acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E)
> efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E)
> crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E)
> crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E)
> r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E)
> i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
> > [ 14.033306] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
> > [ 14.033310] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G E
> 5.9.0-rc3+ #2
> > [ 14.033311] Hardware name: Default string Default string/Default string,
> BIOS 5.0.1.4 02/14/2020
> > [ 14.033324] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
> > [ 14.033327] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b
> 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b
> 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d
> > [ 14.033328] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282
> > [ 14.033329] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX:
> 0000000000000027
> > [ 14.033330] RDX: 0000000000000000 RSI: 0000000000000086 RDI:
> ffff9c6910a18ac8
> > [ 14.033331] RBP: 0000000000000001 R08: 0000000000000064 R09:
> ffffffff9784a724
> > [ 14.033331] R10: 0000000000000001 R11: 0000000000000000 R12:
> ffff9c690bf8b800
> > [ 14.033332] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15:
> 000000000000003f
> > [ 14.033333] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000)
> knlGS:0000000000000000
> > [ 14.033334] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 14.033335] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4:
> 00000000003506f0
> > [ 14.033335] Call Trace:
> > [ 14.033350] drm_dev_register+0x117/0x1e0 [drm]
> > [ 14.033423] amdgpu_pci_probe+0x134/0x200 [amdgpu]
> > [ 14.033428] local_pci_probe+0x42/0x90
> > [ 14.033430] pci_device_probe+0x108/0x1c0
> > [ 14.033433] really_probe+0xef/0x4a0
> > [ 14.033435] driver_probe_device+0xde/0x150
> > [ 14.033436] device_driver_attach+0x4f/0x60
> > [ 14.033438] __driver_attach+0x9a/0x140
> > [ 14.033439] ? device_driver_attach+0x60/0x60
> > [ 14.033441] bus_for_each_dev+0x76/0xc0
> > [ 14.033443] ? klist_add_tail+0x3b/0x70
> > [ 14.033445] bus_add_driver+0x144/0x220
> > [ 14.033446] ? 0xffffffffc0949000
> > [ 14.033447] driver_register+0x5b/0xf0
> > [ 14.033448] ? 0xffffffffc0949000
> > [ 14.033451] do_one_initcall+0x46/0x1f4
> > [ 14.033454] ? _cond_resched+0x15/0x40
> > [ 14.033456] ? kmem_cache_alloc_trace+0x40/0x440
> > [ 14.033459] ? do_init_module+0x22/0x213
> > [ 14.033460] do_init_module+0x5b/0x213
> > [ 14.033462] load_module+0x258c/0x2d30
> > [ 14.033465] ? __kernel_read+0xf5/0x160
> > [ 14.033467] ? __do_sys_finit_module+0xe9/0x110
> > [ 14.033468] __do_sys_finit_module+0xe9/0x110
> > [ 14.033471] do_syscall_64+0x33/0x80
> > [ 14.033473] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [ 14.033474] RIP: 0033:0x7feacf1bef59
> > [ 14.033477] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8
> 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
> > [ 14.033477] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000139
> > [ 14.033478] RAX: ffffffffffffffda RBX: 0000565489749410 RCX:
> 00007feacf1bef59
> > [ 14.033479] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI:
> 0000000000000015
> > [ 14.033479] RBP: 00007feacf0c3cad R08: 0000000000000000 R09:
> 0000000000000000
> > [ 14.033480] R10: 0000000000000015 R11: 0000000000000246 R12:
> 0000000000000000
> > [ 14.033480] R13: 00005654897472c0 R14: 0000000000020000 R15:
> 0000565489749410
> > [ 14.033482] ---[ end trace 16aeaa08847a13d8 ]---
> > [ 14.033483] ------------[ cut here ]------------
> > [ 14.033484] Bogus possible_crtcs: [ENCODER:69:TMDS-69]
> possible_crtcs=0xf (full crtc mask=0x7)
> > [ 14.033507] WARNING: CPU: 0 PID: 282 at
> ../drivers/gpu/drm/drm_mode_config.c:622
> drm_mode_config_validate+0x17d/0x200 [drm]
> > [ 14.033507] Modules linked in: amdgpu(E+) mfd_core(E)
> snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E)
> snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E)
> drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E)
> snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E)
> snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E)
> snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E)
> glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E)
> sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E)
> acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E)
> efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E)
> crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E)
> crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E)
> r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E)
> i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
> > [ 14.033522] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
> > [ 14.033524] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G W E
> 5.9.0-rc3+ #2
> > [ 14.033525] Hardware name: Default string Default string/Default string,
> BIOS 5.0.1.4 02/14/2020
> > [ 14.033538] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
> > [ 14.033539] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b
> 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b
> 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d
> > [ 14.033540] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282
> > [ 14.033540] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX:
> 0000000000000027
> > [ 14.033541] RDX: 0000000000000000 RSI: 0000000000000086 RDI:
> ffff9c6910a18ac8
> > [ 14.033542] RBP: 0000000000000001 R08: 0000000000000064 R09:
> ffffffff9784a724
> > [ 14.033542] R10: 0000000000000001 R11: 0000000000000000 R12:
> ffff9c690bf89800
> > [ 14.033543] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15:
> 000000000000003f
> > [ 14.033544] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000)
> knlGS:0000000000000000
> > [ 14.033544] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 14.033545] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4:
> 00000000003506f0
> > [ 14.033545] Call Trace:
> > [ 14.033557] drm_dev_register+0x117/0x1e0 [drm]
> > [ 14.033625] amdgpu_pci_probe+0x134/0x200 [amdgpu]
> > [ 14.033627] local_pci_probe+0x42/0x90
> > [ 14.033629] pci_device_probe+0x108/0x1c0
> > [ 14.033630] really_probe+0xef/0x4a0
> > [ 14.033632] driver_probe_device+0xde/0x150
> > [ 14.033633] device_driver_attach+0x4f/0x60
> > [ 14.033634] __driver_attach+0x9a/0x140
> > [ 14.033635] ? device_driver_attach+0x60/0x60
> > [ 14.033636] bus_for_each_dev+0x76/0xc0
> > [ 14.033638] ? klist_add_tail+0x3b/0x70
> > [ 14.033639] bus_add_driver+0x144/0x220
> > [ 14.033640] ? 0xffffffffc0949000
> > [ 14.033641] driver_register+0x5b/0xf0
> > [ 14.033642] ? 0xffffffffc0949000
> > [ 14.033643] do_one_initcall+0x46/0x1f4
> > [ 14.033645] ? _cond_resched+0x15/0x40
> > [ 14.033646] ? kmem_cache_alloc_trace+0x40/0x440
> > [ 14.033648] ? do_init_module+0x22/0x213
> > [ 14.033649] do_init_module+0x5b/0x213
> > [ 14.033650] load_module+0x258c/0x2d30
> > [ 14.033652] ? __kernel_read+0xf5/0x160
> > [ 14.033654] ? __do_sys_finit_module+0xe9/0x110
> > [ 14.033655] __do_sys_finit_module+0xe9/0x110
> > [ 14.033657] do_syscall_64+0x33/0x80
> > [ 14.033659] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [ 14.033660] RIP: 0033:0x7feacf1bef59
> > [ 14.033661] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8
> 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
> > [ 14.033662] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000139
> > [ 14.033663] RAX: ffffffffffffffda RBX: 0000565489749410 RCX:
> 00007feacf1bef59
> > [ 14.033663] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI:
> 0000000000000015
> > [ 14.033664] RBP: 00007feacf0c3cad R08: 0000000000000000 R09:
> 0000000000000000
> > [ 14.033664] R10: 0000000000000015 R11: 0000000000000246 R12:
> 0000000000000000
> > [ 14.033665] R13: 00005654897472c0 R14: 0000000000020000 R15:
> 0000565489749410
> > [ 14.033666] ---[ end trace 16aeaa08847a13d9 ]---
> > [ 14.033667] ------------[ cut here ]------------
> > [ 14.033668] Bogus possible_crtcs: [ENCODER:73:TMDS-73]
> possible_crtcs=0xf (full crtc mask=0x7)
> > [ 14.033690] WARNING: CPU: 0 PID: 282 at
> ../drivers/gpu/drm/drm_mode_config.c:622
> drm_mode_config_validate+0x17d/0x200 [drm]
> > [ 14.033690] Modules linked in: amdgpu(E+) mfd_core(E)
> snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E)
> snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E)
> drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E)
> snd_hda_codec(E) cec(E) snd_hwdep(E) drm(E) irqbypass(E)
> snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) ghash_clmulni_intel(E)
> snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) soundcore(E) rng_core(E)
> glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) efi_pstore(E)
> sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) button(E)
> acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E)
> efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E)
> crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E)
> crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E)
> r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E)
> i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
> > [ 14.033704] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
> > [ 14.033706] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: G W E
> 5.9.0-rc3+ #2
> > [ 14.033707] Hardware name: Default string Default string/Default string,
> BIOS 5.0.1.4 02/14/2020
> > [ 14.033719] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
> > [ 14.033721] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 8b
> 54 24 38 41 8b 74 24 18 89 f9 48 c7 c7 80 7d 70 c0 e8 13 66 9a d5 <0f> 0b 49 8b
> 44 24 08 49 39 c5 4c 8d 60 f8 0f 85 e9 fe ff ff 5b 5d
> > [ 14.033721] RSP: 0018:ffffae5f404b7a90 EFLAGS: 00010282
> > [ 14.033722] RAX: 0000000000000000 RBX: ffff9c6907bd0ad0 RCX:
> 0000000000000027
> > [ 14.033723] RDX: 0000000000000000 RSI: 0000000000000086 RDI:
> ffff9c6910a18ac8
> > [ 14.033723] RBP: 0000000000000001 R08: 0000000000000064 R09:
> ffffffff9784a724
> > [ 14.033724] R10: 0000000000000001 R11: 0000000000000000 R12:
> ffff9c690bf7c000
> > [ 14.033724] R13: ffff9c6907bd0ad8 R14: ffff9c6907bd0ad8 R15:
> 000000000000003f
> > [ 14.033725] FS: 00007feace9d4d40(0000) GS:ffff9c6910a00000(0000)
> knlGS:0000000000000000
> > [ 14.033726] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 14.033726] CR2: 0000557a37b7e270 CR3: 000000038d5fa000 CR4:
> 00000000003506f0
> > [ 14.033727] Call Trace:
> > [ 14.033739] drm_dev_register+0x117/0x1e0 [drm]
> > [ 14.033806] amdgpu_pci_probe+0x134/0x200 [amdgpu]
> > [ 14.033808] local_pci_probe+0x42/0x90
> > [ 14.033810] pci_device_probe+0x108/0x1c0
> > [ 14.033811] really_probe+0xef/0x4a0
> > [ 14.033813] driver_probe_device+0xde/0x150
> > [ 14.033814] device_driver_attach+0x4f/0x60
> > [ 14.033815] __driver_attach+0x9a/0x140
> > [ 14.033816] ? device_driver_attach+0x60/0x60
> > [ 14.033817] bus_for_each_dev+0x76/0xc0
> > [ 14.033818] ? klist_add_tail+0x3b/0x70
> > [ 14.033820] bus_add_driver+0x144/0x220
> > [ 14.033821] ? 0xffffffffc0949000
> > [ 14.033822] driver_register+0x5b/0xf0
> > [ 14.033823] ? 0xffffffffc0949000
> > [ 14.033824] do_one_initcall+0x46/0x1f4
> > [ 14.033825] ? _cond_resched+0x15/0x40
> > [ 14.033827] ? kmem_cache_alloc_trace+0x40/0x440
> > [ 14.033828] ? do_init_module+0x22/0x213
> > [ 14.033829] do_init_module+0x5b/0x213
> > [ 14.033831] load_module+0x258c/0x2d30
> > [ 14.033833] ? __kernel_read+0xf5/0x160
> > [ 14.033834] ? __do_sys_finit_module+0xe9/0x110
> > [ 14.033835] __do_sys_finit_module+0xe9/0x110
> > [ 14.033838] do_syscall_64+0x33/0x80
> > [ 14.033839] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [ 14.033840] RIP: 0033:0x7feacf1bef59
> > [ 14.033841] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8
> 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 73 01 c3 48 8b 0d 07 6f 0c 00 f7 d8 64 89 01 48
> > [ 14.033842] RSP: 002b:00007ffd03534438 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000139
> > [ 14.033843] RAX: ffffffffffffffda RBX: 0000565489749410 RCX:
> 00007feacf1bef59
> > [ 14.033843] RDX: 0000000000000000 RSI: 00007feacf0c3cad RDI:
> 0000000000000015
> > [ 14.033844] RBP: 00007feacf0c3cad R08: 0000000000000000 R09:
> 0000000000000000
> > [ 14.033844] R10: 0000000000000015 R11: 0000000000000246 R12:
> 0000000000000000
> > [ 14.033845] R13: 00005654897472c0 R14: 0000000000020000 R15:
> 0000565489749410
> > [ 14.033846] ---[ end trace 16aeaa08847a13da ]---
> >
> > Probably the same issue as in
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
> >
> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D209123&data=02%7C01%7Cal
> exander.deucher%40amd.com%7C28e5e2e1aecd417e7dd608d852fdc391%7C
> 3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637350597170905726&a
> mp;sdata=z9smK6qFP6akCsGs4ToJNigPHke7SUfnb3i2F72u7S0%3D&res
> erved=0. What does it practically mean? Can/should it be silenced in this
> setup?
> >
> > Jan
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.f
> fwll.ch%2F&data=02%7C01%7Calexander.deucher%40amd.com%7C28e
> 5e2e1aecd417e7dd608d852fdc391%7C3dd8961fe4884e608e11a82d994e183d
> %7C0%7C0%7C637350597170905726&sdata=6fPvyO%2FFxcf7mu4j4l%2B
> OlwMe3BsaKY3zulTE3F%2BfeIw%3D&reserved=0
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.
> freedesktop.org%2Fmailman%2Flistinfo%2Famd-
> gfx&data=02%7C01%7Calexander.deucher%40amd.com%7C28e5e2e1a
> ecd417e7dd608d852fdc391%7C3dd8961fe4884e608e11a82d994e183d%7C0%
> 7C0%7C637350597170905726&sdata=%2FV0U72P9e8Vu8NWON7lJdx7k3
> TtMm9TjBz3D%2B7NtvOo%3D&reserved=0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-09-10 18:18 ` Deucher, Alexander
@ 2020-09-29 9:36 ` Jan Kiszka
2020-09-29 20:04 ` Alex Deucher
0 siblings, 1 reply; 28+ messages in thread
From: Jan Kiszka @ 2020-09-29 9:36 UTC (permalink / raw)
To: Deucher, Alexander, Daniel Vetter, amd-gfx list, Wentland, Harry,
Kazlauskas, Nicholas
Cc: intel-gfx, Thomas Zimmermann, dri-devel
On 10.09.20 20:18, Deucher, Alexander wrote:
> [AMD Public Use]
>
>
>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Daniel Vetter
>> Sent: Monday, September 7, 2020 3:15 AM
>> To: Jan Kiszka <jan.kiszka@web.de>; amd-gfx list <amd-
>> gfx@lists.freedesktop.org>; Wentland, Harry <Harry.Wentland@amd.com>;
>> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
>> Cc: dri-devel <dri-devel@lists.freedesktop.org>; intel-gfx <intel-
>> gfx@lists.freedesktop.org>; Thomas Zimmermann
>> <tzimmermann@suse.de>; Ville Syrjala <ville.syrjala@linux.intel.com>
>> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
>>
>> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka <jan.kiszka@web.de> wrote:
>>>
>>> On 11.02.20 18:04, Daniel Vetter wrote:
>>>> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>
>>>>> WARN if the encoder possible_crtcs is effectively empty or contains
>>>>> bits for non-existing crtcs.
>>>>>
>>>>> v2: Move to drm_mode_config_validate() (Daniel)
>>>>> Make the docs say we WARN when this is wrong (Daniel)
>>>>> Extract full_crtc_mask()
>>>>>
>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> When pushing the fixup needs to be applied before the validation
>>>> patch here, because we don't want to anger the bisect gods.
>>>>
>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>
>>>> I think with the fixup we should be good enough with the existing
>>>> nonsense in drivers. Fingers crossed.
>>>> -Daniel
>>>>
>>>>
>>>>> ---
>>>>> drivers/gpu/drm/drm_mode_config.c | 27
>> ++++++++++++++++++++++++++-
>>>>> include/drm/drm_encoder.h | 2 +-
>>>>> 2 files changed, 27 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>>>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>> index afc91447293a..4c1b350ddb95 100644
>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>> @@ -581,6 +581,29 @@ static void
>> validate_encoder_possible_clones(struct drm_encoder *encoder)
>>>>> encoder->possible_clones, encoder_mask); }
>>>>>
>>>>> +static u32 full_crtc_mask(struct drm_device *dev) {
>>>>> + struct drm_crtc *crtc;
>>>>> + u32 crtc_mask = 0;
>>>>> +
>>>>> + drm_for_each_crtc(crtc, dev)
>>>>> + crtc_mask |= drm_crtc_mask(crtc);
>>>>> +
>>>>> + return crtc_mask;
>>>>> +}
>>>>> +
>>>>> +static void validate_encoder_possible_crtcs(struct drm_encoder
>>>>> +*encoder) {
>>>>> + u32 crtc_mask = full_crtc_mask(encoder->dev);
>>>>> +
>>>>> + WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
>>>>> + (encoder->possible_crtcs & ~crtc_mask) != 0,
>>>>> + "Bogus possible_crtcs: "
>>>>> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
>>>>> + encoder->base.id, encoder->name,
>>>>> + encoder->possible_crtcs, crtc_mask); }
>>>>> +
>>>>> void drm_mode_config_validate(struct drm_device *dev) {
>>>>> struct drm_encoder *encoder;
>>>>> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct
>> drm_device *dev)
>>>>> drm_for_each_encoder(encoder, dev)
>>>>> fixup_encoder_possible_clones(encoder);
>>>>>
>>>>> - drm_for_each_encoder(encoder, dev)
>>>>> + drm_for_each_encoder(encoder, dev) {
>>>>> validate_encoder_possible_clones(encoder);
>>>>> + validate_encoder_possible_crtcs(encoder);
>>>>> + }
>>>>> }
>>>>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
>>>>> index 3741963b9587..b236269f41ac 100644
>>>>> --- a/include/drm/drm_encoder.h
>>>>> +++ b/include/drm/drm_encoder.h
>>>>> @@ -142,7 +142,7 @@ struct drm_encoder {
>>>>> * the bits for all &drm_crtc objects this encoder can be connected to
>>>>> * before calling drm_dev_register().
>>>>> *
>>>>> - * In reality almost every driver gets this wrong.
>>>>> + * You will get a WARN if you get this wrong in the driver.
>>>>> *
>>>>> * Note that since CRTC objects can't be hotplugged the assigned
>> indices
>>>>> * are stable and hence known before registering all objects.
>>>>> --
>>>>> 2.24.1
>>>>>
>>>>
>>>
>>> Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
>>
>> Adding amdgpu display folks.
>
> I took a quick look at this and it looks like we limit the number of crtcs later in the mode init process if the number of physical displays can't actually use more crtcs. E.g., the physical board configuration would only allow for 3 active displays even if the hardware technically supports 4 crtcs. I presume that way we can just leave the additional hardware power gated all the time.
>
So, will this be fixed any time soon? I don't feel qualified writing
such a patch but I would obviously be happy to test one.
Jan
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-09-29 9:36 ` Jan Kiszka
@ 2020-09-29 20:04 ` Alex Deucher
2020-12-03 21:30 ` Alex Deucher
0 siblings, 1 reply; 28+ messages in thread
From: Alex Deucher @ 2020-09-29 20:04 UTC (permalink / raw)
To: Jan Kiszka
Cc: Thomas Zimmermann, intel-gfx, amd-gfx list, dri-devel, Deucher,
Alexander, Wentland, Harry, Kazlauskas, Nicholas
On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka <jan.kiszka@web.de> wrote:
>
> On 10.09.20 20:18, Deucher, Alexander wrote:
> > [AMD Public Use]
> >
> >
> >
> >> -----Original Message-----
> >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> >> Daniel Vetter
> >> Sent: Monday, September 7, 2020 3:15 AM
> >> To: Jan Kiszka <jan.kiszka@web.de>; amd-gfx list <amd-
> >> gfx@lists.freedesktop.org>; Wentland, Harry <Harry.Wentland@amd.com>;
> >> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
> >> Cc: dri-devel <dri-devel@lists.freedesktop.org>; intel-gfx <intel-
> >> gfx@lists.freedesktop.org>; Thomas Zimmermann
> >> <tzimmermann@suse.de>; Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
> >>
> >> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka <jan.kiszka@web.de> wrote:
> >>>
> >>> On 11.02.20 18:04, Daniel Vetter wrote:
> >>>> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
> >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>
> >>>>> WARN if the encoder possible_crtcs is effectively empty or contains
> >>>>> bits for non-existing crtcs.
> >>>>>
> >>>>> v2: Move to drm_mode_config_validate() (Daniel)
> >>>>> Make the docs say we WARN when this is wrong (Daniel)
> >>>>> Extract full_crtc_mask()
> >>>>>
> >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
> >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>
> >>>> When pushing the fixup needs to be applied before the validation
> >>>> patch here, because we don't want to anger the bisect gods.
> >>>>
> >>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>
> >>>> I think with the fixup we should be good enough with the existing
> >>>> nonsense in drivers. Fingers crossed.
> >>>> -Daniel
> >>>>
> >>>>
> >>>>> ---
> >>>>> drivers/gpu/drm/drm_mode_config.c | 27
> >> ++++++++++++++++++++++++++-
> >>>>> include/drm/drm_encoder.h | 2 +-
> >>>>> 2 files changed, 27 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
> >>>>> b/drivers/gpu/drm/drm_mode_config.c
> >>>>> index afc91447293a..4c1b350ddb95 100644
> >>>>> --- a/drivers/gpu/drm/drm_mode_config.c
> >>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
> >>>>> @@ -581,6 +581,29 @@ static void
> >> validate_encoder_possible_clones(struct drm_encoder *encoder)
> >>>>> encoder->possible_clones, encoder_mask); }
> >>>>>
> >>>>> +static u32 full_crtc_mask(struct drm_device *dev) {
> >>>>> + struct drm_crtc *crtc;
> >>>>> + u32 crtc_mask = 0;
> >>>>> +
> >>>>> + drm_for_each_crtc(crtc, dev)
> >>>>> + crtc_mask |= drm_crtc_mask(crtc);
> >>>>> +
> >>>>> + return crtc_mask;
> >>>>> +}
> >>>>> +
> >>>>> +static void validate_encoder_possible_crtcs(struct drm_encoder
> >>>>> +*encoder) {
> >>>>> + u32 crtc_mask = full_crtc_mask(encoder->dev);
> >>>>> +
> >>>>> + WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> >>>>> + (encoder->possible_crtcs & ~crtc_mask) != 0,
> >>>>> + "Bogus possible_crtcs: "
> >>>>> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> >>>>> + encoder->base.id, encoder->name,
> >>>>> + encoder->possible_crtcs, crtc_mask); }
> >>>>> +
> >>>>> void drm_mode_config_validate(struct drm_device *dev) {
> >>>>> struct drm_encoder *encoder;
> >>>>> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct
> >> drm_device *dev)
> >>>>> drm_for_each_encoder(encoder, dev)
> >>>>> fixup_encoder_possible_clones(encoder);
> >>>>>
> >>>>> - drm_for_each_encoder(encoder, dev)
> >>>>> + drm_for_each_encoder(encoder, dev) {
> >>>>> validate_encoder_possible_clones(encoder);
> >>>>> + validate_encoder_possible_crtcs(encoder);
> >>>>> + }
> >>>>> }
> >>>>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> >>>>> index 3741963b9587..b236269f41ac 100644
> >>>>> --- a/include/drm/drm_encoder.h
> >>>>> +++ b/include/drm/drm_encoder.h
> >>>>> @@ -142,7 +142,7 @@ struct drm_encoder {
> >>>>> * the bits for all &drm_crtc objects this encoder can be connected to
> >>>>> * before calling drm_dev_register().
> >>>>> *
> >>>>> - * In reality almost every driver gets this wrong.
> >>>>> + * You will get a WARN if you get this wrong in the driver.
> >>>>> *
> >>>>> * Note that since CRTC objects can't be hotplugged the assigned
> >> indices
> >>>>> * are stable and hence known before registering all objects.
> >>>>> --
> >>>>> 2.24.1
> >>>>>
> >>>>
> >>>
> >>> Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
> >>
> >> Adding amdgpu display folks.
> >
> > I took a quick look at this and it looks like we limit the number of crtcs later in the mode init process if the number of physical displays can't actually use more crtcs. E.g., the physical board configuration would only allow for 3 active displays even if the hardware technically supports 4 crtcs. I presume that way we can just leave the additional hardware power gated all the time.
> >
>
> So, will this be fixed any time soon? I don't feel qualified writing
> such a patch but I would obviously be happy to test one.
It's harmless, but I'll send out a patch soon.
Alex
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-09-29 20:04 ` Alex Deucher
@ 2020-12-03 21:30 ` Alex Deucher
2020-12-09 13:17 ` Daniel Vetter
2020-12-14 20:26 ` Jan Kiszka
0 siblings, 2 replies; 28+ messages in thread
From: Alex Deucher @ 2020-12-03 21:30 UTC (permalink / raw)
To: Jan Kiszka
Cc: Thomas Zimmermann, intel-gfx, amd-gfx list, dri-devel, Deucher,
Alexander, Wentland, Harry, Kazlauskas, Nicholas
[-- Attachment #1: Type: text/plain, Size: 5860 bytes --]
On Tue, Sep 29, 2020 at 4:04 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka <jan.kiszka@web.de> wrote:
> >
> > On 10.09.20 20:18, Deucher, Alexander wrote:
> > > [AMD Public Use]
> > >
> > >
> > >
> > >> -----Original Message-----
> > >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > >> Daniel Vetter
> > >> Sent: Monday, September 7, 2020 3:15 AM
> > >> To: Jan Kiszka <jan.kiszka@web.de>; amd-gfx list <amd-
> > >> gfx@lists.freedesktop.org>; Wentland, Harry <Harry.Wentland@amd.com>;
> > >> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
> > >> Cc: dri-devel <dri-devel@lists.freedesktop.org>; intel-gfx <intel-
> > >> gfx@lists.freedesktop.org>; Thomas Zimmermann
> > >> <tzimmermann@suse.de>; Ville Syrjala <ville.syrjala@linux.intel.com>
> > >> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
> > >>
> > >> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka <jan.kiszka@web.de> wrote:
> > >>>
> > >>> On 11.02.20 18:04, Daniel Vetter wrote:
> > >>>> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
> > >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>>>
> > >>>>> WARN if the encoder possible_crtcs is effectively empty or contains
> > >>>>> bits for non-existing crtcs.
> > >>>>>
> > >>>>> v2: Move to drm_mode_config_validate() (Daniel)
> > >>>>> Make the docs say we WARN when this is wrong (Daniel)
> > >>>>> Extract full_crtc_mask()
> > >>>>>
> > >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > >>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
> > >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>>
> > >>>> When pushing the fixup needs to be applied before the validation
> > >>>> patch here, because we don't want to anger the bisect gods.
> > >>>>
> > >>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >>>>
> > >>>> I think with the fixup we should be good enough with the existing
> > >>>> nonsense in drivers. Fingers crossed.
> > >>>> -Daniel
> > >>>>
> > >>>>
> > >>>>> ---
> > >>>>> drivers/gpu/drm/drm_mode_config.c | 27
> > >> ++++++++++++++++++++++++++-
> > >>>>> include/drm/drm_encoder.h | 2 +-
> > >>>>> 2 files changed, 27 insertions(+), 2 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
> > >>>>> b/drivers/gpu/drm/drm_mode_config.c
> > >>>>> index afc91447293a..4c1b350ddb95 100644
> > >>>>> --- a/drivers/gpu/drm/drm_mode_config.c
> > >>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
> > >>>>> @@ -581,6 +581,29 @@ static void
> > >> validate_encoder_possible_clones(struct drm_encoder *encoder)
> > >>>>> encoder->possible_clones, encoder_mask); }
> > >>>>>
> > >>>>> +static u32 full_crtc_mask(struct drm_device *dev) {
> > >>>>> + struct drm_crtc *crtc;
> > >>>>> + u32 crtc_mask = 0;
> > >>>>> +
> > >>>>> + drm_for_each_crtc(crtc, dev)
> > >>>>> + crtc_mask |= drm_crtc_mask(crtc);
> > >>>>> +
> > >>>>> + return crtc_mask;
> > >>>>> +}
> > >>>>> +
> > >>>>> +static void validate_encoder_possible_crtcs(struct drm_encoder
> > >>>>> +*encoder) {
> > >>>>> + u32 crtc_mask = full_crtc_mask(encoder->dev);
> > >>>>> +
> > >>>>> + WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> > >>>>> + (encoder->possible_crtcs & ~crtc_mask) != 0,
> > >>>>> + "Bogus possible_crtcs: "
> > >>>>> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> > >>>>> + encoder->base.id, encoder->name,
> > >>>>> + encoder->possible_crtcs, crtc_mask); }
> > >>>>> +
> > >>>>> void drm_mode_config_validate(struct drm_device *dev) {
> > >>>>> struct drm_encoder *encoder;
> > >>>>> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct
> > >> drm_device *dev)
> > >>>>> drm_for_each_encoder(encoder, dev)
> > >>>>> fixup_encoder_possible_clones(encoder);
> > >>>>>
> > >>>>> - drm_for_each_encoder(encoder, dev)
> > >>>>> + drm_for_each_encoder(encoder, dev) {
> > >>>>> validate_encoder_possible_clones(encoder);
> > >>>>> + validate_encoder_possible_crtcs(encoder);
> > >>>>> + }
> > >>>>> }
> > >>>>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > >>>>> index 3741963b9587..b236269f41ac 100644
> > >>>>> --- a/include/drm/drm_encoder.h
> > >>>>> +++ b/include/drm/drm_encoder.h
> > >>>>> @@ -142,7 +142,7 @@ struct drm_encoder {
> > >>>>> * the bits for all &drm_crtc objects this encoder can be connected to
> > >>>>> * before calling drm_dev_register().
> > >>>>> *
> > >>>>> - * In reality almost every driver gets this wrong.
> > >>>>> + * You will get a WARN if you get this wrong in the driver.
> > >>>>> *
> > >>>>> * Note that since CRTC objects can't be hotplugged the assigned
> > >> indices
> > >>>>> * are stable and hence known before registering all objects.
> > >>>>> --
> > >>>>> 2.24.1
> > >>>>>
> > >>>>
> > >>>
> > >>> Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
> > >>
> > >> Adding amdgpu display folks.
> > >
> > > I took a quick look at this and it looks like we limit the number of crtcs later in the mode init process if the number of physical displays can't actually use more crtcs. E.g., the physical board configuration would only allow for 3 active displays even if the hardware technically supports 4 crtcs. I presume that way we can just leave the additional hardware power gated all the time.
> > >
> >
> > So, will this be fixed any time soon? I don't feel qualified writing
> > such a patch but I would obviously be happy to test one.
>
> It's harmless, but I'll send out a patch soon.
I believe the attached patch should fix this.
Alex
[-- Attachment #2: 0001-drm-amdgpu-disply-set-num_crtc-earlier.patch --]
[-- Type: text/x-patch, Size: 2027 bytes --]
From 441d23a9700ce2d03d8d89686a95a9a6500d866d Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@amd.com>
Date: Thu, 3 Dec 2020 16:06:26 -0500
Subject: [PATCH] drm/amdgpu/disply: set num_crtc earlier
To avoid a recently added warning:
Bogus possible_crtcs: [ENCODER:65:TMDS-65] possible_crtcs=0xf (full crtc mask=0x7)
WARNING: CPU: 3 PID: 439 at drivers/gpu/drm/drm_mode_config.c:617 drm_mode_config_validate+0x178/0x200 [drm]
In this case the warning is harmless, but confusing to users.
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 313501cc39fc..1ec57bc798e2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1130,9 +1130,6 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
goto error;
}
- /* Update the actual used number of crtc */
- adev->mode_info.num_crtc = adev->dm.display_indexes_num;
-
/* create fake encoders for MST */
dm_dp_create_fake_mst_encoders(adev);
@@ -3364,6 +3361,10 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
enum dc_connection_type new_connection_type = dc_connection_none;
const struct dc_plane_cap *plane;
+ dm->display_indexes_num = dm->dc->caps.max_streams;
+ /* Update the actual used number of crtc */
+ adev->mode_info.num_crtc = adev->dm.display_indexes_num;
+
link_cnt = dm->dc->caps.max_links;
if (amdgpu_dm_mode_config_init(dm->adev)) {
DRM_ERROR("DM: Failed to initialize mode config\n");
@@ -3425,8 +3426,6 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
goto fail;
}
- dm->display_indexes_num = dm->dc->caps.max_streams;
-
/* loops over all connectors on the board */
for (i = 0; i < link_cnt; i++) {
struct dc_link *link = NULL;
--
2.25.4
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-12-03 21:30 ` Alex Deucher
@ 2020-12-09 13:17 ` Daniel Vetter
2020-12-14 20:26 ` Jan Kiszka
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2020-12-09 13:17 UTC (permalink / raw)
To: Alex Deucher
Cc: intel-gfx, amd-gfx list, Jan Kiszka, dri-devel,
Thomas Zimmermann, Deucher, Alexander, Wentland, Harry,
Kazlauskas, Nicholas
On Thu, Dec 3, 2020 at 10:31 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Tue, Sep 29, 2020 at 4:04 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka <jan.kiszka@web.de> wrote:
> > >
> > > On 10.09.20 20:18, Deucher, Alexander wrote:
> > > > [AMD Public Use]
> > > >
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > > >> Daniel Vetter
> > > >> Sent: Monday, September 7, 2020 3:15 AM
> > > >> To: Jan Kiszka <jan.kiszka@web.de>; amd-gfx list <amd-
> > > >> gfx@lists.freedesktop.org>; Wentland, Harry <Harry.Wentland@amd.com>;
> > > >> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
> > > >> Cc: dri-devel <dri-devel@lists.freedesktop.org>; intel-gfx <intel-
> > > >> gfx@lists.freedesktop.org>; Thomas Zimmermann
> > > >> <tzimmermann@suse.de>; Ville Syrjala <ville.syrjala@linux.intel.com>
> > > >> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
> > > >>
> > > >> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka <jan.kiszka@web.de> wrote:
> > > >>>
> > > >>> On 11.02.20 18:04, Daniel Vetter wrote:
> > > >>>> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
> > > >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >>>>>
> > > >>>>> WARN if the encoder possible_crtcs is effectively empty or contains
> > > >>>>> bits for non-existing crtcs.
> > > >>>>>
> > > >>>>> v2: Move to drm_mode_config_validate() (Daniel)
> > > >>>>> Make the docs say we WARN when this is wrong (Daniel)
> > > >>>>> Extract full_crtc_mask()
> > > >>>>>
> > > >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > >>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
> > > >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >>>>
> > > >>>> When pushing the fixup needs to be applied before the validation
> > > >>>> patch here, because we don't want to anger the bisect gods.
> > > >>>>
> > > >>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > >>>>
> > > >>>> I think with the fixup we should be good enough with the existing
> > > >>>> nonsense in drivers. Fingers crossed.
> > > >>>> -Daniel
> > > >>>>
> > > >>>>
> > > >>>>> ---
> > > >>>>> drivers/gpu/drm/drm_mode_config.c | 27
> > > >> ++++++++++++++++++++++++++-
> > > >>>>> include/drm/drm_encoder.h | 2 +-
> > > >>>>> 2 files changed, 27 insertions(+), 2 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
> > > >>>>> b/drivers/gpu/drm/drm_mode_config.c
> > > >>>>> index afc91447293a..4c1b350ddb95 100644
> > > >>>>> --- a/drivers/gpu/drm/drm_mode_config.c
> > > >>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
> > > >>>>> @@ -581,6 +581,29 @@ static void
> > > >> validate_encoder_possible_clones(struct drm_encoder *encoder)
> > > >>>>> encoder->possible_clones, encoder_mask); }
> > > >>>>>
> > > >>>>> +static u32 full_crtc_mask(struct drm_device *dev) {
> > > >>>>> + struct drm_crtc *crtc;
> > > >>>>> + u32 crtc_mask = 0;
> > > >>>>> +
> > > >>>>> + drm_for_each_crtc(crtc, dev)
> > > >>>>> + crtc_mask |= drm_crtc_mask(crtc);
> > > >>>>> +
> > > >>>>> + return crtc_mask;
> > > >>>>> +}
> > > >>>>> +
> > > >>>>> +static void validate_encoder_possible_crtcs(struct drm_encoder
> > > >>>>> +*encoder) {
> > > >>>>> + u32 crtc_mask = full_crtc_mask(encoder->dev);
> > > >>>>> +
> > > >>>>> + WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> > > >>>>> + (encoder->possible_crtcs & ~crtc_mask) != 0,
> > > >>>>> + "Bogus possible_crtcs: "
> > > >>>>> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> > > >>>>> + encoder->base.id, encoder->name,
> > > >>>>> + encoder->possible_crtcs, crtc_mask); }
> > > >>>>> +
> > > >>>>> void drm_mode_config_validate(struct drm_device *dev) {
> > > >>>>> struct drm_encoder *encoder;
> > > >>>>> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct
> > > >> drm_device *dev)
> > > >>>>> drm_for_each_encoder(encoder, dev)
> > > >>>>> fixup_encoder_possible_clones(encoder);
> > > >>>>>
> > > >>>>> - drm_for_each_encoder(encoder, dev)
> > > >>>>> + drm_for_each_encoder(encoder, dev) {
> > > >>>>> validate_encoder_possible_clones(encoder);
> > > >>>>> + validate_encoder_possible_crtcs(encoder);
> > > >>>>> + }
> > > >>>>> }
> > > >>>>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > > >>>>> index 3741963b9587..b236269f41ac 100644
> > > >>>>> --- a/include/drm/drm_encoder.h
> > > >>>>> +++ b/include/drm/drm_encoder.h
> > > >>>>> @@ -142,7 +142,7 @@ struct drm_encoder {
> > > >>>>> * the bits for all &drm_crtc objects this encoder can be connected to
> > > >>>>> * before calling drm_dev_register().
> > > >>>>> *
> > > >>>>> - * In reality almost every driver gets this wrong.
> > > >>>>> + * You will get a WARN if you get this wrong in the driver.
> > > >>>>> *
> > > >>>>> * Note that since CRTC objects can't be hotplugged the assigned
> > > >> indices
> > > >>>>> * are stable and hence known before registering all objects.
> > > >>>>> --
> > > >>>>> 2.24.1
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>> Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
> > > >>
> > > >> Adding amdgpu display folks.
> > > >
> > > > I took a quick look at this and it looks like we limit the number of crtcs later in the mode init process if the number of physical displays can't actually use more crtcs. E.g., the physical board configuration would only allow for 3 active displays even if the hardware technically supports 4 crtcs. I presume that way we can just leave the additional hardware power gated all the time.
> > > >
> > >
> > > So, will this be fixed any time soon? I don't feel qualified writing
> > > such a patch but I would obviously be happy to test one.
> >
> > It's harmless, but I'll send out a patch soon.
>
> I believe the attached patch should fix this.
fwiw rb: me
Probably want to add the right Fixes: line too.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
2020-12-03 21:30 ` Alex Deucher
2020-12-09 13:17 ` Daniel Vetter
@ 2020-12-14 20:26 ` Jan Kiszka
1 sibling, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2020-12-14 20:26 UTC (permalink / raw)
To: Alex Deucher
Cc: Thomas Zimmermann, intel-gfx, amd-gfx list, dri-devel, Deucher,
Alexander, Wentland, Harry, Kazlauskas, Nicholas
On 03.12.20 22:30, Alex Deucher wrote:
> On Tue, Sep 29, 2020 at 4:04 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>>
>> On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka <jan.kiszka@web.de> wrote:
>>>
>>> On 10.09.20 20:18, Deucher, Alexander wrote:
>>>> [AMD Public Use]
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>>> Daniel Vetter
>>>>> Sent: Monday, September 7, 2020 3:15 AM
>>>>> To: Jan Kiszka <jan.kiszka@web.de>; amd-gfx list <amd-
>>>>> gfx@lists.freedesktop.org>; Wentland, Harry <Harry.Wentland@amd.com>;
>>>>> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
>>>>> Cc: dri-devel <dri-devel@lists.freedesktop.org>; intel-gfx <intel-
>>>>> gfx@lists.freedesktop.org>; Thomas Zimmermann
>>>>> <tzimmermann@suse.de>; Ville Syrjala <ville.syrjala@linux.intel.com>
>>>>> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
>>>>>
>>>>> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>>>
>>>>>> On 11.02.20 18:04, Daniel Vetter wrote:
>>>>>>> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
>>>>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>>>
>>>>>>>> WARN if the encoder possible_crtcs is effectively empty or contains
>>>>>>>> bits for non-existing crtcs.
>>>>>>>>
>>>>>>>> v2: Move to drm_mode_config_validate() (Daniel)
>>>>>>>> Make the docs say we WARN when this is wrong (Daniel)
>>>>>>>> Extract full_crtc_mask()
>>>>>>>>
>>>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>>
>>>>>>> When pushing the fixup needs to be applied before the validation
>>>>>>> patch here, because we don't want to anger the bisect gods.
>>>>>>>
>>>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>>
>>>>>>> I think with the fixup we should be good enough with the existing
>>>>>>> nonsense in drivers. Fingers crossed.
>>>>>>> -Daniel
>>>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/drm_mode_config.c | 27
>>>>> ++++++++++++++++++++++++++-
>>>>>>>> include/drm/drm_encoder.h | 2 +-
>>>>>>>> 2 files changed, 27 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>>>>>>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>>>>> index afc91447293a..4c1b350ddb95 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>>>>> @@ -581,6 +581,29 @@ static void
>>>>> validate_encoder_possible_clones(struct drm_encoder *encoder)
>>>>>>>> encoder->possible_clones, encoder_mask); }
>>>>>>>>
>>>>>>>> +static u32 full_crtc_mask(struct drm_device *dev) {
>>>>>>>> + struct drm_crtc *crtc;
>>>>>>>> + u32 crtc_mask = 0;
>>>>>>>> +
>>>>>>>> + drm_for_each_crtc(crtc, dev)
>>>>>>>> + crtc_mask |= drm_crtc_mask(crtc);
>>>>>>>> +
>>>>>>>> + return crtc_mask;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void validate_encoder_possible_crtcs(struct drm_encoder
>>>>>>>> +*encoder) {
>>>>>>>> + u32 crtc_mask = full_crtc_mask(encoder->dev);
>>>>>>>> +
>>>>>>>> + WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
>>>>>>>> + (encoder->possible_crtcs & ~crtc_mask) != 0,
>>>>>>>> + "Bogus possible_crtcs: "
>>>>>>>> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
>>>>>>>> + encoder->base.id, encoder->name,
>>>>>>>> + encoder->possible_crtcs, crtc_mask); }
>>>>>>>> +
>>>>>>>> void drm_mode_config_validate(struct drm_device *dev) {
>>>>>>>> struct drm_encoder *encoder;
>>>>>>>> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct
>>>>> drm_device *dev)
>>>>>>>> drm_for_each_encoder(encoder, dev)
>>>>>>>> fixup_encoder_possible_clones(encoder);
>>>>>>>>
>>>>>>>> - drm_for_each_encoder(encoder, dev)
>>>>>>>> + drm_for_each_encoder(encoder, dev) {
>>>>>>>> validate_encoder_possible_clones(encoder);
>>>>>>>> + validate_encoder_possible_crtcs(encoder);
>>>>>>>> + }
>>>>>>>> }
>>>>>>>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
>>>>>>>> index 3741963b9587..b236269f41ac 100644
>>>>>>>> --- a/include/drm/drm_encoder.h
>>>>>>>> +++ b/include/drm/drm_encoder.h
>>>>>>>> @@ -142,7 +142,7 @@ struct drm_encoder {
>>>>>>>> * the bits for all &drm_crtc objects this encoder can be connected to
>>>>>>>> * before calling drm_dev_register().
>>>>>>>> *
>>>>>>>> - * In reality almost every driver gets this wrong.
>>>>>>>> + * You will get a WARN if you get this wrong in the driver.
>>>>>>>> *
>>>>>>>> * Note that since CRTC objects can't be hotplugged the assigned
>>>>> indices
>>>>>>>> * are stable and hence known before registering all objects.
>>>>>>>> --
>>>>>>>> 2.24.1
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
>>>>>
>>>>> Adding amdgpu display folks.
>>>>
>>>> I took a quick look at this and it looks like we limit the number of crtcs later in the mode init process if the number of physical displays can't actually use more crtcs. E.g., the physical board configuration would only allow for 3 active displays even if the hardware technically supports 4 crtcs. I presume that way we can just leave the additional hardware power gated all the time.
>>>>
>>>
>>> So, will this be fixed any time soon? I don't feel qualified writing
>>> such a patch but I would obviously be happy to test one.
>>
>> It's harmless, but I'll send out a patch soon.
>
> I believe the attached patch should fix this.
>
Yep, just booted 5.10, and the warning is gone.
Thanks,
Jan
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0
2020-02-11 16:22 [Intel-gfx] [PATCH v3 0/7] drm: Try to fix encoder possible_clones/crtc Ville Syrjala
` (5 preceding siblings ...)
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs Ville Syrjala
@ 2020-02-11 16:22 ` Ville Syrjala
2020-02-11 17:05 ` Daniel Vetter
2020-12-03 22:16 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm: Try to fix encoder possible_clones/crtc (rev4) Patchwork
7 siblings, 1 reply; 28+ messages in thread
From: Ville Syrjala @ 2020-02-11 16:22 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Thomas Zimmermann
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Let's simplify life of driver by allowing them to leave
encoder->possible_crtcs unset if they have no restrictions
in crtc<->encoder linkage. We'll just populate possible_crtcs
with the full crtc mask when registering the encoder so that
userspace doesn't have to deal with drivers not populating
this correctly.
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
We might not actually need/want this, but included it here for
future reference if that assumption turns out to be wrong.
---
drivers/gpu/drm/drm_mode_config.c | 15 ++++++++++++++-
include/drm/drm_encoder.h | 4 ++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 4c1b350ddb95..ce18c3dd0bde 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -592,6 +592,17 @@ static u32 full_crtc_mask(struct drm_device *dev)
return crtc_mask;
}
+/*
+ * Make life easy for drivers by allowing them to leave
+ * possible_crtcs unset if there are not crtc<->encoder
+ * restrictions.
+ */
+static void fixup_encoder_possible_crtcs(struct drm_encoder *encoder)
+{
+ if (encoder->possible_crtcs == 0)
+ encoder->possible_crtcs = full_crtc_mask(encoder->dev);
+}
+
static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
{
u32 crtc_mask = full_crtc_mask(encoder->dev);
@@ -608,8 +619,10 @@ void drm_mode_config_validate(struct drm_device *dev)
{
struct drm_encoder *encoder;
- drm_for_each_encoder(encoder, dev)
+ drm_for_each_encoder(encoder, dev) {
fixup_encoder_possible_clones(encoder);
+ fixup_encoder_possible_crtcs(encoder);
+ }
drm_for_each_encoder(encoder, dev) {
validate_encoder_possible_clones(encoder);
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index b236269f41ac..bd033c5618bf 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -142,6 +142,10 @@ struct drm_encoder {
* the bits for all &drm_crtc objects this encoder can be connected to
* before calling drm_dev_register().
*
+ * As an exception to the above rule if any crtc can be connected to
+ * the encoder the driver can leave @possible_crtcs set to 0. The core
+ * will automagically fix this up by setting the bit for every crtc.
+ *
* You will get a WARN if you get this wrong in the driver.
*
* Note that since CRTC objects can't be hotplugged the assigned indices
--
2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0 Ville Syrjala
@ 2020-02-11 17:05 ` Daniel Vetter
2020-02-11 17:14 ` Ville Syrjälä
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2020-02-11 17:05 UTC (permalink / raw)
To: Ville Syrjala; +Cc: intel-gfx, Thomas Zimmermann, dri-devel
On Tue, Feb 11, 2020 at 06:22:08PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Let's simplify life of driver by allowing them to leave
> encoder->possible_crtcs unset if they have no restrictions
> in crtc<->encoder linkage. We'll just populate possible_crtcs
> with the full crtc mask when registering the encoder so that
> userspace doesn't have to deal with drivers not populating
> this correctly.
>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> We might not actually need/want this, but included it here for
> future reference if that assumption turns out to be wrong.
I think this one is most definitely needed. _Lots_ of drivers get this
toally wrong and just leave the value blank. It's encoded as official
fallback in most userspace compositors.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/drm_mode_config.c | 15 ++++++++++++++-
> include/drm/drm_encoder.h | 4 ++++
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 4c1b350ddb95..ce18c3dd0bde 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -592,6 +592,17 @@ static u32 full_crtc_mask(struct drm_device *dev)
> return crtc_mask;
> }
>
> +/*
> + * Make life easy for drivers by allowing them to leave
> + * possible_crtcs unset if there are not crtc<->encoder
> + * restrictions.
> + */
> +static void fixup_encoder_possible_crtcs(struct drm_encoder *encoder)
> +{
> + if (encoder->possible_crtcs == 0)
> + encoder->possible_crtcs = full_crtc_mask(encoder->dev);
> +}
> +
> static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
> {
> u32 crtc_mask = full_crtc_mask(encoder->dev);
> @@ -608,8 +619,10 @@ void drm_mode_config_validate(struct drm_device *dev)
> {
> struct drm_encoder *encoder;
>
> - drm_for_each_encoder(encoder, dev)
> + drm_for_each_encoder(encoder, dev) {
> fixup_encoder_possible_clones(encoder);
> + fixup_encoder_possible_crtcs(encoder);
> + }
>
> drm_for_each_encoder(encoder, dev) {
> validate_encoder_possible_clones(encoder);
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index b236269f41ac..bd033c5618bf 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -142,6 +142,10 @@ struct drm_encoder {
> * the bits for all &drm_crtc objects this encoder can be connected to
> * before calling drm_dev_register().
> *
> + * As an exception to the above rule if any crtc can be connected to
> + * the encoder the driver can leave @possible_crtcs set to 0. The core
> + * will automagically fix this up by setting the bit for every crtc.
> + *
> * You will get a WARN if you get this wrong in the driver.
> *
> * Note that since CRTC objects can't be hotplugged the assigned indices
> --
> 2.24.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0
2020-02-11 17:05 ` Daniel Vetter
@ 2020-02-11 17:14 ` Ville Syrjälä
2020-02-12 9:07 ` Daniel Vetter
0 siblings, 1 reply; 28+ messages in thread
From: Ville Syrjälä @ 2020-02-11 17:14 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Thomas Zimmermann, dri-devel
On Tue, Feb 11, 2020 at 06:05:45PM +0100, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 06:22:08PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Let's simplify life of driver by allowing them to leave
> > encoder->possible_crtcs unset if they have no restrictions
> > in crtc<->encoder linkage. We'll just populate possible_crtcs
> > with the full crtc mask when registering the encoder so that
> > userspace doesn't have to deal with drivers not populating
> > this correctly.
> >
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > We might not actually need/want this, but included it here for
> > future reference if that assumption turns out to be wrong.
>
> I think this one is most definitely needed. _Lots_ of drivers get this
> toally wrong and just leave the value blank. It's encoded as official
> fallback in most userspace compositors.
OK. It's been a while since I dug around so can't really remmber how
this was being handled. I'll reorder before pushing.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> > ---
> > drivers/gpu/drm/drm_mode_config.c | 15 ++++++++++++++-
> > include/drm/drm_encoder.h | 4 ++++
> > 2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 4c1b350ddb95..ce18c3dd0bde 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -592,6 +592,17 @@ static u32 full_crtc_mask(struct drm_device *dev)
> > return crtc_mask;
> > }
> >
> > +/*
> > + * Make life easy for drivers by allowing them to leave
> > + * possible_crtcs unset if there are not crtc<->encoder
> > + * restrictions.
> > + */
> > +static void fixup_encoder_possible_crtcs(struct drm_encoder *encoder)
> > +{
> > + if (encoder->possible_crtcs == 0)
> > + encoder->possible_crtcs = full_crtc_mask(encoder->dev);
> > +}
> > +
> > static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
> > {
> > u32 crtc_mask = full_crtc_mask(encoder->dev);
> > @@ -608,8 +619,10 @@ void drm_mode_config_validate(struct drm_device *dev)
> > {
> > struct drm_encoder *encoder;
> >
> > - drm_for_each_encoder(encoder, dev)
> > + drm_for_each_encoder(encoder, dev) {
> > fixup_encoder_possible_clones(encoder);
> > + fixup_encoder_possible_crtcs(encoder);
> > + }
> >
> > drm_for_each_encoder(encoder, dev) {
> > validate_encoder_possible_clones(encoder);
> > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > index b236269f41ac..bd033c5618bf 100644
> > --- a/include/drm/drm_encoder.h
> > +++ b/include/drm/drm_encoder.h
> > @@ -142,6 +142,10 @@ struct drm_encoder {
> > * the bits for all &drm_crtc objects this encoder can be connected to
> > * before calling drm_dev_register().
> > *
> > + * As an exception to the above rule if any crtc can be connected to
> > + * the encoder the driver can leave @possible_crtcs set to 0. The core
> > + * will automagically fix this up by setting the bit for every crtc.
> > + *
> > * You will get a WARN if you get this wrong in the driver.
> > *
> > * Note that since CRTC objects can't be hotplugged the assigned indices
> > --
> > 2.24.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0
2020-02-11 17:14 ` Ville Syrjälä
@ 2020-02-12 9:07 ` Daniel Vetter
2020-02-12 9:08 ` Daniel Vetter
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2020-02-12 9:07 UTC (permalink / raw)
To: Ville Syrjälä, Thierry Reding
Cc: Thomas Zimmermann, intel-gfx, dri-devel
On Tue, Feb 11, 2020 at 07:14:51PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2020 at 06:05:45PM +0100, Daniel Vetter wrote:
> > On Tue, Feb 11, 2020 at 06:22:08PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Let's simplify life of driver by allowing them to leave
> > > encoder->possible_crtcs unset if they have no restrictions
> > > in crtc<->encoder linkage. We'll just populate possible_crtcs
> > > with the full crtc mask when registering the encoder so that
> > > userspace doesn't have to deal with drivers not populating
> > > this correctly.
> > >
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > > We might not actually need/want this, but included it here for
> > > future reference if that assumption turns out to be wrong.
> >
> > I think this one is most definitely needed. _Lots_ of drivers get this
> > toally wrong and just leave the value blank. It's encoded as official
> > fallback in most userspace compositors.
>
> OK. It's been a while since I dug around so can't really remmber how
> this was being handled. I'll reorder before pushing.
Hm otoh having "works with all crtcs" as default is a bit dangerous,
whereas the "cannot be cloned" default for possible_clones is perfectly
safe.
So now I'm kinda not sure whether this is a bright idea, and we shouldn't
just eat the cost of fixing up all the various WARNING backtraces your
previous patch triggers. I've done a full review and the following look
suspect:
- tegara/sor.c Strangely it's the only one, the other output drivers do
seem to set the possible_crtcs mask to something useful.
Everything else seems to be fine. I'd say let's drop this patch, and I'm
adding Thierry to shed some light on what's going on in tegra/sor.c.
-Daniel
>
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > > ---
> > > drivers/gpu/drm/drm_mode_config.c | 15 ++++++++++++++-
> > > include/drm/drm_encoder.h | 4 ++++
> > > 2 files changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index 4c1b350ddb95..ce18c3dd0bde 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -592,6 +592,17 @@ static u32 full_crtc_mask(struct drm_device *dev)
> > > return crtc_mask;
> > > }
> > >
> > > +/*
> > > + * Make life easy for drivers by allowing them to leave
> > > + * possible_crtcs unset if there are not crtc<->encoder
> > > + * restrictions.
> > > + */
> > > +static void fixup_encoder_possible_crtcs(struct drm_encoder *encoder)
> > > +{
> > > + if (encoder->possible_crtcs == 0)
> > > + encoder->possible_crtcs = full_crtc_mask(encoder->dev);
> > > +}
> > > +
> > > static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
> > > {
> > > u32 crtc_mask = full_crtc_mask(encoder->dev);
> > > @@ -608,8 +619,10 @@ void drm_mode_config_validate(struct drm_device *dev)
> > > {
> > > struct drm_encoder *encoder;
> > >
> > > - drm_for_each_encoder(encoder, dev)
> > > + drm_for_each_encoder(encoder, dev) {
> > > fixup_encoder_possible_clones(encoder);
> > > + fixup_encoder_possible_crtcs(encoder);
> > > + }
> > >
> > > drm_for_each_encoder(encoder, dev) {
> > > validate_encoder_possible_clones(encoder);
> > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > > index b236269f41ac..bd033c5618bf 100644
> > > --- a/include/drm/drm_encoder.h
> > > +++ b/include/drm/drm_encoder.h
> > > @@ -142,6 +142,10 @@ struct drm_encoder {
> > > * the bits for all &drm_crtc objects this encoder can be connected to
> > > * before calling drm_dev_register().
> > > *
> > > + * As an exception to the above rule if any crtc can be connected to
> > > + * the encoder the driver can leave @possible_crtcs set to 0. The core
> > > + * will automagically fix this up by setting the bit for every crtc.
> > > + *
> > > * You will get a WARN if you get this wrong in the driver.
> > > *
> > > * Note that since CRTC objects can't be hotplugged the assigned indices
> > > --
> > > 2.24.1
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0
2020-02-12 9:07 ` Daniel Vetter
@ 2020-02-12 9:08 ` Daniel Vetter
2020-03-18 16:44 ` Ville Syrjälä
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2020-02-12 9:08 UTC (permalink / raw)
To: Ville Syrjälä, Thierry Reding
Cc: Thomas Zimmermann, intel-gfx, dri-devel
On Wed, Feb 12, 2020 at 10:07:55AM +0100, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 07:14:51PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 11, 2020 at 06:05:45PM +0100, Daniel Vetter wrote:
> > > On Tue, Feb 11, 2020 at 06:22:08PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Let's simplify life of driver by allowing them to leave
> > > > encoder->possible_crtcs unset if they have no restrictions
> > > > in crtc<->encoder linkage. We'll just populate possible_crtcs
> > > > with the full crtc mask when registering the encoder so that
> > > > userspace doesn't have to deal with drivers not populating
> > > > this correctly.
> > > >
> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > > We might not actually need/want this, but included it here for
> > > > future reference if that assumption turns out to be wrong.
> > >
> > > I think this one is most definitely needed. _Lots_ of drivers get this
> > > toally wrong and just leave the value blank. It's encoded as official
> > > fallback in most userspace compositors.
> >
> > OK. It's been a while since I dug around so can't really remmber how
> > this was being handled. I'll reorder before pushing.
>
> Hm otoh having "works with all crtcs" as default is a bit dangerous,
> whereas the "cannot be cloned" default for possible_clones is perfectly
> safe.
>
> So now I'm kinda not sure whether this is a bright idea, and we shouldn't
> just eat the cost of fixing up all the various WARNING backtraces your
> previous patch triggers. I've done a full review and the following look
> suspect:
>
> - tegara/sor.c Strangely it's the only one, the other output drivers do
> seem to set the possible_crtcs mask to something useful.
Strike that, it sets it using tegra_output_find_possible_crtcs().
I think everything is good and we really don't need this patch here to fix
up possible_crtcs.
-Daniel
>
> Everything else seems to be fine. I'd say let's drop this patch, and I'm
> adding Thierry to shed some light on what's going on in tegra/sor.c.
> -Daniel
>
> >
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > > ---
> > > > drivers/gpu/drm/drm_mode_config.c | 15 ++++++++++++++-
> > > > include/drm/drm_encoder.h | 4 ++++
> > > > 2 files changed, 18 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > > index 4c1b350ddb95..ce18c3dd0bde 100644
> > > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > > @@ -592,6 +592,17 @@ static u32 full_crtc_mask(struct drm_device *dev)
> > > > return crtc_mask;
> > > > }
> > > >
> > > > +/*
> > > > + * Make life easy for drivers by allowing them to leave
> > > > + * possible_crtcs unset if there are not crtc<->encoder
> > > > + * restrictions.
> > > > + */
> > > > +static void fixup_encoder_possible_crtcs(struct drm_encoder *encoder)
> > > > +{
> > > > + if (encoder->possible_crtcs == 0)
> > > > + encoder->possible_crtcs = full_crtc_mask(encoder->dev);
> > > > +}
> > > > +
> > > > static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
> > > > {
> > > > u32 crtc_mask = full_crtc_mask(encoder->dev);
> > > > @@ -608,8 +619,10 @@ void drm_mode_config_validate(struct drm_device *dev)
> > > > {
> > > > struct drm_encoder *encoder;
> > > >
> > > > - drm_for_each_encoder(encoder, dev)
> > > > + drm_for_each_encoder(encoder, dev) {
> > > > fixup_encoder_possible_clones(encoder);
> > > > + fixup_encoder_possible_crtcs(encoder);
> > > > + }
> > > >
> > > > drm_for_each_encoder(encoder, dev) {
> > > > validate_encoder_possible_clones(encoder);
> > > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > > > index b236269f41ac..bd033c5618bf 100644
> > > > --- a/include/drm/drm_encoder.h
> > > > +++ b/include/drm/drm_encoder.h
> > > > @@ -142,6 +142,10 @@ struct drm_encoder {
> > > > * the bits for all &drm_crtc objects this encoder can be connected to
> > > > * before calling drm_dev_register().
> > > > *
> > > > + * As an exception to the above rule if any crtc can be connected to
> > > > + * the encoder the driver can leave @possible_crtcs set to 0. The core
> > > > + * will automagically fix this up by setting the bit for every crtc.
> > > > + *
> > > > * You will get a WARN if you get this wrong in the driver.
> > > > *
> > > > * Note that since CRTC objects can't be hotplugged the assigned indices
> > > > --
> > > > 2.24.1
> > > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Ville Syrjälä
> > Intel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0
2020-02-12 9:08 ` Daniel Vetter
@ 2020-03-18 16:44 ` Ville Syrjälä
0 siblings, 0 replies; 28+ messages in thread
From: Ville Syrjälä @ 2020-03-18 16:44 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Thierry Reding, Thomas Zimmermann, dri-devel
On Wed, Feb 12, 2020 at 10:08:49AM +0100, Daniel Vetter wrote:
> On Wed, Feb 12, 2020 at 10:07:55AM +0100, Daniel Vetter wrote:
> > On Tue, Feb 11, 2020 at 07:14:51PM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 11, 2020 at 06:05:45PM +0100, Daniel Vetter wrote:
> > > > On Tue, Feb 11, 2020 at 06:22:08PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Let's simplify life of driver by allowing them to leave
> > > > > encoder->possible_crtcs unset if they have no restrictions
> > > > > in crtc<->encoder linkage. We'll just populate possible_crtcs
> > > > > with the full crtc mask when registering the encoder so that
> > > > > userspace doesn't have to deal with drivers not populating
> > > > > this correctly.
> > > > >
> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > > We might not actually need/want this, but included it here for
> > > > > future reference if that assumption turns out to be wrong.
> > > >
> > > > I think this one is most definitely needed. _Lots_ of drivers get this
> > > > toally wrong and just leave the value blank. It's encoded as official
> > > > fallback in most userspace compositors.
> > >
> > > OK. It's been a while since I dug around so can't really remmber how
> > > this was being handled. I'll reorder before pushing.
> >
> > Hm otoh having "works with all crtcs" as default is a bit dangerous,
> > whereas the "cannot be cloned" default for possible_clones is perfectly
> > safe.
> >
> > So now I'm kinda not sure whether this is a bright idea, and we shouldn't
> > just eat the cost of fixing up all the various WARNING backtraces your
> > previous patch triggers. I've done a full review and the following look
> > suspect:
> >
> > - tegara/sor.c Strangely it's the only one, the other output drivers do
> > seem to set the possible_crtcs mask to something useful.
>
> Strike that, it sets it using tegra_output_find_possible_crtcs().
>
> I think everything is good and we really don't need this patch here to fix
> up possible_crtcs.
Finally pushed the other patches from the series to drm-misc-next.
Thanks for the reviews.
Should the new possible_{crtcs,clones} WARNs start to trigger for
anyone despite our best efforts, please holler and I'll look into
what needs fixing.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm: Try to fix encoder possible_clones/crtc (rev4)
2020-02-11 16:22 [Intel-gfx] [PATCH v3 0/7] drm: Try to fix encoder possible_clones/crtc Ville Syrjala
` (6 preceding siblings ...)
2020-02-11 16:22 ` [Intel-gfx] [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0 Ville Syrjala
@ 2020-12-03 22:16 ` Patchwork
7 siblings, 0 replies; 28+ messages in thread
From: Patchwork @ 2020-12-03 22:16 UTC (permalink / raw)
To: Alex Deucher; +Cc: intel-gfx
== Series Details ==
Series: drm: Try to fix encoder possible_clones/crtc (rev4)
URL : https://patchwork.freedesktop.org/series/63399/
State : failure
== Summary ==
Applying: drm: Include the encoder itself in possible_clones
Using index info to reconstruct a base tree...
M drivers/gpu/drm/drm_crtc_internal.h
M drivers/gpu/drm/drm_drv.c
M drivers/gpu/drm/drm_mode_config.c
M include/drm/drm_encoder.h
Falling back to patching base and 3-way merge...
Auto-merging include/drm/drm_encoder.h
CONFLICT (content): Merge conflict in include/drm/drm_encoder.h
Auto-merging drivers/gpu/drm/drm_mode_config.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/drm_mode_config.c
Auto-merging drivers/gpu/drm/drm_drv.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/drm_drv.c
Auto-merging drivers/gpu/drm/drm_crtc_internal.h
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm: Include the encoder itself in possible_clones
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread