All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Generalize definition for crtc mask
@ 2017-12-05 10:15 Mika Kahola
  2017-12-05 10:56 ` ✗ Fi.CI.BAT: warning for " Patchwork
  2017-12-05 13:59 ` [PATCH] " Ville Syrjälä
  0 siblings, 2 replies; 5+ messages in thread
From: Mika Kahola @ 2017-12-05 10:15 UTC (permalink / raw)
  To: intel-gfx

crtc_mask is defined explicitly defined for a certain number of pipes per
platform. Let's generalize this in a way that crtc_mask dependens only on
the number of pipes defined in device info.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c  |  9 ++++++---
 drivers/gpu/drm/i915/intel_ddi.c  |  5 ++++-
 drivers/gpu/drm/i915/intel_dp.c   | 12 ++++++++----
 drivers/gpu/drm/i915/intel_hdmi.c |  4 +++-
 drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++-----
 drivers/gpu/drm/i915/intel_sdvo.c |  5 ++++-
 drivers/gpu/drm/i915/intel_tv.c   |  6 +++++-
 7 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 9f31aea..34f65b5 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -903,6 +903,7 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
 	struct intel_connector *intel_connector;
 	i915_reg_t adpa_reg;
 	u32 adpa;
+	enum pipe pipe;
 
 	if (HAS_PCH_SPLIT(dev_priv))
 		adpa_reg = PCH_ADPA;
@@ -950,10 +951,12 @@ 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))
+	if (IS_I830(dev_priv)) {
 		crt->base.crtc_mask = (1 << 0);
-	else
-		crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
+	} else {
+		for_each_pipe(dev_priv, pipe)
+			crt->base.crtc_mask |= (1 << pipe);
+	}
 
 	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 eff3b51..9320542 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2766,6 +2766,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	struct drm_encoder *encoder;
 	bool init_hdmi, init_dp, init_lspcon = false;
 	int max_lanes;
+	enum pipe pipe;
 
 	if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) {
 		switch (port) {
@@ -2884,9 +2885,11 @@ 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->cloneable = 0;
 
+	for_each_pipe(dev_priv, pipe)
+		intel_encoder->crtc_mask |= (1 << pipe);
+
 	intel_infoframe_init(intel_dig_port);
 
 	if (init_dp) {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 957735c..37ba90d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6139,6 +6139,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
 	struct intel_encoder *intel_encoder;
 	struct drm_encoder *encoder;
 	struct intel_connector *intel_connector;
+	enum pipe pipe;
 
 	intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
 	if (!intel_dig_port)
@@ -6190,12 +6191,15 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
 	intel_encoder->type = INTEL_OUTPUT_DP;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);
 	if (IS_CHERRYVIEW(dev_priv)) {
-		if (port == PORT_D)
+		if (port == PORT_D) {
 			intel_encoder->crtc_mask = 1 << 2;
-		else
-			intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
+		} else {
+			for_each_pipe(dev_priv, pipe)
+				intel_encoder->crtc_mask |= (1 << pipe);
+		}
 	} else {
-		intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
+		for_each_pipe(dev_priv, pipe)
+			intel_encoder->crtc_mask |= (1 << pipe);
 	}
 	intel_encoder->cloneable = 0;
 	intel_encoder->port = port;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a40f35a..43584d9 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2072,6 +2072,7 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv,
 	struct intel_digital_port *intel_dig_port;
 	struct intel_encoder *intel_encoder;
 	struct intel_connector *intel_connector;
+	enum pipe pipe;
 
 	intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
 	if (!intel_dig_port)
@@ -2128,7 +2129,8 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv,
 		else
 			intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
 	} else {
-		intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
+		for_each_pipe(dev_priv, pipe)
+			intel_encoder->crtc_mask |= (1 << pipe);
 	}
 	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 ef80499..36eab86 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -945,6 +945,7 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 	u32 lvds;
 	u8 pin;
 	u32 allowed_scalers;
+	enum pipe pipe;
 
 	if (!intel_lvds_supported(dev_priv))
 		return;
@@ -1025,13 +1026,14 @@ 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))
-		intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
-	else if (IS_GEN4(dev_priv))
+	if (HAS_PCH_SPLIT(dev_priv)) {
+		for_each_pipe(dev_priv, pipe)
+			intel_encoder->crtc_mask |= (1 << pipe);
+	} else if (IS_GEN4(dev_priv)) {
 		intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
-	else
+	} else {
 		intel_encoder->crtc_mask = (1 << 1);
-
+	}
 	drm_connector_helper_add(connector, &intel_lvds_connector_helper_funcs);
 	connector->display_info.subpixel_order = SubPixelHorizontalRGB;
 	connector->interlace_allowed = false;
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 2b87648..ac27fb9 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2697,7 +2697,6 @@ 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);
 
 	return true;
 }
@@ -3018,6 +3017,7 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
 	struct intel_encoder *intel_encoder;
 	struct intel_sdvo *intel_sdvo;
 	int i;
+	enum pipe pipe;
 
 	assert_sdvo_port_valid(dev_priv, port);
 
@@ -3065,6 +3065,9 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
 	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
 	intel_encoder->get_config = intel_sdvo_get_config;
 
+	for_each_pipe(dev_priv, pipe)
+		intel_sdvo->base.crtc_mask = (1 << pipe);
+
 	/* In default case sdvo lvds is false */
 	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
 		goto err;
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index b3dabc2..2eb7d67 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1465,6 +1465,7 @@ intel_tv_init(struct drm_i915_private *dev_priv)
 	const char *tv_format_names[ARRAY_SIZE(tv_modes)];
 	int i, initial_mode = 0;
 	struct drm_connector_state *state;
+	enum pipe pipe;
 
 	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
 		return;
@@ -1542,7 +1543,10 @@ 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);
+
+	for_each_pipe(dev_priv, pipe)
+		intel_encoder->crtc_mask |= (1 << pipe);
+
 	intel_encoder->cloneable = 0;
 	intel_encoder->base.possible_crtcs = ((1 << 0) | (1 << 1));
 	intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915: Generalize definition for crtc mask
  2017-12-05 10:15 [PATCH] drm/i915: Generalize definition for crtc mask Mika Kahola
@ 2017-12-05 10:56 ` Patchwork
  2017-12-05 13:59 ` [PATCH] " Ville Syrjälä
  1 sibling, 0 replies; 5+ messages in thread
From: Patchwork @ 2017-12-05 10:56 UTC (permalink / raw)
  To: Mika Kahola; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Generalize definition for crtc mask
URL   : https://patchwork.freedesktop.org/series/34894/
State : warning

== Summary ==

Series 34894v1 drm/i915: Generalize definition for crtc mask
https://patchwork.freedesktop.org/api/1.0/series/34894/revisions/1/mbox/

Test gem_exec_reloc:
        Subgroup basic-cpu-gtt-active:
                pass       -> FAIL       (fi-gdg-551) fdo#102582 +3
Test kms_busy:
        Subgroup basic-flip-a:
                pass       -> SKIP       (fi-bwr-2160)
Test kms_chamelium:
        Subgroup dp-crc-fast:
                dmesg-fail -> PASS       (fi-kbl-7500u) fdo#103841
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> INCOMPLETE (fi-hsw-4770) fdo#103375
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> SKIP       (fi-bwr-2160) fdo#103182

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#103182 https://bugs.freedesktop.org/show_bug.cgi?id=103182

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:442s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:448s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:390s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:531s
fi-bwr-2160      total:288  pass:181  dwarn:0   dfail:0   fail:0   skip:107 time:274s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:504s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:513s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:491s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:480s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:174  dwarn:1   dfail:0   fail:5   skip:108 time:270s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:545s
fi-hsw-4770      total:244  pass:220  dwarn:0   dfail:0   fail:0   skip:23 
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:259s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:399s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:487s
fi-ivb-3770      total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:455s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:487s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:526s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:481s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:533s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:588s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:543s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:567s
fi-skl-6700k     total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:522s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:504s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:447s
fi-snb-2520m     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:549s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:419s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:617s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:628s
fi-glk-dsi       total:103  pass:91   dwarn:0   dfail:0   fail:0   skip:11 

84cd3d972bdd25fd6f7fe3dc2fe92b0617cde2a5 drm-tip: 2017y-12m-05d-08h-41m-59s UTC integration manifest
292c13a60aa3 drm/i915: Generalize definition for crtc mask

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7406/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Generalize definition for crtc mask
  2017-12-05 10:15 [PATCH] drm/i915: Generalize definition for crtc mask Mika Kahola
  2017-12-05 10:56 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2017-12-05 13:59 ` Ville Syrjälä
  2017-12-05 14:49   ` Mika Kahola
  2017-12-05 17:22   ` Daniel Vetter
  1 sibling, 2 replies; 5+ messages in thread
From: Ville Syrjälä @ 2017-12-05 13:59 UTC (permalink / raw)
  To: Mika Kahola; +Cc: intel-gfx

On Tue, Dec 05, 2017 at 12:15:39PM +0200, Mika Kahola wrote:
> crtc_mask is defined explicitly defined for a certain number of pipes per
> platform. Let's generalize this in a way that crtc_mask dependens only on
> the number of pipes defined in device info.
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c  |  9 ++++++---
>  drivers/gpu/drm/i915/intel_ddi.c  |  5 ++++-
>  drivers/gpu/drm/i915/intel_dp.c   | 12 ++++++++----
>  drivers/gpu/drm/i915/intel_hdmi.c |  4 +++-
>  drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++-----
>  drivers/gpu/drm/i915/intel_sdvo.c |  5 ++++-
>  drivers/gpu/drm/i915/intel_tv.c   |  6 +++++-
>  7 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 9f31aea..34f65b5 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -903,6 +903,7 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
>  	struct intel_connector *intel_connector;
>  	i915_reg_t adpa_reg;
>  	u32 adpa;
> +	enum pipe pipe;
>  
>  	if (HAS_PCH_SPLIT(dev_priv))
>  		adpa_reg = PCH_ADPA;
> @@ -950,10 +951,12 @@ 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))
> +	if (IS_I830(dev_priv)) {
>  		crt->base.crtc_mask = (1 << 0);
> -	else
> -		crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> +	} else {
> +		for_each_pipe(dev_priv, pipe)
> +			crt->base.crtc_mask |= (1 << pipe);
> +	}

The only places you have to touch are DDI and MST. None of the other
encoder types are relevant for new platforms at all.

Looks like you actually missed MST in this patch, and it looks like the
code there is just wrong even now. It should really just set
'crtc_mask = BIT(pipe)' since the fake mst encoders are pipe specific.

In fact I think what we should do is have a small function that filters
out the non-existent pipes from the crtc_mask when populating
encoder->possible_crtcs. And I wouldn't be opposed to s/1<<0/BIT(PIPE_A)/
etc. everywhere we populate crtc_mask (maybe even s/crtc_mask/pipe_mask/
to make it clear what we're talking about).

And maybe actually get rid of crtc_mask entirely and just populate
possible_crtcs directly (with the help of aforementioned filtering
function).

Possibly some igt would be nice to confirm that possibly_crtcs etc.
don't advertize invalid crtc indices.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Generalize definition for crtc mask
  2017-12-05 13:59 ` [PATCH] " Ville Syrjälä
@ 2017-12-05 14:49   ` Mika Kahola
  2017-12-05 17:22   ` Daniel Vetter
  1 sibling, 0 replies; 5+ messages in thread
From: Mika Kahola @ 2017-12-05 14:49 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Tue, 2017-12-05 at 15:59 +0200, Ville Syrjälä wrote:
> On Tue, Dec 05, 2017 at 12:15:39PM +0200, Mika Kahola wrote:
> > 
> > crtc_mask is defined explicitly defined for a certain number of
> > pipes per
> > platform. Let's generalize this in a way that crtc_mask dependens
> > only on
> > the number of pipes defined in device info.
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_crt.c  |  9 ++++++---
> >  drivers/gpu/drm/i915/intel_ddi.c  |  5 ++++-
> >  drivers/gpu/drm/i915/intel_dp.c   | 12 ++++++++----
> >  drivers/gpu/drm/i915/intel_hdmi.c |  4 +++-
> >  drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++-----
> >  drivers/gpu/drm/i915/intel_sdvo.c |  5 ++++-
> >  drivers/gpu/drm/i915/intel_tv.c   |  6 +++++-
> >  7 files changed, 37 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c
> > b/drivers/gpu/drm/i915/intel_crt.c
> > index 9f31aea..34f65b5 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -903,6 +903,7 @@ void intel_crt_init(struct drm_i915_private
> > *dev_priv)
> >  	struct intel_connector *intel_connector;
> >  	i915_reg_t adpa_reg;
> >  	u32 adpa;
> > +	enum pipe pipe;
> >  
> >  	if (HAS_PCH_SPLIT(dev_priv))
> >  		adpa_reg = PCH_ADPA;
> > @@ -950,10 +951,12 @@ 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))
> > +	if (IS_I830(dev_priv)) {
> >  		crt->base.crtc_mask = (1 << 0);
> > -	else
> > -		crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 <<
> > 2);
> > +	} else {
> > +		for_each_pipe(dev_priv, pipe)
> > +			crt->base.crtc_mask |= (1 << pipe);
> > +	}
> The only places you have to touch are DDI and MST. None of the other
> encoder types are relevant for new platforms at all.
That's true. For these newer platforms I wouldn't have needed to touch
other encoder types but I decided to go that road due to consistency. I
may drop these for the next iteration of the patch.
> 
> Looks like you actually missed MST in this patch, and it looks like
> the
> code there is just wrong even now. It should really just set
> 'crtc_mask = BIT(pipe)' since the fake mst encoders are pipe
> specific.
Ouch, I completely missed the MST part.

> 
> In fact I think what we should do is have a small function that
> filters
> out the non-existent pipes from the crtc_mask when populating
> encoder->possible_crtcs. And I wouldn't be opposed to
> s/1<<0/BIT(PIPE_A)/
> etc. everywhere we populate crtc_mask (maybe even
> s/crtc_mask/pipe_mask/
> to make it clear what we're talking about).
I'll test this small helper function and see what the outcome looks
like.

Thanks for the review!
> 
> And maybe actually get rid of crtc_mask entirely and just populate
> possible_crtcs directly (with the help of aforementioned filtering
> function).
> 
> Possibly some igt would be nice to confirm that possibly_crtcs etc.
> don't advertize invalid crtc indices.
> 
-- 
Mika Kahola - Intel OTC

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Generalize definition for crtc mask
  2017-12-05 13:59 ` [PATCH] " Ville Syrjälä
  2017-12-05 14:49   ` Mika Kahola
@ 2017-12-05 17:22   ` Daniel Vetter
  1 sibling, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2017-12-05 17:22 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Tue, Dec 05, 2017 at 03:59:35PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 05, 2017 at 12:15:39PM +0200, Mika Kahola wrote:
> > crtc_mask is defined explicitly defined for a certain number of pipes per
> > platform. Let's generalize this in a way that crtc_mask dependens only on
> > the number of pipes defined in device info.
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_crt.c  |  9 ++++++---
> >  drivers/gpu/drm/i915/intel_ddi.c  |  5 ++++-
> >  drivers/gpu/drm/i915/intel_dp.c   | 12 ++++++++----
> >  drivers/gpu/drm/i915/intel_hdmi.c |  4 +++-
> >  drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++-----
> >  drivers/gpu/drm/i915/intel_sdvo.c |  5 ++++-
> >  drivers/gpu/drm/i915/intel_tv.c   |  6 +++++-
> >  7 files changed, 37 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > index 9f31aea..34f65b5 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -903,6 +903,7 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
> >  	struct intel_connector *intel_connector;
> >  	i915_reg_t adpa_reg;
> >  	u32 adpa;
> > +	enum pipe pipe;
> >  
> >  	if (HAS_PCH_SPLIT(dev_priv))
> >  		adpa_reg = PCH_ADPA;
> > @@ -950,10 +951,12 @@ 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))
> > +	if (IS_I830(dev_priv)) {
> >  		crt->base.crtc_mask = (1 << 0);
> > -	else
> > -		crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> > +	} else {
> > +		for_each_pipe(dev_priv, pipe)
> > +			crt->base.crtc_mask |= (1 << pipe);
> > +	}
> 
> The only places you have to touch are DDI and MST. None of the other
> encoder types are relevant for new platforms at all.
> 
> Looks like you actually missed MST in this patch, and it looks like the
> code there is just wrong even now. It should really just set
> 'crtc_mask = BIT(pipe)' since the fake mst encoders are pipe specific.
> 
> In fact I think what we should do is have a small function that filters
> out the non-existent pipes from the crtc_mask when populating
> encoder->possible_crtcs. And I wouldn't be opposed to s/1<<0/BIT(PIPE_A)/
> etc. everywhere we populate crtc_mask (maybe even s/crtc_mask/pipe_mask/
> to make it clear what we're talking about).
> 
> And maybe actually get rid of crtc_mask entirely and just populate
> possible_crtcs directly (with the help of aforementioned filtering
> function).
> 
> Possibly some igt would be nice to confirm that possibly_crtcs etc.
> don't advertize invalid crtc indices.

+1 on a generic (i.e. DRIVER_ANY) igt that validates the various
possible_* masks. Lots of drivers e.g. don't set anything, hooray.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-05 17:22 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-05 10:15 [PATCH] drm/i915: Generalize definition for crtc mask Mika Kahola
2017-12-05 10:56 ` ✗ Fi.CI.BAT: warning for " Patchwork
2017-12-05 13:59 ` [PATCH] " Ville Syrjälä
2017-12-05 14:49   ` Mika Kahola
2017-12-05 17:22   ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.