[PATCH 00/13] pipe_config basic infrastructure

[PATCH 00/13] pipe_config basic infrastructure

[Daniel Vetter]

Hi all,

Changes since last time around (i.e. not including the fixups I've sent
already):
- Apply bikeshed from Paulo.
- Split up patches for Jesse.
- Rebase onto latest dinq.

Happy review!

Cheers, Daniel

Daniel Vetter (13):
  drm/i915: introduce struct intel_crtc_config
  drm/i915: compute pipe_config earlier
  drm/i915: add pipe_config->timings_set
  drm/i915: add pipe_config->pixel_multiplier
  drm/i915: drop helper vtable for sdvo encoder
  drm/i915: add pipe_config->has_pch_encoder
  drm/i915: add pipe_config->limited_color_range
  drm/i915: introduce pipe_config->dither|pipe_bpp
  drm/i915: precompute pipe bpp before touching the hw
  drm/i915: convert DP autodither code to new infrastructure
  drm/i915: clean up plane bpp confusion
  drm/i915: clean up pipe bpp confusion
  drm/i915: clear up the fdi/dp set_m_n confusion

 drivers/gpu/drm/i915/i915_drv.h      |   4 +-
 drivers/gpu/drm/i915/intel_crt.c     |  12 +-
 drivers/gpu/drm/i915/intel_ddi.c     |  27 +-
 drivers/gpu/drm/i915/intel_display.c | 550 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_dp.c      | 155 ++++------
 drivers/gpu/drm/i915/intel_drv.h     |  76 ++---
 drivers/gpu/drm/i915/intel_hdmi.c    |  34 ++-
 drivers/gpu/drm/i915/intel_lvds.c    |  33 ++-
 drivers/gpu/drm/i915/intel_sdvo.c    |  55 ++--
 drivers/gpu/drm/i915/intel_tv.c      |  14 +-
 10 files changed, 467 insertions(+), 493 deletions(-)

-- 
1.7.11.7

[PATCH 01/13] drm/i915: introduce struct intel_crtc_config

[Daniel Vetter]

Currently only containing the requested and the adjusted mode. And
only crtc callbacks are converted somewhat to it, encoders will be
done on a as-needed basis (simply too much churn in one patch
otherwise).

Future patches will add tons more useful stuff to this struct,
starting with the very simple.

v2: Store the pipe_config in the intel_crtc, so that the ->mode-set,
->enable and also ->disable have easy access to it.

v3: Store the pipe config in the right crtc ...

v4: Rebased.

v5: Fixup an OOPS when trying to kfree an ERR_PTR.

v6: Used drm_moode_copy and some other small cleanups as suggested
by Ville Syrjälä.

v7: drm_mode_copy preserves the mode id of the destination, so no need
to clear it again (Ville).

v8: Break a long line spotted by Paulo.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      |  4 +-
 drivers/gpu/drm/i915/intel_display.c | 82 +++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |  7 +++
 3 files changed, 57 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e637d32..d3b4657 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -284,6 +284,8 @@ struct drm_i915_error_state {
 	struct intel_display_error_state *display;
 };
 
+struct intel_crtc_config;
+
 struct drm_i915_display_funcs {
 	bool (*fbc_enabled)(struct drm_device *dev);
 	void (*enable_fbc)(struct drm_crtc *crtc, unsigned long interval);
@@ -297,8 +299,6 @@ struct drm_i915_display_funcs {
 				 struct drm_display_mode *mode);
 	void (*modeset_global_resources)(struct drm_device *dev);
 	int (*crtc_mode_set)(struct drm_crtc *crtc,
-			     struct drm_display_mode *mode,
-			     struct drm_display_mode *adjusted_mode,
 			     int x, int y,
 			     struct drm_framebuffer *old_fb);
 	void (*crtc_enable)(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0b17a19..34986fe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3954,15 +3954,16 @@ bool intel_connector_get_hw_state(struct intel_connector *connector)
 	return encoder->get_hw_state(encoder, &pipe);
 }
 
-static bool intel_crtc_mode_fixup(struct drm_crtc *crtc,
-				  const struct drm_display_mode *mode,
-				  struct drm_display_mode *adjusted_mode)
+static bool intel_crtc_compute_config(struct drm_crtc *crtc,
+				      struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 
 	if (HAS_PCH_SPLIT(dev)) {
 		/* FDI link clock is fixed at 2.7G */
-		if (mode->clock * 3 > IRONLAKE_FDI_FREQ * 4)
+		if (pipe_config->requested_mode.clock * 3
+		    > IRONLAKE_FDI_FREQ * 4)
 			return false;
 	}
 
@@ -4648,14 +4649,15 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc,
 }
 
 static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
-			      struct drm_display_mode *mode,
-			      struct drm_display_mode *adjusted_mode,
 			      int x, int y,
 			      struct drm_framebuffer *fb)
 {
 	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 drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 	int refclk, num_connectors = 0;
@@ -5620,14 +5622,15 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 }
 
 static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
-				  struct drm_display_mode *mode,
-				  struct drm_display_mode *adjusted_mode,
 				  int x, int y,
 				  struct drm_framebuffer *fb)
 {
 	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 drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 	int num_connectors = 0;
@@ -5786,14 +5789,15 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
 }
 
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
-				 struct drm_display_mode *mode,
-				 struct drm_display_mode *adjusted_mode,
 				 int x, int y,
 				 struct drm_framebuffer *fb)
 {
 	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 drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 	int num_connectors = 0;
@@ -5875,8 +5879,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 }
 
 static int intel_crtc_mode_set(struct drm_crtc *crtc,
-			       struct drm_display_mode *mode,
-			       struct drm_display_mode *adjusted_mode,
 			       int x, int y,
 			       struct drm_framebuffer *fb)
 {
@@ -5885,13 +5887,16 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 	struct drm_encoder_helper_funcs *encoder_funcs;
 	struct intel_encoder *encoder;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
 	int pipe = intel_crtc->pipe;
 	int ret;
 
 	drm_vblank_pre_modeset(dev, pipe);
 
-	ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode,
-					      x, y, fb);
+	ret = dev_priv->display.crtc_mode_set(crtc, x, y, fb);
+
 	drm_vblank_post_modeset(dev, pipe);
 
 	if (ret != 0)
@@ -7502,19 +7507,22 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
 	}
 }
 
-static struct drm_display_mode *
-intel_modeset_adjusted_mode(struct drm_crtc *crtc,
-			    struct drm_display_mode *mode)
+static struct intel_crtc_config *
+intel_modeset_pipe_config(struct drm_crtc *crtc,
+			  struct drm_display_mode *mode)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_display_mode *adjusted_mode;
 	struct drm_encoder_helper_funcs *encoder_funcs;
 	struct intel_encoder *encoder;
+	struct intel_crtc_config *pipe_config;
 
-	adjusted_mode = drm_mode_duplicate(dev, mode);
-	if (!adjusted_mode)
+	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
+	if (!pipe_config)
 		return ERR_PTR(-ENOMEM);
 
+	drm_mode_copy(&pipe_config->adjusted_mode, mode);
+	drm_mode_copy(&pipe_config->requested_mode, mode);
+
 	/* Pass our mode to the connectors and the CRTC to give them a chance to
 	 * adjust it according to limitations or connector properties, and also
 	 * a chance to reject the mode entirely.
@@ -7525,22 +7533,23 @@ intel_modeset_adjusted_mode(struct drm_crtc *crtc,
 		if (&encoder->new_crtc->base != crtc)
 			continue;
 		encoder_funcs = encoder->base.helper_private;
-		if (!(encoder_funcs->mode_fixup(&encoder->base, mode,
-						adjusted_mode))) {
+		if (!(encoder_funcs->mode_fixup(&encoder->base,
+						&pipe_config->requested_mode,
+						&pipe_config->adjusted_mode))) {
 			DRM_DEBUG_KMS("Encoder fixup failed\n");
 			goto fail;
 		}
 	}
 
-	if (!(intel_crtc_mode_fixup(crtc, mode, adjusted_mode))) {
+	if (!(intel_crtc_compute_config(crtc, pipe_config))) {
 		DRM_DEBUG_KMS("CRTC fixup failed\n");
 		goto fail;
 	}
 	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
 
-	return adjusted_mode;
+	return pipe_config;
 fail:
-	drm_mode_destroy(dev, adjusted_mode);
+	kfree(pipe_config);
 	return ERR_PTR(-EINVAL);
 }
 
@@ -7806,7 +7815,8 @@ int intel_set_mode(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct drm_display_mode *adjusted_mode, *saved_mode, *saved_hwmode;
+	struct drm_display_mode *saved_mode, *saved_hwmode;
+	struct intel_crtc_config *pipe_config = NULL;
 	struct intel_crtc *intel_crtc;
 	unsigned disable_pipes, prepare_pipes, modeset_pipes;
 	int ret = 0;
@@ -7833,11 +7843,12 @@ int intel_set_mode(struct drm_crtc *crtc,
 	 * Hence simply check whether any bit is set in modeset_pipes in all the
 	 * pieces of code that are not yet converted to deal with mutliple crtcs
 	 * changing their mode at the same time. */
-	adjusted_mode = NULL;
 	if (modeset_pipes) {
-		adjusted_mode = intel_modeset_adjusted_mode(crtc, mode);
-		if (IS_ERR(adjusted_mode)) {
-			ret = PTR_ERR(adjusted_mode);
+		pipe_config = intel_modeset_pipe_config(crtc, mode);
+		if (IS_ERR(pipe_config)) {
+			ret = PTR_ERR(pipe_config);
+			pipe_config = NULL;
+
 			goto out;
 		}
 	}
@@ -7850,8 +7861,12 @@ int intel_set_mode(struct drm_crtc *crtc,
 	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
 	 * to set it here already despite that we pass it down the callchain.
 	 */
-	if (modeset_pipes)
+	if (modeset_pipes) {
 		crtc->mode = *mode;
+		/* mode_set/enable/disable functions rely on a correct pipe
+		 * config. */
+		to_intel_crtc(crtc)->config = *pipe_config;
+	}
 
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
@@ -7865,7 +7880,6 @@ int intel_set_mode(struct drm_crtc *crtc,
 	 */
 	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
 		ret = intel_crtc_mode_set(&intel_crtc->base,
-					  mode, adjusted_mode,
 					  x, y, fb);
 		if (ret)
 			goto done;
@@ -7877,7 +7891,7 @@ int intel_set_mode(struct drm_crtc *crtc,
 
 	if (modeset_pipes) {
 		/* Store real post-adjustment hardware mode. */
-		crtc->hwmode = *adjusted_mode;
+		crtc->hwmode = pipe_config->adjusted_mode;
 
 		/* Calculate and store various constants which
 		 * are later needed by vblank and swap-completion
@@ -7888,7 +7902,6 @@ int intel_set_mode(struct drm_crtc *crtc,
 
 	/* FIXME: add subpixel order */
 done:
-	drm_mode_destroy(dev, adjusted_mode);
 	if (ret && crtc->enabled) {
 		crtc->hwmode = *saved_hwmode;
 		crtc->mode = *saved_mode;
@@ -7897,6 +7910,7 @@ done:
 	}
 
 out:
+	kfree(pipe_config);
 	kfree(saved_mode);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 54bc2ea..4cc6625 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -200,6 +200,11 @@ struct intel_connector {
 	struct edid *edid;
 };
 
+struct intel_crtc_config {
+	struct drm_display_mode requested_mode;
+	struct drm_display_mode adjusted_mode;
+};
+
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -233,6 +238,8 @@ struct intel_crtc {
 	bool cursor_visible;
 	unsigned int bpp;
 
+	struct intel_crtc_config config;
+
 	/* We can share PLLs across outputs if the timings match */
 	struct intel_pch_pll *pch_pll;
 	uint32_t ddi_pll_sel;
-- 
1.7.11.7

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

Re: [PATCH 01/13] drm/i915: introduce struct intel_crtc_config

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:50 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Currently only containing the requested and the adjusted mode. And
> only crtc callbacks are converted somewhat to it, encoders will be
> done on a as-needed basis (simply too much churn in one patch
> otherwise).
> 
> Future patches will add tons more useful stuff to this struct,
> starting with the very simple.
> 
> v2: Store the pipe_config in the intel_crtc, so that the ->mode-set,
> ->enable and also ->disable have easy access to it.
> 
> v3: Store the pipe config in the right crtc ...
> 
> v4: Rebased.
> 
> v5: Fixup an OOPS when trying to kfree an ERR_PTR.
> 
> v6: Used drm_moode_copy and some other small cleanups as suggested
> by Ville Syrjälä.
> 
> v7: drm_mode_copy preserves the mode id of the destination, so no need
> to clear it again (Ville).
> 
> v8: Break a long line spotted by Paulo.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---

Yeah looks like this could be applied immediately.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/13] drm/i915: compute pipe_config earlier

[Daniel Vetter]

To make decent modeset state checking possible (e.g. for the check
mode with atomic modesetting) we want to have the full pipe
configuration and state checks done before we touch the hw.

To ensure that all the little bits&pieces that are now moved to the
pipe_config handle this correctly, move its computation to the right
spot now, before we touch the hw in the disable_pipes step.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 34986fe..56ff8a5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7829,12 +7829,6 @@ int intel_set_mode(struct drm_crtc *crtc,
 	intel_modeset_affected_pipes(crtc, &modeset_pipes,
 				     &prepare_pipes, &disable_pipes);
 
-	DRM_DEBUG_KMS("set mode pipe masks: modeset: %x, prepare: %x, disable: %x\n",
-		      modeset_pipes, prepare_pipes, disable_pipes);
-
-	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
-		intel_crtc_disable(&intel_crtc->base);
-
 	*saved_hwmode = crtc->hwmode;
 	*saved_mode = crtc->mode;
 
@@ -7853,6 +7847,12 @@ int intel_set_mode(struct drm_crtc *crtc,
 		}
 	}
 
+	DRM_DEBUG_KMS("set mode pipe masks: modeset: %x, prepare: %x, disable: %x\n",
+		      modeset_pipes, prepare_pipes, disable_pipes);
+
+	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
+		intel_crtc_disable(&intel_crtc->base);
+
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
 		if (intel_crtc->base.enabled)
 			dev_priv->display.crtc_disable(&intel_crtc->base);
-- 
1.7.11.7

Re: [PATCH 02/13] drm/i915: compute pipe_config earlier

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:51 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> To make decent modeset state checking possible (e.g. for the check
> mode with atomic modesetting) we want to have the full pipe
> configuration and state checks done before we touch the hw.
> 
> To ensure that all the little bits&pieces that are now moved to the
> pipe_config handle this correctly, move its computation to the right
> spot now, before we touch the hw in the disable_pipes step.
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 34986fe..56ff8a5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7829,12 +7829,6 @@ int intel_set_mode(struct drm_crtc *crtc,
>  	intel_modeset_affected_pipes(crtc, &modeset_pipes,
>  				     &prepare_pipes, &disable_pipes);
>  
> -	DRM_DEBUG_KMS("set mode pipe masks: modeset: %x, prepare: %x, disable: %x\n",
> -		      modeset_pipes, prepare_pipes, disable_pipes);
> -
> -	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> -		intel_crtc_disable(&intel_crtc->base);
> -
>  	*saved_hwmode = crtc->hwmode;
>  	*saved_mode = crtc->mode;
>  
> @@ -7853,6 +7847,12 @@ int intel_set_mode(struct drm_crtc *crtc,
>  		}
>  	}
>  
> +	DRM_DEBUG_KMS("set mode pipe masks: modeset: %x, prepare: %x, disable: %x\n",
> +		      modeset_pipes, prepare_pipes, disable_pipes);
> +
> +	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> +		intel_crtc_disable(&intel_crtc->base);
> +
>  	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
>  		if (intel_crtc->base.enabled)
>  			dev_priv->display.crtc_disable(&intel_crtc->base);

Looks safe :)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 03/13] drm/i915: add pipe_config->timings_set

[Daniel Vetter]

Only used by the lvds encoder. Note that we shouldn't do the same
simple conversion with the FORCE_6BPC flag, since that's much better
handled by moving all the pipe_bpc computation around.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++----
 drivers/gpu/drm/i915/intel_lvds.c    | 19 +++++++++----------
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56ff8a5..3e22305 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3970,7 +3970,7 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
 	/* All interlaced capable intel hw wants timings in frames. Note though
 	 * that intel_lvds_mode_fixup does some funny tricks with the crtc
 	 * timings, so we need to be careful not to clobber these.*/
-	if (!(adjusted_mode->private_flags & INTEL_MODE_CRTC_TIMINGS_SET))
+	if (!pipe_config->timings_set)
 		drm_mode_set_crtcinfo(adjusted_mode, 0);
 
 	/* WaPruneModeWithIncorrectHsyncOffset: Cantiga+ cannot handle modes
@@ -7532,6 +7532,16 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 
 		if (&encoder->new_crtc->base != crtc)
 			continue;
+
+		if (encoder->compute_config) {
+			if (!(encoder->compute_config(encoder, pipe_config))) {
+				DRM_DEBUG_KMS("Encoder config failure\n");
+				goto fail;
+			}
+
+			continue;
+		}
+
 		encoder_funcs = encoder->base.helper_private;
 		if (!(encoder_funcs->mode_fixup(&encoder->base,
 						&pipe_config->requested_mode,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4cc6625..054032a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -105,10 +105,6 @@
 #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)
-/* This flag must be set by the encoder's mode_fixup if it changes the crtc
- * timings in the mode to prevent the crtc fixup from overwriting them.
- * Currently only lvds needs that. */
-#define INTEL_MODE_CRTC_TIMINGS_SET (0x20)
 /*
  * Set when limited 16-235 (as opposed to full 0-255) RGB color range is
  * to be used.
@@ -158,6 +154,8 @@ struct intel_encoder {
 	bool cloneable;
 	bool connectors_active;
 	void (*hot_plug)(struct intel_encoder *);
+	bool (*compute_config)(struct intel_encoder *,
+			       struct intel_crtc_config *);
 	void (*pre_pll_enable)(struct intel_encoder *);
 	void (*pre_enable)(struct intel_encoder *);
 	void (*enable)(struct intel_encoder *);
@@ -203,6 +201,10 @@ struct intel_connector {
 struct intel_crtc_config {
 	struct drm_display_mode requested_mode;
 	struct drm_display_mode adjusted_mode;
+	/* This flag must be set by the encoder's compute_config callback if it
+	 * changes the crtc timings in the mode to prevent the crtc fixup from
+	 * overwriting them.  Currently only lvds needs that. */
+	bool timings_set;
 };
 
 struct intel_crtc {
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 6ff145f..a2c516c 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -261,8 +261,6 @@ centre_horizontally(struct drm_display_mode *mode,
 
 	mode->crtc_hsync_start = mode->crtc_hblank_start + sync_pos;
 	mode->crtc_hsync_end = mode->crtc_hsync_start + sync_width;
-
-	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
 }
 
 static void
@@ -284,8 +282,6 @@ centre_vertically(struct drm_display_mode *mode,
 
 	mode->crtc_vsync_start = mode->crtc_vblank_start + sync_pos;
 	mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
-
-	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
 }
 
 static inline u32 panel_fitter_scaling(u32 source, u32 target)
@@ -301,15 +297,17 @@ static inline u32 panel_fitter_scaling(u32 source, u32 target)
 	return (FACTOR * ratio + FACTOR/2) / FACTOR;
 }
 
-static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
-				  const struct drm_display_mode *mode,
-				  struct drm_display_mode *adjusted_mode)
+static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
+				      struct intel_crtc_config *pipe_config)
 {
-	struct drm_device *dev = encoder->dev;
+	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder);
+	struct intel_lvds_encoder *lvds_encoder =
+		to_lvds_encoder(&intel_encoder->base);
 	struct intel_connector *intel_connector =
 		&lvds_encoder->attached_connector->base;
+	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
+	struct drm_display_mode *mode = &pipe_config->requested_mode;
 	struct intel_crtc *intel_crtc = lvds_encoder->base.new_crtc;
 	u32 pfit_control = 0, pfit_pgm_ratios = 0, border = 0;
 	int pipe;
@@ -359,6 +357,7 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 		I915_WRITE(BCLRPAT(pipe), 0);
 
 	drm_mode_set_crtcinfo(adjusted_mode, 0);
+	pipe_config->timings_set = true;
 
 	switch (intel_connector->panel.fitting_mode) {
 	case DRM_MODE_SCALE_CENTER:
@@ -661,7 +660,6 @@ static int intel_lvds_set_property(struct drm_connector *connector,
 }
 
 static const struct drm_encoder_helper_funcs intel_lvds_helper_funcs = {
-	.mode_fixup = intel_lvds_mode_fixup,
 	.mode_set = intel_lvds_mode_set,
 };
 
@@ -1105,6 +1103,7 @@ bool intel_lvds_init(struct drm_device *dev)
 	intel_encoder->enable = intel_enable_lvds;
 	intel_encoder->pre_enable = intel_pre_enable_lvds;
 	intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds;
+	intel_encoder->compute_config = intel_lvds_compute_config;
 	intel_encoder->disable = intel_disable_lvds;
 	intel_encoder->get_hw_state = intel_lvds_get_hw_state;
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
-- 
1.7.11.7

Re: [PATCH 03/13] drm/i915: add pipe_config->timings_set

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:52 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Only used by the lvds encoder. Note that we shouldn't do the same
> simple conversion with the FORCE_6BPC flag, since that's much better
> handled by moving all the pipe_bpc computation around.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++----
>  drivers/gpu/drm/i915/intel_lvds.c    | 19 +++++++++----------
>  3 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 56ff8a5..3e22305 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3970,7 +3970,7 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
>  	/* All interlaced capable intel hw wants timings in frames. Note though
>  	 * that intel_lvds_mode_fixup does some funny tricks with the crtc
>  	 * timings, so we need to be careful not to clobber these.*/
> -	if (!(adjusted_mode->private_flags & INTEL_MODE_CRTC_TIMINGS_SET))
> +	if (!pipe_config->timings_set)
>  		drm_mode_set_crtcinfo(adjusted_mode, 0);
>  
>  	/* WaPruneModeWithIncorrectHsyncOffset: Cantiga+ cannot handle modes
> @@ -7532,6 +7532,16 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  
>  		if (&encoder->new_crtc->base != crtc)
>  			continue;
> +
> +		if (encoder->compute_config) {
> +			if (!(encoder->compute_config(encoder, pipe_config))) {
> +				DRM_DEBUG_KMS("Encoder config failure\n");
> +				goto fail;
> +			}
> +
> +			continue;
> +		}
> +
>  		encoder_funcs = encoder->base.helper_private;
>  		if (!(encoder_funcs->mode_fixup(&encoder->base,
>  						&pipe_config->requested_mode,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4cc6625..054032a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -105,10 +105,6 @@
>  #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)
> -/* This flag must be set by the encoder's mode_fixup if it changes the crtc
> - * timings in the mode to prevent the crtc fixup from overwriting them.
> - * Currently only lvds needs that. */
> -#define INTEL_MODE_CRTC_TIMINGS_SET (0x20)
>  /*
>   * Set when limited 16-235 (as opposed to full 0-255) RGB color range is
>   * to be used.
> @@ -158,6 +154,8 @@ struct intel_encoder {
>  	bool cloneable;
>  	bool connectors_active;
>  	void (*hot_plug)(struct intel_encoder *);
> +	bool (*compute_config)(struct intel_encoder *,
> +			       struct intel_crtc_config *);
>  	void (*pre_pll_enable)(struct intel_encoder *);
>  	void (*pre_enable)(struct intel_encoder *);
>  	void (*enable)(struct intel_encoder *);
> @@ -203,6 +201,10 @@ struct intel_connector {
>  struct intel_crtc_config {
>  	struct drm_display_mode requested_mode;
>  	struct drm_display_mode adjusted_mode;
> +	/* This flag must be set by the encoder's compute_config callback if it
> +	 * changes the crtc timings in the mode to prevent the crtc fixup from
> +	 * overwriting them.  Currently only lvds needs that. */
> +	bool timings_set;
>  };
>  
>  struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 6ff145f..a2c516c 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -261,8 +261,6 @@ centre_horizontally(struct drm_display_mode *mode,
>  
>  	mode->crtc_hsync_start = mode->crtc_hblank_start + sync_pos;
>  	mode->crtc_hsync_end = mode->crtc_hsync_start + sync_width;
> -
> -	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
>  }
>  
>  static void
> @@ -284,8 +282,6 @@ centre_vertically(struct drm_display_mode *mode,
>  
>  	mode->crtc_vsync_start = mode->crtc_vblank_start + sync_pos;
>  	mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
> -
> -	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
>  }
>  
>  static inline u32 panel_fitter_scaling(u32 source, u32 target)
> @@ -301,15 +297,17 @@ static inline u32 panel_fitter_scaling(u32 source, u32 target)
>  	return (FACTOR * ratio + FACTOR/2) / FACTOR;
>  }
>  
> -static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
> -				  const struct drm_display_mode *mode,
> -				  struct drm_display_mode *adjusted_mode)
> +static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
> +				      struct intel_crtc_config *pipe_config)
>  {
> -	struct drm_device *dev = encoder->dev;
> +	struct drm_device *dev = intel_encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder);
> +	struct intel_lvds_encoder *lvds_encoder =
> +		to_lvds_encoder(&intel_encoder->base);
>  	struct intel_connector *intel_connector =
>  		&lvds_encoder->attached_connector->base;
> +	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> +	struct drm_display_mode *mode = &pipe_config->requested_mode;
>  	struct intel_crtc *intel_crtc = lvds_encoder->base.new_crtc;
>  	u32 pfit_control = 0, pfit_pgm_ratios = 0, border = 0;
>  	int pipe;
> @@ -359,6 +357,7 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
>  		I915_WRITE(BCLRPAT(pipe), 0);
>  
>  	drm_mode_set_crtcinfo(adjusted_mode, 0);
> +	pipe_config->timings_set = true;
>  
>  	switch (intel_connector->panel.fitting_mode) {
>  	case DRM_MODE_SCALE_CENTER:
> @@ -661,7 +660,6 @@ static int intel_lvds_set_property(struct drm_connector *connector,
>  }
>  
>  static const struct drm_encoder_helper_funcs intel_lvds_helper_funcs = {
> -	.mode_fixup = intel_lvds_mode_fixup,
>  	.mode_set = intel_lvds_mode_set,
>  };
>  
> @@ -1105,6 +1103,7 @@ bool intel_lvds_init(struct drm_device *dev)
>  	intel_encoder->enable = intel_enable_lvds;
>  	intel_encoder->pre_enable = intel_pre_enable_lvds;
>  	intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds;
> +	intel_encoder->compute_config = intel_lvds_compute_config;
>  	intel_encoder->disable = intel_disable_lvds;
>  	intel_encoder->get_hw_state = intel_lvds_get_hw_state;
>  	intel_connector->get_hw_state = intel_connector_get_hw_state;

Changelog and summary could be better and mention the new
compute_config function and how it replaces the mode_fixup one.

Also, TIMINGS_SET probably wasn't a very good name in the first place,
since it really deals with letter/pillar box configs.  But that's not
really a problem with your patch and could be changed in a follow-on.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 03/13] drm/i915: add pipe_config->timings_set

[Daniel Vetter]

On Wed, Mar 27, 2013 at 09:49:52AM -0700, Jesse Barnes wrote:
> Changelog and summary could be better and mention the new
> compute_config function and how it replaces the mode_fixup one.
> 
> Also, TIMINGS_SET probably wasn't a very good name in the first place,
> since it really deals with letter/pillar box configs.  But that's not
> really a problem with your patch and could be changed in a follow-on.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Well, I've missed to add a commit addition discussed with Ville:


> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 03/13] drm/i915: add pipe_config->timings_set

[Daniel Vetter]

On Wed, Mar 27, 2013 at 5:49 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Changelog and summary could be better and mention the new
> compute_config function and how it replaces the mode_fixup one.
>
> Also, TIMINGS_SET probably wasn't a very good name in the first place,
> since it really deals with letter/pillar box configs.  But that's not
> really a problem with your patch and could be changed in a follow-on.
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I've forgotten to add a commit addition discussed with Ville:

"Note that since the lvds code unconditionally sets the crtc timings, we
can also unconditionally set the respective flag and not just when we set
special timings like the old code did."

I'll smash another paragraph for you on top (which also address an
issue raised by Paulo):

"This requires that we pass the pipe config around to encoders, so
that they can set special attributes and set constraints. To do so
introduce a new ->compute_config encoder callback, which is called in
stead of the drm crtc helper's ->mode_fixup.

"To avoid massive churn all over the codebase we don't want to convert
all existing ->mode_fixup functions. Instead I've opted to convert
them on an as-needed basis (mostly to cut down on rebase conflicts and
to have more freedom to experiment around while developing the
patches)."

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 03/13] drm/i915: add pipe_config->timings_set

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:52 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> +	bool (*compute_config)(struct intel_encoder *,
> +			       struct intel_crtc_config *);
>  	void (*pre_pll_enable)(struct intel_encoder *);
>  	void (*pre_enable)(struct intel_encoder *);
>  	void (*enable)(struct intel_encoder *);
> @@ -203,6 +201,10 @@ struct intel_connector {
>  struct intel_crtc_config {
>  	struct drm_display_mode requested_mode;
>  	struct drm_display_mode adjusted_mode;
> +	/* This flag must be set by the encoder's compute_config callback if it
> +	 * changes the crtc timings in the mode to prevent the crtc fixup from
> +	 * overwriting them.  Currently only lvds needs that. */
> +	bool timings_set;

The compute_config function could actually use some kdoc instead of
putting it over the timings_set function.  It'll need to be expanded to
cover all the pipe_config bits eventually, what they mean and when they
should be set.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 03/13] drm/i915: add pipe_config->timings_set

[Daniel Vetter]

On Wed, Mar 27, 2013 at 5:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> @@ -203,6 +201,10 @@ struct intel_connector {
>>  struct intel_crtc_config {
>>       struct drm_display_mode requested_mode;
>>       struct drm_display_mode adjusted_mode;
>> +     /* This flag must be set by the encoder's compute_config callback if it
>> +      * changes the crtc timings in the mode to prevent the crtc fixup from
>> +      * overwriting them.  Currently only lvds needs that. */
>> +     bool timings_set;
>
> The compute_config function could actually use some kdoc instead of
> putting it over the timings_set function.  It'll need to be expanded to
> cover all the pipe_config bits eventually, what they mean and when they
> should be set.

Now I very much like to claim the opposite, but this isn't designed
but very much organically grown code. So imo documentation doesn't
make too much sense before things settle a bit more (the auto fdi link
dither at the end will introduce quite a bit of fun still ...).

I've promised though in my pipe_config intro a few weeks ago that I'll
create a nice blog post and doc patch once the basic stuff is settled.
I still intend to deliver on that. Is that good enough?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 03/13] drm/i915: add pipe_config->timings_set

[Jesse Barnes]

On Wed, 27 Mar 2013 18:06:44 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Wed, Mar 27, 2013 at 5:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> @@ -203,6 +201,10 @@ struct intel_connector {
> >>  struct intel_crtc_config {
> >>       struct drm_display_mode requested_mode;
> >>       struct drm_display_mode adjusted_mode;
> >> +     /* This flag must be set by the encoder's compute_config callback if it
> >> +      * changes the crtc timings in the mode to prevent the crtc fixup from
> >> +      * overwriting them.  Currently only lvds needs that. */
> >> +     bool timings_set;
> >
> > The compute_config function could actually use some kdoc instead of
> > putting it over the timings_set function.  It'll need to be expanded to
> > cover all the pipe_config bits eventually, what they mean and when they
> > should be set.
> 
> Now I very much like to claim the opposite, but this isn't designed
> but very much organically grown code. So imo documentation doesn't
> make too much sense before things settle a bit more (the auto fdi link
> dither at the end will introduce quite a bit of fun still ...).
> 
> I've promised though in my pipe_config intro a few weeks ago that I'll
> create a nice blog post and doc patch once the basic stuff is settled.
> I still intend to deliver on that. Is that good enough?

I guess so... incrementally adding to the compute_config kdoc with the
new pipe_config bits as added is too much rebase pain?

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 04/13] drm/i915: add pipe_config->pixel_multiplier

[Daniel Vetter]

Used by SDVO (and hopefully, eventually HDMI, if we ever get around
to fixing up the low dotclock CEA modes ...).

This required adding a new encoder->mode_set callback to be able to
pass around the intel_crtc_config.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 80 +++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h     | 19 ++-------
 drivers/gpu/drm/i915/intel_sdvo.c    | 39 +++++++++---------
 3 files changed, 66 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3e22305..3672b8d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4320,14 +4320,15 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
 }
 
 static void vlv_update_pll(struct drm_crtc *crtc,
-			   struct drm_display_mode *mode,
-			   struct drm_display_mode *adjusted_mode,
 			   intel_clock_t *clock, intel_clock_t *reduced_clock,
 			   int num_connectors)
 {
 	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 drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
 	int pipe = intel_crtc->pipe;
 	u32 dpll, mdiv, pdiv;
 	u32 bestn, bestm1, bestm2, bestp1, bestp2;
@@ -4394,11 +4395,11 @@ static void vlv_update_pll(struct drm_crtc *crtc,
 
 	temp = 0;
 	if (is_sdvo) {
-		temp = intel_mode_get_pixel_multiplier(adjusted_mode);
-		if (temp > 1)
-			temp = (temp - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
-		else
-			temp = 0;
+		temp = 0;
+		if (intel_crtc->config.pixel_multiplier > 1) {
+			temp = (intel_crtc->config.pixel_multiplier - 1)
+				<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
+		}
 	}
 	I915_WRITE(DPLL_MD(pipe), temp);
 	POSTING_READ(DPLL_MD(pipe));
@@ -4424,14 +4425,15 @@ static void vlv_update_pll(struct drm_crtc *crtc,
 }
 
 static void i9xx_update_pll(struct drm_crtc *crtc,
-			    struct drm_display_mode *mode,
-			    struct drm_display_mode *adjusted_mode,
 			    intel_clock_t *clock, intel_clock_t *reduced_clock,
 			    int num_connectors)
 {
 	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 drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 	u32 dpll;
@@ -4448,11 +4450,12 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
 		dpll |= DPLLB_MODE_LVDS;
 	else
 		dpll |= DPLLB_MODE_DAC_SERIAL;
+
 	if (is_sdvo) {
-		int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
-		if (pixel_multiplier > 1) {
-			if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))
-				dpll |= (pixel_multiplier - 1) << SDVO_MULTIPLIER_SHIFT_HIRES;
+		if ((intel_crtc->config.pixel_multiplier > 1) &&
+		    (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))) {
+			dpll |= (intel_crtc->config.pixel_multiplier - 1)
+				<< SDVO_MULTIPLIER_SHIFT_HIRES;
 		}
 		dpll |= DPLL_DVO_HIGH_SPEED;
 	}
@@ -4517,11 +4520,11 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
 	if (INTEL_INFO(dev)->gen >= 4) {
 		u32 temp = 0;
 		if (is_sdvo) {
-			temp = intel_mode_get_pixel_multiplier(adjusted_mode);
-			if (temp > 1)
-				temp = (temp - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
-			else
-				temp = 0;
+			temp = 0;
+			if (intel_crtc->config.pixel_multiplier > 1) {
+				temp = (intel_crtc->config.pixel_multiplier - 1)
+					<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
+			}
 		}
 		I915_WRITE(DPLL_MD(pipe), temp);
 	} else {
@@ -4731,11 +4734,11 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 				has_reduced_clock ? &reduced_clock : NULL,
 				num_connectors);
 	else if (IS_VALLEYVIEW(dev))
-		vlv_update_pll(crtc, mode, adjusted_mode, &clock,
+		vlv_update_pll(crtc, &clock,
 				has_reduced_clock ? &reduced_clock : NULL,
 				num_connectors);
 	else
-		i9xx_update_pll(crtc, mode, adjusted_mode, &clock,
+		i9xx_update_pll(crtc, &clock,
 				has_reduced_clock ? &reduced_clock : NULL,
 				num_connectors);
 
@@ -5449,17 +5452,18 @@ int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
 	return bps / (link_bw * 8) + 1;
 }
 
-static void ironlake_set_m_n(struct drm_crtc *crtc,
-			     struct drm_display_mode *mode,
-			     struct drm_display_mode *adjusted_mode)
+static void ironlake_set_m_n(struct drm_crtc *crtc)
 {
 	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 drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
 	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
 	struct intel_encoder *intel_encoder, *edp_encoder = NULL;
 	struct intel_link_m_n m_n = {0};
-	int target_clock, pixel_multiplier, lane, link_bw;
+	int target_clock, lane, link_bw;
 	bool is_dp = false, is_cpu_edp = false;
 
 	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
@@ -5477,7 +5481,6 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
 	}
 
 	/* FDI link */
-	pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
 	lane = 0;
 	/* CPU eDP doesn't require FDI link, so just set DP M/N
 	   according to current link config */
@@ -5508,8 +5511,8 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
 
 	intel_crtc->fdi_lanes = lane;
 
-	if (pixel_multiplier > 1)
-		link_bw *= pixel_multiplier;
+	if (intel_crtc->config.pixel_multiplier > 1)
+		link_bw *= intel_crtc->config.pixel_multiplier;
 	intel_link_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, &m_n);
 
 	I915_WRITE(PIPE_DATA_M1(cpu_transcoder), TU_SIZE(m_n.tu) | m_n.gmch_m);
@@ -5519,7 +5522,6 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
 }
 
 static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
-				      struct drm_display_mode *adjusted_mode,
 				      intel_clock_t *clock, u32 fp)
 {
 	struct drm_crtc *crtc = &intel_crtc->base;
@@ -5527,7 +5529,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
 	uint32_t dpll;
-	int factor, pixel_multiplier, num_connectors = 0;
+	int factor, num_connectors = 0;
 	bool is_lvds = false, is_sdvo = false, is_tv = false;
 	bool is_dp = false, is_cpu_edp = false;
 
@@ -5578,9 +5580,9 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	else
 		dpll |= DPLLB_MODE_DAC_SERIAL;
 	if (is_sdvo) {
-		pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
-		if (pixel_multiplier > 1) {
-			dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
+		if (intel_crtc->config.pixel_multiplier > 1) {
+			dpll |= (intel_crtc->config.pixel_multiplier - 1)
+				<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
 		}
 		dpll |= DPLL_DVO_HIGH_SPEED;
 	}
@@ -5684,7 +5686,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
 			reduced_clock.m2;
 
-	dpll = ironlake_compute_dpll(intel_crtc, adjusted_mode, &clock, fp);
+	dpll = ironlake_compute_dpll(intel_crtc, &clock, fp);
 
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
@@ -5738,7 +5740,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	/* Note, this also computes intel_crtc->fdi_lanes which is used below in
 	 * ironlake_check_fdi_lanes. */
-	ironlake_set_m_n(crtc, mode, adjusted_mode);
+	ironlake_set_m_n(crtc);
 
 	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
 
@@ -5859,7 +5861,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
 	if (!is_dp || is_cpu_edp)
-		ironlake_set_m_n(crtc, mode, adjusted_mode);
+		ironlake_set_m_n(crtc);
 
 	haswell_set_pipeconf(crtc, adjusted_mode, dither);
 
@@ -5907,8 +5909,12 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 			encoder->base.base.id,
 			drm_get_encoder_name(&encoder->base),
 			mode->base.id, mode->name);
-		encoder_funcs = encoder->base.helper_private;
-		encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
+		if (encoder->mode_set) {
+			encoder->mode_set(encoder);
+		} else {
+			encoder_funcs = encoder->base.helper_private;
+			encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
+		}
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 054032a..f0e5462 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -102,8 +102,6 @@
 #define INTEL_DVO_CHIP_TVOUT 4
 
 /* 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)
 /*
  * Set when limited 16-235 (as opposed to full 0-255) RGB color range is
@@ -111,20 +109,6 @@
  */
 #define INTEL_MODE_LIMITED_COLOR_RANGE (0x40)
 
-static inline void
-intel_mode_set_pixel_multiplier(struct drm_display_mode *mode,
-				int multiplier)
-{
-	mode->clock *= multiplier;
-	mode->private_flags |= multiplier;
-}
-
-static inline int
-intel_mode_get_pixel_multiplier(const struct drm_display_mode *mode)
-{
-	return (mode->private_flags & INTEL_MODE_PIXEL_MULTIPLIER_MASK) >> INTEL_MODE_PIXEL_MULTIPLIER_SHIFT;
-}
-
 struct intel_framebuffer {
 	struct drm_framebuffer base;
 	struct drm_i915_gem_object *obj;
@@ -159,6 +143,7 @@ struct intel_encoder {
 	void (*pre_pll_enable)(struct intel_encoder *);
 	void (*pre_enable)(struct intel_encoder *);
 	void (*enable)(struct intel_encoder *);
+	void (*mode_set)(struct intel_encoder *intel_encoder);
 	void (*disable)(struct intel_encoder *);
 	void (*post_disable)(struct intel_encoder *);
 	/* Read out the current hw state of this connector, returning true if
@@ -205,6 +190,8 @@ struct intel_crtc_config {
 	 * changes the crtc timings in the mode to prevent the crtc fixup from
 	 * overwriting them.  Currently only lvds needs that. */
 	bool timings_set;
+	/* Used by SDVO (and if we ever fix it, HDMI). */
+	unsigned pixel_multiplier;
 };
 
 struct intel_crtc {
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 8fdd8f8..4d9fede 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -788,7 +788,6 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	v_sync_offset = mode->vsync_start - mode->vdisplay;
 
 	mode_clock = mode->clock;
-	mode_clock /= intel_mode_get_pixel_multiplier(mode) ?: 1;
 	mode_clock /= 10;
 	dtd->part1.clock = mode_clock;
 
@@ -1041,12 +1040,12 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
 	return true;
 }
 
-static bool intel_sdvo_mode_fixup(struct drm_encoder *encoder,
-				  const struct drm_display_mode *mode,
-				  struct drm_display_mode *adjusted_mode)
+static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
+				      struct intel_crtc_config *pipe_config)
 {
-	struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
-	int multiplier;
+	struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base);
+	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
+	struct drm_display_mode *mode = &pipe_config->requested_mode;
 
 	/* We need to construct preferred input timings based on our
 	 * output timings.  To do that, we have to set the output
@@ -1073,8 +1072,9 @@ static bool intel_sdvo_mode_fixup(struct drm_encoder *encoder,
 	/* Make the CRTC code factor in the SDVO pixel multiplier.  The
 	 * SDVO device will factor out the multiplier during mode_set.
 	 */
-	multiplier = intel_sdvo_get_pixel_multiplier(adjusted_mode);
-	intel_mode_set_pixel_multiplier(adjusted_mode, multiplier);
+	pipe_config->pixel_multiplier =
+		intel_sdvo_get_pixel_multiplier(adjusted_mode);
+	adjusted_mode->clock *= pipe_config->pixel_multiplier;
 
 	if (intel_sdvo->color_range_auto) {
 		/* See CEA-861-E - 5.1 Default Encoding Parameters */
@@ -1093,19 +1093,19 @@ static bool intel_sdvo_mode_fixup(struct drm_encoder *encoder,
 	return true;
 }
 
-static void intel_sdvo_mode_set(struct drm_encoder *encoder,
-				struct drm_display_mode *mode,
-				struct drm_display_mode *adjusted_mode)
+static void intel_sdvo_mode_set(struct intel_encoder *intel_encoder)
 {
-	struct drm_device *dev = encoder->dev;
+	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_crtc *crtc = intel_encoder->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
+	struct intel_sdvo *intel_sdvo = to_intel_sdvo(&intel_encoder->base);
 	u32 sdvox;
 	struct intel_sdvo_in_out_map in_out;
 	struct intel_sdvo_dtd input_dtd, output_dtd;
-	int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
 	int rate;
 
 	if (!mode)
@@ -1165,7 +1165,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 		DRM_INFO("Setting input timings on %s failed\n",
 			 SDVO_NAME(intel_sdvo));
 
-	switch (pixel_multiplier) {
+	switch (intel_crtc->config.pixel_multiplier) {
 	default:
 	case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break;
 	case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break;
@@ -1209,7 +1209,8 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 	} else if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) {
 		/* done in crtc_mode_set as it lives inside the dpll register */
 	} else {
-		sdvox |= (pixel_multiplier - 1) << SDVO_PORT_MULTIPLY_SHIFT;
+		sdvox |= (intel_crtc->config.pixel_multiplier - 1)
+			<< SDVO_PORT_MULTIPLY_SHIFT;
 	}
 
 	if (input_dtd.part2.sdvo_flags & SDVO_NEED_TO_STALL &&
@@ -2041,8 +2042,6 @@ done:
 }
 
 static const struct drm_encoder_helper_funcs intel_sdvo_helper_funcs = {
-	.mode_fixup = intel_sdvo_mode_fixup,
-	.mode_set = intel_sdvo_mode_set,
 };
 
 static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
@@ -2787,7 +2786,9 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 
 	drm_encoder_helper_add(&intel_encoder->base, &intel_sdvo_helper_funcs);
 
+	intel_encoder->compute_config = intel_sdvo_compute_config;
 	intel_encoder->disable = intel_disable_sdvo;
+	intel_encoder->mode_set = intel_sdvo_mode_set;
 	intel_encoder->enable = intel_enable_sdvo;
 	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
 
-- 
1.7.11.7

Re: [PATCH 04/13] drm/i915: add pipe_config->pixel_multiplier

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:53 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Used by SDVO (and hopefully, eventually HDMI, if we ever get around
> to fixing up the low dotclock CEA modes ...).
> 
> This required adding a new encoder->mode_set callback to be able to
> pass around the intel_crtc_config.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 80 +++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_drv.h     | 19 ++-------
>  drivers/gpu/drm/i915/intel_sdvo.c    | 39 +++++++++---------
>  3 files changed, 66 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3e22305..3672b8d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4320,14 +4320,15 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
>  }
>  
>  static void vlv_update_pll(struct drm_crtc *crtc,
> -			   struct drm_display_mode *mode,
> -			   struct drm_display_mode *adjusted_mode,
>  			   intel_clock_t *clock, intel_clock_t *reduced_clock,
>  			   int num_connectors)
>  {
>  	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 drm_display_mode *adjusted_mode =
> +		&intel_crtc->config.adjusted_mode;
> +	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;

These arg compaction changes could probably be squashed into the
initial crtc_config patch to make this one smaller.

> @@ -5907,8 +5909,12 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
>  			encoder->base.base.id,
>  			drm_get_encoder_name(&encoder->base),
>  			mode->base.id, mode->name);
> -		encoder_funcs = encoder->base.helper_private;
> -		encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
> +		if (encoder->mode_set) {
> +			encoder->mode_set(encoder);
> +		} else {
> +			encoder_funcs = encoder->base.helper_private;
> +			encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
> +		}
>  	}

This made me do a double take; maybe it's time to
s/encoder/intel_encoder in this function...

Looks good otherwise.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 04/13] drm/i915: add pipe_config->pixel_multiplier

[Daniel Vetter]

On Wed, Mar 27, 2013 at 5:54 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3e22305..3672b8d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4320,14 +4320,15 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
>>  }
>>
>>  static void vlv_update_pll(struct drm_crtc *crtc,
>> -                        struct drm_display_mode *mode,
>> -                        struct drm_display_mode *adjusted_mode,
>>                          intel_clock_t *clock, intel_clock_t *reduced_clock,
>>                          int num_connectors)
>>  {
>>       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 drm_display_mode *adjusted_mode =
>> +             &intel_crtc->config.adjusted_mode;
>> +     struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
>
> These arg compaction changes could probably be squashed into the
> initial crtc_config patch to make this one smaller.

See my little commit message update, but I've done things in such an
incremental way to avoid rebase hell and allow me to move things
around easier. What you see here is by far not my very first approach
;-)

To make everyone's OCD happy we can do a cleanup pass at the end, but
atm I'd like to avoid massive sed patches - too high churn.

>> @@ -5907,8 +5909,12 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
>>                       encoder->base.base.id,
>>                       drm_get_encoder_name(&encoder->base),
>>                       mode->base.id, mode->name);
>> -             encoder_funcs = encoder->base.helper_private;
>> -             encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
>> +             if (encoder->mode_set) {
>> +                     encoder->mode_set(encoder);
>> +             } else {
>> +                     encoder_funcs = encoder->base.helper_private;
>> +                     encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
>> +             }
>>       }
>
> This made me do a double take; maybe it's time to
> s/encoder/intel_encoder in this function...

Yeah, we're halfway between mostly using intel_encoder and not so much
drm_encoder. Again, I think we can do a sed jobs once things settle,
meanwhile it's gonna be a bit ugly. Personally I don't care ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 05/13] drm/i915: drop helper vtable for sdvo encoder

[Daniel Vetter]

Completely unused by now. Separate patch in case I've missed a
place somewhere which dereferences the helper vtable but actually
shouldn't do so.

v2: Resolve rebase conflict with Egbert Eich's hpd infrastructure
rework.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 4d9fede..6912742 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2041,9 +2041,6 @@ done:
 #undef CHECK_PROPERTY
 }
 
-static const struct drm_encoder_helper_funcs intel_sdvo_helper_funcs = {
-};
-
 static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
 	.dpms = intel_sdvo_dpms,
 	.detect = intel_sdvo_detect,
@@ -2784,8 +2781,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	if (intel_sdvo->hotplug_active)
 		intel_encoder->hpd_pin = HPD_SDVO_B ? HPD_SDVO_B : HPD_SDVO_C;
 
-	drm_encoder_helper_add(&intel_encoder->base, &intel_sdvo_helper_funcs);
-
 	intel_encoder->compute_config = intel_sdvo_compute_config;
 	intel_encoder->disable = intel_disable_sdvo;
 	intel_encoder->mode_set = intel_sdvo_mode_set;
-- 
1.7.11.7

Re: [PATCH 05/13] drm/i915: drop helper vtable for sdvo encoder

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:54 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Completely unused by now. Separate patch in case I've missed a
> place somewhere which dereferences the helper vtable but actually
> shouldn't do so.
> 
> v2: Resolve rebase conflict with Egbert Eich's hpd infrastructure
> rework.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 4d9fede..6912742 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2041,9 +2041,6 @@ done:
>  #undef CHECK_PROPERTY
>  }
>  
> -static const struct drm_encoder_helper_funcs intel_sdvo_helper_funcs = {
> -};
> -
>  static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
>  	.dpms = intel_sdvo_dpms,
>  	.detect = intel_sdvo_detect,
> @@ -2784,8 +2781,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  	if (intel_sdvo->hotplug_active)
>  		intel_encoder->hpd_pin = HPD_SDVO_B ? HPD_SDVO_B : HPD_SDVO_C;
>  
> -	drm_encoder_helper_add(&intel_encoder->base, &intel_sdvo_helper_funcs);
> -
>  	intel_encoder->compute_config = intel_sdvo_compute_config;
>  	intel_encoder->disable = intel_disable_sdvo;
>  	intel_encoder->mode_set = intel_sdvo_mode_set;

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 06/13] drm/i915: add pipe_config->has_pch_encoder

[Daniel Vetter]

This is used way too often in the enable/disable paths. And will
be even more useful in the future.

Note that correct semantics of this change highly depend upon
correct updating of intel_crtc->config: Like with all other
modeset state, we need to call ->disable with the old config,
but ->mode_set and ->enable with the new config.

v2: Do not yet use the flag in the ->disable callbacks - atm we don't
yet have support for the information stored in the pipe_config in the
hw state readout code, so this will be wrong at boot-up/resume.

v3: Rebased on top of the hdmi/dp ddi encoder merging.

v4: Fixup stupid rebase error which lead to a NULL vfunc deref.

v5: On haswell the VGA port is on the PCH!

v6: s/IS_HASWELL/HAS_DDI/, spotted by Paulo Zanoni. Also add a missing
parameter name in a function declaration.

v7: Don't forget to git add ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_crt.c     | 12 +++++++----
 drivers/gpu/drm/i915/intel_ddi.c     | 16 +++++++--------
 drivers/gpu/drm/i915/intel_display.c | 40 ++++++++----------------------------
 drivers/gpu/drm/i915/intel_dp.c      | 16 +++++++++------
 drivers/gpu/drm/i915/intel_drv.h     | 13 ++++++------
 drivers/gpu/drm/i915/intel_hdmi.c    | 14 ++++++++-----
 drivers/gpu/drm/i915/intel_lvds.c    |  2 ++
 drivers/gpu/drm/i915/intel_sdvo.c    |  3 +++
 8 files changed, 54 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 0e9e9e5..1d8d63a 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -199,10 +199,14 @@ static int intel_crt_mode_valid(struct drm_connector *connector,
 	return MODE_OK;
 }
 
-static bool intel_crt_mode_fixup(struct drm_encoder *encoder,
-				 const struct drm_display_mode *mode,
-				 struct drm_display_mode *adjusted_mode)
+static bool intel_crt_compute_config(struct intel_encoder *encoder,
+				     struct intel_crtc_config *pipe_config)
 {
+	struct drm_device *dev = encoder->base.dev;
+
+	if (HAS_PCH_SPLIT(dev))
+		pipe_config->has_pch_encoder = true;
+
 	return true;
 }
 
@@ -676,7 +680,6 @@ static void intel_crt_reset(struct drm_connector *connector)
  */
 
 static const struct drm_encoder_helper_funcs crt_encoder_funcs = {
-	.mode_fixup = intel_crt_mode_fixup,
 	.mode_set = intel_crt_mode_set,
 };
 
@@ -768,6 +771,7 @@ void intel_crt_init(struct drm_device *dev)
 	else
 		crt->adpa_reg = ADPA;
 
+	crt->base.compute_config = intel_crt_compute_config;
 	crt->base.disable = intel_disable_crt;
 	crt->base.enable = intel_enable_crt;
 	if (I915_HAS_HOTPLUG(dev))
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 258e38e..baeb470 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1476,19 +1476,17 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
 	intel_dp_encoder_destroy(encoder);
 }
 
-static bool intel_ddi_mode_fixup(struct drm_encoder *encoder,
-				 const struct drm_display_mode *mode,
-				 struct drm_display_mode *adjusted_mode)
+static bool intel_ddi_compute_config(struct intel_encoder *encoder,
+				     struct intel_crtc_config *pipe_config)
 {
-	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
-	int type = intel_encoder->type;
+	int type = encoder->type;
 
-	WARN(type == INTEL_OUTPUT_UNKNOWN, "mode_fixup() on unknown output!\n");
+	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
 
 	if (type == INTEL_OUTPUT_HDMI)
-		return intel_hdmi_mode_fixup(encoder, mode, adjusted_mode);
+		return intel_hdmi_compute_config(encoder, pipe_config);
 	else
-		return intel_dp_mode_fixup(encoder, mode, adjusted_mode);
+		return intel_dp_compute_config(encoder, pipe_config);
 }
 
 static const struct drm_encoder_funcs intel_ddi_funcs = {
@@ -1496,7 +1494,6 @@ static const struct drm_encoder_funcs intel_ddi_funcs = {
 };
 
 static const struct drm_encoder_helper_funcs intel_ddi_helper_funcs = {
-	.mode_fixup = intel_ddi_mode_fixup,
 	.mode_set = intel_ddi_mode_set,
 };
 
@@ -1536,6 +1533,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 			 DRM_MODE_ENCODER_TMDS);
 	drm_encoder_helper_add(encoder, &intel_ddi_helper_funcs);
 
+	intel_encoder->compute_config = intel_ddi_compute_config;
 	intel_encoder->enable = intel_enable_ddi;
 	intel_encoder->pre_enable = intel_ddi_pre_enable;
 	intel_encoder->disable = intel_disable_ddi;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3672b8d..fda0754 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2960,27 +2960,6 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-static bool ironlake_crtc_driving_pch(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct intel_encoder *intel_encoder;
-
-	/*
-	 * If there's a non-PCH eDP on this crtc, it must be DP_A, and that
-	 * must be driven by its own crtc; no sharing is possible.
-	 */
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
-		switch (intel_encoder->type) {
-		case INTEL_OUTPUT_EDP:
-			if (!intel_encoder_is_pch_edp(&intel_encoder->base))
-				return false;
-			continue;
-		}
-	}
-
-	return true;
-}
-
 static bool haswell_crtc_driving_pch(struct drm_crtc *crtc)
 {
 	return intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG);
@@ -3321,7 +3300,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 	u32 temp;
-	bool is_pch_port;
 
 	WARN_ON(!crtc->enabled);
 
@@ -3337,9 +3315,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 			I915_WRITE(PCH_LVDS, temp | LVDS_PORT_EN);
 	}
 
-	is_pch_port = ironlake_crtc_driving_pch(crtc);
 
-	if (is_pch_port) {
+	if (intel_crtc->config.has_pch_encoder) {
 		/* Note: FDI PLL enabling _must_ be done before we enable the
 		 * cpu pipes, hence this is separate from all the other fdi/pch
 		 * enabling. */
@@ -3376,10 +3353,11 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	 */
 	intel_crtc_load_lut(crtc);
 
-	intel_enable_pipe(dev_priv, pipe, is_pch_port);
+	intel_enable_pipe(dev_priv, pipe,
+			  intel_crtc->config.has_pch_encoder);
 	intel_enable_plane(dev_priv, plane, pipe);
 
-	if (is_pch_port)
+	if (intel_crtc->config.has_pch_encoder)
 		ironlake_pch_enable(crtc);
 
 	mutex_lock(&dev->struct_mutex);
@@ -3413,7 +3391,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
-	bool is_pch_port;
 
 	WARN_ON(!crtc->enabled);
 
@@ -3423,9 +3400,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc->active = true;
 	intel_update_watermarks(dev);
 
-	is_pch_port = haswell_crtc_driving_pch(crtc);
-
-	if (is_pch_port)
+	if (intel_crtc->config.has_pch_encoder)
 		dev_priv->display.fdi_link_train(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -3456,10 +3431,11 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_ddi_set_pipe_settings(crtc);
 	intel_ddi_enable_transcoder_func(crtc);
 
-	intel_enable_pipe(dev_priv, pipe, is_pch_port);
+	intel_enable_pipe(dev_priv, pipe,
+			  intel_crtc->config.has_pch_encoder);
 	intel_enable_plane(dev_priv, plane, pipe);
 
-	if (is_pch_port)
+	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
 
 	mutex_lock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 88d4eec..bc73e5e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -689,12 +689,13 @@ intel_dp_i2c_init(struct intel_dp *intel_dp,
 }
 
 bool
-intel_dp_mode_fixup(struct drm_encoder *encoder,
-		    const struct drm_display_mode *mode,
-		    struct drm_display_mode *adjusted_mode)
+intel_dp_compute_config(struct intel_encoder *encoder,
+			struct intel_crtc_config *pipe_config)
 {
-	struct drm_device *dev = encoder->dev;
-	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
+	struct drm_display_mode *mode = &pipe_config->requested_mode;
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	int lane_count, clock;
 	int max_lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
@@ -702,6 +703,9 @@ intel_dp_mode_fixup(struct drm_encoder *encoder,
 	int bpp, mode_rate;
 	static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
 
+	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && !is_cpu_edp(intel_dp))
+		pipe_config->has_pch_encoder = true;
+
 	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
 		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
 				       adjusted_mode);
@@ -2540,7 +2544,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 }
 
 static const struct drm_encoder_helper_funcs intel_dp_helper_funcs = {
-	.mode_fixup = intel_dp_mode_fixup,
 	.mode_set = intel_dp_mode_set,
 };
 
@@ -2960,6 +2963,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 			 DRM_MODE_ENCODER_TMDS);
 	drm_encoder_helper_add(&intel_encoder->base, &intel_dp_helper_funcs);
 
+	intel_encoder->compute_config = intel_dp_compute_config;
 	intel_encoder->enable = intel_enable_dp;
 	intel_encoder->pre_enable = intel_pre_enable_dp;
 	intel_encoder->disable = intel_disable_dp;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f0e5462..8de1855 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -190,6 +190,9 @@ struct intel_crtc_config {
 	 * changes the crtc timings in the mode to prevent the crtc fixup from
 	 * overwriting them.  Currently only lvds needs that. */
 	bool timings_set;
+	/* Whether to set up the PCH/FDI. Note that we never allow sharing
+	 * between pch encoders and cpu encoders. */
+	bool has_pch_encoder;
 	/* Used by SDVO (and if we ever fix it, HDMI). */
 	unsigned pixel_multiplier;
 };
@@ -449,9 +452,8 @@ extern void intel_hdmi_init(struct drm_device *dev,
 extern void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 				      struct intel_connector *intel_connector);
 extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
-extern bool intel_hdmi_mode_fixup(struct drm_encoder *encoder,
-				  const struct drm_display_mode *mode,
-				  struct drm_display_mode *adjusted_mode);
+extern bool intel_hdmi_compute_config(struct intel_encoder *encoder,
+				      struct intel_crtc_config *pipe_config);
 extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if);
 extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg,
 			    bool is_sdvob);
@@ -475,9 +477,8 @@ extern void intel_dp_complete_link_train(struct intel_dp *intel_dp);
 extern void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 extern void intel_dp_encoder_destroy(struct drm_encoder *encoder);
 extern void intel_dp_check_link_status(struct intel_dp *intel_dp);
-extern bool intel_dp_mode_fixup(struct drm_encoder *encoder,
-				const struct drm_display_mode *mode,
-				struct drm_display_mode *adjusted_mode);
+extern bool intel_dp_compute_config(struct intel_encoder *encoder,
+				    struct intel_crtc_config *pipe_config);
 extern bool intel_dpd_is_edp(struct drm_device *dev);
 extern void ironlake_edp_backlight_on(struct intel_dp *intel_dp);
 extern void ironlake_edp_backlight_off(struct intel_dp *intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index b9a83d7..b588e6c 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -768,11 +768,12 @@ static int intel_hdmi_mode_valid(struct drm_connector *connector,
 	return MODE_OK;
 }
 
-bool intel_hdmi_mode_fixup(struct drm_encoder *encoder,
-			   const struct drm_display_mode *mode,
-			   struct drm_display_mode *adjusted_mode)
+bool intel_hdmi_compute_config(struct intel_encoder *encoder,
+			       struct intel_crtc_config *pipe_config)
 {
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 
 	if (intel_hdmi->color_range_auto) {
 		/* See CEA-861-E - 5.1 Default Encoding Parameters */
@@ -786,6 +787,9 @@ bool intel_hdmi_mode_fixup(struct drm_encoder *encoder,
 	if (intel_hdmi->color_range)
 		adjusted_mode->private_flags |= INTEL_MODE_LIMITED_COLOR_RANGE;
 
+	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev))
+		pipe_config->has_pch_encoder = true;
+
 	return true;
 }
 
@@ -937,7 +941,6 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_encoder_helper_funcs intel_hdmi_helper_funcs = {
-	.mode_fixup = intel_hdmi_mode_fixup,
 	.mode_set = intel_hdmi_mode_set,
 };
 
@@ -1066,6 +1069,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 			 DRM_MODE_ENCODER_TMDS);
 	drm_encoder_helper_add(&intel_encoder->base, &intel_hdmi_helper_funcs);
 
+	intel_encoder->compute_config = intel_hdmi_compute_config;
 	intel_encoder->enable = intel_enable_hdmi;
 	intel_encoder->disable = intel_disable_hdmi;
 	intel_encoder->get_hw_state = intel_hdmi_get_hw_state;
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index a2c516c..9d6ed91 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -331,6 +331,8 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 			       adjusted_mode);
 
 	if (HAS_PCH_SPLIT(dev)) {
+		pipe_config->has_pch_encoder = true;
+
 		intel_pch_panel_fitting(dev,
 					intel_connector->panel.fitting_mode,
 					mode, adjusted_mode);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 6912742..5f3f9e9 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1047,6 +1047,9 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
 	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 	struct drm_display_mode *mode = &pipe_config->requested_mode;
 
+	if (HAS_PCH_SPLIT(encoder->base.dev))
+		pipe_config->has_pch_encoder = true;
+
 	/* We need to construct preferred input timings based on our
 	 * output timings.  To do that, we have to set the output
 	 * timings, even though this isn't really the right place in
-- 
1.7.11.7

Re: [PATCH 06/13] drm/i915: add pipe_config->has_pch_encoder

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:55 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> This is used way too often in the enable/disable paths. And will
> be even more useful in the future.
> 
> Note that correct semantics of this change highly depend upon
> correct updating of intel_crtc->config: Like with all other
> modeset state, we need to call ->disable with the old config,
> but ->mode_set and ->enable with the new config.
> 
> v2: Do not yet use the flag in the ->disable callbacks - atm we don't
> yet have support for the information stored in the pipe_config in the
> hw state readout code, so this will be wrong at boot-up/resume.
> 
> v3: Rebased on top of the hdmi/dp ddi encoder merging.
> 
> v4: Fixup stupid rebase error which lead to a NULL vfunc deref.
> 
> v5: On haswell the VGA port is on the PCH!
> 
> v6: s/IS_HASWELL/HAS_DDI/, spotted by Paulo Zanoni. Also add a missing
> parameter name in a function declaration.
> 
> v7: Don't forget to git add ...

Looks like you got all the outputs covered.  But we always seem to get
this bit wrong, so we'll need to test on all the configs we know about
at least...

+	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && !is_cpu_edp(intel_dp))
+		pipe_config->has_pch_encoder = true;
+

This could just do
 if (intel_dp->is_pch_edp)
	pipe_config->has_pch_encoder = true;
right?  Since we cover the other cases in dp_init_connector?

But either way:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 06/13] drm/i915: add pipe_config->has_pch_encoder

[Daniel Vetter]

On Wed, Mar 27, 2013 at 6:06 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> +       if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && !is_cpu_edp(intel_dp))
> +               pipe_config->has_pch_encoder = true;
> +
>
> This could just do
>  if (intel_dp->is_pch_edp)
>         pipe_config->has_pch_encoder = true;
> right?  Since we cover the other cases in dp_init_connector?

That would give you two wrong case currently:
- hsw port D eDP would be marked as pch port
- any pch non-eDP DP ports would not be marked as pch ports

The ugly thing with this patch here is that this property is actually
fixed to the encoder, but I dynamically compute it in compute_config.
We have a few other such cases (e.g. the cpu transcoder for edp on
hsw). But I've figured there's no point in adding something clever,
which then updates the pipe_config according to connected encoders
with data structures ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 07/13] drm/i915: add pipe_config->limited_color_range

[Daniel Vetter]

Now that we have a useful struct for this, let's use it. Some neat
pointer-chasing required, but it's all there already.

v2: Rebased on top of the added Haswell limited color range support.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
 drivers/gpu/drm/i915/intel_dp.c      |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     | 12 +++++++-----
 drivers/gpu/drm/i915/intel_hdmi.c    |  5 +++--
 drivers/gpu/drm/i915/intel_sdvo.c    |  5 +++--
 5 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fda0754..bfed546 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5173,7 +5173,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
 	else
 		val |= PIPECONF_PROGRESSIVE;
 
-	if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
+	if (intel_crtc->config.limited_color_range)
 		val |= PIPECONF_COLOR_RANGE_SELECT;
 	else
 		val &= ~PIPECONF_COLOR_RANGE_SELECT;
@@ -5189,8 +5189,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
  * is supported, but eventually this should handle various
  * RGB<->YCbCr scenarios as well.
  */
-static void intel_set_pipe_csc(struct drm_crtc *crtc,
-			       const struct drm_display_mode *adjusted_mode)
+static void intel_set_pipe_csc(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5205,7 +5204,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc,
 	 * consideration.
 	 */
 
-	if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
+	if (intel_crtc->config.limited_color_range)
 		coeff = ((235 - 16) * (1 << 12) / 255) & 0xff8; /* 0.xxx... */
 
 	/*
@@ -5229,7 +5228,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc,
 	if (INTEL_INFO(dev)->gen > 6) {
 		uint16_t postoff = 0;
 
-		if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
+		if (intel_crtc->config.limited_color_range)
 			postoff = (16 * (1 << 13) / 255) & 0x1fff;
 
 		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
@@ -5240,7 +5239,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc,
 	} else {
 		uint32_t mode = CSC_MODE_YUV_TO_RGB;
 
-		if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
+		if (intel_crtc->config.limited_color_range)
 			mode |= CSC_BLACK_SCREEN_OFFSET;
 
 		I915_WRITE(PIPE_CSC_MODE(pipe), mode);
@@ -5841,7 +5840,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 
 	haswell_set_pipeconf(crtc, adjusted_mode, dither);
 
-	intel_set_pipe_csc(crtc, adjusted_mode);
+	intel_set_pipe_csc(crtc);
 
 	/* Set up the display plane register */
 	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE | DISPPLANE_PIPE_CSC_ENABLE);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bc73e5e..d7c1403 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -739,7 +739,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	}
 
 	if (intel_dp->color_range)
-		adjusted_mode->private_flags |= INTEL_MODE_LIMITED_COLOR_RANGE;
+		pipe_config->limited_color_range = true;
 
 	mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8de1855..63160c6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -103,11 +103,6 @@
 
 /* drm_display_mode->private_flags */
 #define INTEL_MODE_DP_FORCE_6BPC (0x10)
-/*
- * Set when limited 16-235 (as opposed to full 0-255) RGB color range is
- * to be used.
- */
-#define INTEL_MODE_LIMITED_COLOR_RANGE (0x40)
 
 struct intel_framebuffer {
 	struct drm_framebuffer base;
@@ -193,6 +188,13 @@ struct intel_crtc_config {
 	/* Whether to set up the PCH/FDI. Note that we never allow sharing
 	 * between pch encoders and cpu encoders. */
 	bool has_pch_encoder;
+
+	/*
+	 * Use reduced/limited/broadcast rbg range, compressing from the full
+	 * range fed into the crtcs.
+	 */
+	bool limited_color_range;
+
 	/* Used by SDVO (and if we ever fix it, HDMI). */
 	unsigned pixel_multiplier;
 };
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index b588e6c..5508687 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -333,6 +333,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 					 struct drm_display_mode *adjusted_mode)
 {
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 	struct dip_infoframe avi_if = {
 		.type = DIP_TYPE_AVI,
 		.ver = DIP_VERSION_AVI,
@@ -343,7 +344,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 		avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
 
 	if (intel_hdmi->rgb_quant_range_selectable) {
-		if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
+		if (intel_crtc->config.limited_color_range)
 			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
 		else
 			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
@@ -785,7 +786,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	}
 
 	if (intel_hdmi->color_range)
-		adjusted_mode->private_flags |= INTEL_MODE_LIMITED_COLOR_RANGE;
+		pipe_config->limited_color_range = true;
 
 	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev))
 		pipe_config->has_pch_encoder = true;
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 5f3f9e9..c6fbfd1 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -956,9 +956,10 @@ static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo,
 		.len = DIP_LEN_AVI,
 	};
 	uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)];
+	struct intel_crtc *intel_crtc = to_intel_crtc(intel_sdvo->base.base.crtc);
 
 	if (intel_sdvo->rgb_quant_range_selectable) {
-		if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
+		if (intel_crtc->config.limited_color_range)
 			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
 		else
 			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
@@ -1091,7 +1092,7 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
 	}
 
 	if (intel_sdvo->color_range)
-		adjusted_mode->private_flags |= INTEL_MODE_LIMITED_COLOR_RANGE;
+		pipe_config->limited_color_range = true;
 
 	return true;
 }
-- 
1.7.11.7

Re: [PATCH 07/13] drm/i915: add pipe_config->limited_color_range

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:56 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Now that we have a useful struct for this, let's use it. Some neat
> pointer-chasing required, but it's all there already.
> 
> v2: Rebased on top of the added Haswell limited color range support.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
>  drivers/gpu/drm/i915/intel_dp.c      |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 12 +++++++-----
>  drivers/gpu/drm/i915/intel_hdmi.c    |  5 +++--
>  drivers/gpu/drm/i915/intel_sdvo.c    |  5 +++--
>  5 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fda0754..bfed546 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5173,7 +5173,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
>  	else
>  		val |= PIPECONF_PROGRESSIVE;
>  
> -	if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> +	if (intel_crtc->config.limited_color_range)
>  		val |= PIPECONF_COLOR_RANGE_SELECT;
>  	else
>  		val &= ~PIPECONF_COLOR_RANGE_SELECT;
> @@ -5189,8 +5189,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
>   * is supported, but eventually this should handle various
>   * RGB<->YCbCr scenarios as well.
>   */
> -static void intel_set_pipe_csc(struct drm_crtc *crtc,
> -			       const struct drm_display_mode *adjusted_mode)
> +static void intel_set_pipe_csc(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5205,7 +5204,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc,
>  	 * consideration.
>  	 */
>  
> -	if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> +	if (intel_crtc->config.limited_color_range)
>  		coeff = ((235 - 16) * (1 << 12) / 255) & 0xff8; /* 0.xxx... */
>  
>  	/*
> @@ -5229,7 +5228,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc,
>  	if (INTEL_INFO(dev)->gen > 6) {
>  		uint16_t postoff = 0;
>  
> -		if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> +		if (intel_crtc->config.limited_color_range)
>  			postoff = (16 * (1 << 13) / 255) & 0x1fff;
>  
>  		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
> @@ -5240,7 +5239,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc,
>  	} else {
>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>  
> -		if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> +		if (intel_crtc->config.limited_color_range)
>  			mode |= CSC_BLACK_SCREEN_OFFSET;
>  
>  		I915_WRITE(PIPE_CSC_MODE(pipe), mode);
> @@ -5841,7 +5840,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	haswell_set_pipeconf(crtc, adjusted_mode, dither);
>  
> -	intel_set_pipe_csc(crtc, adjusted_mode);
> +	intel_set_pipe_csc(crtc);
>  
>  	/* Set up the display plane register */
>  	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE | DISPPLANE_PIPE_CSC_ENABLE);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bc73e5e..d7c1403 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -739,7 +739,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	}
>  
>  	if (intel_dp->color_range)
> -		adjusted_mode->private_flags |= INTEL_MODE_LIMITED_COLOR_RANGE;
> +		pipe_config->limited_color_range = true;
>  
>  	mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8de1855..63160c6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -103,11 +103,6 @@
>  
>  /* drm_display_mode->private_flags */
>  #define INTEL_MODE_DP_FORCE_6BPC (0x10)
> -/*
> - * Set when limited 16-235 (as opposed to full 0-255) RGB color range is
> - * to be used.
> - */
> -#define INTEL_MODE_LIMITED_COLOR_RANGE (0x40)
>  
>  struct intel_framebuffer {
>  	struct drm_framebuffer base;
> @@ -193,6 +188,13 @@ struct intel_crtc_config {
>  	/* Whether to set up the PCH/FDI. Note that we never allow sharing
>  	 * between pch encoders and cpu encoders. */
>  	bool has_pch_encoder;
> +
> +	/*
> +	 * Use reduced/limited/broadcast rbg range, compressing from the full
> +	 * range fed into the crtcs.
> +	 */
> +	bool limited_color_range;
> +
>  	/* Used by SDVO (and if we ever fix it, HDMI). */
>  	unsigned pixel_multiplier;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index b588e6c..5508687 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -333,6 +333,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  					 struct drm_display_mode *adjusted_mode)
>  {
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>  	struct dip_infoframe avi_if = {
>  		.type = DIP_TYPE_AVI,
>  		.ver = DIP_VERSION_AVI,
> @@ -343,7 +344,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  		avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
>  
>  	if (intel_hdmi->rgb_quant_range_selectable) {
> -		if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> +		if (intel_crtc->config.limited_color_range)
>  			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
>  		else
>  			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
> @@ -785,7 +786,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	}
>  
>  	if (intel_hdmi->color_range)
> -		adjusted_mode->private_flags |= INTEL_MODE_LIMITED_COLOR_RANGE;
> +		pipe_config->limited_color_range = true;
>  
>  	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev))
>  		pipe_config->has_pch_encoder = true;
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 5f3f9e9..c6fbfd1 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -956,9 +956,10 @@ static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo,
>  		.len = DIP_LEN_AVI,
>  	};
>  	uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)];
> +	struct intel_crtc *intel_crtc = to_intel_crtc(intel_sdvo->base.base.crtc);
>  
>  	if (intel_sdvo->rgb_quant_range_selectable) {
> -		if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> +		if (intel_crtc->config.limited_color_range)
>  			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
>  		else
>  			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
> @@ -1091,7 +1092,7 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
>  	}
>  
>  	if (intel_sdvo->color_range)
> -		adjusted_mode->private_flags |= INTEL_MODE_LIMITED_COLOR_RANGE;
> +		pipe_config->limited_color_range = true;
>  
>  	return true;
>  }

Looks nice.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 08/13] drm/i915: introduce pipe_config->dither|pipe_bpp

[Daniel Vetter]

We want to compute this earlier. To avoid a big complicated patch,
this patch here just does the big search&replace and still calls the
old functions at the same places.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ddi.c     |  8 ++++----
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++--------
 drivers/gpu/drm/i915/intel_dp.c      |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  4 +++-
 drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index baeb470..3d09df0 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -931,7 +931,7 @@ void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
 	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
 
 		temp = TRANS_MSA_SYNC_CLK;
-		switch (intel_crtc->bpp) {
+		switch (intel_crtc->config.pipe_bpp) {
 		case 18:
 			temp |= TRANS_MSA_6_BPC;
 			break;
@@ -947,7 +947,7 @@ void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
 		default:
 			temp |= TRANS_MSA_8_BPC;
 			WARN(1, "%d bpp unsupported by DDI function\n",
-			     intel_crtc->bpp);
+			     intel_crtc->config.pipe_bpp);
 		}
 		I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
 	}
@@ -969,7 +969,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
 	temp = TRANS_DDI_FUNC_ENABLE;
 	temp |= TRANS_DDI_SELECT_PORT(port);
 
-	switch (intel_crtc->bpp) {
+	switch (intel_crtc->config.pipe_bpp) {
 	case 18:
 		temp |= TRANS_DDI_BPC_6;
 		break;
@@ -984,7 +984,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
 		break;
 	default:
 		WARN(1, "%d bpp unsupported by transcoder DDI function\n",
-		     intel_crtc->bpp);
+		     intel_crtc->config.pipe_bpp);
 	}
 
 	if (crtc->mode.flags & DRM_MODE_FLAG_PVSYNC)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bfed546..b495629 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4648,6 +4648,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	const intel_limit_t *limit;
 	int ret;
 
+	/* temporary hack */
+	intel_crtc->config.dither =
+		adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC;
+
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
@@ -4748,7 +4752,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	/* default to 8bpc */
 	pipeconf &= ~(PIPECONF_BPC_MASK | PIPECONF_DITHER_EN);
 	if (is_dp) {
-		if (adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
+		if (intel_crtc->config.dither) {
 			pipeconf |= PIPECONF_6BPC |
 				    PIPECONF_DITHER_EN |
 				    PIPECONF_DITHER_TYPE_SP;
@@ -4756,7 +4760,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	}
 
 	if (IS_VALLEYVIEW(dev) && intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP)) {
-		if (adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
+		if (intel_crtc->config.dither) {
 			pipeconf |= PIPECONF_6BPC |
 					PIPECONF_ENABLE |
 					I965_PIPECONF_ACTIVE;
@@ -5145,7 +5149,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
 	val = I915_READ(PIPECONF(pipe));
 
 	val &= ~PIPECONF_BPC_MASK;
-	switch (intel_crtc->bpp) {
+	switch (intel_crtc->config.pipe_bpp) {
 	case 18:
 		val |= PIPECONF_6BPC;
 		break;
@@ -5482,13 +5486,14 @@ static void ironlake_set_m_n(struct drm_crtc *crtc)
 
 	if (!lane)
 		lane = ironlake_get_lanes_required(target_clock, link_bw,
-						   intel_crtc->bpp);
+						   intel_crtc->config.pipe_bpp);
 
 	intel_crtc->fdi_lanes = lane;
 
 	if (intel_crtc->config.pixel_multiplier > 1)
 		link_bw *= intel_crtc->config.pixel_multiplier;
-	intel_link_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, &m_n);
+	intel_link_compute_m_n(intel_crtc->config.pipe_bpp, lane, target_clock,
+			       link_bw, &m_n);
 
 	I915_WRITE(PIPE_DATA_M1(cpu_transcoder), TU_SIZE(m_n.tu) | m_n.gmch_m);
 	I915_WRITE(PIPE_DATA_N1(cpu_transcoder), m_n.gmch_n);
@@ -5651,8 +5656,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	intel_crtc_update_cursor(crtc, true);
 
 	/* determine panel color depth */
-	dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp,
+	dither = intel_choose_pipe_bpp_dither(crtc, fb,
+					      &intel_crtc->config.pipe_bpp,
 					      adjusted_mode);
+	intel_crtc->config.dither = dither;
 	if (is_lvds && dev_priv->lvds_dither)
 		dither = true;
 
@@ -5822,8 +5829,10 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	intel_crtc_update_cursor(crtc, true);
 
 	/* determine panel color depth */
-	dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp,
+	dither = intel_choose_pipe_bpp_dither(crtc, fb,
+					      &intel_crtc->config.pipe_bpp,
 					      adjusted_mode);
+	intel_crtc->config.dither = dither;
 
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
@@ -8268,7 +8277,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
-	intel_crtc->bpp = 24; /* default for pre-Ironlake */
+	intel_crtc->config.pipe_bpp = 24; /* default for pre-Ironlake */
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 }
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d7c1403..34006f7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -811,7 +811,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_link_compute_m_n(intel_crtc->bpp, lane_count,
+	intel_link_compute_m_n(intel_crtc->config.pipe_bpp, lane_count,
 			       target_clock, adjusted_mode->clock, &m_n);
 
 	if (HAS_DDI(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 63160c6..0ca0d76 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -195,6 +195,9 @@ struct intel_crtc_config {
 	 */
 	bool limited_color_range;
 
+	bool dither;
+	int pipe_bpp;
+
 	/* Used by SDVO (and if we ever fix it, HDMI). */
 	unsigned pixel_multiplier;
 };
@@ -230,7 +233,6 @@ struct intel_crtc {
 	int16_t cursor_x, cursor_y;
 	int16_t cursor_width, cursor_height;
 	bool cursor_visible;
-	unsigned int bpp;
 
 	struct intel_crtc_config config;
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 5508687..0731ba6 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -609,7 +609,7 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
 	if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
 		hdmi_val |= SDVO_HSYNC_ACTIVE_HIGH;
 
-	if (intel_crtc->bpp > 24)
+	if (intel_crtc->config.pipe_bpp > 24)
 		hdmi_val |= HDMI_COLOR_FORMAT_12bpc;
 	else
 		hdmi_val |= SDVO_COLOR_FORMAT_8bpc;
-- 
1.7.11.7

Re: [PATCH 08/13] drm/i915: introduce pipe_config->dither|pipe_bpp

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:57 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> We want to compute this earlier. To avoid a big complicated patch,
> this patch here just does the big search&replace and still calls the
> old functions at the same places.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |  8 ++++----
>  drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_dp.c      |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  4 +++-
>  drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
>  5 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index baeb470..3d09df0 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -931,7 +931,7 @@ void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
>  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
>  
>  		temp = TRANS_MSA_SYNC_CLK;
> -		switch (intel_crtc->bpp) {
> +		switch (intel_crtc->config.pipe_bpp) {
>  		case 18:
>  			temp |= TRANS_MSA_6_BPC;
>  			break;
> @@ -947,7 +947,7 @@ void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
>  		default:
>  			temp |= TRANS_MSA_8_BPC;
>  			WARN(1, "%d bpp unsupported by DDI function\n",
> -			     intel_crtc->bpp);
> +			     intel_crtc->config.pipe_bpp);
>  		}
>  		I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>  	}
> @@ -969,7 +969,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>  	temp = TRANS_DDI_FUNC_ENABLE;
>  	temp |= TRANS_DDI_SELECT_PORT(port);
>  
> -	switch (intel_crtc->bpp) {
> +	switch (intel_crtc->config.pipe_bpp) {
>  	case 18:
>  		temp |= TRANS_DDI_BPC_6;
>  		break;
> @@ -984,7 +984,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>  		break;
>  	default:
>  		WARN(1, "%d bpp unsupported by transcoder DDI function\n",
> -		     intel_crtc->bpp);
> +		     intel_crtc->config.pipe_bpp);
>  	}
>  
>  	if (crtc->mode.flags & DRM_MODE_FLAG_PVSYNC)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bfed546..b495629 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4648,6 +4648,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	const intel_limit_t *limit;
>  	int ret;
>  
> +	/* temporary hack */
> +	intel_crtc->config.dither =
> +		adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC;
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
>  		case INTEL_OUTPUT_LVDS:
> @@ -4748,7 +4752,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	/* default to 8bpc */
>  	pipeconf &= ~(PIPECONF_BPC_MASK | PIPECONF_DITHER_EN);
>  	if (is_dp) {
> -		if (adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
> +		if (intel_crtc->config.dither) {
>  			pipeconf |= PIPECONF_6BPC |
>  				    PIPECONF_DITHER_EN |
>  				    PIPECONF_DITHER_TYPE_SP;
> @@ -4756,7 +4760,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	}
>  
>  	if (IS_VALLEYVIEW(dev) && intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP)) {
> -		if (adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
> +		if (intel_crtc->config.dither) {
>  			pipeconf |= PIPECONF_6BPC |
>  					PIPECONF_ENABLE |
>  					I965_PIPECONF_ACTIVE;
> @@ -5145,7 +5149,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
>  	val = I915_READ(PIPECONF(pipe));
>  
>  	val &= ~PIPECONF_BPC_MASK;
> -	switch (intel_crtc->bpp) {
> +	switch (intel_crtc->config.pipe_bpp) {
>  	case 18:
>  		val |= PIPECONF_6BPC;
>  		break;
> @@ -5482,13 +5486,14 @@ static void ironlake_set_m_n(struct drm_crtc *crtc)
>  
>  	if (!lane)
>  		lane = ironlake_get_lanes_required(target_clock, link_bw,
> -						   intel_crtc->bpp);
> +						   intel_crtc->config.pipe_bpp);
>  
>  	intel_crtc->fdi_lanes = lane;
>  
>  	if (intel_crtc->config.pixel_multiplier > 1)
>  		link_bw *= intel_crtc->config.pixel_multiplier;
> -	intel_link_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, &m_n);
> +	intel_link_compute_m_n(intel_crtc->config.pipe_bpp, lane, target_clock,
> +			       link_bw, &m_n);
>  
>  	I915_WRITE(PIPE_DATA_M1(cpu_transcoder), TU_SIZE(m_n.tu) | m_n.gmch_m);
>  	I915_WRITE(PIPE_DATA_N1(cpu_transcoder), m_n.gmch_n);
> @@ -5651,8 +5656,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	intel_crtc_update_cursor(crtc, true);
>  
>  	/* determine panel color depth */
> -	dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp,
> +	dither = intel_choose_pipe_bpp_dither(crtc, fb,
> +					      &intel_crtc->config.pipe_bpp,
>  					      adjusted_mode);
> +	intel_crtc->config.dither = dither;
>  	if (is_lvds && dev_priv->lvds_dither)
>  		dither = true;
>  
> @@ -5822,8 +5829,10 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	intel_crtc_update_cursor(crtc, true);
>  
>  	/* determine panel color depth */
> -	dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp,
> +	dither = intel_choose_pipe_bpp_dither(crtc, fb,
> +					      &intel_crtc->config.pipe_bpp,
>  					      adjusted_mode);
> +	intel_crtc->config.dither = dither;
>  
>  	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
>  	drm_mode_debug_printmodeline(mode);
> @@ -8268,7 +8277,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
>  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>  
> -	intel_crtc->bpp = 24; /* default for pre-Ironlake */
> +	intel_crtc->config.pipe_bpp = 24; /* default for pre-Ironlake */
>  
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d7c1403..34006f7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -811,7 +811,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_link_compute_m_n(intel_crtc->bpp, lane_count,
> +	intel_link_compute_m_n(intel_crtc->config.pipe_bpp, lane_count,
>  			       target_clock, adjusted_mode->clock, &m_n);
>  
>  	if (HAS_DDI(dev)) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 63160c6..0ca0d76 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -195,6 +195,9 @@ struct intel_crtc_config {
>  	 */
>  	bool limited_color_range;
>  
> +	bool dither;
> +	int pipe_bpp;
> +
>  	/* Used by SDVO (and if we ever fix it, HDMI). */
>  	unsigned pixel_multiplier;
>  };
> @@ -230,7 +233,6 @@ struct intel_crtc {
>  	int16_t cursor_x, cursor_y;
>  	int16_t cursor_width, cursor_height;
>  	bool cursor_visible;
> -	unsigned int bpp;
>  
>  	struct intel_crtc_config config;
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 5508687..0731ba6 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -609,7 +609,7 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
>  		hdmi_val |= SDVO_HSYNC_ACTIVE_HIGH;
>  
> -	if (intel_crtc->bpp > 24)
> +	if (intel_crtc->config.pipe_bpp > 24)
>  		hdmi_val |= HDMI_COLOR_FORMAT_12bpc;
>  	else
>  		hdmi_val |= SDVO_COLOR_FORMAT_8bpc;

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 09/13] drm/i915: precompute pipe bpp before touching the hw

[Daniel Vetter]

The procedure has now 3 steps:

1. Compute the bpp that the plane will output, this is done in
   pipe_config_set_bpp and stored into pipe_config->pipe_bpp. Also,
   this function clamps the pipe_bpp to whatever limit the EDID of any
   connected output specifies.
2. Adjust the pipe_bpp in the encoder and crtc functions, according to
   whatever constraints there are.
3. Decide whether to use dither by comparing the stored plane bpp with
   computed pipe_bpp.

There are a few slight functional changes in this patch:
- LVDS connector are now also going through the EDID clamping. But in
  a 2nd change we now unconditionally force the lvds bpc value - this
  shouldn't matter in reality when the panel setup is consistent, but
  better safe than sorry.
- HDMI now forces the pipe_bpp to the selected value - I think that's
  what we actually want, since otherwise at least the pixelclock
  computations are wrong (I'm not sure whether the port would accept
  e.g. 10 bpc when in 12bpc mode). Contrary to the old code, we pick
  the next higher bpc value, since otherwise there's no way to make
  use of the 12 bpc mode (since the next patch will remove the 12bpc
  plane format, it doesn't exist).

Both of these changes are due to the removal of the

	pipe_bpp = min(display_bpp, plane_bpp);

statement.

Another slight change is the reworking of the dp bpc code:
- For the mode_valid callback it's sufficient to only check whether
  the mode would fit at the lowest bpc.
- The bandwidth computation code is a bit restructured: It now walks
  all available bpp values in an outer loop and the codeblock that
  computes derived values (once a good configuration is found) has been
  moved out of the for loop maze. This is prep work to allow us to
  successively fall back on bpc values, and also correctly support bpc
  values != 8 or 6.

v2: Rebased on top of Paulo Zanoni's little refactoring to use more
drm dp helper functions.

v3: Rebased on top of Jani's eDP bpp fix and Ville's limited color
range work.

v4: Remove the INTEL_MODE_DP_FORCE_6BPC #define, no longer needed.

v5: Remove intel_crtc->bpp, too, and fix up the 12bpc check in the
hdmi code. Also fixup the bpp check in intel_dp.c, it'll get reworked
in a later patch though again.

v6: Fix spelling in a comment.

v7: Debug output improvements for the bpp computation.

v8: Fixup 6bpc lvds check - dual-link and 8bpc mode are different
things!

v9: Reinstate the fix to properly ignore the firmware edp bpp ... this
was lost in a rebase.

v10: Both g4x and vlv lack 12bpc pipes, so don't enforce that we have
that. Still unsure whether this is the way to go, but at least 6bpc
for a 8bpc hdmi output seems to work.

v11: And g4x/vlv also lack 12bpc hdmi support, so only support high
depth on DP. Adjust the code.

v12: Rebased.

v13: Split out the introduction of pipe_config->dither|pipe_bpp, as
requested from Jesse Barnes.

v14: Split out the special 6BPC handling for DP, as requested by Jesse
Barnes.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ddi.c     |   7 +-
 drivers/gpu/drm/i915/intel_display.c | 224 ++++++++++++-----------------------
 drivers/gpu/drm/i915/intel_hdmi.c    |  13 ++
 drivers/gpu/drm/i915/intel_lvds.c    |  12 ++
 4 files changed, 100 insertions(+), 156 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 3d09df0..6c6b012 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -945,9 +945,7 @@ void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
 			temp |= TRANS_MSA_12_BPC;
 			break;
 		default:
-			temp |= TRANS_MSA_8_BPC;
-			WARN(1, "%d bpp unsupported by DDI function\n",
-			     intel_crtc->config.pipe_bpp);
+			BUG();
 		}
 		I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
 	}
@@ -983,8 +981,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
 		temp |= TRANS_DDI_BPC_12;
 		break;
 	default:
-		WARN(1, "%d bpp unsupported by transcoder DDI function\n",
-		     intel_crtc->config.pipe_bpp);
+		BUG();
 	}
 
 	if (crtc->mode.flags & DRM_MODE_FLAG_PVSYNC)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b495629..6a60bf2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4059,142 +4059,6 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
 		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
 }
 
-/**
- * intel_choose_pipe_bpp_dither - figure out what color depth the pipe should send
- * @crtc: CRTC structure
- * @mode: requested mode
- *
- * 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.
- *    DP may want to dither down to 6bpc to fit larger modes
- *
- * 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_dither(struct drm_crtc *crtc,
-					 struct drm_framebuffer *fb,
-					 unsigned int *pipe_bpp,
-					 struct drm_display_mode *mode)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_connector *connector;
-	struct intel_encoder *intel_encoder;
-	unsigned int display_bpc = UINT_MAX, bpc;
-
-	/* Walk the encoders & connectors on this crtc, get min bpc */
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
-
-		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) {
-				DRM_DEBUG_KMS("clamping display bpc (was %d) to LVDS (%d)\n", display_bpc, lvds_bpc);
-				display_bpc = lvds_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 != &intel_encoder->base)
-				continue;
-
-			/* Don't use an invalid EDID bpc value */
-			if (connector->display_info.bpc &&
-			    connector->display_info.bpc < display_bpc) {
-				DRM_DEBUG_KMS("clamping display bpc (was %d) to EDID reported max of %d\n", display_bpc, connector->display_info.bpc);
-				display_bpc = connector->display_info.bpc;
-			}
-		}
-
-		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 && edp_bpc < display_bpc) {
-				DRM_DEBUG_KMS("clamping display bpc (was %d) to eDP (%d)\n", display_bpc, edp_bpc);
-				display_bpc = edp_bpc;
-			}
-			continue;
-		}
-
-		/*
-		 * HDMI is either 12 or 8, so if the display lets 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) {
-				DRM_DEBUG_KMS("forcing bpc to 12 for HDMI\n");
-				display_bpc = 12;
-			} else {
-				DRM_DEBUG_KMS("forcing bpc to 8 for HDMI\n");
-				display_bpc = 8;
-			}
-		}
-	}
-
-	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
-	 * the minimum value that expresses the full color range of the fb but
-	 * also stays within the max display bpc discovered above.
-	 */
-
-	switch (fb->depth) {
-	case 8:
-		bpc = 8; /* since we go through a colormap */
-		break;
-	case 15:
-	case 16:
-		bpc = 6; /* min is 18bpp */
-		break;
-	case 24:
-		bpc = 8;
-		break;
-	case 30:
-		bpc = 10;
-		break;
-	case 48:
-		bpc = 12;
-		break;
-	default:
-		DRM_DEBUG("unsupported depth, assuming 24 bits\n");
-		bpc = min((unsigned int)8, display_bpc);
-		break;
-	}
-
-	display_bpc = min(display_bpc, bpc);
-
-	DRM_DEBUG_KMS("setting pipe bpc to %d (max display bpc %d)\n",
-		      bpc, display_bpc);
-
-	*pipe_bpp = display_bpc * 3;
-
-	return display_bpc != bpc;
-}
-
 static int vlv_get_refclk(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -4648,10 +4512,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	const intel_limit_t *limit;
 	int ret;
 
-	/* temporary hack */
-	intel_crtc->config.dither =
-		adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC;
-
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
@@ -5656,10 +5516,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	intel_crtc_update_cursor(crtc, true);
 
 	/* determine panel color depth */
-	dither = intel_choose_pipe_bpp_dither(crtc, fb,
-					      &intel_crtc->config.pipe_bpp,
-					      adjusted_mode);
-	intel_crtc->config.dither = dither;
+	dither = intel_crtc->config.dither;
 	if (is_lvds && dev_priv->lvds_dither)
 		dither = true;
 
@@ -5829,10 +5686,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	intel_crtc_update_cursor(crtc, true);
 
 	/* determine panel color depth */
-	dither = intel_choose_pipe_bpp_dither(crtc, fb,
-					      &intel_crtc->config.pipe_bpp,
-					      adjusted_mode);
-	intel_crtc->config.dither = dither;
+	dither = intel_crtc->config.dither;
 
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
@@ -7497,14 +7351,72 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
 	}
 }
 
+static int
+pipe_config_set_bpp(struct drm_crtc *crtc,
+		    struct drm_framebuffer *fb,
+		    struct intel_crtc_config *pipe_config)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_connector *connector;
+	int bpp;
+
+	switch (fb->depth) {
+	case 8:
+		bpp = 8*3; /* since we go through a colormap */
+		break;
+	case 15:
+	case 16:
+		bpp = 6*3; /* min is 18bpp */
+		break;
+	case 24:
+		bpp = 8*3;
+		break;
+	case 30:
+		bpp = 10*3;
+		break;
+	case 48:
+		bpp = 12*3;
+		break;
+	default:
+		DRM_DEBUG_KMS("unsupported depth\n");
+		return -EINVAL;
+	}
+
+	if (fb->depth > 24 && !HAS_PCH_SPLIT(dev)) {
+		DRM_DEBUG_KMS("high depth not supported on gmch platforms\n");
+		return -EINVAL;
+	}
+
+	pipe_config->pipe_bpp = bpp;
+
+	/* Clamp display bpp to EDID value */
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    head) {
+		if (connector->encoder && connector->encoder->crtc != crtc)
+			continue;
+
+		/* Don't use an invalid EDID bpc value */
+		if (connector->display_info.bpc &&
+		    connector->display_info.bpc * 3 < bpp) {
+			DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID reported max of %d\n",
+				      bpp, connector->display_info.bpc*3);
+			pipe_config->pipe_bpp = connector->display_info.bpc*3;
+		}
+	}
+
+	return bpp;
+}
+
 static struct intel_crtc_config *
 intel_modeset_pipe_config(struct drm_crtc *crtc,
+			  struct drm_framebuffer *fb,
 			  struct drm_display_mode *mode)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_encoder_helper_funcs *encoder_funcs;
 	struct intel_encoder *encoder;
 	struct intel_crtc_config *pipe_config;
+	int plane_bpp;
 
 	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
 	if (!pipe_config)
@@ -7513,6 +7425,10 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	drm_mode_copy(&pipe_config->adjusted_mode, mode);
 	drm_mode_copy(&pipe_config->requested_mode, mode);
 
+	plane_bpp = pipe_config_set_bpp(crtc, fb, pipe_config);
+	if (plane_bpp < 0)
+		goto fail;
+
 	/* Pass our mode to the connectors and the CRTC to give them a chance to
 	 * adjust it according to limitations or connector properties, and also
 	 * a chance to reject the mode entirely.
@@ -7541,12 +7457,20 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 		}
 	}
 
+	/* temporary hack until the DP code doesn't use the 6BPC flag any more */
+	if (pipe_config->adjusted_mode.private_flags & INTEL_MODE_DP_FORCE_6BPC)
+		pipe_config->pipe_bpp = 6*8;
+
 	if (!(intel_crtc_compute_config(crtc, pipe_config))) {
 		DRM_DEBUG_KMS("CRTC fixup failed\n");
 		goto fail;
 	}
 	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
 
+	pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
+	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
+		      plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
+
 	return pipe_config;
 fail:
 	kfree(pipe_config);
@@ -7838,7 +7762,7 @@ int intel_set_mode(struct drm_crtc *crtc,
 	 * pieces of code that are not yet converted to deal with mutliple crtcs
 	 * changing their mode at the same time. */
 	if (modeset_pipes) {
-		pipe_config = intel_modeset_pipe_config(crtc, mode);
+		pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
 		if (IS_ERR(pipe_config)) {
 			ret = PTR_ERR(pipe_config);
 			pipe_config = NULL;
@@ -8277,8 +8201,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
-	intel_crtc->config.pipe_bpp = 24; /* default for pre-Ironlake */
-
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 0731ba6..b206a0d 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -791,6 +791,19 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev))
 		pipe_config->has_pch_encoder = true;
 
+	/*
+	 * HDMI is either 12 or 8, so if the display lets 10bpc sneak
+	 * through, clamp it down. Note that g4x/vlv don't support 12bpc hdmi
+	 * outputs.
+	 */
+	if (pipe_config->pipe_bpp > 8*3 && HAS_PCH_SPLIT(dev)) {
+		DRM_DEBUG_KMS("forcing bpc to 12 for HDMI\n");
+		pipe_config->pipe_bpp = 12*3;
+	} else {
+		DRM_DEBUG_KMS("forcing bpc to 8 for HDMI\n");
+		pipe_config->pipe_bpp = 8*3;
+	}
+
 	return true;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 9d6ed91..7b6d07b 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -310,6 +310,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 	struct drm_display_mode *mode = &pipe_config->requested_mode;
 	struct intel_crtc *intel_crtc = lvds_encoder->base.new_crtc;
 	u32 pfit_control = 0, pfit_pgm_ratios = 0, border = 0;
+	unsigned int lvds_bpp;
 	int pipe;
 
 	/* Should never happen!! */
@@ -321,6 +322,17 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 	if (intel_encoder_check_is_cloned(&lvds_encoder->base))
 		return false;
 
+	if ((I915_READ(lvds_encoder->reg) & LVDS_A3_POWER_MASK) ==
+	    LVDS_A3_POWER_UP)
+		lvds_bpp = 8*3;
+	else
+		lvds_bpp = 6*3;
+
+	if (lvds_bpp != pipe_config->pipe_bpp) {
+		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
+			      pipe_config->pipe_bpp, lvds_bpp);
+		pipe_config->pipe_bpp = lvds_bpp;
+	}
 	/*
 	 * We have timings from the BIOS for the panel, put them in
 	 * to the adjusted mode.  The CRTC will be set up for this mode,
-- 
1.7.11.7

Re: [PATCH 09/13] drm/i915: precompute pipe bpp before touching the hw

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:58 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> The procedure has now 3 steps:
> 
> 1. Compute the bpp that the plane will output, this is done in
>    pipe_config_set_bpp and stored into pipe_config->pipe_bpp. Also,
>    this function clamps the pipe_bpp to whatever limit the EDID of any
>    connected output specifies.
> 2. Adjust the pipe_bpp in the encoder and crtc functions, according to
>    whatever constraints there are.
> 3. Decide whether to use dither by comparing the stored plane bpp with
>    computed pipe_bpp.
> 
> There are a few slight functional changes in this patch:
> - LVDS connector are now also going through the EDID clamping. But in
>   a 2nd change we now unconditionally force the lvds bpc value - this
>   shouldn't matter in reality when the panel setup is consistent, but
>   better safe than sorry.
> - HDMI now forces the pipe_bpp to the selected value - I think that's
>   what we actually want, since otherwise at least the pixelclock
>   computations are wrong (I'm not sure whether the port would accept
>   e.g. 10 bpc when in 12bpc mode). Contrary to the old code, we pick
>   the next higher bpc value, since otherwise there's no way to make
>   use of the 12 bpc mode (since the next patch will remove the 12bpc
>   plane format, it doesn't exist).
> 
> Both of these changes are due to the removal of the
> 
> 	pipe_bpp = min(display_bpp, plane_bpp);
> 
> statement.
> 
> Another slight change is the reworking of the dp bpc code:
> - For the mode_valid callback it's sufficient to only check whether
>   the mode would fit at the lowest bpc.
> - The bandwidth computation code is a bit restructured: It now walks
>   all available bpp values in an outer loop and the codeblock that
>   computes derived values (once a good configuration is found) has been
>   moved out of the for loop maze. This is prep work to allow us to
>   successively fall back on bpc values, and also correctly support bpc
>   values != 8 or 6.
> 
> v2: Rebased on top of Paulo Zanoni's little refactoring to use more
> drm dp helper functions.
> 
> v3: Rebased on top of Jani's eDP bpp fix and Ville's limited color
> range work.
> 
> v4: Remove the INTEL_MODE_DP_FORCE_6BPC #define, no longer needed.
> 
> v5: Remove intel_crtc->bpp, too, and fix up the 12bpc check in the
> hdmi code. Also fixup the bpp check in intel_dp.c, it'll get reworked
> in a later patch though again.
> 
> v6: Fix spelling in a comment.
> 
> v7: Debug output improvements for the bpp computation.
> 
> v8: Fixup 6bpc lvds check - dual-link and 8bpc mode are different
> things!
> 
> v9: Reinstate the fix to properly ignore the firmware edp bpp ... this
> was lost in a rebase.
> 
> v10: Both g4x and vlv lack 12bpc pipes, so don't enforce that we have
> that. Still unsure whether this is the way to go, but at least 6bpc
> for a 8bpc hdmi output seems to work.
> 
> v11: And g4x/vlv also lack 12bpc hdmi support, so only support high
> depth on DP. Adjust the code.
> 
> v12: Rebased.
> 
> v13: Split out the introduction of pipe_config->dither|pipe_bpp, as
> requested from Jesse Barnes.
> 
> v14: Split out the special 6BPC handling for DP, as requested by Jesse
> Barnes.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |   7 +-
>  drivers/gpu/drm/i915/intel_display.c | 224 ++++++++++++-----------------------
>  drivers/gpu/drm/i915/intel_hdmi.c    |  13 ++
>  drivers/gpu/drm/i915/intel_lvds.c    |  12 ++
>  4 files changed, 100 insertions(+), 156 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 3d09df0..6c6b012 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -945,9 +945,7 @@ void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
>  			temp |= TRANS_MSA_12_BPC;
>  			break;
>  		default:
> -			temp |= TRANS_MSA_8_BPC;
> -			WARN(1, "%d bpp unsupported by DDI function\n",
> -			     intel_crtc->config.pipe_bpp);
> +			BUG();
>  		}
>  		I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>  	}
> @@ -983,8 +981,7 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>  		temp |= TRANS_DDI_BPC_12;
>  		break;
>  	default:
> -		WARN(1, "%d bpp unsupported by transcoder DDI function\n",
> -		     intel_crtc->config.pipe_bpp);
> +		BUG();
>  	}
>  
>  	if (crtc->mode.flags & DRM_MODE_FLAG_PVSYNC)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b495629..6a60bf2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4059,142 +4059,6 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>  		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
>  }
>  
> -/**
> - * intel_choose_pipe_bpp_dither - figure out what color depth the pipe should send
> - * @crtc: CRTC structure
> - * @mode: requested mode
> - *
> - * 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.
> - *    DP may want to dither down to 6bpc to fit larger modes
> - *
> - * 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_dither(struct drm_crtc *crtc,
> -					 struct drm_framebuffer *fb,
> -					 unsigned int *pipe_bpp,
> -					 struct drm_display_mode *mode)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_connector *connector;
> -	struct intel_encoder *intel_encoder;
> -	unsigned int display_bpc = UINT_MAX, bpc;
> -
> -	/* Walk the encoders & connectors on this crtc, get min bpc */
> -	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
> -
> -		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) {
> -				DRM_DEBUG_KMS("clamping display bpc (was %d) to LVDS (%d)\n", display_bpc, lvds_bpc);
> -				display_bpc = lvds_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 != &intel_encoder->base)
> -				continue;
> -
> -			/* Don't use an invalid EDID bpc value */
> -			if (connector->display_info.bpc &&
> -			    connector->display_info.bpc < display_bpc) {
> -				DRM_DEBUG_KMS("clamping display bpc (was %d) to EDID reported max of %d\n", display_bpc, connector->display_info.bpc);
> -				display_bpc = connector->display_info.bpc;
> -			}
> -		}
> -
> -		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 && edp_bpc < display_bpc) {
> -				DRM_DEBUG_KMS("clamping display bpc (was %d) to eDP (%d)\n", display_bpc, edp_bpc);
> -				display_bpc = edp_bpc;
> -			}
> -			continue;
> -		}
> -
> -		/*
> -		 * HDMI is either 12 or 8, so if the display lets 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) {
> -				DRM_DEBUG_KMS("forcing bpc to 12 for HDMI\n");
> -				display_bpc = 12;
> -			} else {
> -				DRM_DEBUG_KMS("forcing bpc to 8 for HDMI\n");
> -				display_bpc = 8;
> -			}
> -		}
> -	}
> -
> -	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
> -	 * the minimum value that expresses the full color range of the fb but
> -	 * also stays within the max display bpc discovered above.
> -	 */
> -
> -	switch (fb->depth) {
> -	case 8:
> -		bpc = 8; /* since we go through a colormap */
> -		break;
> -	case 15:
> -	case 16:
> -		bpc = 6; /* min is 18bpp */
> -		break;
> -	case 24:
> -		bpc = 8;
> -		break;
> -	case 30:
> -		bpc = 10;
> -		break;
> -	case 48:
> -		bpc = 12;
> -		break;
> -	default:
> -		DRM_DEBUG("unsupported depth, assuming 24 bits\n");
> -		bpc = min((unsigned int)8, display_bpc);
> -		break;
> -	}
> -
> -	display_bpc = min(display_bpc, bpc);
> -
> -	DRM_DEBUG_KMS("setting pipe bpc to %d (max display bpc %d)\n",
> -		      bpc, display_bpc);
> -
> -	*pipe_bpp = display_bpc * 3;
> -
> -	return display_bpc != bpc;
> -}
> -
>  static int vlv_get_refclk(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -4648,10 +4512,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	const intel_limit_t *limit;
>  	int ret;
>  
> -	/* temporary hack */
> -	intel_crtc->config.dither =
> -		adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC;
> -
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
>  		case INTEL_OUTPUT_LVDS:
> @@ -5656,10 +5516,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	intel_crtc_update_cursor(crtc, true);
>  
>  	/* determine panel color depth */
> -	dither = intel_choose_pipe_bpp_dither(crtc, fb,
> -					      &intel_crtc->config.pipe_bpp,
> -					      adjusted_mode);
> -	intel_crtc->config.dither = dither;
> +	dither = intel_crtc->config.dither;
>  	if (is_lvds && dev_priv->lvds_dither)
>  		dither = true;
>  
> @@ -5829,10 +5686,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	intel_crtc_update_cursor(crtc, true);
>  
>  	/* determine panel color depth */
> -	dither = intel_choose_pipe_bpp_dither(crtc, fb,
> -					      &intel_crtc->config.pipe_bpp,
> -					      adjusted_mode);
> -	intel_crtc->config.dither = dither;
> +	dither = intel_crtc->config.dither;
>  
>  	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
>  	drm_mode_debug_printmodeline(mode);
> @@ -7497,14 +7351,72 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
>  	}
>  }
>  
> +static int
> +pipe_config_set_bpp(struct drm_crtc *crtc,
> +		    struct drm_framebuffer *fb,
> +		    struct intel_crtc_config *pipe_config)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_connector *connector;
> +	int bpp;
> +
> +	switch (fb->depth) {
> +	case 8:
> +		bpp = 8*3; /* since we go through a colormap */
> +		break;
> +	case 15:
> +	case 16:
> +		bpp = 6*3; /* min is 18bpp */
> +		break;
> +	case 24:
> +		bpp = 8*3;
> +		break;
> +	case 30:
> +		bpp = 10*3;
> +		break;
> +	case 48:
> +		bpp = 12*3;
> +		break;
> +	default:
> +		DRM_DEBUG_KMS("unsupported depth\n");
> +		return -EINVAL;
> +	}
> +
> +	if (fb->depth > 24 && !HAS_PCH_SPLIT(dev)) {
> +		DRM_DEBUG_KMS("high depth not supported on gmch platforms\n");
> +		return -EINVAL;
> +	}
> +
> +	pipe_config->pipe_bpp = bpp;
> +
> +	/* Clamp display bpp to EDID value */
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +			    head) {
> +		if (connector->encoder && connector->encoder->crtc != crtc)
> +			continue;
> +
> +		/* Don't use an invalid EDID bpc value */
> +		if (connector->display_info.bpc &&
> +		    connector->display_info.bpc * 3 < bpp) {
> +			DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID reported max of %d\n",
> +				      bpp, connector->display_info.bpc*3);
> +			pipe_config->pipe_bpp = connector->display_info.bpc*3;
> +		}
> +	}
> +
> +	return bpp;
> +}
> +
>  static struct intel_crtc_config *
>  intel_modeset_pipe_config(struct drm_crtc *crtc,
> +			  struct drm_framebuffer *fb,
>  			  struct drm_display_mode *mode)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_encoder_helper_funcs *encoder_funcs;
>  	struct intel_encoder *encoder;
>  	struct intel_crtc_config *pipe_config;
> +	int plane_bpp;
>  
>  	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
>  	if (!pipe_config)
> @@ -7513,6 +7425,10 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	drm_mode_copy(&pipe_config->adjusted_mode, mode);
>  	drm_mode_copy(&pipe_config->requested_mode, mode);
>  
> +	plane_bpp = pipe_config_set_bpp(crtc, fb, pipe_config);
> +	if (plane_bpp < 0)
> +		goto fail;
> +
>  	/* Pass our mode to the connectors and the CRTC to give them a chance to
>  	 * adjust it according to limitations or connector properties, and also
>  	 * a chance to reject the mode entirely.
> @@ -7541,12 +7457,20 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  		}
>  	}
>  
> +	/* temporary hack until the DP code doesn't use the 6BPC flag any more */
> +	if (pipe_config->adjusted_mode.private_flags & INTEL_MODE_DP_FORCE_6BPC)
> +		pipe_config->pipe_bpp = 6*8;
> +
>  	if (!(intel_crtc_compute_config(crtc, pipe_config))) {
>  		DRM_DEBUG_KMS("CRTC fixup failed\n");
>  		goto fail;
>  	}
>  	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
>  
> +	pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
> +	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
> +		      plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
> +
>  	return pipe_config;
>  fail:
>  	kfree(pipe_config);
> @@ -7838,7 +7762,7 @@ int intel_set_mode(struct drm_crtc *crtc,
>  	 * pieces of code that are not yet converted to deal with mutliple crtcs
>  	 * changing their mode at the same time. */
>  	if (modeset_pipes) {
> -		pipe_config = intel_modeset_pipe_config(crtc, mode);
> +		pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
>  		if (IS_ERR(pipe_config)) {
>  			ret = PTR_ERR(pipe_config);
>  			pipe_config = NULL;
> @@ -8277,8 +8201,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
>  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>  
> -	intel_crtc->config.pipe_bpp = 24; /* default for pre-Ironlake */
> -
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 0731ba6..b206a0d 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -791,6 +791,19 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev))
>  		pipe_config->has_pch_encoder = true;
>  
> +	/*
> +	 * HDMI is either 12 or 8, so if the display lets 10bpc sneak
> +	 * through, clamp it down. Note that g4x/vlv don't support 12bpc hdmi
> +	 * outputs.
> +	 */
> +	if (pipe_config->pipe_bpp > 8*3 && HAS_PCH_SPLIT(dev)) {
> +		DRM_DEBUG_KMS("forcing bpc to 12 for HDMI\n");
> +		pipe_config->pipe_bpp = 12*3;
> +	} else {
> +		DRM_DEBUG_KMS("forcing bpc to 8 for HDMI\n");
> +		pipe_config->pipe_bpp = 8*3;
> +	}
> +
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 9d6ed91..7b6d07b 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -310,6 +310,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  	struct drm_display_mode *mode = &pipe_config->requested_mode;
>  	struct intel_crtc *intel_crtc = lvds_encoder->base.new_crtc;
>  	u32 pfit_control = 0, pfit_pgm_ratios = 0, border = 0;
> +	unsigned int lvds_bpp;
>  	int pipe;
>  
>  	/* Should never happen!! */
> @@ -321,6 +322,17 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  	if (intel_encoder_check_is_cloned(&lvds_encoder->base))
>  		return false;
>  
> +	if ((I915_READ(lvds_encoder->reg) & LVDS_A3_POWER_MASK) ==
> +	    LVDS_A3_POWER_UP)
> +		lvds_bpp = 8*3;
> +	else
> +		lvds_bpp = 6*3;
> +
> +	if (lvds_bpp != pipe_config->pipe_bpp) {
> +		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
> +			      pipe_config->pipe_bpp, lvds_bpp);
> +		pipe_config->pipe_bpp = lvds_bpp;
> +	}
>  	/*
>  	 * We have timings from the BIOS for the panel, put them in
>  	 * to the adjusted mode.  The CRTC will be set up for this mode,

This looks good and seems to cover the bugs we've had here before.  My
only concern is the one I mentioned before: on older chipsets we could
dither between plane, pipe, *and* port.  Nowadays the pipe always does
it.

So in the old LVDS case it would be cool if someone could test the
difference.  The LVDS port may do a better job on 6bpc panels than the
pipe...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 09/13] drm/i915: precompute pipe bpp before touching the hw

[Daniel Vetter]

On Wed, Mar 27, 2013 at 6:24 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> This looks good and seems to cover the bugs we've had here before.  My
> only concern is the one I mentioned before: on older chipsets we could
> dither between plane, pipe, *and* port.  Nowadays the pipe always does
> it.
>
> So in the old LVDS case it would be cool if someone could test the
> difference.  The LVDS port may do a better job on 6bpc panels than the
> pipe...

I've considered this again, and it should fit neatly into the existing
framework. If we want to use the dither on the lvds port on those
platforms, but keep dithering on the pipe for e.g. DP we could switch
lvds_compute_config to not clamp the bpp and then just enable the port
dithering if config.pipe_bpp != 18.

There's some further patches which unify this a bit, I guess we can
discuss this a bit more once I resend them.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 10/13] drm/i915: convert DP autodither code to new infrastructure

[Daniel Vetter]

The old code only handled either 6bpc or 8bpc. Since it's easy to do,
reorganize the code to be a bit more generic so that it can also handle
10bpc and 12bpc. Note that we still start with 8bpc, so there's no
functional change.

Also, since we no don't need to compute the 6BPC flag in the mode_valid
callback, we can consolidate things a bit. That requires though that
the link bw computation is moved up in the compute_config callback.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |   4 --
 drivers/gpu/drm/i915/intel_dp.c      | 103 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |   3 -
 3 files changed, 47 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6a60bf2..51557ba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7457,10 +7457,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 		}
 	}
 
-	/* temporary hack until the DP code doesn't use the 6BPC flag any more */
-	if (pipe_config->adjusted_mode.private_flags & INTEL_MODE_DP_FORCE_6BPC)
-		pipe_config->pipe_bpp = 6*8;
-
 	if (!(intel_crtc_compute_config(crtc, pipe_config))) {
 		DRM_DEBUG_KMS("CRTC fixup failed\n");
 		goto fail;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 34006f7..251aa6b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -177,34 +177,6 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
 	return (max_link_clock * max_lanes * 8) / 10;
 }
 
-static bool
-intel_dp_adjust_dithering(struct intel_dp *intel_dp,
-			  struct drm_display_mode *mode,
-			  bool adjust_mode)
-{
-	int max_link_clock =
-		drm_dp_bw_code_to_link_rate(intel_dp_max_link_bw(intel_dp));
-	int max_lanes = drm_dp_max_lane_count(intel_dp->dpcd);
-	int max_rate, mode_rate;
-
-	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 false;
-
-		if (adjust_mode)
-			mode->private_flags
-				|= INTEL_MODE_DP_FORCE_6BPC;
-
-		return true;
-	}
-
-	return true;
-}
-
 static int
 intel_dp_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
@@ -212,6 +184,8 @@ intel_dp_mode_valid(struct drm_connector *connector,
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
+	int target_clock = mode->clock;
+	int max_rate, mode_rate, max_lanes, max_link_clock;
 
 	if (is_edp(intel_dp) && fixed_mode) {
 		if (mode->hdisplay > fixed_mode->hdisplay)
@@ -221,7 +195,13 @@ intel_dp_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 	}
 
-	if (!intel_dp_adjust_dithering(intel_dp, mode, false))
+	max_link_clock = drm_dp_bw_code_to_link_rate(intel_dp_max_link_bw(intel_dp));
+	max_lanes = drm_dp_max_lane_count(intel_dp->dpcd);
+
+	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
+	mode_rate = intel_dp_link_required(target_clock, 18);
+
+	if (mode_rate > max_rate)
 		return MODE_CLOCK_HIGH;
 
 	if (mode->clock < 10000)
@@ -693,6 +673,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 	struct drm_display_mode *mode = &pipe_config->requested_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
@@ -702,6 +683,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
 	int bpp, mode_rate;
 	static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
+	int target_clock, link_avail, link_clock;
 
 	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && !is_cpu_edp(intel_dp))
 		pipe_config->has_pch_encoder = true;
@@ -713,6 +695,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 					intel_connector->panel.fitting_mode,
 					mode, adjusted_mode);
 	}
+	/* We need to take the panel's fixed mode into account. */
+	target_clock = adjusted_mode->clock;
 
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
 		return false;
@@ -721,11 +705,31 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 		      "max bw %02x pixel clock %iKHz\n",
 		      max_lane_count, bws[max_clock], adjusted_mode->clock);
 
-	if (!intel_dp_adjust_dithering(intel_dp, adjusted_mode, true))
-		return false;
+	/* Walk through all bpp values. Luckily they're all nicely spaced with 2
+	 * bpc in between. */
+	bpp = 8*3;
+	if (is_edp(intel_dp) && dev_priv->edp.bpp)
+		bpp = min_t(int, bpp, dev_priv->edp.bpp);
+
+	for (; bpp >= 6*3; bpp -= 2*3) {
+		mode_rate = intel_dp_link_required(target_clock, bpp);
+
+		for (clock = 0; clock <= max_clock; clock++) {
+			for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
+				link_clock = drm_dp_bw_code_to_link_rate(bws[clock]);
+				link_avail = intel_dp_max_data_rate(link_clock,
+								    lane_count);
+
+				if (mode_rate <= link_avail) {
+					goto found;
+				}
+			}
+		}
+	}
 
-	bpp = adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24;
+	return false;
 
+found:
 	if (intel_dp->color_range_auto) {
 		/*
 		 * See:
@@ -741,31 +745,18 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	if (intel_dp->color_range)
 		pipe_config->limited_color_range = true;
 
-	mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
-
-	for (clock = 0; clock <= max_clock; clock++) {
-		for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
-			int link_bw_clock =
-				drm_dp_bw_code_to_link_rate(bws[clock]);
-			int link_avail = intel_dp_max_data_rate(link_bw_clock,
-								lane_count);
-
-			if (mode_rate <= link_avail) {
-				intel_dp->link_bw = bws[clock];
-				intel_dp->lane_count = lane_count;
-				adjusted_mode->clock = link_bw_clock;
-				DRM_DEBUG_KMS("DP link bw %02x lane "
-						"count %d clock %d bpp %d\n",
-				       intel_dp->link_bw, intel_dp->lane_count,
-				       adjusted_mode->clock, bpp);
-				DRM_DEBUG_KMS("DP link bw required %i available %i\n",
-					      mode_rate, link_avail);
-				return true;
-			}
-		}
-	}
+	intel_dp->link_bw = bws[clock];
+	intel_dp->lane_count = lane_count;
+	adjusted_mode->clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
+	pipe_config->pipe_bpp = bpp;
 
-	return false;
+	DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
+		      intel_dp->link_bw, intel_dp->lane_count,
+		      adjusted_mode->clock, bpp);
+	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
+		      mode_rate, link_avail);
+
+	return true;
 }
 
 void
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0ca0d76..5c7b04b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -101,9 +101,6 @@
 #define INTEL_DVO_CHIP_TMDS 2
 #define INTEL_DVO_CHIP_TVOUT 4
 
-/* drm_display_mode->private_flags */
-#define INTEL_MODE_DP_FORCE_6BPC (0x10)
-
 struct intel_framebuffer {
 	struct drm_framebuffer base;
 	struct drm_i915_gem_object *obj;
-- 
1.7.11.7

Re: [PATCH 10/13] drm/i915: convert DP autodither code to new infrastructure

[Jesse Barnes]

On Wed, 27 Mar 2013 00:44:59 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> The old code only handled either 6bpc or 8bpc. Since it's easy to do,
> reorganize the code to be a bit more generic so that it can also handle
> 10bpc and 12bpc. Note that we still start with 8bpc, so there's no
> functional change.
> 
> Also, since we no don't need to compute the 6BPC flag in the mode_valid
> callback, we can consolidate things a bit. That requires though that
> the link bw computation is moved up in the compute_config callback.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   4 --
>  drivers/gpu/drm/i915/intel_dp.c      | 103 ++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   3 -
>  3 files changed, 47 insertions(+), 63 deletions(-)
> 

I had a harder time following this one (DP is just complex), but it
looks ok so far.  I'll feel better with lots of testing.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 11/13] drm/i915: clean up plane bpp confusion

[Daniel Vetter]

- There is no 16bpc linear color format in our hw. gen4+ has a 16 bpc
  float layout, but we don't really support it.
- 10bpc is a gen4+ feature, fix up the support for it.
- Update_plane should never see a wrong fb bpp value, BUG in the
  corresponding cases.

v2: Rebase on top of Ville's plane pixel layout changes.

v3: Actually drop the old gen4 check for 10bpc planes, spotted
by Ville Syrjälä.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 51557ba..bbf31aa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 		dspcntr |= DISPPLANE_RGBX101010;
 		break;
 	default:
-		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
-		return -EINVAL;
+		BUG();
 	}
 
 	if (INTEL_INFO(dev)->gen >= 4) {
@@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
 		dspcntr |= DISPPLANE_RGBX101010;
 		break;
 	default:
-		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
-		return -EINVAL;
+		BUG();
 	}
 
 	if (obj->tiling_mode != I915_TILING_NONE)
@@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
 		bpp = 8*3;
 		break;
 	case 30:
+		if (INTEL_INFO(dev)->gen < 4) {
+			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
+			return -EINVAL;
+		}
+
 		bpp = 10*3;
 		break;
-	case 48:
-		bpp = 12*3;
-		break;
+	/* TODO: gen4+ supports 16 bpc floating point, too. */
 	default:
 		DRM_DEBUG_KMS("unsupported depth\n");
 		return -EINVAL;
 	}
 
-	if (fb->depth > 24 && !HAS_PCH_SPLIT(dev)) {
-		DRM_DEBUG_KMS("high depth not supported on gmch platforms\n");
-		return -EINVAL;
-	}
-
 	pipe_config->pipe_bpp = bpp;
 
 	/* Clamp display bpp to EDID value */
-- 
1.7.11.7

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

Re: [PATCH 11/13] drm/i915: clean up plane bpp confusion

[Jesse Barnes]

On Wed, 27 Mar 2013 00:45:00 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> - There is no 16bpc linear color format in our hw. gen4+ has a 16 bpc
>   float layout, but we don't really support it.
> - 10bpc is a gen4+ feature, fix up the support for it.
> - Update_plane should never see a wrong fb bpp value, BUG in the
>   corresponding cases.
> 
> v2: Rebase on top of Ville's plane pixel layout changes.
> 
> v3: Actually drop the old gen4 check for 10bpc planes, spotted
> by Ville Syrjälä.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 51557ba..bbf31aa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  		dspcntr |= DISPPLANE_RGBX101010;
>  		break;
>  	default:
> -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> -		return -EINVAL;
> +		BUG();
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 4) {
> @@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
>  		dspcntr |= DISPPLANE_RGBX101010;
>  		break;
>  	default:
> -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> -		return -EINVAL;
> +		BUG();
>  	}
>  
>  	if (obj->tiling_mode != I915_TILING_NONE)
> @@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
>  		bpp = 8*3;
>  		break;
>  	case 30:
> +		if (INTEL_INFO(dev)->gen < 4) {
> +			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
> +			return -EINVAL;
> +		}
> +
>  		bpp = 10*3;
>  		break;
> -	case 48:
> -		bpp = 12*3;
> -		break;
> +	/* TODO: gen4+ supports 16 bpc floating point, too. */
>  	default:
>  		DRM_DEBUG_KMS("unsupported depth\n");
>  		return -EINVAL;
>  	}
>  
> -	if (fb->depth > 24 && !HAS_PCH_SPLIT(dev)) {
> -		DRM_DEBUG_KMS("high depth not supported on gmch platforms\n");
> -		return -EINVAL;
> -	}
> -
>  	pipe_config->pipe_bpp = bpp;
>  
>  	/* Clamp display bpp to EDID value */

You don't want to squash this into 8/13?  It looks ok.

Sorry about the 48; it's 16:16:16:16 ignoring alpha, so you end up with
48bpp and my backwards calc for bpc ignored alpha again and ended up at
12. :)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/13] drm/i915: clean up plane bpp confusion

[Ville Syrjälä]

On Wed, Mar 27, 2013 at 12:45:00AM +0100, Daniel Vetter wrote:
> - Update_plane should never see a wrong fb bpp value, BUG in the
>   corresponding cases.

That's not true actually. For sprites the common drm code already
checks the format against the list provided by the driver, but for
primary planes there's no such check. The check in
intel_framebuffer_init() isn't enough since it'll also accept
formats that are supported by sprites but not by the primary planes.

> 
> v2: Rebase on top of Ville's plane pixel layout changes.
> 
> v3: Actually drop the old gen4 check for 10bpc planes, spotted
> by Ville Syrjälä.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 51557ba..bbf31aa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  		dspcntr |= DISPPLANE_RGBX101010;
>  		break;
>  	default:
> -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> -		return -EINVAL;
> +		BUG();
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 4) {
> @@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
>  		dspcntr |= DISPPLANE_RGBX101010;
>  		break;
>  	default:
> -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> -		return -EINVAL;
> +		BUG();
>  	}
>  
>  	if (obj->tiling_mode != I915_TILING_NONE)
> @@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
>  		bpp = 8*3;
>  		break;
>  	case 30:
> +		if (INTEL_INFO(dev)->gen < 4) {
> +			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
> +			return -EINVAL;
> +		}
> +
>  		bpp = 10*3;
>  		break;
> -	case 48:
> -		bpp = 12*3;
> -		break;
> +	/* TODO: gen4+ supports 16 bpc floating point, too. */
>  	default:
>  		DRM_DEBUG_KMS("unsupported depth\n");
>  		return -EINVAL;
>  	}
>  
> -	if (fb->depth > 24 && !HAS_PCH_SPLIT(dev)) {
> -		DRM_DEBUG_KMS("high depth not supported on gmch platforms\n");
> -		return -EINVAL;
> -	}
> -
>  	pipe_config->pipe_bpp = bpp;
>  
>  	/* Clamp display bpp to EDID value */
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 11/13] drm/i915: clean up plane bpp confusion

[Daniel Vetter]

On Thu, Mar 28, 2013 at 01:26:28PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 27, 2013 at 12:45:00AM +0100, Daniel Vetter wrote:
> > - Update_plane should never see a wrong fb bpp value, BUG in the
> >   corresponding cases.
> 
> That's not true actually. For sprites the common drm code already
> checks the format against the list provided by the driver, but for
> primary planes there's no such check. The check in
> intel_framebuffer_init() isn't enough since it'll also accept
> formats that are supported by sprites but not by the primary planes.

With the new pipe_config step we check plane bpp in the new compute config
stage, so before we start touching the hw. Which means by the time we
reach the various *_update_plane functions, we shouldn't see an
unsupported pixel format any more.

There's the shortpath in the setcrtc ioctl implementation which goes
directly to set_base, but that one checks whether the bits_per_pixel of
the fb matches. I guess we should switch that one over to
fb->pixel_format, but besides that nit I think we really should be
covered, and the below default cases are indeed BUGs.

Or have I missed a place somewhere?

I'll follow up with the pixel_format patch.

Cheers, Daniel
> 
> > 
> > v2: Rebase on top of Ville's plane pixel layout changes.
> > 
> > v3: Actually drop the old gen4 check for 10bpc planes, spotted
> > by Ville Syrjälä.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++------------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 51557ba..bbf31aa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> >  		dspcntr |= DISPPLANE_RGBX101010;
> >  		break;
> >  	default:
> > -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> > -		return -EINVAL;
> > +		BUG();
> >  	}
> >  
> >  	if (INTEL_INFO(dev)->gen >= 4) {
> > @@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
> >  		dspcntr |= DISPPLANE_RGBX101010;
> >  		break;
> >  	default:
> > -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> > -		return -EINVAL;
> > +		BUG();
> >  	}
> >  
> >  	if (obj->tiling_mode != I915_TILING_NONE)
> > @@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
> >  		bpp = 8*3;
> >  		break;
> >  	case 30:
> > +		if (INTEL_INFO(dev)->gen < 4) {
> > +			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
> > +			return -EINVAL;
> > +		}
> > +
> >  		bpp = 10*3;
> >  		break;
> > -	case 48:
> > -		bpp = 12*3;
> > -		break;
> > +	/* TODO: gen4+ supports 16 bpc floating point, too. */
> >  	default:
> >  		DRM_DEBUG_KMS("unsupported depth\n");
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (fb->depth > 24 && !HAS_PCH_SPLIT(dev)) {
> > -		DRM_DEBUG_KMS("high depth not supported on gmch platforms\n");
> > -		return -EINVAL;
> > -	}
> > -
> >  	pipe_config->pipe_bpp = bpp;
> >  
> >  	/* Clamp display bpp to EDID value */
> > -- 
> > 1.7.11.7
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 11/13] drm/i915: clean up plane bpp confusion

[Ville Syrjälä]

On Thu, Mar 28, 2013 at 12:46:42PM +0100, Daniel Vetter wrote:
> On Thu, Mar 28, 2013 at 01:26:28PM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 27, 2013 at 12:45:00AM +0100, Daniel Vetter wrote:
> > > - Update_plane should never see a wrong fb bpp value, BUG in the
> > >   corresponding cases.
> > 
> > That's not true actually. For sprites the common drm code already
> > checks the format against the list provided by the driver, but for
> > primary planes there's no such check. The check in
> > intel_framebuffer_init() isn't enough since it'll also accept
> > formats that are supported by sprites but not by the primary planes.
> 
> With the new pipe_config step we check plane bpp in the new compute config
> stage, so before we start touching the hw. Which means by the time we
> reach the various *_update_plane functions, we shouldn't see an
> unsupported pixel format any more.

Do you mean the fb->depth check in pipe_config_set_bpp()? That's not
enough. It doesn't have any gen checks, so it could very well let some
unsupported format through, even though the depth/bpp might match. Or
did I miss some more thorough check somewhere?

I've been pondering if I should just propose removing depth/bpp from
drm_framebuffer altogether to make it harder to write code that doesn't
do proper format checks...

> There's the shortpath in the setcrtc ioctl implementation which goes
> directly to set_base, but that one checks whether the bits_per_pixel of
> the fb matches. I guess we should switch that one over to
> fb->pixel_format, but besides that nit I think we really should be
> covered, and the below default cases are indeed BUGs.
> 
> Or have I missed a place somewhere?
> 
> I'll follow up with the pixel_format patch.

Perhaps some common func that you can call early in both set_base
and full modeset paths?

> 
> Cheers, Daniel
> > 
> > > 
> > > v2: Rebase on top of Ville's plane pixel layout changes.
> > > 
> > > v3: Actually drop the old gen4 check for 10bpc planes, spotted
> > > by Ville Syrjälä.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++------------
> > >  1 file changed, 8 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 51557ba..bbf31aa 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > >  		dspcntr |= DISPPLANE_RGBX101010;
> > >  		break;
> > >  	default:
> > > -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> > > -		return -EINVAL;
> > > +		BUG();
> > >  	}
> > >  
> > >  	if (INTEL_INFO(dev)->gen >= 4) {
> > > @@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
> > >  		dspcntr |= DISPPLANE_RGBX101010;
> > >  		break;
> > >  	default:
> > > -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> > > -		return -EINVAL;
> > > +		BUG();
> > >  	}
> > >  
> > >  	if (obj->tiling_mode != I915_TILING_NONE)
> > > @@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
> > >  		bpp = 8*3;
> > >  		break;
> > >  	case 30:
> > > +		if (INTEL_INFO(dev)->gen < 4) {
> > > +			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
> > > +			return -EINVAL;
> > > +		}
> > > +
> > >  		bpp = 10*3;
> > >  		break;
> > > -	case 48:
> > > -		bpp = 12*3;
> > > -		break;
> > > +	/* TODO: gen4+ supports 16 bpc floating point, too. */
> > >  	default:
> > >  		DRM_DEBUG_KMS("unsupported depth\n");
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	if (fb->depth > 24 && !HAS_PCH_SPLIT(dev)) {
> > > -		DRM_DEBUG_KMS("high depth not supported on gmch platforms\n");
> > > -		return -EINVAL;
> > > -	}
> > > -
> > >  	pipe_config->pipe_bpp = bpp;
> > >  
> > >  	/* Clamp display bpp to EDID value */
> > > -- 
> > > 1.7.11.7
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

[PATCH 1/2] drm/i915: check fb->pixel_format instead of bits_per_pixel

[Daniel Vetter]

We've mostly switched over to the new more flexible schema, but
there's one check left in the modeset code.

Motivated by a question from Ville whether there's really no way an
unsupported pixel_format can escape into our platform update_plane
callbacks.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 84bd8d3..8cbb65c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7956,8 +7956,8 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 			config->mode_changed = true;
 		} else if (set->fb->depth != set->crtc->fb->depth) {
 			config->mode_changed = true;
-		} else if (set->fb->bits_per_pixel !=
-			   set->crtc->fb->bits_per_pixel) {
+		} else if (set->fb->pixel_format !=
+			   set->crtc->fb->pixel_format) {
 			config->mode_changed = true;
 		} else
 			config->fb_changed = true;
-- 
1.7.10.4

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

[PATCH 2/2] drm/i915: fixup fb bpp computation in pipe_config_set_bpp

[Daniel Vetter]

Ville pointed out that my assumption that no unsupported pixel format
can get past the pipe config computation stage to the platform
update_plane callbacks is wrong. The reason is that this function
still checks the old fb->depth value instead of the new pixel_format.

While checking with all the other places that use this I've noticed
that intel_framebuffer_init already has all the platform checks we
need, so replace those checks with a WARN_ON.

Since fb->depth isn't set for YUV pixel formats and since we already
can't create an fb with an rgb layout not support on the running
platform I /think/ this patch doesn't fix any bug.

But it surely looks better!

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |   28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8cbb65c..86130a2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7394,23 +7394,31 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
 	struct drm_connector *connector;
 	int bpp;
 
-	switch (fb->depth) {
-	case 8:
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_C8:
 		bpp = 8*3; /* since we go through a colormap */
 		break;
-	case 15:
-	case 16:
+	case DRM_FORMAT_XRGB1555:
+	case DRM_FORMAT_ARGB1555:
+		/* checked in intel_framebuffer_init already */
+		if (WARN_ON(INTEL_INFO(dev)->gen > 3))
+			return -EINVAL;
+	case DRM_FORMAT_RGB565:
 		bpp = 6*3; /* min is 18bpp */
 		break;
-	case 24:
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ARGB8888:
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_ABGR8888:
 		bpp = 8*3;
 		break;
-	case 30:
-		if (INTEL_INFO(dev)->gen < 4) {
-			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
+	case DRM_FORMAT_XRGB2101010:
+	case DRM_FORMAT_ARGB2101010:
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_ABGR2101010:
+		/* checked in intel_framebuffer_init already */
+		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
 			return -EINVAL;
-		}
-
 		bpp = 10*3;
 		break;
 	/* TODO: gen4+ supports 16 bpc floating point, too. */
-- 
1.7.10.4

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

Re: [PATCH 2/2] drm/i915: fixup fb bpp computation in pipe_config_set_bpp

[Ville Syrjälä]

On Thu, Mar 28, 2013 at 01:49:27PM +0100, Daniel Vetter wrote:
> Ville pointed out that my assumption that no unsupported pixel format
> can get past the pipe config computation stage to the platform
> update_plane callbacks is wrong. The reason is that this function
> still checks the old fb->depth value instead of the new pixel_format.
> 
> While checking with all the other places that use this I've noticed
> that intel_framebuffer_init already has all the platform checks we
> need, so replace those checks with a WARN_ON.
> 
> Since fb->depth isn't set for YUV pixel formats and since we already
> can't create an fb with an rgb layout not support on the running
> platform I /think/ this patch doesn't fix any bug.

Yeah I tried to go over the possible combinations of supported
formats and came to the same conclusion. The bpp=0 thing for YUV
saved us, and otherwise the sprite RGB formats always seem to be
a subset of the primary plane formats. At least it's now safe
against someone getting the urge to populate depth/bpp for YUV
formats.

> But it surely looks better!
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> ---
>  drivers/gpu/drm/i915/intel_display.c |   28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8cbb65c..86130a2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7394,23 +7394,31 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
>  	struct drm_connector *connector;
>  	int bpp;
>  
> -	switch (fb->depth) {
> -	case 8:
> +	switch (fb->pixel_format) {
> +	case DRM_FORMAT_C8:
>  		bpp = 8*3; /* since we go through a colormap */
>  		break;
> -	case 15:
> -	case 16:
> +	case DRM_FORMAT_XRGB1555:
> +	case DRM_FORMAT_ARGB1555:
> +		/* checked in intel_framebuffer_init already */
> +		if (WARN_ON(INTEL_INFO(dev)->gen > 3))
> +			return -EINVAL;
> +	case DRM_FORMAT_RGB565:
>  		bpp = 6*3; /* min is 18bpp */
>  		break;
> -	case 24:
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ARGB8888:
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_ABGR8888:


If you want to be entirely pedantic XBGR8888/ABGR8888 should also have
the 'gen < 4' check.

>  		bpp = 8*3;
>  		break;
> -	case 30:
> -		if (INTEL_INFO(dev)->gen < 4) {
> -			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
> +	case DRM_FORMAT_XRGB2101010:
> +	case DRM_FORMAT_ARGB2101010:
> +	case DRM_FORMAT_XBGR2101010:
> +	case DRM_FORMAT_ABGR2101010:
> +		/* checked in intel_framebuffer_init already */
> +		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
>  			return -EINVAL;
> -		}
> -
>  		bpp = 10*3;
>  		break;
>  	/* TODO: gen4+ supports 16 bpc floating point, too. */
> -- 
> 1.7.10.4

-- 
Ville Syrjälä
Intel OTC

[PATCH] drm/i915: remove "inline" keyword from ironlake_disable_display_irq

[Daniel Vetter]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

 - It's a static function
 - I just added a few more users to it
 - Its sister ironlake_enable_display_irq is not marked as inline
 - The compiler will still inline if it thinks it should do

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 425b192..4c5bdd0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -101,7 +101,7 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 	}
 }
 
-static inline void
+static void
 ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	if ((dev_priv->irq_mask & mask) != mask) {
-- 
1.7.10.4

[PATCH] drm/i915: fixup fb bpp computation in pipe_config_set_bpp

[Daniel Vetter]

Ville pointed out that my assumption that no unsupported pixel format
can get past the pipe config computation stage to the platform
update_plane callbacks is wrong. The reason is that this function
still checks the old fb->depth value instead of the new pixel_format.

While checking with all the other places that use this I've noticed
that intel_framebuffer_init already has all the platform checks we
need, so replace those checks with a WARN_ON.

Since fb->depth isn't set for YUV pixel formats and since we already
can't create an fb with an rgb layout not support on the running
platform I /think/ this patch doesn't fix any bug.

But it surely looks better!

v2: BGR formats are also only gen4+, so add the corresponding WARN_ON,
too (Ville).

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3f3a3dc..5e8b91f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7394,23 +7394,34 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
 	struct drm_connector *connector;
 	int bpp;
 
-	switch (fb->depth) {
-	case 8:
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_C8:
 		bpp = 8*3; /* since we go through a colormap */
 		break;
-	case 15:
-	case 16:
+	case DRM_FORMAT_XRGB1555:
+	case DRM_FORMAT_ARGB1555:
+		/* checked in intel_framebuffer_init already */
+		if (WARN_ON(INTEL_INFO(dev)->gen > 3))
+			return -EINVAL;
+	case DRM_FORMAT_RGB565:
 		bpp = 6*3; /* min is 18bpp */
 		break;
-	case 24:
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_ABGR8888:
+		/* checked in intel_framebuffer_init already */
+		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
+			return -EINVAL;
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ARGB8888:
 		bpp = 8*3;
 		break;
-	case 30:
-		if (INTEL_INFO(dev)->gen < 4) {
-			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
+	case DRM_FORMAT_XRGB2101010:
+	case DRM_FORMAT_ARGB2101010:
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_ABGR2101010:
+		/* checked in intel_framebuffer_init already */
+		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
 			return -EINVAL;
-		}
-
 		bpp = 10*3;
 		break;
 	/* TODO: gen4+ supports 16 bpc floating point, too. */
-- 
1.7.10.4

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

Re: [PATCH] drm/i915: fixup fb bpp computation in pipe_config_set_bpp

[Ville Syrjälä]

On Thu, Mar 28, 2013 at 04:38:08PM +0100, Daniel Vetter wrote:
> Ville pointed out that my assumption that no unsupported pixel format
> can get past the pipe config computation stage to the platform
> update_plane callbacks is wrong. The reason is that this function
> still checks the old fb->depth value instead of the new pixel_format.
> 
> While checking with all the other places that use this I've noticed
> that intel_framebuffer_init already has all the platform checks we
> need, so replace those checks with a WARN_ON.
> 
> Since fb->depth isn't set for YUV pixel formats and since we already
> can't create an fb with an rgb layout not support on the running
> platform I /think/ this patch doesn't fix any bug.
> 
> But it surely looks better!
> 
> v2: BGR formats are also only gen4+, so add the corresponding WARN_ON,
> too (Ville).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3f3a3dc..5e8b91f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7394,23 +7394,34 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
>  	struct drm_connector *connector;
>  	int bpp;
>  
> -	switch (fb->depth) {
> -	case 8:
> +	switch (fb->pixel_format) {
> +	case DRM_FORMAT_C8:
>  		bpp = 8*3; /* since we go through a colormap */
>  		break;
> -	case 15:
> -	case 16:
> +	case DRM_FORMAT_XRGB1555:
> +	case DRM_FORMAT_ARGB1555:
> +		/* checked in intel_framebuffer_init already */
> +		if (WARN_ON(INTEL_INFO(dev)->gen > 3))
> +			return -EINVAL;
> +	case DRM_FORMAT_RGB565:
>  		bpp = 6*3; /* min is 18bpp */
>  		break;
> -	case 24:
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_ABGR8888:
> +		/* checked in intel_framebuffer_init already */
> +		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
> +			return -EINVAL;
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ARGB8888:
>  		bpp = 8*3;
>  		break;
> -	case 30:
> -		if (INTEL_INFO(dev)->gen < 4) {
> -			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
> +	case DRM_FORMAT_XRGB2101010:
> +	case DRM_FORMAT_ARGB2101010:
> +	case DRM_FORMAT_XBGR2101010:
> +	case DRM_FORMAT_ABGR2101010:
> +		/* checked in intel_framebuffer_init already */
> +		if (WARN_ON(INTEL_INFO(dev)->gen < 4))
>  			return -EINVAL;
> -		}
> -
>  		bpp = 10*3;
>  		break;
>  	/* TODO: gen4+ supports 16 bpc floating point, too. */
> -- 
> 1.7.10.4

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: fixup fb bpp computation in pipe_config_set_bpp

[Daniel Vetter]

On Thu, Mar 28, 2013 at 05:45:06PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 28, 2013 at 04:38:08PM +0100, Daniel Vetter wrote:
> > Ville pointed out that my assumption that no unsupported pixel format
> > can get past the pipe config computation stage to the platform
> > update_plane callbacks is wrong. The reason is that this function
> > still checks the old fb->depth value instead of the new pixel_format.
> > 
> > While checking with all the other places that use this I've noticed
> > that intel_framebuffer_init already has all the platform checks we
> > need, so replace those checks with a WARN_ON.
> > 
> > Since fb->depth isn't set for YUV pixel formats and since we already
> > can't create an fb with an rgb layout not support on the running
> > platform I /think/ this patch doesn't fix any bug.
> > 
> > But it surely looks better!
> > 
> > v2: BGR formats are also only gen4+, so add the corresponding WARN_ON,
> > too (Ville).
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Both patches merged to dinq, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: check fb->pixel_format instead of bits_per_pixel

[Ville Syrjälä]

On Thu, Mar 28, 2013 at 01:49:26PM +0100, Daniel Vetter wrote:
> We've mostly switched over to the new more flexible schema, but
> there's one check left in the modeset code.

Having to do a full modeset for an fb change kind of sucks, but
as long as we're choosing the pipe bpp based on the primary plane
format I guess we don't have much of a choice.

> 
> Motivated by a question from Ville whether there's really no way an
> unsupported pixel_format can escape into our platform update_plane
> callbacks.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 84bd8d3..8cbb65c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7956,8 +7956,8 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  			config->mode_changed = true;
>  		} else if (set->fb->depth != set->crtc->fb->depth) {
>  			config->mode_changed = true;

With the format check in place you can drop the depth check too.

> -		} else if (set->fb->bits_per_pixel !=
> -			   set->crtc->fb->bits_per_pixel) {
> +		} else if (set->fb->pixel_format !=
> +			   set->crtc->fb->pixel_format) {
>  			config->mode_changed = true;
>  		} else
>  			config->fb_changed = true;
> -- 
> 1.7.10.4

-- 
Ville Syrjälä
Intel OTC

[PATCH] drm/i915: check fb->pixel_format instead of bits_per_pixel

[Daniel Vetter]

We've mostly switched over to the new more flexible schema, but
there's one check left in the modeset code.

Motivated by a question from Ville whether there's really no way an
unsupported pixel_format can escape into our platform update_plane
callbacks.

v2: Ville noticed that the fb->depth check is redudant when we already
check fb->pixel_format.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 84bd8d3..3f3a3dc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7954,10 +7954,8 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 			config->mode_changed = true;
 		} else if (set->fb == NULL) {
 			config->mode_changed = true;
-		} else if (set->fb->depth != set->crtc->fb->depth) {
-			config->mode_changed = true;
-		} else if (set->fb->bits_per_pixel !=
-			   set->crtc->fb->bits_per_pixel) {
+		} else if (set->fb->pixel_format !=
+			   set->crtc->fb->pixel_format) {
 			config->mode_changed = true;
 		} else
 			config->fb_changed = true;
-- 
1.7.10.4

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

Re: [PATCH] drm/i915: check fb->pixel_format instead of bits_per_pixel

[Ville Syrjälä]

On Thu, Mar 28, 2013 at 04:01:35PM +0100, Daniel Vetter wrote:
> We've mostly switched over to the new more flexible schema, but
> there's one check left in the modeset code.
> 
> Motivated by a question from Ville whether there's really no way an
> unsupported pixel_format can escape into our platform update_plane
> callbacks.
> 
> v2: Ville noticed that the fb->depth check is redudant when we already
> check fb->pixel_format.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 84bd8d3..3f3a3dc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7954,10 +7954,8 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  			config->mode_changed = true;
>  		} else if (set->fb == NULL) {
>  			config->mode_changed = true;
> -		} else if (set->fb->depth != set->crtc->fb->depth) {
> -			config->mode_changed = true;
> -		} else if (set->fb->bits_per_pixel !=
> -			   set->crtc->fb->bits_per_pixel) {
> +		} else if (set->fb->pixel_format !=
> +			   set->crtc->fb->pixel_format) {
>  			config->mode_changed = true;
>  		} else
>  			config->fb_changed = true;
> -- 
> 1.7.10.4

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 11/13] drm/i915: clean up plane bpp confusion

[Daniel Vetter]

On Thu, Mar 28, 2013 at 01:59:59PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 28, 2013 at 12:46:42PM +0100, Daniel Vetter wrote:
> > On Thu, Mar 28, 2013 at 01:26:28PM +0200, Ville Syrjälä wrote:
> > > On Wed, Mar 27, 2013 at 12:45:00AM +0100, Daniel Vetter wrote:
> > > > - Update_plane should never see a wrong fb bpp value, BUG in the
> > > >   corresponding cases.
> > > 
> > > That's not true actually. For sprites the common drm code already
> > > checks the format against the list provided by the driver, but for
> > > primary planes there's no such check. The check in
> > > intel_framebuffer_init() isn't enough since it'll also accept
> > > formats that are supported by sprites but not by the primary planes.
> > 
> > With the new pipe_config step we check plane bpp in the new compute config
> > stage, so before we start touching the hw. Which means by the time we
> > reach the various *_update_plane functions, we shouldn't see an
> > unsupported pixel format any more.
> 
> Do you mean the fb->depth check in pipe_config_set_bpp()? That's not
> enough. It doesn't have any gen checks, so it could very well let some
> unsupported format through, even though the depth/bpp might match. Or
> did I miss some more thorough check somewhere?

Yeah, although on closer examination I think we get away since all the
missing checks are done by intel_framebuffer_init, and yuv doesn't have a
depth. Still checking pixel_format looks much better.

> I've been pondering if I should just propose removing depth/bpp from
> drm_framebuffer altogether to make it harder to write code that doesn't
> do proper format checks...

Yeah, I think we should aim for that, at least in our own driver codebase.

> > There's the shortpath in the setcrtc ioctl implementation which goes
> > directly to set_base, but that one checks whether the bits_per_pixel of
> > the fb matches. I guess we should switch that one over to
> > fb->pixel_format, but besides that nit I think we really should be
> > covered, and the below default cases are indeed BUGs.
> > 
> > Or have I missed a place somewhere?
> > 
> > I'll follow up with the pixel_format patch.
> 
> Perhaps some common func that you can call early in both set_base
> and full modeset paths?

Well, if the pixel_format check fails we'll do a full modeset, where any
inappropriate framebuffers will be caught in the (now hopefully solid)
checks in pipe_config_set_bpp.

Cheers, Daniel

> 
> > 
> > Cheers, Daniel
> > > 
> > > > 
> > > > v2: Rebase on top of Ville's plane pixel layout changes.
> > > > 
> > > > v3: Actually drop the old gen4 check for 10bpc planes, spotted
> > > > by Ville Syrjälä.
> > > > 
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++------------
> > > >  1 file changed, 8 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 51557ba..bbf31aa 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -2096,8 +2096,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > > >  		dspcntr |= DISPPLANE_RGBX101010;
> > > >  		break;
> > > >  	default:
> > > > -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> > > > -		return -EINVAL;
> > > > +		BUG();
> > > >  	}
> > > >  
> > > >  	if (INTEL_INFO(dev)->gen >= 4) {
> > > > @@ -2190,8 +2189,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
> > > >  		dspcntr |= DISPPLANE_RGBX101010;
> > > >  		break;
> > > >  	default:
> > > > -		DRM_ERROR("Unknown pixel format 0x%08x\n", fb->pixel_format);
> > > > -		return -EINVAL;
> > > > +		BUG();
> > > >  	}
> > > >  
> > > >  	if (obj->tiling_mode != I915_TILING_NONE)
> > > > @@ -7372,21 +7370,19 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
> > > >  		bpp = 8*3;
> > > >  		break;
> > > >  	case 30:
> > > > +		if (INTEL_INFO(dev)->gen < 4) {
> > > > +			DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n");
> > > > +			return -EINVAL;
> > > > +		}
> > > > +
> > > >  		bpp = 10*3;
> > > >  		break;
> > > > -	case 48:
> > > > -		bpp = 12*3;
> > > > -		break;
> > > > +	/* TODO: gen4+ supports 16 bpc floating point, too. */
> > > >  	default:
> > > >  		DRM_DEBUG_KMS("unsupported depth\n");
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > -	if (fb->depth > 24 && !HAS_PCH_SPLIT(dev)) {
> > > > -		DRM_DEBUG_KMS("high depth not supported on gmch platforms\n");
> > > > -		return -EINVAL;
> > > > -	}
> > > > -
> > > >  	pipe_config->pipe_bpp = bpp;
> > > >  
> > > >  	/* Clamp display bpp to EDID value */
> > > > -- 
> > > > 1.7.11.7
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 12/13] drm/i915: clean up pipe bpp confusion

[Daniel Vetter]

- gen4 and earlier (save for g4x) only really have a 8bpc pipe, with
  the possibility to dither to 6bpc using the panel fitter
- g4x has hdmi, but no 12 bpc pipe ... !? Clamp hdmi accordingly.
- TV/SDVO out are the only connectors available on platforms with
  a pipe bpp != 8, add code to force the pipe to 8bpc unconditionally.

<rant>
The dither handling on gmch platforms is one giant disaster. I'm hoping
somewhat that vlv enabling will fix this up, but given that the 6bpc
handling for edp was simply added with another quick hack, I don't have
high hopes ...
</rant>

v2: Neither vlv nor g4x have 12bpc pipes. Still set pipe_bpp to 12*3,
but let the crtc code clamp things down to 10bpc on these platforms.

v3: Fix a bpc vs. bpp mixup in the gen4 and earlier pipe_bpp limiter
code.

v4: Drop the hunk in intel_hdmi.c about g4x/vlv 12bpc, it was wrong.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |  8 ++++++++
 drivers/gpu/drm/i915/intel_sdvo.c    |  3 +++
 drivers/gpu/drm/i915/intel_tv.c      | 14 ++++++++------
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bbf31aa..8ab7520 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3954,6 +3954,14 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
 		adjusted_mode->hsync_start == adjusted_mode->hdisplay)
 		return false;
 
+	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10) {
+		pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */
+	} else if (INTEL_INFO(dev)->gen <= 4 && pipe_config->pipe_bpp > 8) {
+		/* only a 8bpc pipe, with 6bpc dither through the panel fitter
+		 * for lvds. */
+		pipe_config->pipe_bpp = 8*3;
+	}
+
 	return true;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index c6fbfd1..80f8680 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1048,6 +1048,9 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
 	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 	struct drm_display_mode *mode = &pipe_config->requested_mode;
 
+	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
+	pipe_config->pipe_bpp = 8*3;
+
 	if (HAS_PCH_SPLIT(encoder->base.dev))
 		pipe_config->has_pch_encoder = true;
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index d808421..6673726 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -905,11 +905,10 @@ intel_tv_mode_valid(struct drm_connector *connector,
 
 
 static bool
-intel_tv_mode_fixup(struct drm_encoder *encoder,
-		    const struct drm_display_mode *mode,
-		    struct drm_display_mode *adjusted_mode)
+intel_tv_compute_config(struct intel_encoder *encoder,
+			struct intel_crtc_config *pipe_config)
 {
-	struct intel_tv *intel_tv = enc_to_intel_tv(encoder);
+	struct intel_tv *intel_tv = enc_to_intel_tv(&encoder->base);
 	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
 
 	if (!tv_mode)
@@ -918,7 +917,10 @@ intel_tv_mode_fixup(struct drm_encoder *encoder,
 	if (intel_encoder_check_is_cloned(&intel_tv->base))
 		return false;
 
-	adjusted_mode->clock = tv_mode->clock;
+	pipe_config->adjusted_mode.clock = tv_mode->clock;
+	DRM_DEBUG_KMS("forcing bpc to 8 for TV\n");
+	pipe_config->pipe_bpp = 8*3;
+
 	return true;
 }
 
@@ -1485,7 +1487,6 @@ out:
 }
 
 static const struct drm_encoder_helper_funcs intel_tv_helper_funcs = {
-	.mode_fixup = intel_tv_mode_fixup,
 	.mode_set = intel_tv_mode_set,
 };
 
@@ -1620,6 +1621,7 @@ intel_tv_init(struct drm_device *dev)
 	drm_encoder_init(dev, &intel_encoder->base, &intel_tv_enc_funcs,
 			 DRM_MODE_ENCODER_TVDAC);
 
+	intel_encoder->compute_config = intel_tv_compute_config;
 	intel_encoder->enable = intel_enable_tv;
 	intel_encoder->disable = intel_disable_tv;
 	intel_encoder->get_hw_state = intel_tv_get_hw_state;
-- 
1.7.11.7

Re: [PATCH 12/13] drm/i915: clean up pipe bpp confusion

[Jesse Barnes]

On Wed, 27 Mar 2013 00:45:01 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> - gen4 and earlier (save for g4x) only really have a 8bpc pipe, with
>   the possibility to dither to 6bpc using the panel fitter
> - g4x has hdmi, but no 12 bpc pipe ... !? Clamp hdmi accordingly.
> - TV/SDVO out are the only connectors available on platforms with
>   a pipe bpp != 8, add code to force the pipe to 8bpc unconditionally.
> 
> <rant>
> The dither handling on gmch platforms is one giant disaster. I'm hoping
> somewhat that vlv enabling will fix this up, but given that the 6bpc
> handling for edp was simply added with another quick hack, I don't have
> high hopes ...
> </rant>
> 
> v2: Neither vlv nor g4x have 12bpc pipes. Still set pipe_bpp to 12*3,
> but let the crtc code clamp things down to 10bpc on these platforms.
> 
> v3: Fix a bpc vs. bpp mixup in the gen4 and earlier pipe_bpp limiter
> code.
> 
> v4: Drop the hunk in intel_hdmi.c about g4x/vlv 12bpc, it was wrong.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_sdvo.c    |  3 +++
>  drivers/gpu/drm/i915/intel_tv.c      | 14 ++++++++------
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bbf31aa..8ab7520 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3954,6 +3954,14 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
>  		adjusted_mode->hsync_start == adjusted_mode->hdisplay)
>  		return false;
>  
> +	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10) {
> +		pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */
> +	} else if (INTEL_INFO(dev)->gen <= 4 && pipe_config->pipe_bpp > 8) {
> +		/* only a 8bpc pipe, with 6bpc dither through the panel fitter
> +		 * for lvds. */
> +		pipe_config->pipe_bpp = 8*3;
> +	}
> +
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index c6fbfd1..80f8680 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1048,6 +1048,9 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
>  	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
>  	struct drm_display_mode *mode = &pipe_config->requested_mode;
>  
> +	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
> +	pipe_config->pipe_bpp = 8*3;
> +
>  	if (HAS_PCH_SPLIT(encoder->base.dev))
>  		pipe_config->has_pch_encoder = true;
>  
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index d808421..6673726 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -905,11 +905,10 @@ intel_tv_mode_valid(struct drm_connector *connector,
>  
>  
>  static bool
> -intel_tv_mode_fixup(struct drm_encoder *encoder,
> -		    const struct drm_display_mode *mode,
> -		    struct drm_display_mode *adjusted_mode)
> +intel_tv_compute_config(struct intel_encoder *encoder,
> +			struct intel_crtc_config *pipe_config)
>  {
> -	struct intel_tv *intel_tv = enc_to_intel_tv(encoder);
> +	struct intel_tv *intel_tv = enc_to_intel_tv(&encoder->base);
>  	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>  
>  	if (!tv_mode)
> @@ -918,7 +917,10 @@ intel_tv_mode_fixup(struct drm_encoder *encoder,
>  	if (intel_encoder_check_is_cloned(&intel_tv->base))
>  		return false;
>  
> -	adjusted_mode->clock = tv_mode->clock;
> +	pipe_config->adjusted_mode.clock = tv_mode->clock;
> +	DRM_DEBUG_KMS("forcing bpc to 8 for TV\n");
> +	pipe_config->pipe_bpp = 8*3;
> +
>  	return true;
>  }
>  
> @@ -1485,7 +1487,6 @@ out:
>  }
>  
>  static const struct drm_encoder_helper_funcs intel_tv_helper_funcs = {
> -	.mode_fixup = intel_tv_mode_fixup,
>  	.mode_set = intel_tv_mode_set,
>  };
>  
> @@ -1620,6 +1621,7 @@ intel_tv_init(struct drm_device *dev)
>  	drm_encoder_init(dev, &intel_encoder->base, &intel_tv_enc_funcs,
>  			 DRM_MODE_ENCODER_TVDAC);
>  
> +	intel_encoder->compute_config = intel_tv_compute_config;
>  	intel_encoder->enable = intel_enable_tv;
>  	intel_encoder->disable = intel_disable_tv;
>  	intel_encoder->get_hw_state = intel_tv_get_hw_state;

I had to double check this against 9/13... I guess the order will be
clearer with the actual code as opposed to patches.

But won't these override the pipe_config bpp we set in
pipe_config_set_bpp()?  Why bother setting it there if each of the
encoders will set it themselves, and the real comparison is against
the plane bpp?  And doesn't that mean we'd need to set pipe_config->bpp
in the DP version too?

Maybe I've been looking at this too hard and I've just confused
myself...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 12/13] drm/i915: clean up pipe bpp confusion

[Daniel Vetter]

On Wed, Mar 27, 2013 at 10:28 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> I had to double check this against 9/13... I guess the order will be
> clearer with the actual code as opposed to patches.
>
> But won't these override the pipe_config bpp we set in
> pipe_config_set_bpp()?  Why bother setting it there if each of the
> encoders will set it themselves, and the real comparison is against
> the plane bpp?  And doesn't that mean we'd need to set pipe_config->bpp
> in the DP version too?

The pipe_bpp we set from the planes is the proposed one, used when
nothing else overrides it. Then encoders can come around and add in
their opinion about what's possible. Note that encoders want to know
which pipe_bpp is the proposed one (from looking just at the plane) to
make their own decision. E.g. hdmi wants to updither 10bpc to 12bpc
(if possible) since it doesn't support 10bpc natively. Whereas DP only
ever down-dithers.

This way we gain a notch more flexibility in handling bpp.

My auto-fdi dither work which is based on top of this goes one step
further and checks (after plane/pipe/encoder all had their say)
whether it actually fits into the fdi link. If it doesn't fit it tries
to dither down. If that's possible we'll restart the pipe_config
arbitrage, but with the new proposed pipe_bpp plus a flag telling
everyone that they'll get shot if they try to increase bw
requirements.

> Maybe I've been looking at this too hard and I've just confused
> myself...

Maybe it's a bit overdesigned ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 12/13] drm/i915: clean up pipe bpp confusion

[Jesse Barnes]

On Wed, 27 Mar 2013 23:41:55 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Wed, Mar 27, 2013 at 10:28 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > I had to double check this against 9/13... I guess the order will be
> > clearer with the actual code as opposed to patches.
> >
> > But won't these override the pipe_config bpp we set in
> > pipe_config_set_bpp()?  Why bother setting it there if each of the
> > encoders will set it themselves, and the real comparison is against
> > the plane bpp?  And doesn't that mean we'd need to set pipe_config->bpp
> > in the DP version too?
> 
> The pipe_bpp we set from the planes is the proposed one, used when
> nothing else overrides it. Then encoders can come around and add in
> their opinion about what's possible. Note that encoders want to know
> which pipe_bpp is the proposed one (from looking just at the plane) to
> make their own decision. E.g. hdmi wants to updither 10bpc to 12bpc
> (if possible) since it doesn't support 10bpc natively. Whereas DP only
> ever down-dithers.
> 
> This way we gain a notch more flexibility in handling bpp.
> 
> My auto-fdi dither work which is based on top of this goes one step
> further and checks (after plane/pipe/encoder all had their say)
> whether it actually fits into the fdi link. If it doesn't fit it tries
> to dither down. If that's possible we'll restart the pipe_config
> arbitrage, but with the new proposed pipe_bpp plus a flag telling
> everyone that they'll get shot if they try to increase bw
> requirements.
> 
> > Maybe I've been looking at this too hard and I've just confused
> > myself...
> 
> Maybe it's a bit overdesigned ;-)

Ok it makes some sense... though maybe if we passed down the plane bpp
directly we'd be able to avoid some of the re-calc stuff in your FDI
dither patch.

We can always improve it after it lands and becomes clearer.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 12/13] drm/i915: clean up pipe bpp confusion

[Daniel Vetter]

On Wed, Mar 27, 2013 at 04:13:13PM -0700, Jesse Barnes wrote:
> On Wed, 27 Mar 2013 23:41:55 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > On Wed, Mar 27, 2013 at 10:28 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > I had to double check this against 9/13... I guess the order will be
> > > clearer with the actual code as opposed to patches.
> > >
> > > But won't these override the pipe_config bpp we set in
> > > pipe_config_set_bpp()?  Why bother setting it there if each of the
> > > encoders will set it themselves, and the real comparison is against
> > > the plane bpp?  And doesn't that mean we'd need to set pipe_config->bpp
> > > in the DP version too?
> > 
> > The pipe_bpp we set from the planes is the proposed one, used when
> > nothing else overrides it. Then encoders can come around and add in
> > their opinion about what's possible. Note that encoders want to know
> > which pipe_bpp is the proposed one (from looking just at the plane) to
> > make their own decision. E.g. hdmi wants to updither 10bpc to 12bpc
> > (if possible) since it doesn't support 10bpc natively. Whereas DP only
> > ever down-dithers.
> > 
> > This way we gain a notch more flexibility in handling bpp.
> > 
> > My auto-fdi dither work which is based on top of this goes one step
> > further and checks (after plane/pipe/encoder all had their say)
> > whether it actually fits into the fdi link. If it doesn't fit it tries
> > to dither down. If that's possible we'll restart the pipe_config
> > arbitrage, but with the new proposed pipe_bpp plus a flag telling
> > everyone that they'll get shot if they try to increase bw
> > requirements.
> > 
> > > Maybe I've been looking at this too hard and I've just confused
> > > myself...
> > 
> > Maybe it's a bit overdesigned ;-)
> 
> Ok it makes some sense... though maybe if we passed down the plane bpp
> directly we'd be able to avoid some of the re-calc stuff in your FDI
> dither patch.
> 
> We can always improve it after it lands and becomes clearer.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Slurped them all into dinq, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 13/13] drm/i915: clear up the fdi/dp set_m_n confusion

[Daniel Vetter]

There's a rather decent confusion going on around transcoder m_n
values. So let's clarify:
- All dp encoders need this, either on the pch transcoder if it's a
  pch port, or on the cpu transcoder/pipe if it's a cpu port.
- fdi links need to have the right m_n values for the fdi link set in
  the cpu transcoder.

To handle the pch vs transcoder stuff a bit better, extract transcoder
set_m_n helpers. To make them simpler, set intel_crtc->cpu_transcoder
als in ironlake_crtc_mode_set, so that gen5+ (where the cpu m_n
registers are all at the same offset) can use it.

Haswell modeset is decently confused about dp vs. edp vs. fdi. dp vs.
edp works exactly the same as dp (since there's no pch dp any more),
so use that as a check. And only set up the fdi m_n values if we
really have a pch encoder present (which means we have a VGA encoder).

On ilk+ we've called ironlake_set_m_n both for cpu_edp and for pch
encoders. Now that dp_set_m_n handles all dp links (thanks to the
pch encoder check), we can ditch the cpu_edp stuff from the
fdi_set_m_n function.

Since the dp_m_n values are not readily available, we need to
carefully coax the edp values out of the encoder. Hence we can't (yet)
kill this superflous complexity.

v2: Rebase on top of the ivb fdi B/C check patch - we need to properly
clear intel_crtc->fdi_lane, otherwise those checks will misfire.

v3: Rebased on top of a s/IS_HASWELL/HAS_DDI/ patch from Paulo Zanoni.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 86 +++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_dp.c      | 30 ++-----------
 drivers/gpu/drm/i915/intel_drv.h     |  8 ++++
 3 files changed, 67 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ab7520..b076665 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5297,15 +5297,47 @@ int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
 	return bps / (link_bw * 8) + 1;
 }
 
-static void ironlake_set_m_n(struct drm_crtc *crtc)
+void intel_pch_transcoder_set_m_n(struct intel_crtc *crtc,
+				  struct intel_link_m_n *m_n)
 {
-	struct drm_device *dev = crtc->dev;
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int pipe = crtc->pipe;
+
+	I915_WRITE(TRANSDATA_M1(pipe), TU_SIZE(m_n->tu) | m_n->gmch_m);
+	I915_WRITE(TRANSDATA_N1(pipe), m_n->gmch_n);
+	I915_WRITE(TRANSDPLINK_M1(pipe), m_n->link_m);
+	I915_WRITE(TRANSDPLINK_N1(pipe), m_n->link_n);
+}
+
+void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
+				  struct intel_link_m_n *m_n)
+{
+	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int pipe = crtc->pipe;
+	enum transcoder transcoder = crtc->cpu_transcoder;
+
+	if (INTEL_INFO(dev)->gen >= 5) {
+		I915_WRITE(PIPE_DATA_M1(transcoder), TU_SIZE(m_n->tu) | m_n->gmch_m);
+		I915_WRITE(PIPE_DATA_N1(transcoder), m_n->gmch_n);
+		I915_WRITE(PIPE_LINK_M1(transcoder), m_n->link_m);
+		I915_WRITE(PIPE_LINK_N1(transcoder), m_n->link_n);
+	} else {
+		I915_WRITE(PIPE_GMCH_DATA_M(pipe), TU_SIZE(m_n->tu) | m_n->gmch_m);
+		I915_WRITE(PIPE_GMCH_DATA_N(pipe), m_n->gmch_n);
+		I915_WRITE(PIPE_DP_LINK_M(pipe), m_n->link_m);
+		I915_WRITE(PIPE_DP_LINK_N(pipe), m_n->link_n);
+	}
+}
+
+static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_display_mode *adjusted_mode =
 		&intel_crtc->config.adjusted_mode;
 	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
-	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
 	struct intel_encoder *intel_encoder, *edp_encoder = NULL;
 	struct intel_link_m_n m_n = {0};
 	int target_clock, lane, link_bw;
@@ -5325,22 +5357,14 @@ static void ironlake_set_m_n(struct drm_crtc *crtc)
 		}
 	}
 
-	/* FDI link */
-	lane = 0;
-	/* CPU eDP doesn't require FDI link, so just set DP M/N
-	   according to current link config */
-	if (is_cpu_edp) {
-		intel_edp_link_config(edp_encoder, &lane, &link_bw);
-	} else {
-		/* FDI is a binary signal running at ~2.7GHz, encoding
-		 * each output octet as 10 bits. The actual frequency
-		 * is stored as a divider into a 100MHz clock, and the
-		 * mode pixel clock is stored in units of 1KHz.
-		 * Hence the bw of each lane in terms of the mode signal
-		 * is:
-		 */
-		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
-	}
+	/* FDI is a binary signal running at ~2.7GHz, encoding
+	 * each output octet as 10 bits. The actual frequency
+	 * is stored as a divider into a 100MHz clock, and the
+	 * mode pixel clock is stored in units of 1KHz.
+	 * Hence the bw of each lane in terms of the mode signal
+	 * is:
+	 */
+	link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
 
 	/* [e]DP over FDI requires target mode clock instead of link clock. */
 	if (edp_encoder)
@@ -5350,9 +5374,8 @@ static void ironlake_set_m_n(struct drm_crtc *crtc)
 	else
 		target_clock = adjusted_mode->clock;
 
-	if (!lane)
-		lane = ironlake_get_lanes_required(target_clock, link_bw,
-						   intel_crtc->config.pipe_bpp);
+	lane = ironlake_get_lanes_required(target_clock, link_bw,
+					   intel_crtc->config.pipe_bpp);
 
 	intel_crtc->fdi_lanes = lane;
 
@@ -5361,10 +5384,7 @@ static void ironlake_set_m_n(struct drm_crtc *crtc)
 	intel_link_compute_m_n(intel_crtc->config.pipe_bpp, lane, target_clock,
 			       link_bw, &m_n);
 
-	I915_WRITE(PIPE_DATA_M1(cpu_transcoder), TU_SIZE(m_n.tu) | m_n.gmch_m);
-	I915_WRITE(PIPE_DATA_N1(cpu_transcoder), m_n.gmch_n);
-	I915_WRITE(PIPE_LINK_M1(cpu_transcoder), m_n.link_m);
-	I915_WRITE(PIPE_LINK_N1(cpu_transcoder), m_n.link_n);
+	intel_cpu_transcoder_set_m_n(intel_crtc, &m_n);
 }
 
 static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
@@ -5511,6 +5531,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
 	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
 
+	intel_crtc->cpu_transcoder = pipe;
+
 	ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
 				     &has_reduced_clock, &reduced_clock);
 	if (!ok) {
@@ -5549,7 +5571,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	} else
 		intel_put_pch_pll(intel_crtc);
 
-	if (is_dp && !is_cpu_edp)
+	if (is_dp)
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -5585,7 +5607,9 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	/* Note, this also computes intel_crtc->fdi_lanes which is used below in
 	 * ironlake_check_fdi_lanes. */
-	ironlake_set_m_n(crtc);
+	intel_crtc->fdi_lanes = 0;
+	if (intel_crtc->config.has_pch_encoder)
+		ironlake_fdi_set_m_n(crtc);
 
 	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
 
@@ -5697,15 +5721,15 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
 
-	if (is_dp && !is_cpu_edp)
+	if (is_dp)
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 
 	intel_crtc->lowfreq_avail = false;
 
 	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
-	if (!is_dp || is_cpu_edp)
-		ironlake_set_m_n(crtc);
+	if (intel_crtc->config.has_pch_encoder)
+		ironlake_fdi_set_m_n(crtc);
 
 	haswell_set_pipeconf(crtc, adjusted_mode, dither);
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 251aa6b..6b8a279 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -766,12 +766,9 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	struct drm_device *dev = crtc->dev;
 	struct intel_encoder *intel_encoder;
 	struct intel_dp *intel_dp;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int lane_count = 4;
 	struct intel_link_m_n m_n;
-	int pipe = intel_crtc->pipe;
-	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
 	int target_clock;
 
 	/*
@@ -805,29 +802,10 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	intel_link_compute_m_n(intel_crtc->config.pipe_bpp, lane_count,
 			       target_clock, adjusted_mode->clock, &m_n);
 
-	if (HAS_DDI(dev)) {
-		I915_WRITE(PIPE_DATA_M1(cpu_transcoder),
-			   TU_SIZE(m_n.tu) | m_n.gmch_m);
-		I915_WRITE(PIPE_DATA_N1(cpu_transcoder), m_n.gmch_n);
-		I915_WRITE(PIPE_LINK_M1(cpu_transcoder), m_n.link_m);
-		I915_WRITE(PIPE_LINK_N1(cpu_transcoder), m_n.link_n);
-	} else if (HAS_PCH_SPLIT(dev)) {
-		I915_WRITE(TRANSDATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
-		I915_WRITE(TRANSDATA_N1(pipe), m_n.gmch_n);
-		I915_WRITE(TRANSDPLINK_M1(pipe), m_n.link_m);
-		I915_WRITE(TRANSDPLINK_N1(pipe), m_n.link_n);
-	} else if (IS_VALLEYVIEW(dev)) {
-		I915_WRITE(PIPE_DATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
-		I915_WRITE(PIPE_DATA_N1(pipe), m_n.gmch_n);
-		I915_WRITE(PIPE_LINK_M1(pipe), m_n.link_m);
-		I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
-	} else {
-		I915_WRITE(PIPE_GMCH_DATA_M(pipe),
-			   TU_SIZE(m_n.tu) | m_n.gmch_m);
-		I915_WRITE(PIPE_GMCH_DATA_N(pipe), m_n.gmch_n);
-		I915_WRITE(PIPE_DP_LINK_M(pipe), m_n.link_m);
-		I915_WRITE(PIPE_DP_LINK_N(pipe), m_n.link_n);
-	}
+	if (intel_crtc->config.has_pch_encoder)
+		intel_pch_transcoder_set_m_n(intel_crtc, &m_n);
+	else
+		intel_cpu_transcoder_set_m_n(intel_crtc, &m_n);
 }
 
 void intel_dp_init_link_config(struct intel_dp *intel_dp)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5c7b04b..3a9b7be 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -192,8 +192,12 @@ struct intel_crtc_config {
 	 */
 	bool limited_color_range;
 
+	/* DP has a bunch of special case unfortunately, so mark the pipe
+	 * accordingly. */
+	bool has_dp_encoder;
 	bool dither;
 	int pipe_bpp;
+	struct intel_link_m_n dp_m_n;
 
 	/* Used by SDVO (and if we ever fix it, HDMI). */
 	unsigned pixel_multiplier;
@@ -641,6 +645,10 @@ extern void intel_init_clock_gating(struct drm_device *dev);
 extern void intel_write_eld(struct drm_encoder *encoder,
 			    struct drm_display_mode *mode);
 extern void intel_cpt_verify_modeset(struct drm_device *dev, int pipe);
+extern void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
+					 struct intel_link_m_n *m_n);
+extern void intel_pch_transcoder_set_m_n(struct intel_crtc *crtc,
+					 struct intel_link_m_n *m_n);
 extern void intel_prepare_ddi(struct drm_device *dev);
 extern void hsw_fdi_link_train(struct drm_crtc *crtc);
 extern void intel_ddi_init(struct drm_device *dev, enum port port);
-- 
1.7.11.7

Re: [PATCH 13/13] drm/i915: clear up the fdi/dp set_m_n confusion

[Daniel Vetter]

On Wed, Mar 27, 2013 at 12:45:02AM +0100, Daniel Vetter wrote:
> There's a rather decent confusion going on around transcoder m_n
> values. So let's clarify:
> - All dp encoders need this, either on the pch transcoder if it's a
>   pch port, or on the cpu transcoder/pipe if it's a cpu port.
> - fdi links need to have the right m_n values for the fdi link set in
>   the cpu transcoder.
> 
> To handle the pch vs transcoder stuff a bit better, extract transcoder
> set_m_n helpers. To make them simpler, set intel_crtc->cpu_transcoder
> als in ironlake_crtc_mode_set, so that gen5+ (where the cpu m_n
> registers are all at the same offset) can use it.
> 
> Haswell modeset is decently confused about dp vs. edp vs. fdi. dp vs.
> edp works exactly the same as dp (since there's no pch dp any more),
> so use that as a check. And only set up the fdi m_n values if we
> really have a pch encoder present (which means we have a VGA encoder).
> 
> On ilk+ we've called ironlake_set_m_n both for cpu_edp and for pch
> encoders. Now that dp_set_m_n handles all dp links (thanks to the
> pch encoder check), we can ditch the cpu_edp stuff from the
> fdi_set_m_n function.
> 
> Since the dp_m_n values are not readily available, we need to
> carefully coax the edp values out of the encoder. Hence we can't (yet)
> kill this superflous complexity.
> 
> v2: Rebase on top of the ivb fdi B/C check patch - we need to properly
> clear intel_crtc->fdi_lane, otherwise those checks will misfire.
> 
> v3: Rebased on top of a s/IS_HASWELL/HAS_DDI/ patch from Paulo Zanoni.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Oops, that patch is already part of the next pile of pipe_config stuff.
Please disregard (for now ...).
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c | 86 +++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_dp.c      | 30 ++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |  8 ++++
>  3 files changed, 67 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ab7520..b076665 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5297,15 +5297,47 @@ int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
>  	return bps / (link_bw * 8) + 1;
>  }
>  
> -static void ironlake_set_m_n(struct drm_crtc *crtc)
> +void intel_pch_transcoder_set_m_n(struct intel_crtc *crtc,
> +				  struct intel_link_m_n *m_n)
>  {
> -	struct drm_device *dev = crtc->dev;
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int pipe = crtc->pipe;
> +
> +	I915_WRITE(TRANSDATA_M1(pipe), TU_SIZE(m_n->tu) | m_n->gmch_m);
> +	I915_WRITE(TRANSDATA_N1(pipe), m_n->gmch_n);
> +	I915_WRITE(TRANSDPLINK_M1(pipe), m_n->link_m);
> +	I915_WRITE(TRANSDPLINK_N1(pipe), m_n->link_n);
> +}
> +
> +void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
> +				  struct intel_link_m_n *m_n)
> +{
> +	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int pipe = crtc->pipe;
> +	enum transcoder transcoder = crtc->cpu_transcoder;
> +
> +	if (INTEL_INFO(dev)->gen >= 5) {
> +		I915_WRITE(PIPE_DATA_M1(transcoder), TU_SIZE(m_n->tu) | m_n->gmch_m);
> +		I915_WRITE(PIPE_DATA_N1(transcoder), m_n->gmch_n);
> +		I915_WRITE(PIPE_LINK_M1(transcoder), m_n->link_m);
> +		I915_WRITE(PIPE_LINK_N1(transcoder), m_n->link_n);
> +	} else {
> +		I915_WRITE(PIPE_GMCH_DATA_M(pipe), TU_SIZE(m_n->tu) | m_n->gmch_m);
> +		I915_WRITE(PIPE_GMCH_DATA_N(pipe), m_n->gmch_n);
> +		I915_WRITE(PIPE_DP_LINK_M(pipe), m_n->link_m);
> +		I915_WRITE(PIPE_DP_LINK_N(pipe), m_n->link_n);
> +	}
> +}
> +
> +static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_display_mode *adjusted_mode =
>  		&intel_crtc->config.adjusted_mode;
>  	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> -	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
>  	struct intel_encoder *intel_encoder, *edp_encoder = NULL;
>  	struct intel_link_m_n m_n = {0};
>  	int target_clock, lane, link_bw;
> @@ -5325,22 +5357,14 @@ static void ironlake_set_m_n(struct drm_crtc *crtc)
>  		}
>  	}
>  
> -	/* FDI link */
> -	lane = 0;
> -	/* CPU eDP doesn't require FDI link, so just set DP M/N
> -	   according to current link config */
> -	if (is_cpu_edp) {
> -		intel_edp_link_config(edp_encoder, &lane, &link_bw);
> -	} else {
> -		/* FDI is a binary signal running at ~2.7GHz, encoding
> -		 * each output octet as 10 bits. The actual frequency
> -		 * is stored as a divider into a 100MHz clock, and the
> -		 * mode pixel clock is stored in units of 1KHz.
> -		 * Hence the bw of each lane in terms of the mode signal
> -		 * is:
> -		 */
> -		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> -	}
> +	/* FDI is a binary signal running at ~2.7GHz, encoding
> +	 * each output octet as 10 bits. The actual frequency
> +	 * is stored as a divider into a 100MHz clock, and the
> +	 * mode pixel clock is stored in units of 1KHz.
> +	 * Hence the bw of each lane in terms of the mode signal
> +	 * is:
> +	 */
> +	link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
>  
>  	/* [e]DP over FDI requires target mode clock instead of link clock. */
>  	if (edp_encoder)
> @@ -5350,9 +5374,8 @@ static void ironlake_set_m_n(struct drm_crtc *crtc)
>  	else
>  		target_clock = adjusted_mode->clock;
>  
> -	if (!lane)
> -		lane = ironlake_get_lanes_required(target_clock, link_bw,
> -						   intel_crtc->config.pipe_bpp);
> +	lane = ironlake_get_lanes_required(target_clock, link_bw,
> +					   intel_crtc->config.pipe_bpp);
>  
>  	intel_crtc->fdi_lanes = lane;
>  
> @@ -5361,10 +5384,7 @@ static void ironlake_set_m_n(struct drm_crtc *crtc)
>  	intel_link_compute_m_n(intel_crtc->config.pipe_bpp, lane, target_clock,
>  			       link_bw, &m_n);
>  
> -	I915_WRITE(PIPE_DATA_M1(cpu_transcoder), TU_SIZE(m_n.tu) | m_n.gmch_m);
> -	I915_WRITE(PIPE_DATA_N1(cpu_transcoder), m_n.gmch_n);
> -	I915_WRITE(PIPE_LINK_M1(cpu_transcoder), m_n.link_m);
> -	I915_WRITE(PIPE_LINK_N1(cpu_transcoder), m_n.link_n);
> +	intel_cpu_transcoder_set_m_n(intel_crtc, &m_n);
>  }
>  
>  static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> @@ -5511,6 +5531,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
>  	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
>  
> +	intel_crtc->cpu_transcoder = pipe;
> +
>  	ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
>  				     &has_reduced_clock, &reduced_clock);
>  	if (!ok) {
> @@ -5549,7 +5571,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	} else
>  		intel_put_pch_pll(intel_crtc);
>  
> -	if (is_dp && !is_cpu_edp)
> +	if (is_dp)
>  		intel_dp_set_m_n(crtc, mode, adjusted_mode);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -5585,7 +5607,9 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	/* Note, this also computes intel_crtc->fdi_lanes which is used below in
>  	 * ironlake_check_fdi_lanes. */
> -	ironlake_set_m_n(crtc);
> +	intel_crtc->fdi_lanes = 0;
> +	if (intel_crtc->config.has_pch_encoder)
> +		ironlake_fdi_set_m_n(crtc);
>  
>  	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
>  
> @@ -5697,15 +5721,15 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
>  	drm_mode_debug_printmodeline(mode);
>  
> -	if (is_dp && !is_cpu_edp)
> +	if (is_dp)
>  		intel_dp_set_m_n(crtc, mode, adjusted_mode);
>  
>  	intel_crtc->lowfreq_avail = false;
>  
>  	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>  
> -	if (!is_dp || is_cpu_edp)
> -		ironlake_set_m_n(crtc);
> +	if (intel_crtc->config.has_pch_encoder)
> +		ironlake_fdi_set_m_n(crtc);
>  
>  	haswell_set_pipeconf(crtc, adjusted_mode, dither);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 251aa6b..6b8a279 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -766,12 +766,9 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_encoder *intel_encoder;
>  	struct intel_dp *intel_dp;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int lane_count = 4;
>  	struct intel_link_m_n m_n;
> -	int pipe = intel_crtc->pipe;
> -	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
>  	int target_clock;
>  
>  	/*
> @@ -805,29 +802,10 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
>  	intel_link_compute_m_n(intel_crtc->config.pipe_bpp, lane_count,
>  			       target_clock, adjusted_mode->clock, &m_n);
>  
> -	if (HAS_DDI(dev)) {
> -		I915_WRITE(PIPE_DATA_M1(cpu_transcoder),
> -			   TU_SIZE(m_n.tu) | m_n.gmch_m);
> -		I915_WRITE(PIPE_DATA_N1(cpu_transcoder), m_n.gmch_n);
> -		I915_WRITE(PIPE_LINK_M1(cpu_transcoder), m_n.link_m);
> -		I915_WRITE(PIPE_LINK_N1(cpu_transcoder), m_n.link_n);
> -	} else if (HAS_PCH_SPLIT(dev)) {
> -		I915_WRITE(TRANSDATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
> -		I915_WRITE(TRANSDATA_N1(pipe), m_n.gmch_n);
> -		I915_WRITE(TRANSDPLINK_M1(pipe), m_n.link_m);
> -		I915_WRITE(TRANSDPLINK_N1(pipe), m_n.link_n);
> -	} else if (IS_VALLEYVIEW(dev)) {
> -		I915_WRITE(PIPE_DATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
> -		I915_WRITE(PIPE_DATA_N1(pipe), m_n.gmch_n);
> -		I915_WRITE(PIPE_LINK_M1(pipe), m_n.link_m);
> -		I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
> -	} else {
> -		I915_WRITE(PIPE_GMCH_DATA_M(pipe),
> -			   TU_SIZE(m_n.tu) | m_n.gmch_m);
> -		I915_WRITE(PIPE_GMCH_DATA_N(pipe), m_n.gmch_n);
> -		I915_WRITE(PIPE_DP_LINK_M(pipe), m_n.link_m);
> -		I915_WRITE(PIPE_DP_LINK_N(pipe), m_n.link_n);
> -	}
> +	if (intel_crtc->config.has_pch_encoder)
> +		intel_pch_transcoder_set_m_n(intel_crtc, &m_n);
> +	else
> +		intel_cpu_transcoder_set_m_n(intel_crtc, &m_n);
>  }
>  
>  void intel_dp_init_link_config(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5c7b04b..3a9b7be 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -192,8 +192,12 @@ struct intel_crtc_config {
>  	 */
>  	bool limited_color_range;
>  
> +	/* DP has a bunch of special case unfortunately, so mark the pipe
> +	 * accordingly. */
> +	bool has_dp_encoder;
>  	bool dither;
>  	int pipe_bpp;
> +	struct intel_link_m_n dp_m_n;
>  
>  	/* Used by SDVO (and if we ever fix it, HDMI). */
>  	unsigned pixel_multiplier;
> @@ -641,6 +645,10 @@ extern void intel_init_clock_gating(struct drm_device *dev);
>  extern void intel_write_eld(struct drm_encoder *encoder,
>  			    struct drm_display_mode *mode);
>  extern void intel_cpt_verify_modeset(struct drm_device *dev, int pipe);
> +extern void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
> +					 struct intel_link_m_n *m_n);
> +extern void intel_pch_transcoder_set_m_n(struct intel_crtc *crtc,
> +					 struct intel_link_m_n *m_n);
>  extern void intel_prepare_ddi(struct drm_device *dev);
>  extern void hsw_fdi_link_train(struct drm_crtc *crtc);
>  extern void intel_ddi_init(struct drm_device *dev, enum port port);
> -- 
> 1.7.11.7
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch