[PATCH 00/14] drm: Try to fix encoder possible_clones/crtcs

[PATCH 00/14] drm: Try to fix encoder possible_clones/crtcs

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apparently several drivers fill encoder->possible_clones with bogus
data, which means userspace can't trust that information at all.
Let's try to fix up some of the problems and add some sanity checks
into the core to persuade drivers to get it right from now on.
What we can't enforce is drivers actually implementing what they
advertize, so userspace would still have to take this information
with a grain of salt. But at least the information should be self
consistent.

I also figured we migth do something similar for possible_crtcs.
Although that is less important as there is no need for this to
match between two encoders. I also suspect the validation WARNs
would trip on some drivers. So we might not even want this part,
and instead we should just filter out bits for non-existing
crtcs.

Smoke tested on i915, and compile tested otherwise.

Pushed the lot here:
git://github.com/vsyrjala/linux.git possible_clones_clean

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Vincent Abriou <vincent.abriou@st.com>
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>
Cc: Philipp Zabel <p.zabel@pengutronix.de>

Ville Syrjälä (14):
  drm: Add drm_encoder_mask()
  drm: Include the encoder itself in possible_clones
  drm/gma500: Sanitize possible_clones
  drm/sti: Remove pointless casts
  drm/sti: Try to fix up the tvout possible clones
  drm/exynos: Use drm_encoder_mask()
  drm/imx: Remove the bogus possible_clones setup
  drm: Validate encoder->possible_clones
  drm/i915: Use drm_encoder_mask()
  drm/i915: Populate possible_crtcs correctly
  drm/i915: Fix DP-MST crtc_mask
  drm/i915: Clean up encoder->crtc_mask setup
  drm/i915: Simplfy LVDS crtc_mask setup
  drm: Validate encoder->possible_crtcs

 drivers/gpu/drm/drm_encoder.c           | 66 ++++++++++++++++++++++++++++++++-
 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/i915/intel_crt.c        |  4 +-
 drivers/gpu/drm/i915/intel_ddi.c        |  2 +-
 drivers/gpu/drm/i915/intel_display.c    | 20 ++++++++--
 drivers/gpu/drm/i915/intel_dp.c         |  6 +--
 drivers/gpu/drm/i915/intel_dp_mst.c     |  2 +-
 drivers/gpu/drm/i915/intel_dvo.c        |  2 +-
 drivers/gpu/drm/i915/intel_hdmi.c       |  6 +--
 drivers/gpu/drm/i915/intel_lvds.c       |  8 ++--
 drivers/gpu/drm/i915/intel_sdvo.c       |  2 +-
 drivers/gpu/drm/i915/intel_tv.c         |  3 +-
 drivers/gpu/drm/imx/imx-drm-core.c      |  2 +-
 drivers/gpu/drm/sti/sti_tvout.c         | 15 +++++---
 include/drm/drm_encoder.h               | 14 ++++++-
 17 files changed, 132 insertions(+), 45 deletions(-)

-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/14] drm: Add drm_encoder_mask()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add drm_encoder_mask() which returns the 1<<index for the encoder.
We already have an identical drm_crtc_mask() for crtcs.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_encoder.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index fb299696c7c4..d804a6897f4b 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -191,11 +191,23 @@ int drm_encoder_init(struct drm_device *dev,
  * Given a registered encoder, return the index of that encoder within a DRM
  * device's list of encoders.
  */
-static inline unsigned int drm_encoder_index(struct drm_encoder *encoder)
+static inline unsigned int drm_encoder_index(const struct drm_encoder *encoder)
 {
 	return encoder->index;
 }
 
+/**
+ * drm_encoder_mask - find the mask of a registered encoder
+ * @encoder: encoder to find mask for
+ *
+ * Given a registered encoder, return the mask bit of that encoder for an
+ * encoder's possible_clones field.
+ */
+static inline uint32_t drm_encoder_mask(const struct drm_encoder *encoder)
+{
+	return 1 << drm_encoder_index(encoder);
+}
+
 /**
  * drm_encoder_crtc_ok - can a given crtc drive a given encoder?
  * @encoder: encoder to test
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/14] drm: Include the encoder itself in possible_clones

[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 the fallback whenever someone wants to look
at possible_clones. Or maybe we could do it in the late register?

TODO: Should we add the bit even if possible_clones was otherwise
populated by the driver?

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_encoder.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 273e1c59c54a..ad13c9cf4312 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -63,6 +63,19 @@ 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 u32 encoder_possible_clones(struct drm_encoder *encoder)
+{
+	if (encoder->possible_clones)
+		return encoder->possible_clones;
+	else
+		return drm_encoder_mask(encoder);
+}
+
 int drm_encoder_register_all(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
@@ -240,7 +253,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 	enc_resp->encoder_id = encoder->base.id;
 	enc_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv,
 							  encoder->possible_crtcs);
-	enc_resp->possible_clones = encoder->possible_clones;
+	enc_resp->possible_clones = encoder_possible_clones(encoder);
 
 	return 0;
 }
-- 
2.16.4

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

[PATCH 03/14] drm/gma500: Sanitize possible_clones

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I double 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 8fa4ef192c1e..4072dc3dfe23 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -606,31 +606,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 a05c020602bd..94e362da3384 100644
--- a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
+++ b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
@@ -1004,10 +1004,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.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/14] drm/sti: Remove pointless casts

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There's no point in the cast for accessing the base class. Just
take the address of the struct instead.

Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Vincent Abriou <vincent.abriou@st.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/sti/sti_tvout.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
index ea4a3b87fa55..7d495307fe79 100644
--- a/drivers/gpu/drm/sti/sti_tvout.c
+++ b/drivers/gpu/drm/sti/sti_tvout.c
@@ -665,7 +665,7 @@ sti_tvout_create_dvo_encoder(struct drm_device *dev,
 
 	encoder->tvout = tvout;
 
-	drm_encoder = (struct drm_encoder *)encoder;
+	drm_encoder = &encoder->encoder;
 
 	drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
 	drm_encoder->possible_clones = 1 << 0;
@@ -718,7 +718,7 @@ static struct drm_encoder *sti_tvout_create_hda_encoder(struct drm_device *dev,
 
 	encoder->tvout = tvout;
 
-	drm_encoder = (struct drm_encoder *) encoder;
+	drm_encoder = &encoder->encoder;
 
 	drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
 	drm_encoder->possible_clones = 1 << 0;
@@ -767,7 +767,7 @@ static struct drm_encoder *sti_tvout_create_hdmi_encoder(struct drm_device *dev,
 
 	encoder->tvout = tvout;
 
-	drm_encoder = (struct drm_encoder *) encoder;
+	drm_encoder = &encoder->encoder;
 
 	drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
 	drm_encoder->possible_clones = 1 << 1;
-- 
2.16.4

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

Re: [PATCH 04/14] drm/sti: Remove pointless casts

[Benjamin Gaignard]

2018-06-15 18:49 GMT+02:00 Ville Syrjala <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> There's no point in the cast for accessing the base class. Just
> take the address of the struct instead.
>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

Thanks.

> ---
>  drivers/gpu/drm/sti/sti_tvout.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
> index ea4a3b87fa55..7d495307fe79 100644
> --- a/drivers/gpu/drm/sti/sti_tvout.c
> +++ b/drivers/gpu/drm/sti/sti_tvout.c
> @@ -665,7 +665,7 @@ sti_tvout_create_dvo_encoder(struct drm_device *dev,
>
>         encoder->tvout = tvout;
>
> -       drm_encoder = (struct drm_encoder *)encoder;
> +       drm_encoder = &encoder->encoder;
>
>         drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
>         drm_encoder->possible_clones = 1 << 0;
> @@ -718,7 +718,7 @@ static struct drm_encoder *sti_tvout_create_hda_encoder(struct drm_device *dev,
>
>         encoder->tvout = tvout;
>
> -       drm_encoder = (struct drm_encoder *) encoder;
> +       drm_encoder = &encoder->encoder;
>
>         drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
>         drm_encoder->possible_clones = 1 << 0;
> @@ -767,7 +767,7 @@ static struct drm_encoder *sti_tvout_create_hdmi_encoder(struct drm_device *dev,
>
>         encoder->tvout = tvout;
>
> -       drm_encoder = (struct drm_encoder *) encoder;
> +       drm_encoder = &encoder->encoder;
>
>         drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
>         drm_encoder->possible_clones = 1 << 1;
> --
> 2.16.4
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/14] drm/sti: Try to fix up the tvout possible clones

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current possible_clones setup doesn't look sensible. I'm assuming
the 0 and 1 are supposed to refer to the indexes of the hdmi and hda
encoders? So it kinda looks like we want hda+hdmi cloning, but then
dvo also claims to be cloneable with hdmi, but hdmi won't recipricate.

So let's assume we just want hdmi+hda and nothing more and populate the
possible_clones to match. The docs say the encoder itself should always
be included in the possible_clones bitmask so we'll throw it in there
as well. For hda we'll leave possible_clones==0 and we'll add a global
fallback for that case to add the encoder itself to the bitmask since
that makes life easier for most drivers.

Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Vincent Abriou <vincent.abriou@st.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/sti/sti_tvout.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
index 7d495307fe79..2359c111c7ed 100644
--- a/drivers/gpu/drm/sti/sti_tvout.c
+++ b/drivers/gpu/drm/sti/sti_tvout.c
@@ -668,7 +668,6 @@ sti_tvout_create_dvo_encoder(struct drm_device *dev,
 	drm_encoder = &encoder->encoder;
 
 	drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
-	drm_encoder->possible_clones = 1 << 0;
 
 	drm_encoder_init(dev, drm_encoder,
 			 &sti_tvout_encoder_funcs, DRM_MODE_ENCODER_LVDS,
@@ -721,7 +720,6 @@ static struct drm_encoder *sti_tvout_create_hda_encoder(struct drm_device *dev,
 	drm_encoder = &encoder->encoder;
 
 	drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
-	drm_encoder->possible_clones = 1 << 0;
 
 	drm_encoder_init(dev, drm_encoder,
 			&sti_tvout_encoder_funcs, DRM_MODE_ENCODER_DAC, NULL);
@@ -770,7 +768,6 @@ static struct drm_encoder *sti_tvout_create_hdmi_encoder(struct drm_device *dev,
 	drm_encoder = &encoder->encoder;
 
 	drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
-	drm_encoder->possible_clones = 1 << 1;
 
 	drm_encoder_init(dev, drm_encoder,
 			&sti_tvout_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL);
@@ -786,6 +783,12 @@ static void sti_tvout_create_encoders(struct drm_device *dev,
 	tvout->hdmi = sti_tvout_create_hdmi_encoder(dev, tvout);
 	tvout->hda = sti_tvout_create_hda_encoder(dev, tvout);
 	tvout->dvo = sti_tvout_create_dvo_encoder(dev, tvout);
+
+	/* FIXME what's the correct thing here? */
+	tvout->hdmi->possible_clones =
+		drm_encoder_mask(tvout->hdmi) | drm_encoder_mask(tvout->hda);
+	tvout->hda->possible_clones =
+		drm_encoder_mask(tvout->hdmi) | drm_encoder_mask(tvout->hda);
 }
 
 static void sti_tvout_destroy_encoders(struct sti_tvout *tvout)
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/14] drm/sti: Try to fix up the tvout possible clones

[Benjamin Gaignard]

2018-06-15 18:49 GMT+02:00 Ville Syrjala <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The current possible_clones setup doesn't look sensible. I'm assuming
> the 0 and 1 are supposed to refer to the indexes of the hdmi and hda
> encoders? So it kinda looks like we want hda+hdmi cloning, but then
> dvo also claims to be cloneable with hdmi, but hdmi won't recipricate.

All cloning combinaisons are possible: hdmi+hda, hda+dvo, hdmi+dvo.
The correct solution should be:
possible_clones = drm_encoder_mask(tvout->hdmi) |
drm_encoder_mask(tvout->hda) | drm_encoder_mask(tvout->dvo);

Thanks,
Benjamin

>
> So let's assume we just want hdmi+hda and nothing more and populate the
> possible_clones to match. The docs say the encoder itself should always
> be included in the possible_clones bitmask so we'll throw it in there
> as well. For hda we'll leave possible_clones==0 and we'll add a global
> fallback for that case to add the encoder itself to the bitmask since
> that makes life easier for most drivers.
>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/sti/sti_tvout.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
> index 7d495307fe79..2359c111c7ed 100644
> --- a/drivers/gpu/drm/sti/sti_tvout.c
> +++ b/drivers/gpu/drm/sti/sti_tvout.c
> @@ -668,7 +668,6 @@ sti_tvout_create_dvo_encoder(struct drm_device *dev,
>         drm_encoder = &encoder->encoder;
>
>         drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
> -       drm_encoder->possible_clones = 1 << 0;
>
>         drm_encoder_init(dev, drm_encoder,
>                          &sti_tvout_encoder_funcs, DRM_MODE_ENCODER_LVDS,
> @@ -721,7 +720,6 @@ static struct drm_encoder *sti_tvout_create_hda_encoder(struct drm_device *dev,
>         drm_encoder = &encoder->encoder;
>
>         drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
> -       drm_encoder->possible_clones = 1 << 0;
>
>         drm_encoder_init(dev, drm_encoder,
>                         &sti_tvout_encoder_funcs, DRM_MODE_ENCODER_DAC, NULL);
> @@ -770,7 +768,6 @@ static struct drm_encoder *sti_tvout_create_hdmi_encoder(struct drm_device *dev,
>         drm_encoder = &encoder->encoder;
>
>         drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
> -       drm_encoder->possible_clones = 1 << 1;
>
>         drm_encoder_init(dev, drm_encoder,
>                         &sti_tvout_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL);
> @@ -786,6 +783,12 @@ static void sti_tvout_create_encoders(struct drm_device *dev,
>         tvout->hdmi = sti_tvout_create_hdmi_encoder(dev, tvout);
>         tvout->hda = sti_tvout_create_hda_encoder(dev, tvout);
>         tvout->dvo = sti_tvout_create_dvo_encoder(dev, tvout);
> +
> +       /* FIXME what's the correct thing here? */
> +       tvout->hdmi->possible_clones =
> +               drm_encoder_mask(tvout->hdmi) | drm_encoder_mask(tvout->hda);
> +       tvout->hda->possible_clones =
> +               drm_encoder_mask(tvout->hdmi) | drm_encoder_mask(tvout->hda);
>  }
>
>  static void sti_tvout_destroy_encoders(struct sti_tvout *tvout)
> --
> 2.16.4
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 05/14] drm/sti: Try to fix up the tvout possible clones

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current possible_clones setup doesn't look sensible. I'm assuming
the 0 and 1 are supposed to refer to the indexes of the hdmi and hda
encoders? So it kinda looks like we want hda+hdmi cloning, but then
dvo also claims to be cloneable with hdmi, but hdmi won't recipricate.

Benjamin tells me all encoders should be cloneable with each other,
so let's fix up the masks to indicate that.

Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Vincent Abriou <vincent.abriou@st.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/sti/sti_tvout.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
index 7d495307fe79..8bca519b3bfe 100644
--- a/drivers/gpu/drm/sti/sti_tvout.c
+++ b/drivers/gpu/drm/sti/sti_tvout.c
@@ -668,7 +668,6 @@ sti_tvout_create_dvo_encoder(struct drm_device *dev,
 	drm_encoder = &encoder->encoder;
 
 	drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
-	drm_encoder->possible_clones = 1 << 0;
 
 	drm_encoder_init(dev, drm_encoder,
 			 &sti_tvout_encoder_funcs, DRM_MODE_ENCODER_LVDS,
@@ -721,7 +720,6 @@ static struct drm_encoder *sti_tvout_create_hda_encoder(struct drm_device *dev,
 	drm_encoder = &encoder->encoder;
 
 	drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
-	drm_encoder->possible_clones = 1 << 0;
 
 	drm_encoder_init(dev, drm_encoder,
 			&sti_tvout_encoder_funcs, DRM_MODE_ENCODER_DAC, NULL);
@@ -770,7 +768,6 @@ static struct drm_encoder *sti_tvout_create_hdmi_encoder(struct drm_device *dev,
 	drm_encoder = &encoder->encoder;
 
 	drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
-	drm_encoder->possible_clones = 1 << 1;
 
 	drm_encoder_init(dev, drm_encoder,
 			&sti_tvout_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL);
@@ -786,6 +783,13 @@ static void sti_tvout_create_encoders(struct drm_device *dev,
 	tvout->hdmi = sti_tvout_create_hdmi_encoder(dev, tvout);
 	tvout->hda = sti_tvout_create_hda_encoder(dev, tvout);
 	tvout->dvo = sti_tvout_create_dvo_encoder(dev, tvout);
+
+	tvout->hdmi->possible_clones = drm_encoder_mask(tvout->hdmi) |
+		drm_encoder_mask(tvout->hda) | drm_encoder_mask(tvout->dvo);
+	tvout->hda->possible_clones = drm_encoder_mask(tvout->hdmi) |
+		drm_encoder_mask(tvout->hda) | drm_encoder_mask(tvout->dvo);
+	tvout->dvo->possible_clones = drm_encoder_mask(tvout->hdmi) |
+		drm_encoder_mask(tvout->hda) | drm_encoder_mask(tvout->dvo);
 }
 
 static void sti_tvout_destroy_encoders(struct sti_tvout *tvout)
-- 
2.16.4

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

Re: [PATCH v2 05/14] drm/sti: Try to fix up the tvout possible clones

[Benjamin Gaignard]

2018-06-18 14:38 GMT+02:00 Ville Syrjala <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The current possible_clones setup doesn't look sensible. I'm assuming
> the 0 and 1 are supposed to refer to the indexes of the hdmi and hda
> encoders? So it kinda looks like we want hda+hdmi cloning, but then
> dvo also claims to be cloneable with hdmi, but hdmi won't recipricate.
>
> Benjamin tells me all encoders should be cloneable with each other,
> so let's fix up the masks to indicate that.
>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

Thanks

> ---
>  drivers/gpu/drm/sti/sti_tvout.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
> index 7d495307fe79..8bca519b3bfe 100644
> --- a/drivers/gpu/drm/sti/sti_tvout.c
> +++ b/drivers/gpu/drm/sti/sti_tvout.c
> @@ -668,7 +668,6 @@ sti_tvout_create_dvo_encoder(struct drm_device *dev,
>         drm_encoder = &encoder->encoder;
>
>         drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
> -       drm_encoder->possible_clones = 1 << 0;
>
>         drm_encoder_init(dev, drm_encoder,
>                          &sti_tvout_encoder_funcs, DRM_MODE_ENCODER_LVDS,
> @@ -721,7 +720,6 @@ static struct drm_encoder *sti_tvout_create_hda_encoder(struct drm_device *dev,
>         drm_encoder = &encoder->encoder;
>
>         drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
> -       drm_encoder->possible_clones = 1 << 0;
>
>         drm_encoder_init(dev, drm_encoder,
>                         &sti_tvout_encoder_funcs, DRM_MODE_ENCODER_DAC, NULL);
> @@ -770,7 +768,6 @@ static struct drm_encoder *sti_tvout_create_hdmi_encoder(struct drm_device *dev,
>         drm_encoder = &encoder->encoder;
>
>         drm_encoder->possible_crtcs = ENCODER_CRTC_MASK;
> -       drm_encoder->possible_clones = 1 << 1;
>
>         drm_encoder_init(dev, drm_encoder,
>                         &sti_tvout_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL);
> @@ -786,6 +783,13 @@ static void sti_tvout_create_encoders(struct drm_device *dev,
>         tvout->hdmi = sti_tvout_create_hdmi_encoder(dev, tvout);
>         tvout->hda = sti_tvout_create_hda_encoder(dev, tvout);
>         tvout->dvo = sti_tvout_create_dvo_encoder(dev, tvout);
> +
> +       tvout->hdmi->possible_clones = drm_encoder_mask(tvout->hdmi) |
> +               drm_encoder_mask(tvout->hda) | drm_encoder_mask(tvout->dvo);
> +       tvout->hda->possible_clones = drm_encoder_mask(tvout->hdmi) |
> +               drm_encoder_mask(tvout->hda) | drm_encoder_mask(tvout->dvo);
> +       tvout->dvo->possible_clones = drm_encoder_mask(tvout->hdmi) |
> +               drm_encoder_mask(tvout->hda) | drm_encoder_mask(tvout->dvo);
>  }
>
>  static void sti_tvout_destroy_encoders(struct sti_tvout *tvout)
> --
> 2.16.4
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/14] drm/exynos: Use drm_encoder_mask()

[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>
---
 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 a81b4a5e24a7..f76ae48c1156 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -319,7 +319,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))
@@ -359,10 +359,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.16.4

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

[PATCH 07/14] drm/imx: Remove the bogus possible_clones setup

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

dw_hdmi_imx_bind() does not call so the possible_clones mask being set
here is entirely bogus, not to mention the bits set for non-existent
encoders. Just clear possible_clones until someone fixes this properly.

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 1d053bbefc02..032cdc310a88 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -158,7 +158,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;
 }
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/14] drm: Validate encoder->possible_clones

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

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_encoder.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index ad13c9cf4312..1c289b227bbc 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -76,12 +76,45 @@ static u32 encoder_possible_clones(struct drm_encoder *encoder)
 		return drm_encoder_mask(encoder);
 }
 
+static void validate_possible_clones(struct drm_encoder *encoder)
+{
+	u32 possible_clones = encoder_possible_clones(encoder);
+	struct drm_device *dev = encoder->dev;
+	struct drm_encoder *other;
+	u32 encoder_mask = 0;
+
+	drm_for_each_encoder(other, dev) {
+		u32 other_possible_clones = encoder_possible_clones(other);
+
+		encoder_mask |= drm_encoder_mask(other);
+
+		WARN(!(possible_clones & drm_encoder_mask(other)) !=
+		     !(other_possible_clones & drm_encoder_mask(encoder)),
+		     "possible_clones mismatch: "
+		     "[ENCODER:%d:%s] index=%d, possible_clones=0x%x vs. "
+		     "[ENCODER:%d:%s] index=%d possible_clones=0x%x\n",
+		     encoder->base.id, encoder->name,
+		     drm_encoder_index(encoder), possible_clones,
+		     other->base.id, other->name,
+		     drm_encoder_index(other), other_possible_clones);
+	}
+
+	WARN((possible_clones & drm_encoder_mask(encoder)) == 0 ||
+	     (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,
+	     possible_clones, encoder_mask);
+}
+
 int drm_encoder_register_all(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
 	int ret = 0;
 
 	drm_for_each_encoder(encoder, dev) {
+		validate_possible_clones(encoder);
+
 		if (encoder->funcs->late_register)
 			ret = encoder->funcs->late_register(encoder);
 		if (ret)
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/14] drm/i915: Use drm_encoder_mask()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the hand rolled stuff with drm_encoder_mask() when populating
possible_clones.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8251e189a8bb..b095899d68a9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13950,13 +13950,10 @@ static int intel_encoder_clones(struct intel_encoder *encoder)
 	struct drm_device *dev = encoder->base.dev;
 	struct intel_encoder *source_encoder;
 	int index_mask = 0;
-	int entry = 0;
 
 	for_each_intel_encoder(dev, source_encoder) {
 		if (encoders_cloneable(encoder, source_encoder))
-			index_mask |= (1 << entry);
-
-		entry++;
+			index_mask |= drm_encoder_mask(&source_encoder->base);
 	}
 
 	return index_mask;
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/14] drm/i915: Populate possible_crtcs correctly

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Don't advertize non-exisiting crtcs in the encoder possible_crtcs
bitmask.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b095899d68a9..3fa9da714403 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13959,6 +13959,20 @@ static int intel_encoder_clones(struct intel_encoder *encoder)
 	return index_mask;
 }
 
+static int intel_encoder_crtcs(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct intel_crtc *crtc;
+	int index_mask = 0;
+
+	for_each_intel_crtc(dev, crtc) {
+		if (encoder->crtc_mask & BIT(crtc->pipe))
+			index_mask |= drm_crtc_mask(&crtc->base);
+	}
+
+	return index_mask;
+}
+
 static bool has_edp_a(struct drm_i915_private *dev_priv)
 {
 	if (!IS_MOBILE(dev_priv))
@@ -14211,7 +14225,8 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 	intel_psr_init(dev_priv);
 
 	for_each_intel_encoder(&dev_priv->drm, encoder) {
-		encoder->base.possible_crtcs = encoder->crtc_mask;
+		encoder->base.possible_crtcs =
+			intel_encoder_crtcs(encoder);
 		encoder->base.possible_clones =
 			intel_encoder_clones(encoder);
 	}
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/14] drm/i915: Populate possible_crtcs correctly

[Dhinakaran Pandiyan]

On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Don't advertize non-exisiting crtcs in the encoder possible_crtcs
> bitmask.
> 
How do we end up advertising non-existing CRTCs? encoder->crtc_mask
seems to be populated in the encoder init functions based on possible
pipes. Do you mean pipe and crtc->index can potentially differ?


> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b095899d68a9..3fa9da714403 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13959,6 +13959,20 @@ static int intel_encoder_clones(struct
> intel_encoder *encoder)
>  	return index_mask;
>  }
>  
> +static int intel_encoder_crtcs(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct intel_crtc *crtc;
> +	int index_mask = 0;
> +
> +	for_each_intel_crtc(dev, crtc) {
> +		if (encoder->crtc_mask & BIT(crtc->pipe))
> +			index_mask |= drm_crtc_mask(&crtc->base);
> +	}
> +
> +	return index_mask;
> +}
> +
>  static bool has_edp_a(struct drm_i915_private *dev_priv)
>  {
>  	if (!IS_MOBILE(dev_priv))
> @@ -14211,7 +14225,8 @@ static void intel_setup_outputs(struct
> drm_i915_private *dev_priv)
>  	intel_psr_init(dev_priv);
>  
>  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> -		encoder->base.possible_crtcs = encoder->crtc_mask;
> +		encoder->base.possible_crtcs =
> +			intel_encoder_crtcs(encoder);
>  		encoder->base.possible_clones =
>  			intel_encoder_clones(encoder);
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/14] drm/i915: Populate possible_crtcs correctly

[Ville Syrjälä]

On Thu, Jun 21, 2018 at 06:26:04PM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Don't advertize non-exisiting crtcs in the encoder possible_crtcs
> > bitmask.
> > 
> How do we end up advertising non-existing CRTCs? encoder->crtc_mask
> seems to be populated in the encoder init functions based on possible
> pipes. Do you mean pipe and crtc->index can potentially differ?

No. Just that we may sometimes set BIT(PIPE_C) in crtc_mask when
there are only two pipes on the device.

> 
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index b095899d68a9..3fa9da714403 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13959,6 +13959,20 @@ static int intel_encoder_clones(struct
> > intel_encoder *encoder)
> >  	return index_mask;
> >  }
> >  
> > +static int intel_encoder_crtcs(struct intel_encoder *encoder)
> > +{
> > +	struct drm_device *dev = encoder->base.dev;
> > +	struct intel_crtc *crtc;
> > +	int index_mask = 0;
> > +
> > +	for_each_intel_crtc(dev, crtc) {
> > +		if (encoder->crtc_mask & BIT(crtc->pipe))
> > +			index_mask |= drm_crtc_mask(&crtc->base);
> > +	}
> > +
> > +	return index_mask;
> > +}
> > +
> >  static bool has_edp_a(struct drm_i915_private *dev_priv)
> >  {
> >  	if (!IS_MOBILE(dev_priv))
> > @@ -14211,7 +14225,8 @@ static void intel_setup_outputs(struct
> > drm_i915_private *dev_priv)
> >  	intel_psr_init(dev_priv);
> >  
> >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> > -		encoder->base.possible_crtcs = encoder->crtc_mask;
> > +		encoder->base.possible_crtcs =
> > +			intel_encoder_crtcs(encoder);
> >  		encoder->base.possible_clones =
> >  			intel_encoder_clones(encoder);
> >  	}

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/14] drm/i915: Populate possible_crtcs correctly

[Dhinakaran Pandiyan]

On Mon, 2018-06-25 at 14:10 +0300, Ville Syrjälä wrote:
> On Thu, Jun 21, 2018 at 06:26:04PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote:
> > > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Don't advertize non-exisiting crtcs in the encoder possible_crtcs
> > > bitmask.
> > > 
> > How do we end up advertising non-existing CRTCs? encoder->crtc_mask
> > seems to be populated in the encoder init functions based on
> > possible
> > pipes. Do you mean pipe and crtc->index can potentially differ?
> No. Just that we may sometimes set BIT(PIPE_C) in crtc_mask when
> there are only two pipes on the device.
> 
Got it, thanks.
Since crtc_mask seems to be initialized based on the platform, we
should probably warn if we find out a bit is set incorrectly.

With or without that, this patch is
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>



> > 
> > 
> > 
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index b095899d68a9..3fa9da714403 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13959,6 +13959,20 @@ static int intel_encoder_clones(struct
> > > intel_encoder *encoder)
> > >  	return index_mask;
> > >  }
> > >  
> > > +static int intel_encoder_crtcs(struct intel_encoder *encoder)
> > > +{
> > > +	struct drm_device *dev = encoder->base.dev;
> > > +	struct intel_crtc *crtc;
> > > +	int index_mask = 0;
> > > +
> > > +	for_each_intel_crtc(dev, crtc) {
> > > +		if (encoder->crtc_mask & BIT(crtc->pipe))
> > > +			index_mask |= drm_crtc_mask(&crtc-
> > > >base);
> > > +	}
> > > +
> > > +	return index_mask;
> > > +}
> > > +
> > >  static bool has_edp_a(struct drm_i915_private *dev_priv)
> > >  {
> > >  	if (!IS_MOBILE(dev_priv))
> > > @@ -14211,7 +14225,8 @@ static void intel_setup_outputs(struct
> > > drm_i915_private *dev_priv)
> > >  	intel_psr_init(dev_priv);
> > >  
> > >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> > > -		encoder->base.possible_crtcs = encoder-
> > > >crtc_mask;
> > > +		encoder->base.possible_crtcs =
> > > +			intel_encoder_crtcs(encoder);
> > >  		encoder->base.possible_clones =
> > >  			intel_encoder_clones(encoder);
> > >  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/14] drm/i915: Fix DP-MST crtc_mask

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Each fake MST encoder is tied to a specific pipe. Fix the encoder's
crtc_mask to reflect that fact.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 5890500a3a8b..8e30765402b4 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -565,7 +565,7 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
 	intel_encoder->type = INTEL_OUTPUT_DP_MST;
 	intel_encoder->power_domain = intel_dig_port->base.power_domain;
 	intel_encoder->port = intel_dig_port->base.port;
-	intel_encoder->crtc_mask = 0x7;
+	intel_encoder->crtc_mask = BIT(pipe);
 	intel_encoder->cloneable = 0;
 
 	intel_encoder->compute_config = intel_dp_mst_compute_config;
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/14] drm/i915: Fix DP-MST crtc_mask

[Dhinakaran Pandiyan]

On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Each fake MST encoder is tied to a specific pipe. Fix the encoder's
> crtc_mask to reflect that fact.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 5890500a3a8b..8e30765402b4 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -565,7 +565,7 @@ intel_dp_create_fake_mst_encoder(struct
> intel_digital_port *intel_dig_port, enum
>  	intel_encoder->type = INTEL_OUTPUT_DP_MST;
>  	intel_encoder->power_domain = intel_dig_port-
> >base.power_domain;
>  	intel_encoder->port = intel_dig_port->base.port;
> -	intel_encoder->crtc_mask = 0x7;
> +	intel_encoder->crtc_mask = BIT(pipe);

How did this not cause any problems? Does this mean this field was/is
unused?
Disclaimer: I didn't look at the whole series.

>  	intel_encoder->cloneable = 0;
>  
>  	intel_encoder->compute_config = intel_dp_mst_compute_config;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/14] drm/i915: Fix DP-MST crtc_mask

[Ville Syrjälä]

On Fri, Jun 15, 2018 at 11:33:01AM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Each fake MST encoder is tied to a specific pipe. Fix the encoder's
> > crtc_mask to reflect that fact.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 5890500a3a8b..8e30765402b4 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -565,7 +565,7 @@ intel_dp_create_fake_mst_encoder(struct
> > intel_digital_port *intel_dig_port, enum
> >  	intel_encoder->type = INTEL_OUTPUT_DP_MST;
> >  	intel_encoder->power_domain = intel_dig_port-
> > >base.power_domain;
> >  	intel_encoder->port = intel_dig_port->base.port;
> > -	intel_encoder->crtc_mask = 0x7;
> > +	intel_encoder->crtc_mask = BIT(pipe);
> 
> How did this not cause any problems? Does this mean this field was/is
> unused?

This is a hint to userspace. So userspace would pick the connector
and crtc based on the hints, and then the kernel gets to pick the
actual encoder. In this case the bogus hint was good enough to tell
userspace that it can pick any crtc for any MST connector.

Hmm. Why on earth do we have .atomic_best_encoder() and .best_encoder()
for MST? The fb_helper appears to want to use the non-atomic one for
some reason... On boy, I guess I'll need to do something about that.
As is this patch would probably break it :(

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/14] drm/i915: Fix DP-MST crtc_mask

[Dhinakaran Pandiyan]

On Fri, 2018-06-15 at 21:43 +0300, Ville Syrjälä wrote:
> On Fri, Jun 15, 2018 at 11:33:01AM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote:
> > > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Each fake MST encoder is tied to a specific pipe. Fix the
> > > encoder's
> > > crtc_mask to reflect that fact.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 5890500a3a8b..8e30765402b4 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -565,7 +565,7 @@ intel_dp_create_fake_mst_encoder(struct
> > > intel_digital_port *intel_dig_port, enum
> > >  	intel_encoder->type = INTEL_OUTPUT_DP_MST;
> > >  	intel_encoder->power_domain = intel_dig_port-
> > > > 
> > > > base.power_domain;
> > >  	intel_encoder->port = intel_dig_port->base.port;
> > > -	intel_encoder->crtc_mask = 0x7;
> > > +	intel_encoder->crtc_mask = BIT(pipe);
> > How did this not cause any problems? Does this mean this field
> > was/is
> > unused?
> This is a hint to userspace. So userspace would pick the connector
> and crtc based on the hints, and then the kernel gets to pick the
> actual encoder. In this case the bogus hint was good enough to tell
> userspace that it can pick any crtc for any MST connector.
> 
> Hmm. Why on earth do we have .atomic_best_encoder() and
> .best_encoder()
> for MST? The fb_helper appears to want to use the non-atomic one for
> some reason... On boy, I guess I'll need to do something about that.
> As is this patch would probably break it :(
> 
So we end up picking crtc-0 for all MST connectors with the same
primary.

Since this patch does the right thing and assuming
https://patchwork.freedesktop.org/patch/229905/ gets merged before this

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>



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

[PATCH 12/14] drm/i915: Clean up encoder->crtc_mask setup

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use BIT(pipe) for better legibility when populating the crtc_mask
for encoders.

Also remove the redundant possible_crtcs setup for the TV encoder.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c  | 4 ++--
 drivers/gpu/drm/i915/intel_ddi.c  | 2 +-
 drivers/gpu/drm/i915/intel_dp.c   | 6 +++---
 drivers/gpu/drm/i915/intel_dvo.c  | 2 +-
 drivers/gpu/drm/i915/intel_hdmi.c | 6 +++---
 drivers/gpu/drm/i915/intel_lvds.c | 6 +++---
 drivers/gpu/drm/i915/intel_sdvo.c | 2 +-
 drivers/gpu/drm/i915/intel_tv.c   | 3 +--
 8 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 95aa29cf2d9c..b6b1464a47b2 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -967,9 +967,9 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
 	crt->base.type = INTEL_OUTPUT_ANALOG;
 	crt->base.cloneable = (1 << INTEL_OUTPUT_DVO) | (1 << INTEL_OUTPUT_HDMI);
 	if (IS_I830(dev_priv))
-		crt->base.crtc_mask = (1 << 0);
+		crt->base.crtc_mask = BIT(PIPE_A);
 	else
-		crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
+		crt->base.crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
 
 	if (IS_GEN2(dev_priv))
 		connector->interlace_allowed = 0;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ca73387bd596..0462d9b31f7d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3514,7 +3514,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->type = INTEL_OUTPUT_DDI;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);
 	intel_encoder->port = port;
-	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
+	intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
 	intel_encoder->cloneable = 0;
 
 	if (INTEL_GEN(dev_priv) >= 11)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a58bac01aeea..d983924ed69a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6474,11 +6474,11 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
 	intel_encoder->power_domain = intel_port_to_power_domain(port);
 	if (IS_CHERRYVIEW(dev_priv)) {
 		if (port == PORT_D)
-			intel_encoder->crtc_mask = 1 << 2;
+			intel_encoder->crtc_mask = BIT(PIPE_C);
 		else
-			intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
+			intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
 	} else {
-		intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
+		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
 	}
 	intel_encoder->cloneable = 0;
 	intel_encoder->port = port;
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 27f16db8953a..27c18680deb4 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -499,7 +499,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv)
 		intel_encoder->type = INTEL_OUTPUT_DVO;
 		intel_encoder->power_domain = POWER_DOMAIN_PORT_OTHER;
 		intel_encoder->port = port;
-		intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
+		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
 
 		switch (dvo->type) {
 		case INTEL_DVO_CHIP_TMDS:
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 09d7d38f0d4e..666bcc1a4660 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2405,11 +2405,11 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv,
 	intel_encoder->port = port;
 	if (IS_CHERRYVIEW(dev_priv)) {
 		if (port == PORT_D)
-			intel_encoder->crtc_mask = 1 << 2;
+			intel_encoder->crtc_mask = BIT(PIPE_C);
 		else
-			intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
+			intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
 	} else {
-		intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
+		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
 	}
 	intel_encoder->cloneable = 1 << INTEL_OUTPUT_ANALOG;
 	/*
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index bb06744d28a4..753df8dd9a03 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1085,11 +1085,11 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 	intel_encoder->port = PORT_NONE;
 	intel_encoder->cloneable = 0;
 	if (HAS_PCH_SPLIT(dev_priv))
-		intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
+		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
 	else if (IS_GEN4(dev_priv))
-		intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
+		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
 	else
-		intel_encoder->crtc_mask = (1 << 1);
+		intel_encoder->crtc_mask = BIT(PIPE_B);
 
 	drm_connector_helper_add(connector, &intel_lvds_connector_helper_funcs);
 	connector->display_info.subpixel_order = SubPixelHorizontalRGB;
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index e6a64b3ecd91..ef540cf00373 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2741,7 +2741,7 @@ intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags)
 			      bytes[0], bytes[1]);
 		return false;
 	}
-	intel_sdvo->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
+	intel_sdvo->base.crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
 
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 24dc368fdaa1..7a68c132d08c 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1520,9 +1520,8 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	intel_encoder->type = INTEL_OUTPUT_TVOUT;
 	intel_encoder->power_domain = POWER_DOMAIN_PORT_OTHER;
 	intel_encoder->port = PORT_NONE;
-	intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
+	intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
 	intel_encoder->cloneable = 0;
-	intel_encoder->base.possible_crtcs = ((1 << 0) | (1 << 1));
 	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
 
 	/* BIOS margin values */
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/14] drm/i915: Clean up encoder->crtc_mask setup

[Jani Nikula]

On Fri, 15 Jun 2018, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Use BIT(pipe) for better legibility when populating the crtc_mask
> for encoders.
>
> Also remove the redundant possible_crtcs setup for the TV encoder.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_crt.c  | 4 ++--
>  drivers/gpu/drm/i915/intel_ddi.c  | 2 +-
>  drivers/gpu/drm/i915/intel_dp.c   | 6 +++---
>  drivers/gpu/drm/i915/intel_dvo.c  | 2 +-
>  drivers/gpu/drm/i915/intel_hdmi.c | 6 +++---
>  drivers/gpu/drm/i915/intel_lvds.c | 6 +++---
>  drivers/gpu/drm/i915/intel_sdvo.c | 2 +-
>  drivers/gpu/drm/i915/intel_tv.c   | 3 +--
>  8 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 95aa29cf2d9c..b6b1464a47b2 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -967,9 +967,9 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
>  	crt->base.type = INTEL_OUTPUT_ANALOG;
>  	crt->base.cloneable = (1 << INTEL_OUTPUT_DVO) | (1 << INTEL_OUTPUT_HDMI);
>  	if (IS_I830(dev_priv))
> -		crt->base.crtc_mask = (1 << 0);
> +		crt->base.crtc_mask = BIT(PIPE_A);
>  	else
> -		crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> +		crt->base.crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
>  
>  	if (IS_GEN2(dev_priv))
>  		connector->interlace_allowed = 0;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ca73387bd596..0462d9b31f7d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3514,7 +3514,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  	intel_encoder->type = INTEL_OUTPUT_DDI;
>  	intel_encoder->power_domain = intel_port_to_power_domain(port);
>  	intel_encoder->port = port;
> -	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> +	intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
>  	intel_encoder->cloneable = 0;
>  
>  	if (INTEL_GEN(dev_priv) >= 11)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a58bac01aeea..d983924ed69a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6474,11 +6474,11 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
>  	intel_encoder->power_domain = intel_port_to_power_domain(port);
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		if (port == PORT_D)
> -			intel_encoder->crtc_mask = 1 << 2;
> +			intel_encoder->crtc_mask = BIT(PIPE_C);
>  		else
> -			intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
> +			intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
>  	} else {
> -		intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> +		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
>  	}
>  	intel_encoder->cloneable = 0;
>  	intel_encoder->port = port;
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 27f16db8953a..27c18680deb4 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -499,7 +499,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv)
>  		intel_encoder->type = INTEL_OUTPUT_DVO;
>  		intel_encoder->power_domain = POWER_DOMAIN_PORT_OTHER;
>  		intel_encoder->port = port;
> -		intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
> +		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
>  
>  		switch (dvo->type) {
>  		case INTEL_DVO_CHIP_TMDS:
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 09d7d38f0d4e..666bcc1a4660 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -2405,11 +2405,11 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv,
>  	intel_encoder->port = port;
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		if (port == PORT_D)
> -			intel_encoder->crtc_mask = 1 << 2;
> +			intel_encoder->crtc_mask = BIT(PIPE_C);
>  		else
> -			intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
> +			intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
>  	} else {
> -		intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> +		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
>  	}
>  	intel_encoder->cloneable = 1 << INTEL_OUTPUT_ANALOG;
>  	/*
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index bb06744d28a4..753df8dd9a03 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -1085,11 +1085,11 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>  	intel_encoder->port = PORT_NONE;
>  	intel_encoder->cloneable = 0;
>  	if (HAS_PCH_SPLIT(dev_priv))
> -		intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> +		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
>  	else if (IS_GEN4(dev_priv))
> -		intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
> +		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
>  	else
> -		intel_encoder->crtc_mask = (1 << 1);
> +		intel_encoder->crtc_mask = BIT(PIPE_B);
>  
>  	drm_connector_helper_add(connector, &intel_lvds_connector_helper_funcs);
>  	connector->display_info.subpixel_order = SubPixelHorizontalRGB;
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index e6a64b3ecd91..ef540cf00373 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2741,7 +2741,7 @@ intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags)
>  			      bytes[0], bytes[1]);
>  		return false;
>  	}
> -	intel_sdvo->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> +	intel_sdvo->base.crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
>  
>  	return true;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 24dc368fdaa1..7a68c132d08c 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1520,9 +1520,8 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	intel_encoder->type = INTEL_OUTPUT_TVOUT;
>  	intel_encoder->power_domain = POWER_DOMAIN_PORT_OTHER;
>  	intel_encoder->port = PORT_NONE;
> -	intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
> +	intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
>  	intel_encoder->cloneable = 0;
> -	intel_encoder->base.possible_crtcs = ((1 << 0) | (1 << 1));
>  	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
>  
>  	/* BIOS margin values */

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

[PATCH 13/14] drm/i915: Simplfy LVDS crtc_mask setup

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We don't need to special case PCH vs. gen4 when setting up the LVDS
crtc_mask. Just claim pipes A|B|C work and intel_encoder_crtcs() drop
out any crtc that doesn't exist.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lvds.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 753df8dd9a03..5b8a9d14edbe 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1084,10 +1084,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 	intel_encoder->power_domain = POWER_DOMAIN_PORT_OTHER;
 	intel_encoder->port = PORT_NONE;
 	intel_encoder->cloneable = 0;
-	if (HAS_PCH_SPLIT(dev_priv))
+	if (INTEL_GEN(dev_priv) >= 4)
 		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C);
-	else if (IS_GEN4(dev_priv))
-		intel_encoder->crtc_mask = BIT(PIPE_A) | BIT(PIPE_B);
 	else
 		intel_encoder->crtc_mask = BIT(PIPE_B);
 
-- 
2.16.4

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

[PATCH 14/14] drm: Validate encoder->possible_crtcs

[Ville Syrjala]

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?

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 1c289b227bbc..cfe6e719ee2b 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -107,12 +107,30 @@ static void validate_possible_clones(struct drm_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;
 	int ret = 0;
 
 	drm_for_each_encoder(encoder, dev) {
+		validate_possible_crtcs(encoder);
 		validate_possible_clones(encoder);
 
 		if (encoder->funcs->late_register)
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx