[RFC] Deep color support

[RFC] Deep color support

[Jesse Barnes]

This collection of patches sits on top of my two DRM EDID patches to fetch
display color format support.  They fix a few bugs in our depth handling
code, and allow for deep color configurations to be enabled in the core
display code (still working on good tests for this).

Looking for comments on how things are split out and lots of review for
implications on various chipsets.

Thanks,
Jesse

[PATCH 1/7] drm/i915: color range only works on pre-ILK

[Jesse Barnes]

ILK+ provides this feature in the transcoder and pipe configuration instead.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index f289b86..a6871d8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -125,7 +125,8 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
 	u32 sdvox;
 
 	sdvox = SDVO_ENCODING_HDMI | SDVO_BORDER_ENABLE;
-	sdvox |= intel_hdmi->color_range;
+	if (!HAS_PCH_SPLIT(dev))
+		sdvox |= intel_hdmi->color_range;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
 		sdvox |= SDVO_VSYNC_ACTIVE_HIGH;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
-- 
1.7.4.1

Re: [PATCH 1/7] drm/i915: color range only works on pre-ILK

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 364 bytes --]

On Tue, 19 Apr 2011 12:12:35 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> ILK+ provides this feature in the transcoder and pipe configuration
> instead.

This doesn't seem to add in a new implementation of this feature for
ILK, just stops using the pre-ILK version on ILK. Please explain how
this is sufficient?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/7] drm/i915: color range only works on pre-ILK

[Jesse Barnes]

On Tue, 10 May 2011 14:12:48 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Tue, 19 Apr 2011 12:12:35 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > ILK+ provides this feature in the transcoder and pipe configuration
> > instead.
> 
> This doesn't seem to add in a new implementation of this feature for
> ILK, just stops using the pre-ILK version on ILK. Please explain how
> this is sufficient?

That's in patch 7/7, you want me to combine this fix with the feature
addition for ILK+?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/7] drm/i915: color range only works on pre-ILK

[Jesse Barnes]

On Tue, 10 May 2011 14:16:57 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Tue, 10 May 2011 14:12:48 -0700
> Keith Packard <keithp@keithp.com> wrote:
> 
> > On Tue, 19 Apr 2011 12:12:35 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > 
> > > ILK+ provides this feature in the transcoder and pipe configuration
> > > instead.
> > 
> > This doesn't seem to add in a new implementation of this feature for
> > ILK, just stops using the pre-ILK version on ILK. Please explain how
> > this is sufficient?
> 
> That's in patch 7/7, you want me to combine this fix with the feature
> addition for ILK+?

Actually not; this is just a bug fix.  I haven't looked at adding
broadcast colorspace support to ILK+ yet.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/7] drm/i915: color range only works on pre-ILK

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 301 bytes --]

On Tue, 10 May 2011 14:16:57 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> That's in patch 7/7, you want me to combine this fix with the feature
> addition for ILK+?

No, I'm good with separate patches, just want to see the patch described
correctly.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 2/7] drm/i915: don't set transcoder bpc on CougarPoint

[Jesse Barnes]

BPC is specified in FDI RX on CPT and above, rather than in the transcoder.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 09b20b1..69649b2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1157,12 +1157,15 @@ static void intel_enable_transcoder(struct drm_i915_private *dev_priv,
 
 	reg = TRANSCONF(pipe);
 	val = I915_READ(reg);
-	/*
-	 * make the BPC in transcoder be consistent with
-	 * that in pipeconf reg.
-	 */
-	val &= ~PIPE_BPC_MASK;
-	val |= I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK;
+
+	if (HAS_PCH_IBX(dev_priv->dev)) {
+		/*
+		 * make the BPC in transcoder be consistent with
+		 * that in pipeconf reg.
+		 */
+		val &= ~PIPE_BPC_MASK;
+		val |= I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK;
+	}
 	I915_WRITE(reg, val | TRANS_ENABLE);
 	if (wait_for(I915_READ(reg) & TRANS_STATE_ENABLE, 100))
 		DRM_ERROR("failed to enable transcoder %d\n", pipe);
-- 
1.7.4.1

[PATCH 3/7] drm/i915: set bpc for DP transcoder

[Jesse Barnes]

This may not be the default value, so pull the bpc out of the pipe reg
and write it to the DP transcoder so proper dithering and signaling
occurs.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 69649b2..8ef0c39 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2504,6 +2504,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 	/* For PCH DP, enable TRANS_DP_CTL */
 	if (HAS_PCH_CPT(dev) &&
 	    intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
+		u32 bpc = (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) >> 5;
 		reg = TRANS_DP_CTL(pipe);
 		temp = I915_READ(reg);
 		temp &= ~(TRANS_DP_PORT_SEL_MASK |
@@ -2511,7 +2512,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 			  TRANS_DP_BPC_MASK);
 		temp |= (TRANS_DP_OUTPUT_ENABLE |
 			 TRANS_DP_ENH_FRAMING);
-		temp |= TRANS_DP_8BPC;
+		temp |= bpc << 9; /* same format but at 11:9 */
 
 		if (crtc->mode.flags & DRM_MODE_FLAG_PHSYNC)
 			temp |= TRANS_DP_HSYNC_ACTIVE_HIGH;
-- 
1.7.4.1

[PATCH 4/7] drm/i915: split out Ironlake pipe bpp picking code

[Jesse Barnes]

Figuring out which pipe bpp to use is a bit painful.  It depends on both
the encoder and display configuration attached to a pipe.  For instance,
to drive a 24bpp framebuffer out to an 18bpp panel, we need to use 6bpc
on the pipe but also enable dithering.  But driving that same
framebuffer to a DisplayPort output on another pipe means using 8bpc and
no dithering.

So split out and enhance the code to handle the various cases, returning
an appropriate pipe bpp as well as whether dithering should be enabled.

Save the resulting pipe bpp in the intel_crtc struct for use by encoders
in calculating bandwidth requirements (defaults to 24bpp on pre-ILK).

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |  181 ++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 2 files changed, 140 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ef0c39..e81e418 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4232,6 +4232,120 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
 	return dev_priv->lvds_use_ssc && i915_panel_use_ssc;
 }
 
+/**
+ * intel_choose_pipe_bpp - figure out what color depth the pipe should send
+ * @crtc: CRTC structure
+ *
+ * A pipe may be connected to one or more outputs.  Based on the depth of the
+ * attached framebuffer, choose a good color depth to use on the pipe.
+ *
+ * If possible, match the pipe depth to the fb depth.  In some cases, this
+ * isn't ideal, because the connected output supports a lesser or restricted
+ * set of depths.  Resolve that here:
+ *    LVDS typically supports only 6bpc, so clamp down in that case
+ *    HDMI supports only 8bpc or 12bpc, so clamp to 8bpc with dither for 10bpc
+ *    Displays may support a restricted set as well, check EDID and clamp as
+ *      appropriate.
+ *
+ * RETURNS:
+ * Dithering requirement (i.e. false if display bpc and pipe bpc match,
+ * true if they don't match).
+ */
+static bool intel_choose_pipe_bpp(struct drm_crtc *crtc, unsigned int *pipe_bpp)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+	unsigned int display_bpc = UINT_MAX, fb_bpc, bpc;
+
+	fb_bpc = crtc->fb->depth / 3;
+
+	/* Walk the encoders & connectors on this crtc, get min bpc */
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
+
+		if (encoder->crtc != crtc)
+			continue;
+
+		if (intel_encoder->type == INTEL_OUTPUT_LVDS) {
+			unsigned int lvds_bpc;
+
+			if ((I915_READ(PCH_LVDS) & LVDS_A3_POWER_MASK) ==
+			    LVDS_A3_POWER_UP)
+				lvds_bpc = 8;
+			else
+				lvds_bpc = 6;
+
+			if (lvds_bpc < display_bpc)
+				display_bpc = lvds_bpc;
+			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 (edp_bpc < display_bpc)
+				display_bpc = edp_bpc;
+			continue;
+		}
+
+		/* Not one of the known troublemakers, check the EDID */
+		list_for_each_entry(connector, &dev->mode_config.connector_list,
+				    head) {
+			if (connector->encoder != encoder)
+				continue;
+
+			if (connector->display_info.bpc < display_bpc)
+				display_bpc = connector->display_info.bpc;
+		}
+
+		/*
+		 * HDMI is either 12 or 8, so if the display let 10bpc sneak
+		 * through, clamp it down.  (Note: >12bpc will be caught below.)
+		 */
+		if (intel_encoder->type == INTEL_OUTPUT_HDMI) {
+			if (display_bpc > 8 && display_bpc < 12)
+				display_bpc = 12;
+			else
+				display_bpc = 8;
+		}
+	}
+
+	/*
+	 * We could just drive the pipe at the highest bpc all the time and
+	 * enable dithering as needed, but that costs bandwidth.  So choose
+	 * the minimum value that expresses the full color range of the fb but
+	 * also stays within the max display bpc discovered above.
+	 */
+
+	switch (crtc->fb->depth) {
+	case 8:
+	case 15:
+	case 16:
+		bpc = 6; /* min is 18bpp */
+		break;
+	case 24:
+		bpc = min((unsigned int)8, display_bpc);
+		break;
+	case 30:
+		bpc = min((unsigned int)10, display_bpc);
+		break;
+	case 48:
+		bpc = min((unsigned int)12, display_bpc);
+		break;
+	default:
+		DRM_DEBUG("unsupported depth, assuming 24 bits\n");
+		bpc = min((unsigned int)8, display_bpc);
+		break;
+	}
+
+	*pipe_bpp = bpc * 3;
+
+	return display_bpc != bpc;
+}
+
 static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 			      struct drm_display_mode *mode,
 			      struct drm_display_mode *adjusted_mode,
@@ -4644,7 +4758,9 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	struct fdi_m_n m_n = {0};
 	u32 temp;
 	u32 lvds_sync = 0;
-	int target_clock, pixel_multiplier, lane, link_bw, bpp, factor;
+	int target_clock, pixel_multiplier, lane, link_bw, factor;
+	unsigned int pipe_bpp;
+	bool dither;
 
 	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
 		if (encoder->base.crtc != crtc)
@@ -4771,56 +4887,37 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	/* determine panel color depth */
 	temp = I915_READ(PIPECONF(pipe));
 	temp &= ~PIPE_BPC_MASK;
-	if (is_lvds) {
-		/* the BPC will be 6 if it is 18-bit LVDS panel */
-		if ((I915_READ(PCH_LVDS) & LVDS_A3_POWER_MASK) == LVDS_A3_POWER_UP)
-			temp |= PIPE_8BPC;
-		else
-			temp |= PIPE_6BPC;
-	} else if (has_edp_encoder) {
-		switch (dev_priv->edp.bpp/3) {
-		case 8:
-			temp |= PIPE_8BPC;
-			break;
-		case 10:
-			temp |= PIPE_10BPC;
-			break;
-		case 6:
-			temp |= PIPE_6BPC;
-			break;
-		case 12:
-			temp |= PIPE_12BPC;
-			break;
-		}
-	} else
-		temp |= PIPE_8BPC;
-	I915_WRITE(PIPECONF(pipe), temp);
-
-	switch (temp & PIPE_BPC_MASK) {
-	case PIPE_8BPC:
-		bpp = 24;
+	dither = intel_choose_pipe_bpp(crtc, &pipe_bpp);
+	switch (pipe_bpp) {
+	case 18:
+		temp |= PIPE_6BPC;
 		break;
-	case PIPE_10BPC:
-		bpp = 30;
+	case 24:
+		temp |= PIPE_8BPC;
 		break;
-	case PIPE_6BPC:
-		bpp = 18;
+	case 30:
+		temp |= PIPE_10BPC;
 		break;
-	case PIPE_12BPC:
-		bpp = 36;
+	case 36:
+		temp |= PIPE_12BPC;
 		break;
 	default:
-		DRM_ERROR("unknown pipe bpc value\n");
-		bpp = 24;
+		WARN(1, "intel_choose_pipe_bpp returned invalid value\n");
+		temp |= PIPE_8BPC;
+		pipe_bpp = 24;
+		break;
 	}
 
+	intel_crtc->bpp = pipe_bpp;
+	I915_WRITE(PIPECONF(pipe), temp);
+
 	if (!lane) {
 		/*
 		 * Account for spread spectrum to avoid
 		 * oversubscribing the link. Max center spread
 		 * is 2.5%; use 5% for safety's sake.
 		 */
-		u32 bps = target_clock * bpp * 21 / 20;
+		u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
 		lane = bps / (link_bw * 8) + 1;
 	}
 
@@ -4828,7 +4925,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	if (pixel_multiplier > 1)
 		link_bw *= pixel_multiplier;
-	ironlake_compute_m_n(bpp, lane, target_clock, link_bw, &m_n);
+	ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
+			     &m_n);
 
 	/* Ironlake: try to setup display ref clock before DPLL
 	 * enabling. This is only under driver's control after
@@ -5031,14 +5129,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		I915_WRITE(PCH_LVDS, temp);
 	}
 
-	/* set the dithering flag and clear for anything other than a panel. */
 	pipeconf &= ~PIPECONF_DITHER_EN;
 	pipeconf &= ~PIPECONF_DITHER_TYPE_MASK;
-	if (dev_priv->lvds_dither && (is_lvds || has_edp_encoder)) {
+	if ((is_lvds && dev_priv->lvds_dither) || dither) {
 		pipeconf |= PIPECONF_DITHER_EN;
 		pipeconf |= PIPECONF_DITHER_TYPE_ST1;
 	}
-
 	if (is_dp || intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 	} else {
@@ -6330,6 +6426,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	intel_crtc_reset(&intel_crtc->base);
 	intel_crtc->active = true; /* force the pipe off on setup_init_config */
+	intel_crtc->bpp = 24; /* default for pre-Ironlake */
 
 	if (HAS_PCH_SPLIT(dev)) {
 		intel_helper_funcs.prepare = ironlake_crtc_prepare;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f5b0d83..051345c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -171,6 +171,7 @@ struct intel_crtc {
 	int16_t cursor_x, cursor_y;
 	int16_t cursor_width, cursor_height;
 	bool cursor_visible;
+	unsigned int bpp;
 };
 
 #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
-- 
1.7.4.1

Re: [PATCH 4/7] drm/i915: split out Ironlake pipe bpp picking code

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 3998 bytes --]

On Tue, 19 Apr 2011 12:12:38 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Figuring out which pipe bpp to use is a bit painful.  It depends on both
> the encoder and display configuration attached to a pipe.  For instance,
> to drive a 24bpp framebuffer out to an 18bpp panel, we need to use 6bpc
> on the pipe but also enable dithering.  But driving that same
> framebuffer to a DisplayPort output on another pipe means using 8bpc and
> no dithering.
> 
> So split out and enhance the code to handle the various cases, returning
> an appropriate pipe bpp as well as whether dithering should be enabled.
> 
> Save the resulting pipe bpp in the intel_crtc struct for use by encoders
> in calculating bandwidth requirements (defaults to 24bpp on pre-ILK).
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  181 ++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  2 files changed, 140 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ef0c39..e81e418 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4232,6 +4232,120 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>  	return dev_priv->lvds_use_ssc && i915_panel_use_ssc;
>  }
>  
> +/**
> + * intel_choose_pipe_bpp - figure out what color depth the pipe should send
> + * @crtc: CRTC structure
> + *
> + * A pipe may be connected to one or more outputs.  Based on the depth of the
> + * attached framebuffer, choose a good color depth to use on the pipe.
> + *
> + * If possible, match the pipe depth to the fb depth.  In some cases, this
> + * isn't ideal, because the connected output supports a lesser or restricted
> + * set of depths.  Resolve that here:
> + *    LVDS typically supports only 6bpc, so clamp down in that case
> + *    HDMI supports only 8bpc or 12bpc, so clamp to 8bpc with dither for 10bpc
> + *    Displays may support a restricted set as well, check EDID and clamp as
> + *      appropriate.
> + *
> + * RETURNS:
> + * Dithering requirement (i.e. false if display bpc and pipe bpc match,
> + * true if they don't match).
> + */
> +static bool intel_choose_pipe_bpp(struct drm_crtc *crtc, unsigned int
> *pipe_bpp)

Should mention dithering in the name, perhaps
choose_pipe_bpp_and_dithering or some such? Perhaps returning the
dithering in another by-reference parameter?

> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_encoder *encoder;
> +	struct drm_connector *connector;
> +	unsigned int display_bpc = UINT_MAX, fb_bpc, bpc;
> +
> +	fb_bpc = crtc->fb->depth / 3;
> +
> +	/* Walk the encoders & connectors on this crtc, get min bpc */

Don't you want the max bpc?

> +
> +	/*
> +	 * We could just drive the pipe at the highest bpc all the time and
> +	 * enable dithering as needed, but that costs bandwidth.  So choose
> +	 * the minimum value that expresses the full color range of the fb but
> +	 * also stays within the max display bpc discovered above.
> +	 */
> +
> +	switch (crtc->fb->depth) {
> +	case 8:
> +	case 15:
> +	case 16:
> +		bpc = 6; /* min is 18bpp */
> +		break;
> +	case 24:
> +		bpc = min((unsigned int)8, display_bpc);
> +		break;
> +	case 30:
> +		bpc = min((unsigned int)10, display_bpc);
> +		break;
> +	case 48:
> +		bpc = min((unsigned int)12, display_bpc);
> +		break;
> +	default:
> +		DRM_DEBUG("unsupported depth, assuming 24 bits\n");
> +		bpc = min((unsigned int)8, display_bpc);
> +		break;
> +	}

Hrm. 8-bit through a colormap should probably use 8 bpc? We can probably
ignore 15/16-bit direct color (which might want 8bpc as well).

The simple fix would be to use 8bpc on 8bpp and otherwise leave things alone.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/7] drm/i915: split out Ironlake pipe bpp picking code

[Jesse Barnes]

On Tue, 10 May 2011 14:23:18 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Tue, 19 Apr 2011 12:12:38 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Figuring out which pipe bpp to use is a bit painful.  It depends on both
> > the encoder and display configuration attached to a pipe.  For instance,
> > to drive a 24bpp framebuffer out to an 18bpp panel, we need to use 6bpc
> > on the pipe but also enable dithering.  But driving that same
> > framebuffer to a DisplayPort output on another pipe means using 8bpc and
> > no dithering.
> > 
> > So split out and enhance the code to handle the various cases, returning
> > an appropriate pipe bpp as well as whether dithering should be enabled.
> > 
> > Save the resulting pipe bpp in the intel_crtc struct for use by encoders
> > in calculating bandwidth requirements (defaults to 24bpp on pre-ILK).
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  181 ++++++++++++++++++++++++++--------
> >  drivers/gpu/drm/i915/intel_drv.h     |    1 +
> >  2 files changed, 140 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8ef0c39..e81e418 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4232,6 +4232,120 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
> >  	return dev_priv->lvds_use_ssc && i915_panel_use_ssc;
> >  }
> >  
> > +/**
> > + * intel_choose_pipe_bpp - figure out what color depth the pipe should send
> > + * @crtc: CRTC structure
> > + *
> > + * A pipe may be connected to one or more outputs.  Based on the depth of the
> > + * attached framebuffer, choose a good color depth to use on the pipe.
> > + *
> > + * If possible, match the pipe depth to the fb depth.  In some cases, this
> > + * isn't ideal, because the connected output supports a lesser or restricted
> > + * set of depths.  Resolve that here:
> > + *    LVDS typically supports only 6bpc, so clamp down in that case
> > + *    HDMI supports only 8bpc or 12bpc, so clamp to 8bpc with dither for 10bpc
> > + *    Displays may support a restricted set as well, check EDID and clamp as
> > + *      appropriate.
> > + *
> > + * RETURNS:
> > + * Dithering requirement (i.e. false if display bpc and pipe bpc match,
> > + * true if they don't match).
> > + */
> > +static bool intel_choose_pipe_bpp(struct drm_crtc *crtc, unsigned int
> > *pipe_bpp)
> 
> Should mention dithering in the name, perhaps
> choose_pipe_bpp_and_dithering or some such? Perhaps returning the
> dithering in another by-reference parameter?

Ok fixed, just added _dither to the name.

> 
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_encoder *encoder;
> > +	struct drm_connector *connector;
> > +	unsigned int display_bpc = UINT_MAX, fb_bpc, bpc;
> > +
> > +	fb_bpc = crtc->fb->depth / 3;
> > +
> > +	/* Walk the encoders & connectors on this crtc, get min bpc */
> 
> Don't you want the max bpc?

If the comment is confusing, I can remove it.  Or maybe I need to
expand it.

What we're trying to do is find a common bpc for all the displays
attached to the pipe.  But we need to use the minimum of all attached in
order to set the pipe's dithering correctly, since dithering occurs at
the pipe level, not the output (at least on ILK+).

> > +
> > +	/*
> > +	 * We could just drive the pipe at the highest bpc all the time and
> > +	 * enable dithering as needed, but that costs bandwidth.  So choose
> > +	 * the minimum value that expresses the full color range of the fb but
> > +	 * also stays within the max display bpc discovered above.
> > +	 */
> > +
> > +	switch (crtc->fb->depth) {
> > +	case 8:
> > +	case 15:
> > +	case 16:
> > +		bpc = 6; /* min is 18bpp */
> > +		break;
> > +	case 24:
> > +		bpc = min((unsigned int)8, display_bpc);
> > +		break;
> > +	case 30:
> > +		bpc = min((unsigned int)10, display_bpc);
> > +		break;
> > +	case 48:
> > +		bpc = min((unsigned int)12, display_bpc);
> > +		break;
> > +	default:
> > +		DRM_DEBUG("unsupported depth, assuming 24 bits\n");
> > +		bpc = min((unsigned int)8, display_bpc);
> > +		break;
> > +	}
> 
> Hrm. 8-bit through a colormap should probably use 8 bpc? We can probably
> ignore 15/16-bit direct color (which might want 8bpc as well).
> 
> The simple fix would be to use 8bpc on 8bpp and otherwise leave things alone.

Ok, fixed.  New patches on their way.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 5/7] drm/i915: split out plane update code

[Jesse Barnes]

Updating the planes is device specific, so create a new display callback
and use it in pipe_set_base.  (In fact we could go even further, valid
display plane bits have changed with each generation, as has tiled
buffer handling.)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h      |    2 +
 drivers/gpu/drm/i915/intel_display.c |  107 +++++++++++++++++++++++++++++++---
 2 files changed, 100 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7ee0ac8..04fe37e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -208,6 +208,8 @@ struct drm_i915_display_funcs {
 			     struct drm_display_mode *adjusted_mode,
 			     int x, int y,
 			     struct drm_framebuffer *old_fb);
+	int (*update_plane)(struct drm_crtc *crtc, struct drm_framebuffer *fb,
+			    int x, int y);
 
 	/* clock updates for mode set */
 	/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e81e418..b727d7f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1839,10 +1839,8 @@ err_interruptible:
 	return ret;
 }
 
-/* Assume fb object is pinned & idle & fenced and just update base pointers */
-static int
-intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
-			   int x, int y, enum mode_set_atomic state)
+static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
+			     int x, int y)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1885,7 +1883,7 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 		dspcntr |= DISPPLANE_32BPP_NO_ALPHA;
 		break;
 	default:
-		DRM_ERROR("Unknown color depth\n");
+		DRM_ERROR("Unknown color depth %d\n", fb->bits_per_pixel);
 		return -EINVAL;
 	}
 	if (INTEL_INFO(dev)->gen >= 4) {
@@ -1895,10 +1893,6 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 			dspcntr &= ~DISPPLANE_TILED;
 	}
 
-	if (HAS_PCH_SPLIT(dev))
-		/* must disable */
-		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
-
 	I915_WRITE(reg, dspcntr);
 
 	Start = obj->gtt_offset;
@@ -1915,6 +1909,99 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 		I915_WRITE(DSPADDR(plane), Start + Offset);
 	POSTING_READ(reg);
 
+	return 0;
+}
+
+static int ironlake_update_plane(struct drm_crtc *crtc,
+				 struct drm_framebuffer *fb, int x, int y)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_framebuffer *intel_fb;
+	struct drm_i915_gem_object *obj;
+	int plane = intel_crtc->plane;
+	unsigned long Start, Offset;
+	u32 dspcntr;
+	u32 reg;
+
+	switch (plane) {
+	case 0:
+	case 1:
+		break;
+	default:
+		DRM_ERROR("Can't update plane %d in SAREA\n", plane);
+		return -EINVAL;
+	}
+
+	intel_fb = to_intel_framebuffer(fb);
+	obj = intel_fb->obj;
+
+	reg = DSPCNTR(plane);
+	dspcntr = I915_READ(reg);
+	/* Mask out pixel format bits in case we change it */
+	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
+	switch (fb->bits_per_pixel) {
+	case 8:
+		dspcntr |= DISPPLANE_8BPP;
+		break;
+	case 16:
+		if (fb->depth != 16)
+			return -EINVAL;
+
+		dspcntr |= DISPPLANE_16BPP;
+		break;
+	case 24:
+	case 32:
+		if (fb->depth == 24)
+			dspcntr |= DISPPLANE_32BPP_NO_ALPHA;
+		else if (fb->depth == 30)
+			dspcntr |= DISPPLANE_32BPP_30BIT_NO_ALPHA;
+		else
+			return -EINVAL;
+		break;
+	default:
+		DRM_ERROR("Unknown color depth %d\n", fb->bits_per_pixel);
+		return -EINVAL;
+	}
+
+	if (obj->tiling_mode != I915_TILING_NONE)
+		dspcntr |= DISPPLANE_TILED;
+	else
+		dspcntr &= ~DISPPLANE_TILED;
+
+	/* must disable */
+	dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
+
+	I915_WRITE(reg, dspcntr);
+
+	Start = obj->gtt_offset;
+	Offset = y * fb->pitch + x * (fb->bits_per_pixel / 8);
+
+	DRM_DEBUG_KMS("Writing base %08lX %08lX %d %d %d\n",
+		      Start, Offset, x, y, fb->pitch);
+	I915_WRITE(DSPSTRIDE(plane), fb->pitch);
+	I915_WRITE(DSPSURF(plane), Start);
+	I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
+	I915_WRITE(DSPADDR(plane), Offset);
+	POSTING_READ(reg);
+
+	return 0;
+}
+
+/* Assume fb object is pinned & idle & fenced and just update base pointers */
+static int
+intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
+			   int x, int y, enum mode_set_atomic state)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	ret = dev_priv->display.update_plane(crtc, fb, x, y);
+	if (ret)
+		return ret;
+
 	intel_update_fbc(dev);
 	intel_increase_pllclock(crtc);
 
@@ -7346,9 +7433,11 @@ static void intel_init_display(struct drm_device *dev)
 	if (HAS_PCH_SPLIT(dev)) {
 		dev_priv->display.dpms = ironlake_crtc_dpms;
 		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
+		dev_priv->display.update_plane = ironlake_update_plane;
 	} else {
 		dev_priv->display.dpms = i9xx_crtc_dpms;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
+		dev_priv->display.update_plane = i9xx_update_plane;
 	}
 
 	if (I915_HAS_FBC(dev)) {
-- 
1.7.4.1

[PATCH 6/7] drm/i915: use pipe bpp in DP link bandwidth calculations

[Jesse Barnes]

The pipe may be driving various bpp values depending on the display
configuration, so take that into account when calculating link bandwidth
requirements.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0daefca..bce9edb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -684,7 +684,7 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	struct drm_encoder *encoder;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int lane_count = 4, bpp = 24;
+	int lane_count = 4;
 	struct intel_dp_m_n m_n;
 	int pipe = intel_crtc->pipe;
 
@@ -703,7 +703,6 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 			break;
 		} else if (is_edp(intel_dp)) {
 			lane_count = dev_priv->edp.lanes;
-			bpp = dev_priv->edp.bpp;
 			break;
 		}
 	}
@@ -713,7 +712,7 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	 * the number of bytes_per_pixel post-LUT, which we always
 	 * set up for 8-bits of R/G/B, or 3 bytes total.
 	 */
-	intel_dp_compute_m_n(bpp, lane_count,
+	intel_dp_compute_m_n(intel_crtc->bpp, lane_count,
 			     mode->clock, adjusted_mode->clock, &m_n);
 
 	if (HAS_PCH_SPLIT(dev)) {
-- 
1.7.4.1

Re: [PATCH 6/7] drm/i915: use pipe bpp in DP link bandwidth calculations

[Jesse Barnes]

On Tue, 19 Apr 2011 12:12:40 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> The pipe may be driving various bpp values depending on the display
> configuration, so take that into account when calculating link bandwidth
> requirements.
> 

Oops, mixed up this changelog with another piece.  The bpp is actually
being used to calculate the PLL values here, not bw requirements.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 7/7] drm/i915: use pipe bpp when setting HDMI bpc

[Jesse Barnes]

The Intel HDMI encoder can support 8bpc or 12bpc.  Set the appropriate
value based on the pipe bpp when configuring the output.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a6871d8..5cf7b23 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -132,6 +132,11 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
 	if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
 		sdvox |= SDVO_HSYNC_ACTIVE_HIGH;
 
+	if (intel_crtc->bpp > 24)
+		sdvox |= COLOR_FORMAT_12bpc;
+	else
+		sdvox |= COLOR_FORMAT_8bpc;
+
 	/* Required on CPT */
 	if (intel_hdmi->has_hdmi_sink && HAS_PCH_CPT(dev))
 		sdvox |= HDMI_MODE_SELECT;
-- 
1.7.4.1

[ANCIENT PATCH] Enable 30-bit depth

[Andy Lutomirski]

Signed-off-by: Andy Lutomirski <luto@mit.edu>
---

This patch is over a year old, caused problems, and probably doesn't even apply anymore.  It worked at least a little bit, though.  There's a lot more that needs doing, especially in relation to DirectColor mode.

 src/intel_driver.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/intel_driver.c b/src/intel_driver.c
index 1ef16ed..99d32b8 100644
--- a/src/intel_driver.c
+++ b/src/intel_driver.c
@@ -372,10 +372,10 @@ static void intel_check_dri_option(ScrnInfoPtr scrn)
 	if (!xf86ReturnOptValBool(intel->Options, OPTION_DRI, TRUE))
 		intel->directRenderingType = DRI_DISABLED;
 
-	if (scrn->depth != 16 && scrn->depth != 24) {
+	if (scrn->depth != 16 && scrn->depth != 24 && scrn->depth != 30) {
 		xf86DrvMsg(scrn->scrnIndex, X_CONFIG,
 			   "DRI is disabled because it "
-			   "runs only at depths 16 and 24.\n");
+			   "runs only at depths 16, 24, and 30.\n");
 		intel->directRenderingType = DRI_DISABLED;
 	}
 }
@@ -570,6 +570,7 @@ static Bool I830PreInit(ScrnInfoPtr scrn, int flags)
 	case 15:
 	case 16:
 	case 24:
+	case 30:
 		break;
 	default:
 		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
-- 
1.7.4.2

Re: [ANCIENT PATCH] Enable 30-bit depth

[Jesse Barnes]

On Tue, 19 Apr 2011 15:50:01 -0400
Andy Lutomirski <luto@MIT.EDU> wrote:

> Signed-off-by: Andy Lutomirski <luto@mit.edu>
> ---
> 
> This patch is over a year old, caused problems, and probably doesn't even apply anymore.  It worked at least a little bit, though.  There's a lot more that needs doing, especially in relation to DirectColor mode.

Wouldn't be surprised if you ran into kernel issues as well; the bits
have moved around and various paths had issues with non-24bpp formats.

I'll try mucking with the DDX though and see what I can get working...

-- 
Jesse Barnes, Intel Open Source Technology Center