[PATCH 0/2] drm/i915: Use correct bpc computations for DisplayPort

[PATCH 0/2] drm/i915: Use correct bpc computations for DisplayPort

[Keith Packard]

Here's a couple of patches that fix some bpc (bits per component)
computation issues with DisplayPort. The problem was that the
DisplayPort code tried to figure out the 'current' bpc by looking at
the bpp stored in an associated crtc, but that was never right (as
described in the message for the first patch).

The first patch assumes that the display will run at 8bpc (24bpp) if
the link has enough bandwidth, otherwise at 6bpc (18bpp). This is
essentially what the existing code ends up doing at boot time; modes
are computed before any crtc is assigned, so intel_dp_link_required
would have used 24bpp for bandwidth computations.

The second patch allows for arbitrary bpc values, computing the
display bpc in both intel_dp_mode_fixup and the two crtc_mode_set
functions. Obviously doing the computation once would be nice, but
there isn't an obvious place to stick the result between those two
functions as the bpc computation is also needed for non-DP encoders.

This should fix problems where display port doesn't come back after
resume for panels which need 6bpc modes.

--
keith.packard@intel.com

[PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Keith Packard]

It is never correct to use intel_crtc->bpp in intel_dp_link_required,
so instead pass an explicit bpp in to this function. This patch
only supports 18bpp and 24bpp modes, which means that 10bpc modes will
be computed incorrectly. Fixing that will require more extensive
changes, and so must be addressed separately from this bugfix.

intel_dp_link_required is called from intel_dp_mode_valid and
intel_dp_mode_fixup.

* intel_dp_mode_valid is called to list supported modes; in this case,
  the current crtc values cannot be relevant as the modes in question
  may never be selected. Thus, using intel_crtc->bpp is never right.

* intel_dp_mode_fixup is called during mode setting, but it is run
  well before ironlake_crtc_mode_set is called to set intel_crtc->bpp,
  so using intel_crtc-bpp in this path can only ever get a stale
  value.

Cc: Lubos Kolouch <lubos.kolouch@gmail.com>
Cc: Adam Jackson <ajax@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   20 +++++---------------
 1 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index db3b461..94f860c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -208,17 +208,8 @@ intel_dp_link_clock(uint8_t link_bw)
  */
 
 static int
-intel_dp_link_required(struct intel_dp *intel_dp, int pixel_clock, int check_bpp)
+intel_dp_link_required(int pixel_clock, int bpp)
 {
-	struct drm_crtc *crtc = intel_dp->base.base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int bpp = 24;
-
-	if (check_bpp)
-		bpp = check_bpp;
-	else if (intel_crtc)
-		bpp = intel_crtc->bpp;
-
 	return (pixel_clock * bpp + 9) / 10;
 }
 
@@ -245,12 +236,11 @@ intel_dp_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 	}
 
-	mode_rate = intel_dp_link_required(intel_dp, mode->clock, 0);
+	mode_rate = intel_dp_link_required(mode->clock, 24);
 	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
 
 	if (mode_rate > max_rate) {
-			mode_rate = intel_dp_link_required(intel_dp,
-							   mode->clock, 18);
+			mode_rate = intel_dp_link_required(mode->clock, 18);
 			if (mode_rate > max_rate)
 				return MODE_CLOCK_HIGH;
 			else
@@ -683,7 +673,7 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	int lane_count, clock;
 	int max_lane_count = intel_dp_max_lane_count(intel_dp);
 	int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
-	int bpp = mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 0;
+	int bpp = mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24;
 	static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
 
 	if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) {
@@ -701,7 +691,7 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		for (clock = 0; clock <= max_clock; clock++) {
 			int link_avail = intel_dp_max_data_rate(intel_dp_link_clock(bws[clock]), lane_count);
 
-			if (intel_dp_link_required(intel_dp, mode->clock, bpp)
+			if (intel_dp_link_required(mode->clock, bpp)
 					<= link_avail) {
 				intel_dp->link_bw = bws[clock];
 				intel_dp->lane_count = lane_count;
-- 
1.7.8.3

[PATCH 2/2] drm/i915: Select DP BPC at mode set, rather than mode validate

[Keith Packard]

The DP configuration must match the pipe configuration, and until mode
set we don't know what BPC will be used. Delay all decisions about BPC
until mode set, computing the target BPC in both intel_dp_mode_fixup
and either i9xx_crtc_mode_set or ironlake_crtc_mode_set. It might be
slightly better to compute this only once, but storing the value
between those two calls would be a pain.

Cc: Lubos Kolouch <lubos.kolouch@gmail.com>
Cc: Adam Jackson <ajax@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   27 +++++-------
 drivers/gpu/drm/i915/intel_dp.c      |   77 +++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |    6 ++-
 3 files changed, 78 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b3b51c4..d613676 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4850,9 +4850,9 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
  * Dithering requirement (i.e. false if display bpc and pipe bpc match,
  * true if they don't match).
  */
-static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
-					 unsigned int *pipe_bpp,
-					 struct drm_display_mode *mode)
+bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
+				  unsigned int *pipe_bpp,
+				  struct drm_display_mode *mode)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4883,13 +4883,13 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
 			continue;
 		}
 
-		if (intel_encoder->type == INTEL_OUTPUT_EDP) {
-			/* Use VBT settings if we have an eDP panel */
-			unsigned int edp_bpc = dev_priv->edp.bpp / 3;
+		if (intel_encoder->type == INTEL_OUTPUT_EDP ||
+		    intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
+			unsigned int dp_bpc = intel_dp_max_bpp(&intel_encoder->base, mode);
 
-			if (edp_bpc < display_bpc) {
-				DRM_DEBUG_KMS("clamping display bpc (was %d) to eDP (%d)\n", display_bpc, edp_bpc);
-				display_bpc = edp_bpc;
+			if (dp_bpc < display_bpc) {
+				DRM_DEBUG_KMS("clamping display bpc (was %d) to DP (%d)\n", display_bpc, dp_bpc);
+				display_bpc = dp_bpc;
 			}
 			continue;
 		}
@@ -4923,11 +4923,6 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
 		}
 	}
 
-	if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
-		DRM_DEBUG_KMS("Dithering DP to 6bpc\n");
-		display_bpc = 6;
-	}
-
 	/*
 	 * We could just drive the pipe at the highest bpc all the time and
 	 * enable dithering as needed, but that costs bandwidth.  So choose
@@ -4990,6 +4985,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	int ret;
 	u32 temp;
 	u32 lvds_sync = 0;
+	int dp_max_bpp = 0;
 
 	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
 		if (encoder->base.crtc != crtc)
@@ -5016,6 +5012,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 			break;
 		case INTEL_OUTPUT_DISPLAYPORT:
 			is_dp = true;
+			dp_max_bpp = intel_dp_max_bpp(&encoder->base, mode);
 			break;
 		}
 
@@ -5192,7 +5189,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	/* default to 8bpc */
 	pipeconf &= ~(PIPECONF_BPP_MASK | PIPECONF_DITHER_EN);
 	if (is_dp) {
-		if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
+		if (dp_max_bpp <= 18) {
 			pipeconf |= PIPECONF_BPP_6 |
 				    PIPECONF_DITHER_EN |
 				    PIPECONF_DITHER_TYPE_SP;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 94f860c..2482796 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -219,14 +219,51 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
 	return (max_link_clock * max_lanes * 8) / 10;
 }
 
+/*
+ * For the specified encoder, return the maximum bpp that can be used
+ * for the specified mode.
+ */
+
+static const int dp_supported_bpc[] = { 6, 8, 10, 12, 16 };
+
+#define NUM_DP_SUPPORTED_BPC (sizeof dp_supported_bpc/sizeof dp_supported_bpc[0])
+
+int
+intel_dp_max_bpp(struct drm_encoder *encoder, struct drm_display_mode *mode)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
+	int max_lanes = intel_dp_max_lane_count(intel_dp);
+	int max_rate, mode_rate;
+	int i;
+
+	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
+	for (i = NUM_DP_SUPPORTED_BPC - 1; i >= 0; i--) {
+		int bpp = dp_supported_bpc[i] * 3;	/* 3 channels of data (RGB) */
+
+		mode_rate = intel_dp_link_required(mode->clock, bpp);
+		if (mode_rate <= max_rate) {
+
+			/* Limit EDP bpp to panel ability */
+			if (intel_dp->base.type == INTEL_OUTPUT_EDP) {
+				struct drm_device *dev = encoder->dev;
+				struct drm_i915_private *dev_priv = dev->dev_private;
+				int edp_bpp = dev_priv->edp.bpp;
+
+				if (bpp > edp_bpp)
+					bpp = edp_bpp;
+			}
+			return bpp;
+		}
+	}
+	return 0;
+}
+
 static int
 intel_dp_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
-	int max_lanes = intel_dp_max_lane_count(intel_dp);
-	int max_rate, mode_rate;
 
 	if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) {
 		if (mode->hdisplay > intel_dp->panel_fixed_mode->hdisplay)
@@ -236,16 +273,8 @@ intel_dp_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 	}
 
-	mode_rate = intel_dp_link_required(mode->clock, 24);
-	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
-
-	if (mode_rate > max_rate) {
-			mode_rate = intel_dp_link_required(mode->clock, 18);
-			if (mode_rate > max_rate)
-				return MODE_CLOCK_HIGH;
-			else
-				mode->private_flags |= INTEL_MODE_DP_FORCE_6BPC;
-	}
+	if (intel_dp_max_bpp(&intel_dp->base.base, mode) == 0)
+		return MODE_CLOCK_HIGH;
 
 	if (mode->clock < 10000)
 		return MODE_CLOCK_LOW;
@@ -673,9 +702,26 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	int lane_count, clock;
 	int max_lane_count = intel_dp_max_lane_count(intel_dp);
 	int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
-	int bpp = mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24;
+	unsigned int bpp;
 	static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
 
+	if (HAS_PCH_SPLIT(dev))
+		(void) intel_choose_pipe_bpp_dither(intel_dp->base.base.crtc, &bpp, mode);
+	else {
+		bpp = intel_dp_max_bpp(encoder, mode);
+		if (bpp > 24)
+			bpp = 24;
+	}
+
+	DRM_DEBUG_KMS("encoder assuming bpp %d (bpc %d)\n",
+		      bpp, bpp / 3);
+
+	if (bpp == 0) {
+		DRM_DEBUG_KMS("Display port cannot support requested clock %d\n",
+			      mode->clock);
+		return false;
+	}
+
 	if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) {
 		intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode);
 		intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN,
@@ -691,8 +737,7 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		for (clock = 0; clock <= max_clock; clock++) {
 			int link_avail = intel_dp_max_data_rate(intel_dp_link_clock(bws[clock]), lane_count);
 
-			if (intel_dp_link_required(mode->clock, bpp)
-					<= link_avail) {
+			if (intel_dp_link_required(mode->clock, bpp) <= link_avail) {
 				intel_dp->link_bw = bws[clock];
 				intel_dp->lane_count = lane_count;
 				adjusted_mode->clock = intel_dp_link_clock(intel_dp->link_bw);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1348705..03b4595 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -104,7 +104,6 @@
 /* drm_display_mode->private_flags */
 #define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0)
 #define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT)
-#define INTEL_MODE_DP_FORCE_6BPC (0x10)
 
 static inline void
 intel_mode_set_pixel_multiplier(struct drm_display_mode *mode,
@@ -303,11 +302,16 @@ extern void intel_dp_init(struct drm_device *dev, int dp_reg);
 void
 intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 		 struct drm_display_mode *adjusted_mode);
+extern int intel_dp_max_bpp(struct drm_encoder *encoder, struct drm_display_mode *mode);
 extern bool intel_dpd_is_edp(struct drm_device *dev);
 extern void intel_edp_link_config(struct intel_encoder *, int *, int *);
 extern bool intel_encoder_is_pch_edp(struct drm_encoder *encoder);
 extern int intel_plane_init(struct drm_device *dev, enum pipe pipe);
 
+bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
+				  unsigned int *pipe_bpp,
+				  struct drm_display_mode *mode);
+
 /* intel_panel.c */
 extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 				   struct drm_display_mode *adjusted_mode);
-- 
1.7.8.3

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

On Wed, 25 Jan 2012 08:16:25 -0800
Keith Packard <keithp@keithp.com> wrote:

> It is never correct to use intel_crtc->bpp in intel_dp_link_required,
> so instead pass an explicit bpp in to this function. This patch
> only supports 18bpp and 24bpp modes, which means that 10bpc modes will
> be computed incorrectly. Fixing that will require more extensive
> changes, and so must be addressed separately from this bugfix.
> 
> intel_dp_link_required is called from intel_dp_mode_valid and
> intel_dp_mode_fixup.
> 
> * intel_dp_mode_valid is called to list supported modes; in this case,
>   the current crtc values cannot be relevant as the modes in question
>   may never be selected. Thus, using intel_crtc->bpp is never right.
> 
> * intel_dp_mode_fixup is called during mode setting, but it is run
>   well before ironlake_crtc_mode_set is called to set intel_crtc->bpp,
>   so using intel_crtc-bpp in this path can only ever get a stale
>   value.

Yeah, looks like I got this wrong when I added the pipe bpp field.
Wonder if there's a good way to catch this sort of bug with an assert
so we don't get the order mixed up again...

The big downside here is that we'll be very pessimistic about the link
bw requirements for say 16 or 8bpp modes.

I see you have another patch to address some of this, but I wonder if
we have enough info to calculate the bpp at prepare time so it's set
early on for use by both the crtc and encoder code?

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]

On Wed, 25 Jan 2012 12:51:16 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Yeah, looks like I got this wrong when I added the pipe bpp field.
> Wonder if there's a good way to catch this sort of bug with an assert
> so we don't get the order mixed up again...

Ideally, we'd figure out a way to compute this only once. I think the
only relevant piece of data here is what bpc the pipe should run at, and
that can be computed at any time during the mode set. We'd then be able
to compare the pipe bpc with the frame buffer bpp to decide when to
dither in the crtc mode set code.

> The big downside here is that we'll be very pessimistic about the link
> bw requirements for say 16 or 8bpp modes.

Right, with this patch, we'll choose link parameters capable of
supporting 8bpc, even for 16bpp video modes. Note that 8bpp video modes
need 8bpc links as they have colormaps with 8bpc data in them.

> I see you have another patch to address some of this, but I wonder if
> we have enough info to calculate the bpp at prepare time so it's set
> early on for use by both the crtc and encoder code?

fixup gets exactly the same info as mode set, so it should end up with
exactly the same resulting bpc, which will set the link bandwidth to
support 6bpc mode if that's all that is needed.

One problem with the patch is that the pre-PCH mode set path doesn't use
intel_choose_pipe_bpp_dither, so the condition for 6bpc mode is
duplicated in i9xx_crtc_mode_set and intel_dp_mode_fixup.

If crtc->fixup was called before encoder->fixup, we could have computed
bpp in the crtc->fixup function and then used it in encoder->fixup.

Alternatively, crtc->fixup could call into the DP code to set the
link_bw, lane_count and adjusted_mode->clock values after it sets the
bpp, but that seems to break the abstraction even more.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Daniel Vetter]

On Wed, Jan 25, 2012 at 01:17:51PM -0800, Keith Packard wrote:
> On Wed, 25 Jan 2012 12:51:16 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > Yeah, looks like I got this wrong when I added the pipe bpp field.
> > Wonder if there's a good way to catch this sort of bug with an assert
> > so we don't get the order mixed up again...
> 
> Ideally, we'd figure out a way to compute this only once. I think the
> only relevant piece of data here is what bpc the pipe should run at, and
> that can be computed at any time during the mode set. We'd then be able
> to compare the pipe bpc with the frame buffer bpp to decide when to
> dither in the crtc mode set code.

I think we could compute this in crtc->mode_fixup (crtc->prepare doesn't
have the mode and adjusted_mode arguments). We could then store the
computed bpc and dithering in one of the private fields. We'd still have
to loop over all encoders, but alas ...

> > The big downside here is that we'll be very pessimistic about the link
> > bw requirements for say 16 or 8bpp modes.
> 
> Right, with this patch, we'll choose link parameters capable of
> supporting 8bpc, even for 16bpp video modes. Note that 8bpp video modes
> need 8bpc links as they have colormaps with 8bpc data in them.

Afaics we'll still correctly fall back to 6bpc (undithered for 16bpp
obviously) and hence things should keep on working.

> > I see you have another patch to address some of this, but I wonder if
> > we have enough info to calculate the bpp at prepare time so it's set
> > early on for use by both the crtc and encoder code?
> 
> fixup gets exactly the same info as mode set, so it should end up with
> exactly the same resulting bpc, which will set the link bandwidth to
> support 6bpc mode if that's all that is needed.
> 
> One problem with the patch is that the pre-PCH mode set path doesn't use
> intel_choose_pipe_bpp_dither, so the condition for 6bpc mode is
> duplicated in i9xx_crtc_mode_set and intel_dp_mode_fixup.
> 
> If crtc->fixup was called before encoder->fixup, we could have computed
> bpp in the crtc->fixup function and then used it in encoder->fixup.
> 
> Alternatively, crtc->fixup could call into the DP code to set the
> link_bw, lane_count and adjusted_mode->clock values after it sets the
> bpp, but that seems to break the abstraction even more.

Yeah, there are a few rough corners with the bpc computation in patch 2.
I'll try to throw around a few ideas that crossed my mind while reading
through it in a reply there.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Daniel Vetter]

On Wed, Jan 25, 2012 at 08:16:25AM -0800, Keith Packard wrote:
> It is never correct to use intel_crtc->bpp in intel_dp_link_required,
> so instead pass an explicit bpp in to this function. This patch
> only supports 18bpp and 24bpp modes, which means that 10bpc modes will
> be computed incorrectly. Fixing that will require more extensive
> changes, and so must be addressed separately from this bugfix.
> 
> intel_dp_link_required is called from intel_dp_mode_valid and
> intel_dp_mode_fixup.
> 
> * intel_dp_mode_valid is called to list supported modes; in this case,
>   the current crtc values cannot be relevant as the modes in question
>   may never be selected. Thus, using intel_crtc->bpp is never right.
> 
> * intel_dp_mode_fixup is called during mode setting, but it is run
>   well before ironlake_crtc_mode_set is called to set intel_crtc->bpp,
>   so using intel_crtc-bpp in this path can only ever get a stale
>   value.
> 
> Cc: Lubos Kolouch <lubos.kolouch@gmail.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Signed-off-by: Keith Packard <keithp@keithp.com>

Afaics this is correct and should fix quite a few "dp doesn't light up
issue" (in combination with 6bpc auto-dither code that's already there). I
think all the open issues are only about how to make this less
pessimistic and more generic, i.e. patch 2.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Select DP BPC at mode set, rather than mode validate

[Daniel Vetter]

On Wed, Jan 25, 2012 at 08:16:26AM -0800, Keith Packard wrote:
> The DP configuration must match the pipe configuration, and until mode
> set we don't know what BPC will be used. Delay all decisions about BPC
> until mode set, computing the target BPC in both intel_dp_mode_fixup
> and either i9xx_crtc_mode_set or ironlake_crtc_mode_set. It might be
> slightly better to compute this only once, but storing the value
> between those two calls would be a pain.
> 
> Cc: Lubos Kolouch <lubos.kolouch@gmail.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   27 +++++-------
>  drivers/gpu/drm/i915/intel_dp.c      |   77 +++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |    6 ++-
>  3 files changed, 78 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b3b51c4..d613676 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4850,9 +4850,9 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>   * Dithering requirement (i.e. false if display bpc and pipe bpc match,
>   * true if they don't match).
>   */
> -static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
> -					 unsigned int *pipe_bpp,
> -					 struct drm_display_mode *mode)
> +bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
> +				  unsigned int *pipe_bpp,
> +				  struct drm_display_mode *mode)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4883,13 +4883,13 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>  			continue;
>  		}
>  
> -		if (intel_encoder->type == INTEL_OUTPUT_EDP) {
> -			/* Use VBT settings if we have an eDP panel */
> -			unsigned int edp_bpc = dev_priv->edp.bpp / 3;
> +		if (intel_encoder->type == INTEL_OUTPUT_EDP ||
> +		    intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
> +			unsigned int dp_bpc = intel_dp_max_bpp(&intel_encoder->base, mode);
>  
> -			if (edp_bpc < display_bpc) {
> -				DRM_DEBUG_KMS("clamping display bpc (was %d) to eDP (%d)\n", display_bpc, edp_bpc);
> -				display_bpc = edp_bpc;
> +			if (dp_bpc < display_bpc) {
> +				DRM_DEBUG_KMS("clamping display bpc (was %d) to DP (%d)\n", display_bpc, dp_bpc);
> +				display_bpc = dp_bpc;
>  			}
>  			continue;
>  		}

I'm a bit unhappy how generic code in intel_display.c calls function out
of intel_dp.c. And choose_pipe_bpp_dither already has special cases for
quite a few other encoders ... Could we add an ->adjust_bpc function to
intel_encoder to separate this in a cleaner fashion?

I know that this isn't really the only layering violation in
intel_display.c, but functions in that file have an uncanny ability to
grow without bounds ;-)

> @@ -4923,11 +4923,6 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>  		}
>  	}
>  
> -	if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
> -		DRM_DEBUG_KMS("Dithering DP to 6bpc\n");
> -		display_bpc = 6;
> -	}
> -
>  	/*
>  	 * We could just drive the pipe at the highest bpc all the time and
>  	 * enable dithering as needed, but that costs bandwidth.  So choose
> @@ -4990,6 +4985,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	int ret;
>  	u32 temp;
>  	u32 lvds_sync = 0;
> +	int dp_max_bpp = 0;
>  
>  	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
>  		if (encoder->base.crtc != crtc)
> @@ -5016,6 +5012,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  			break;
>  		case INTEL_OUTPUT_DISPLAYPORT:
>  			is_dp = true;
> +			dp_max_bpp = intel_dp_max_bpp(&encoder->base, mode);
>  			break;
>  		}
>  
> @@ -5192,7 +5189,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	/* default to 8bpc */
>  	pipeconf &= ~(PIPECONF_BPP_MASK | PIPECONF_DITHER_EN);
>  	if (is_dp) {
> -		if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
> +		if (dp_max_bpp <= 18) {
>  			pipeconf |= PIPECONF_BPP_6 |
>  				    PIPECONF_DITHER_EN |
>  				    PIPECONF_DITHER_TYPE_SP;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 94f860c..2482796 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -219,14 +219,51 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
>  	return (max_link_clock * max_lanes * 8) / 10;
>  }
>  
> +/*
> + * For the specified encoder, return the maximum bpp that can be used
> + * for the specified mode.
> + */
> +
> +static const int dp_supported_bpc[] = { 6, 8, 10, 12, 16 };
> +
> +#define NUM_DP_SUPPORTED_BPC (sizeof dp_supported_bpc/sizeof dp_supported_bpc[0])
> +
> +int
> +intel_dp_max_bpp(struct drm_encoder *encoder, struct drm_display_mode *mode)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +	int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
> +	int max_lanes = intel_dp_max_lane_count(intel_dp);
> +	int max_rate, mode_rate;
> +	int i;
> +
> +	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
> +	for (i = NUM_DP_SUPPORTED_BPC - 1; i >= 0; i--) {
> +		int bpp = dp_supported_bpc[i] * 3;	/* 3 channels of data (RGB) */
> +
> +		mode_rate = intel_dp_link_required(mode->clock, bpp);
> +		if (mode_rate <= max_rate) {
> +
> +			/* Limit EDP bpp to panel ability */
> +			if (intel_dp->base.type == INTEL_OUTPUT_EDP) {
> +				struct drm_device *dev = encoder->dev;
> +				struct drm_i915_private *dev_priv = dev->dev_private;
> +				int edp_bpp = dev_priv->edp.bpp;
> +
> +				if (bpp > edp_bpp)
> +					bpp = edp_bpp;
> +			}
> +			return bpp;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int
>  intel_dp_mode_valid(struct drm_connector *connector,
>  		    struct drm_display_mode *mode)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
> -	int max_lanes = intel_dp_max_lane_count(intel_dp);
> -	int max_rate, mode_rate;
>  
>  	if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) {
>  		if (mode->hdisplay > intel_dp->panel_fixed_mode->hdisplay)
> @@ -236,16 +273,8 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  			return MODE_PANEL;
>  	}
>  
> -	mode_rate = intel_dp_link_required(mode->clock, 24);
> -	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
> -
> -	if (mode_rate > max_rate) {
> -			mode_rate = intel_dp_link_required(mode->clock, 18);
> -			if (mode_rate > max_rate)
> -				return MODE_CLOCK_HIGH;
> -			else
> -				mode->private_flags |= INTEL_MODE_DP_FORCE_6BPC;
> -	}
> +	if (intel_dp_max_bpp(&intel_dp->base.base, mode) == 0)
> +		return MODE_CLOCK_HIGH;
>  
>  	if (mode->clock < 10000)
>  		return MODE_CLOCK_LOW;
> @@ -673,9 +702,26 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
>  	int lane_count, clock;
>  	int max_lane_count = intel_dp_max_lane_count(intel_dp);
>  	int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
> -	int bpp = mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24;
> +	unsigned int bpp;
>  	static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
>  
> +	if (HAS_PCH_SPLIT(dev))
> +		(void) intel_choose_pipe_bpp_dither(intel_dp->base.base.crtc, &bpp, mode);
> +	else {
> +		bpp = intel_dp_max_bpp(encoder, mode);
> +		if (bpp > 24)
> +			bpp = 24;
> +	}

As you've already said in another mail, this PCH_SPLIT here looks a bit
strange. Could we unify these two paths here a bit?

Otherwise I couldn't poke holes into it.
-Daniel

> +
> +	DRM_DEBUG_KMS("encoder assuming bpp %d (bpc %d)\n",
> +		      bpp, bpp / 3);
> +
> +	if (bpp == 0) {
> +		DRM_DEBUG_KMS("Display port cannot support requested clock %d\n",
> +			      mode->clock);
> +		return false;
> +	}
> +
>  	if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) {
>  		intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode);
>  		intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN,
> @@ -691,8 +737,7 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
>  		for (clock = 0; clock <= max_clock; clock++) {
>  			int link_avail = intel_dp_max_data_rate(intel_dp_link_clock(bws[clock]), lane_count);
>  
> -			if (intel_dp_link_required(mode->clock, bpp)
> -					<= link_avail) {
> +			if (intel_dp_link_required(mode->clock, bpp) <= link_avail) {
>  				intel_dp->link_bw = bws[clock];
>  				intel_dp->lane_count = lane_count;
>  				adjusted_mode->clock = intel_dp_link_clock(intel_dp->link_bw);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1348705..03b4595 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -104,7 +104,6 @@
>  /* drm_display_mode->private_flags */
>  #define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0)
>  #define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT)
> -#define INTEL_MODE_DP_FORCE_6BPC (0x10)
>  
>  static inline void
>  intel_mode_set_pixel_multiplier(struct drm_display_mode *mode,
> @@ -303,11 +302,16 @@ extern void intel_dp_init(struct drm_device *dev, int dp_reg);
>  void
>  intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
>  		 struct drm_display_mode *adjusted_mode);
> +extern int intel_dp_max_bpp(struct drm_encoder *encoder, struct drm_display_mode *mode);
>  extern bool intel_dpd_is_edp(struct drm_device *dev);
>  extern void intel_edp_link_config(struct intel_encoder *, int *, int *);
>  extern bool intel_encoder_is_pch_edp(struct drm_encoder *encoder);
>  extern int intel_plane_init(struct drm_device *dev, enum pipe pipe);
>  
> +bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
> +				  unsigned int *pipe_bpp,
> +				  struct drm_display_mode *mode);
> +
>  /* intel_panel.c */
>  extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
>  				   struct drm_display_mode *adjusted_mode);
> -- 
> 1.7.8.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

On Wed, 25 Jan 2012 22:50:55 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:

> I think we could compute this in crtc->mode_fixup (crtc->prepare doesn't
> have the mode and adjusted_mode arguments). We could then store the
> computed bpc and dithering in one of the private fields. We'd still have
> to loop over all encoders, but alas ...

Alas, intel_crtc_mode_fixup is called *after* the intel_dp_mode_fixup. So,
we'd either need to change drm_crtc_helper, or have
intel_crtc_mode_fixup call down into intel_dp.c to set the link
parameters. In either case, ick.

> Afaics we'll still correctly fall back to 6bpc (undithered for 16bpp
> obviously) and hence things should keep on working.

Right, the problem is that the DP link parameters will be set to support
24bpp color, so we'll use a higher clock/lane-count than strictly
necessary as intel_dp_mode_fixup doesn't take the frame buffer format
into consideration when computing the link values.

> Yeah, there are a few rough corners with the bpc computation in patch 2.
> I'll try to throw around a few ideas that crossed my mind while reading
> through it in a reply there.

Thanks. I'm not happy with it either.

In short, I think we can (and should) apply the simple first patch to
drm-intel-fixes so that at least displays work consistently, and then
come up with a nicer patch that computes correct link parameters, and
also supports 10bpc formats.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Select DP BPC at mode set, rather than mode validate

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

On Wed, 25 Jan 2012 23:11:00 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:

> I'm a bit unhappy how generic code in intel_display.c calls function out
> of intel_dp.c. And choose_pipe_bpp_dither already has special cases for
> quite a few other encoders ... Could we add an ->adjust_bpc function to
> intel_encoder to separate this in a cleaner fashion?

Yeah, seems quite reasonable.

I can't find any reason why the lane count and link bw values are set in
fixup_mode and not just in intel_dp_set_mode. If we moved that, we could
use the bpp value computed in intel_display.c.

There's a weird mixture of code in ironlake_crtc_mode_set where it calls
intel_edp_link_config and uses those values when setting the CPU M/N
values for non-PCH eDP panels. That would also need fixing.

> I know that this isn't really the only layering violation in
> intel_display.c, but functions in that file have an uncanny ability to
> grow without bounds ;-)

The more we clean things up, the easier fixing bugs is in the future.

> As you've already said in another mail, this PCH_SPLIT here looks a bit
> strange. Could we unify these two paths here a bit?

The simple way to unify them would be to use
intel_choose_pipe_bpp_dither from the i9xx_crtc_mode_set path. Perhaps
that function could codify the currently simplistic rule used on i9xx?

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Daniel Vetter]

On Wed, Jan 25, 2012 at 08:16:25AM -0800, Keith Packard wrote:
> It is never correct to use intel_crtc->bpp in intel_dp_link_required,
> so instead pass an explicit bpp in to this function. This patch
> only supports 18bpp and 24bpp modes, which means that 10bpc modes will
> be computed incorrectly. Fixing that will require more extensive
> changes, and so must be addressed separately from this bugfix.
> 
> intel_dp_link_required is called from intel_dp_mode_valid and
> intel_dp_mode_fixup.
> 
> * intel_dp_mode_valid is called to list supported modes; in this case,
>   the current crtc values cannot be relevant as the modes in question
>   may never be selected. Thus, using intel_crtc->bpp is never right.
> 
> * intel_dp_mode_fixup is called during mode setting, but it is run
>   well before ironlake_crtc_mode_set is called to set intel_crtc->bpp,
>   so using intel_crtc-bpp in this path can only ever get a stale
>   value.
> 
> Cc: Lubos Kolouch <lubos.kolouch@gmail.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Signed-off-by: Keith Packard <keithp@keithp.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42263
Tested-by: camalot@picnicpark.org (Dell Latitude 6510)

Not the original reporter and might not exactly be this bug, but likely.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Dave Airlie]

On Fri, Jan 27, 2012 at 10:30 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jan 25, 2012 at 08:16:25AM -0800, Keith Packard wrote:
>> It is never correct to use intel_crtc->bpp in intel_dp_link_required,
>> so instead pass an explicit bpp in to this function. This patch
>> only supports 18bpp and 24bpp modes, which means that 10bpc modes will
>> be computed incorrectly. Fixing that will require more extensive
>> changes, and so must be addressed separately from this bugfix.
>>
>> intel_dp_link_required is called from intel_dp_mode_valid and
>> intel_dp_mode_fixup.
>>
>> * intel_dp_mode_valid is called to list supported modes; in this case,
>>   the current crtc values cannot be relevant as the modes in question
>>   may never be selected. Thus, using intel_crtc->bpp is never right.
>>
>> * intel_dp_mode_fixup is called during mode setting, but it is run
>>   well before ironlake_crtc_mode_set is called to set intel_crtc->bpp,
>>   so using intel_crtc-bpp in this path can only ever get a stale
>>   value.
>>
>> Cc: Lubos Kolouch <lubos.kolouch@gmail.com>
>> Cc: Adam Jackson <ajax@redhat.com>
>> Signed-off-by: Keith Packard <keithp@keithp.com>
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42263
> Tested-by: camalot@picnicpark.org (Dell Latitude 6510)
>
> Not the original reporter and might not exactly be this bug, but likely.
> -Daniel

Tested-by: Dave Airlie <airlied@redhat.com>

Without this patch the eDP panel on my HP 2540p won't resume.

Please get this into -fixes and too me asap.

Dave.

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Daniel Vetter]

On Mon, Feb 06, 2012 at 12:12:16PM +0000, Dave Airlie wrote:
> On Fri, Jan 27, 2012 at 10:30 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Jan 25, 2012 at 08:16:25AM -0800, Keith Packard wrote:
> >> It is never correct to use intel_crtc->bpp in intel_dp_link_required,
> >> so instead pass an explicit bpp in to this function. This patch
> >> only supports 18bpp and 24bpp modes, which means that 10bpc modes will
> >> be computed incorrectly. Fixing that will require more extensive
> >> changes, and so must be addressed separately from this bugfix.
> >>
> >> intel_dp_link_required is called from intel_dp_mode_valid and
> >> intel_dp_mode_fixup.
> >>
> >> * intel_dp_mode_valid is called to list supported modes; in this case,
> >>   the current crtc values cannot be relevant as the modes in question
> >>   may never be selected. Thus, using intel_crtc->bpp is never right.
> >>
> >> * intel_dp_mode_fixup is called during mode setting, but it is run
> >>   well before ironlake_crtc_mode_set is called to set intel_crtc->bpp,
> >>   so using intel_crtc-bpp in this path can only ever get a stale
> >>   value.
> >>
> >> Cc: Lubos Kolouch <lubos.kolouch@gmail.com>
> >> Cc: Adam Jackson <ajax@redhat.com>
> >> Signed-off-by: Keith Packard <keithp@keithp.com>
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42263
> > Tested-by: camalot@picnicpark.org (Dell Latitude 6510)
> >
> > Not the original reporter and might not exactly be this bug, but likely.
> > -Daniel
> 
> Tested-by: Dave Airlie <airlied@redhat.com>

Another bugzilla for this patch:

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44881
Tested-by: Roland Dreier <roland@digitalvampire.org>

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

On Mon, 6 Feb 2012 12:12:16 +0000, Dave Airlie <airlied@gmail.com> wrote:

> Please get this into -fixes and too me asap.

It's in -fixes; will be on its way to you soon.

This patch is sufficient to make machines work again; the second patch
serves to improve things a bit, and (as such) should probably wait in
drm-intel-next for another release.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]