From: Ville Syrjälä ville.syrjala@linux.intel.com
Remainder of my possible_clones/crtcs cleanup. All the i915 bits and a few other driver bits got merged already.
Ville Syrjälä (6): drm: Include the encoder itself in possible_clones drm/gma500: Sanitize possible_clones drm/exynos: Use drm_encoder_mask() drm/imx: Remove the bogus possible_clones setup drm: Validate encoder->possible_clones drm: Validate encoder->possible_crtcs
drivers/gpu/drm/drm_encoder.c | 63 +++++++++++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 +- drivers/gpu/drm/gma500/framebuffer.c | 16 +++---- drivers/gpu/drm/gma500/mdfld_dsi_dpi.c | 4 +- drivers/gpu/drm/imx/imx-drm-core.c | 2 +- 5 files changed, 76 insertions(+), 14 deletions(-)
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 encoders.
TODO: Should we do something similar for possible_crtcs==0?
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_encoder.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index e555281f43d4..f761d9306028 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { { DRM_MODE_ENCODER_DPI, "DPI" }, };
+/* + * 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_possible_clones(struct drm_device *dev) +{ + struct drm_encoder *encoder; + + drm_for_each_encoder(encoder, dev) + encoder->possible_clones |= drm_encoder_mask(encoder); +} + int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; int ret = 0;
+ fixup_possible_clones(dev); + drm_for_each_encoder(encoder, dev) { if (encoder->funcs->late_register) ret = encoder->funcs->late_register(encoder);
Hi
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
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 encoders.
TODO: Should we do something similar for possible_crtcs==0?
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
May this fixup function should warn iff possible_clones was set to non-0 by the driver, but the encoder itself is missing. In any case
Acked-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index e555281f43d4..f761d9306028 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { { DRM_MODE_ENCODER_DPI, "DPI" }, };
+/*
- 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_possible_clones(struct drm_device *dev) +{
- struct drm_encoder *encoder;
- drm_for_each_encoder(encoder, dev)
encoder->possible_clones |= drm_encoder_mask(encoder);+}
int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; int ret = 0;
- fixup_possible_clones(dev);
- drm_for_each_encoder(encoder, dev) { if (encoder->funcs->late_register) ret = encoder->funcs->late_register(encoder);
On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
Hi
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
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 encoders.
TODO: Should we do something similar for possible_crtcs==0?
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
May this fixup function should warn iff possible_clones was set to non-0 by the driver, but the encoder itself is missing.
Yeah, I guess we could do that.
In any case
Acked-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index e555281f43d4..f761d9306028 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { { DRM_MODE_ENCODER_DPI, "DPI" }, };
+/*
- 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_possible_clones(struct drm_device *dev) +{
- struct drm_encoder *encoder;
- drm_for_each_encoder(encoder, dev)
encoder->possible_clones |= drm_encoder_mask(encoder);+}
int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; int ret = 0;
- fixup_possible_clones(dev);
- drm_for_each_encoder(encoder, dev) { if (encoder->funcs->late_register) ret = encoder->funcs->late_register(encoder);
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
Hi
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
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 encoders.
TODO: Should we do something similar for possible_crtcs==0?
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
May this fixup function should warn iff possible_clones was set to non-0 by the driver, but the encoder itself is missing.
Yeah, I guess we could do that.
+1 on that, should catch some bugs at least.
Also can you pls fix up the kerneldoc for drm_encoder.possible_clones, defacto this now means that 0 is a totally fine setting. -Daniel
In any case
Acked-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index e555281f43d4..f761d9306028 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { { DRM_MODE_ENCODER_DPI, "DPI" }, };
+/*
- 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_possible_clones(struct drm_device *dev) +{
- struct drm_encoder *encoder;
- drm_for_each_encoder(encoder, dev)
encoder->possible_clones |= drm_encoder_mask(encoder);+}
int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; int ret = 0;
- fixup_possible_clones(dev);
- drm_for_each_encoder(encoder, dev) { if (encoder->funcs->late_register) ret = encoder->funcs->late_register(encoder);
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
-- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Feb 07, 2020 at 05:27:51PM +0100, Daniel Vetter wrote:
On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
Hi
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
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 encoders.
TODO: Should we do something similar for possible_crtcs==0?
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
May this fixup function should warn iff possible_clones was set to non-0 by the driver, but the encoder itself is missing.
Yeah, I guess we could do that.
+1 on that, should catch some bugs at least.
Also can you pls fix up the kerneldoc for drm_encoder.possible_clones, defacto this now means that 0 is a totally fine setting.
Sure.
And for possible_crtcs I was thinking similar concept:
for_each_encoder() if (possible_crtc == 0) possible_crtcs = all_crtc_mask;
On Fri, Feb 07, 2020 at 06:34:47PM +0200, Ville Syrjälä wrote:
On Fri, Feb 07, 2020 at 05:27:51PM +0100, Daniel Vetter wrote:
On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
Hi
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
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 encoders.
TODO: Should we do something similar for possible_crtcs==0?
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
May this fixup function should warn iff possible_clones was set to non-0 by the driver, but the encoder itself is missing.
Yeah, I guess we could do that.
+1 on that, should catch some bugs at least.
Also can you pls fix up the kerneldoc for drm_encoder.possible_clones, defacto this now means that 0 is a totally fine setting.
Sure.
And for possible_crtcs I was thinking similar concept:
for_each_encoder() if (possible_crtc == 0) possible_crtcs = all_crtc_mask;
A quick grep shows that I think we can risk enforcing this. If that turns out to be a misconception we can always go back to the fixup approach if possible_crtcs is 0. But unlike possible_clones I think for possible_crtcs the fixup-less approach looks possible at least. -Daniel
Hi
Am 07.02.20 um 17:27 schrieb Daniel Vetter:
On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
Hi
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
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 encoders.
TODO: Should we do something similar for possible_crtcs==0?
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
May this fixup function should warn iff possible_clones was set to non-0 by the driver, but the encoder itself is missing.
Yeah, I guess we could do that.
+1 on that, should catch some bugs at least.
Also can you pls fix up the kerneldoc for drm_encoder.possible_clones, defacto this now means that 0 is a totally fine setting.
Just a thought: we should encourage, or maybe even require, the use of such defaults (i.e., 0); unless a driver has a good reason to override them. So we don't have to fix drivers if the defaults ever change.
Best regards Thomas
-Daniel
In any case
Acked-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index e555281f43d4..f761d9306028 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { { DRM_MODE_ENCODER_DPI, "DPI" }, };
+/*
- 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_possible_clones(struct drm_device *dev) +{
- struct drm_encoder *encoder;
- drm_for_each_encoder(encoder, dev)
encoder->possible_clones |= drm_encoder_mask(encoder);+}
int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; int ret = 0;
- fixup_possible_clones(dev);
- drm_for_each_encoder(encoder, dev) { if (encoder->funcs->late_register) ret = encoder->funcs->late_register(encoder);
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
-- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Feb 07, 2020 at 03:59:45PM +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 encoders.
TODO: Should we do something similar for possible_crtcs==0?
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_encoder.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index e555281f43d4..f761d9306028 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = { { DRM_MODE_ENCODER_DPI, "DPI" }, };
+/*
- 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_possible_clones(struct drm_device *dev) +{
- struct drm_encoder *encoder;
- drm_for_each_encoder(encoder, dev)
encoder->possible_clones |= drm_encoder_mask(encoder);+}
int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; int ret = 0;
- fixup_possible_clones(dev);
This is way too late, we've already registered the chardev minors at this point. I think we need a new drm_mode_config_validate() at the top of drm_dev_register, but which does _not_ run when the driver has a ->load callback (which soon will be no driver at all).
Cheers, Daniel
- drm_for_each_encoder(encoder, dev) { if (encoder->funcs->late_register) ret = encoder->funcs->late_register(encoder);
-- 2.24.1
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 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;
On Fri, Feb 07, 2020 at 03:59:46PM +0200, Ville Syrjala wrote:
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 Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Looks reasonable.
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
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;
crtc_mask = dev_priv->ops->lvds_mask;clone_mask = (1 << INTEL_OUTPUT_LVDS);
crtc_mask = dev_priv->ops->lvds_mask;clone_mask = 0; break;
clone_mask = (1 << INTEL_OUTPUT_MIPI);
clone_mask = 0; break;
clone_mask = (1 << INTEL_OUTPUT_MIPI2);
clone_mask = 0; break;
crtc_mask = dev_priv->ops->hdmi_mask;
crtc_mask = dev_priv->ops->hdmi_mask; clone_mask = (1 << INTEL_OUTPUT_HDMI); break;
clone_mask = (1 << INTEL_OUTPUT_DISPLAYPORT);
clone_mask = 0; break;
clone_mask = (1 << INTEL_OUTPUT_EDP);
clone_mask = 0;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;
encoder->possible_clones = (1 << 0);
encoder->possible_clones = 0;}
dsi_connector->base.encoder = &dpi_output->base.base;
-- 2.24.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 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;
Am 07.02.20 um 14:59 schrieb 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 Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Acked-by: Thomas Zimmermann tzimmermann@suse.de
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;
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.
Cc: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/imx/imx-drm-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index da87c70e413b..a0a709dfba34 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -140,7 +140,7 @@ 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; + encoder->possible_clones = 0;
return 0; }
Hi
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
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.
Cc: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/imx/imx-drm-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index da87c70e413b..a0a709dfba34 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -140,7 +140,7 @@ 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;
- encoder->possible_clones = 0;
Maybe remove the comment as well. It's pointless.
Best regards Thomas
return 0; }
On Fri, Feb 07, 2020 at 03:20:44PM +0100, Thomas Zimmermann wrote:
Hi
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
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.
Cc: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/imx/imx-drm-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index da87c70e413b..a0a709dfba34 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -140,7 +140,7 @@ 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;
- encoder->possible_clones = 0;
Maybe remove the comment as well. It's pointless.
Maybe change it to "FIXME: cloning support not clear, disable it all for now". With that
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
Best regards Thomas
return 0; }
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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.
TODO: Or should we just silently filter out any bits for non-existing encoders?
v2: encoder->possible_clones now includes the encoder itelf
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_encoder.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index f761d9306028..bc2246f27e0d 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -79,6 +79,34 @@ static void fixup_possible_clones(struct drm_device *dev) encoder->possible_clones |= drm_encoder_mask(encoder); }
+static void validate_possible_clones(struct drm_encoder *encoder) +{ + struct drm_device *dev = encoder->dev; + struct drm_encoder *other; + u32 encoder_mask = 0; + + drm_for_each_encoder(other, dev) { + encoder_mask |= drm_encoder_mask(other); + + 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); +} + int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; @@ -87,6 +115,8 @@ int drm_encoder_register_all(struct drm_device *dev) fixup_possible_clones(dev);
drm_for_each_encoder(encoder, dev) { + validate_possible_clones(encoder); + if (encoder->funcs->late_register) ret = encoder->funcs->late_register(encoder); if (ret)
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
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.
TODO: Or should we just silently filter out any bits for non-existing encoders?
Please be noisy. Setting these masks incorrectly could be a bug.
v2: encoder->possible_clones now includes the encoder itelf
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Acked-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_encoder.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index f761d9306028..bc2246f27e0d 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -79,6 +79,34 @@ static void fixup_possible_clones(struct drm_device *dev) encoder->possible_clones |= drm_encoder_mask(encoder); }
+static void validate_possible_clones(struct drm_encoder *encoder) +{
- struct drm_device *dev = encoder->dev;
- struct drm_encoder *other;
- u32 encoder_mask = 0;
- drm_for_each_encoder(other, dev) {
encoder_mask |= drm_encoder_mask(other);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);+}
int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; @@ -87,6 +115,8 @@ int drm_encoder_register_all(struct drm_device *dev) fixup_possible_clones(dev);
drm_for_each_encoder(encoder, dev) {
validate_possible_clones(encoder);- if (encoder->funcs->late_register) ret = encoder->funcs->late_register(encoder); if (ret)
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.
TODO: Or should we perhapst just filter out any bit for a non-exisiting crtc?
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_encoder.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index bc2246f27e0d..f16b2a2518d7 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -107,6 +107,23 @@ static void validate_possible_clones(struct drm_encoder *encoder) encoder->possible_clones, encoder_mask); }
+static void validate_possible_crtcs(struct drm_encoder *encoder) +{ + struct drm_device *dev = encoder->dev; + struct drm_crtc *crtc; + u32 crtc_mask = 0; + + drm_for_each_crtc(crtc, dev) + crtc_mask |= drm_crtc_mask(crtc); + + 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); +} + int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; @@ -115,6 +132,7 @@ int drm_encoder_register_all(struct drm_device *dev) fixup_possible_clones(dev);
drm_for_each_encoder(encoder, dev) { + validate_possible_crtcs(encoder); validate_possible_clones(encoder);
if (encoder->funcs->late_register)
On Fri, Feb 07, 2020 at 03:59:50PM +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.
TODO: Or should we perhapst just filter out any bit for a non-exisiting crtc?
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
From a quick grep it looks like at least most drivers seem to get this
right. Worth a shot to find the hold-outs.
Two things: - Imo also best to move into the drm_mode_config_validate I suggested. - Please update the kerneldoc for drm_encoder.possible_crtcs to mention that this will WARN if you get it wrong (and maybe remove the line that most drivers screw this up).
Check itself lgtm. -Daniel
drivers/gpu/drm/drm_encoder.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index bc2246f27e0d..f16b2a2518d7 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -107,6 +107,23 @@ static void validate_possible_clones(struct drm_encoder *encoder) encoder->possible_clones, encoder_mask); }
+static void validate_possible_crtcs(struct drm_encoder *encoder) +{
- struct drm_device *dev = encoder->dev;
- struct drm_crtc *crtc;
- u32 crtc_mask = 0;
- drm_for_each_crtc(crtc, dev)
crtc_mask |= drm_crtc_mask(crtc);- 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);+}
int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; @@ -115,6 +132,7 @@ int drm_encoder_register_all(struct drm_device *dev) fixup_possible_clones(dev);
drm_for_each_encoder(encoder, dev) {
validate_possible_crtcs(encoder);validate_possible_clones(encoder);
if (encoder->funcs->late_register)
-- 2.24.1
On Fri, Feb 07, 2020 at 05:39:26PM +0100, Daniel Vetter wrote:
On Fri, Feb 07, 2020 at 03:59:50PM +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.
TODO: Or should we perhapst just filter out any bit for a non-exisiting crtc?
Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
From a quick grep it looks like at least most drivers seem to get this
right. Worth a shot to find the hold-outs.
Two things:
- Imo also best to move into the drm_mode_config_validate I suggested.
- Please update the kerneldoc for drm_encoder.possible_crtcs to mention that this will WARN if you get it wrong (and maybe remove the line that most drivers screw this up).
ack
Check itself lgtm. -Daniel
drivers/gpu/drm/drm_encoder.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index bc2246f27e0d..f16b2a2518d7 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -107,6 +107,23 @@ static void validate_possible_clones(struct drm_encoder *encoder) encoder->possible_clones, encoder_mask); }
+static void validate_possible_crtcs(struct drm_encoder *encoder) +{
- struct drm_device *dev = encoder->dev;
- struct drm_crtc *crtc;
- u32 crtc_mask = 0;
- drm_for_each_crtc(crtc, dev)
crtc_mask |= drm_crtc_mask(crtc);- 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);+}
int drm_encoder_register_all(struct drm_device *dev) { struct drm_encoder *encoder; @@ -115,6 +132,7 @@ int drm_encoder_register_all(struct drm_device *dev) fixup_possible_clones(dev);
drm_for_each_encoder(encoder, dev) {
validate_possible_crtcs(encoder);validate_possible_clones(encoder);
if (encoder->funcs->late_register)
-- 2.24.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Thomas Zimmermann -
Ville Syrjala -
Ville Syrjälä