Re: [PATCH 1/4] drm/i915/dsi: Fix pipe_bpp for handling for 6 bpc pixel-formats

On Sat, 01 Dec 2018, Hans de Goede hdegoede@redhat.com wrote:

There are 3 problems with the dsi code's pipe_bpp handling for 6 bpc pixel-formats which this commit addresses:

1. It assumes that the pipe_bpp is the same as the bpp going over the dsi

lanes. This assumption is not valid for MIPI_DSI_FMT_RGB666, where pipe_bpp should be 18 so that we do proper dithering but we actually send 24 bpp over the dsi lanes (MIPI_DSI_FMT_RGB666_PACKED sends 18 bpp).

This assumption is enforced by an assert in *_dsi_get_pclk(). This assert triggers on the initial hw-state readback on BYT/CHT devices which use MIPI_DSI_FMT_RGB666, such as the Prowise PT301 tablet. PIPECONF is set to 6BPC / 18 bpp by the GOP, while mipi_dsi_pixel_format_to_bpp() returns 24.

This commits switches the calculations in *_dsi_get_pclk() to use the bpp from mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format) which returns the bpp going over the mipi lanes and drops the assert.

2. On BXT bxt_dsi_get_pipe_config() wrongly overrides the pipe_bpp which

i9xx_get_pipe_config() reads from PIPECONF with the return value from mipi_dsi_pixel_format_to_bpp(). This avoids the assert from 1. but is wrong since the pipe is actually running at the value configured in PIPECONF.

This commit drops the override of pipe_bpp from bxt_dsi_get_pipe_config().

3. The dsi encoder's compute_config() never assigns a value to pipe_bpp,

unlike most other encoders. Falling back on compute_baseline_pipe_bpp() which always picks 24. 24 is only correct for MIPI_DSI_FMT_RGB88 for the others we should use 18 bpp so that we correctly do 6bpc color dithering.

This commit adds code to intel_dsi_compute_config() to properly set pipe_bpp based on intel_dsi->pixel_format.

Signed-off-by: Hans de Goede hdegoede@redhat.com

drivers/gpu/drm/i915/intel_dsi.h | 4 ++-- drivers/gpu/drm/i915/vlv_dsi.c | 17 ++++++++-------- drivers/gpu/drm/i915/vlv_dsi_pll.c | 31 ++++++------------------------ 3 files changed, 17 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index c888c219835f..c796a2962a43 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -160,7 +160,7 @@ int vlv_dsi_pll_compute(struct intel_encoder *encoder, void vlv_dsi_pll_enable(struct intel_encoder *encoder, const struct intel_crtc_state *config); void vlv_dsi_pll_disable(struct intel_encoder *encoder); -u32 vlv_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp, +u32 vlv_dsi_get_pclk(struct intel_encoder *encoder, struct intel_crtc_state *config); void vlv_dsi_reset_clocks(struct intel_encoder *encoder, enum port port);

@@ -170,7 +170,7 @@ int bxt_dsi_pll_compute(struct intel_encoder *encoder, void bxt_dsi_pll_enable(struct intel_encoder *encoder, const struct intel_crtc_state *config); void bxt_dsi_pll_disable(struct intel_encoder *encoder); -u32 bxt_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp, +u32 bxt_dsi_get_pclk(struct intel_encoder *encoder, struct intel_crtc_state *config); void bxt_dsi_reset_clocks(struct intel_encoder *encoder, enum port port);

diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index be3af5f6c7a0..c10def5efa22 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -322,6 +322,11 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder, /* DSI uses short packets for sync events, so clear mode flags for DSI */ adjusted_mode->flags = 0;

• if (intel_dsi->pixel_format == MIPI_DSI_FMT_RGB888)

• pipe_config->pipe_bpp = 24;
  

• else

• pipe_config->pipe_bpp = 18;
  

• if (IS_GEN9_LP(dev_priv)) { /* Enable Frame time stamp based scanline reporting */ adjusted_mode->private_flags |=

@@ -1097,10 +1102,8 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder, }

fmt = I915_READ(MIPI_DSI_FUNC_PRG(port)) & VID_MODE_FORMAT_MASK;

• pipe_config->pipe_bpp =

• 	mipi_dsi_pixel_format_to_bpp(
  

• 		pixel_format_from_register_bits(fmt));
  

This part here was crucial for BXT/GLK hardware state readout. The CI found it, but the result was ignored. :(

https://bugs.freedesktop.org/show_bug.cgi?id=109516

BR, Jani.

• bpp = pipe_config->pipe_bpp;

• bpp = mipi_dsi_pixel_format_to_bpp(

• 	pixel_format_from_register_bits(fmt));
  
  

  /* Enable Frame time stamo based scanline reporting */ adjusted_mode->private_flags |=

@@ -1238,11 +1241,9 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,

if (IS_GEN9_LP(dev_priv)) { bxt_dsi_get_pipe_config(encoder, pipe_config);

• pclk = bxt_dsi_get_pclk(encoder, pipe_config->pipe_bpp,
  

• 			pipe_config);
  

• pclk = bxt_dsi_get_pclk(encoder, pipe_config);
  

  } else {

• pclk = vlv_dsi_get_pclk(encoder, pipe_config->pipe_bpp,
  

• 			pipe_config);
  

• pclk = vlv_dsi_get_pclk(encoder, pipe_config);
  

  }

  if (pclk) {

diff --git a/drivers/gpu/drm/i915/vlv_dsi_pll.c b/drivers/gpu/drm/i915/vlv_dsi_pll.c index a132a8037ecc..954d5a8c4fa7 100644 --- a/drivers/gpu/drm/i915/vlv_dsi_pll.c +++ b/drivers/gpu/drm/i915/vlv_dsi_pll.c @@ -252,20 +252,12 @@ void bxt_dsi_pll_disable(struct intel_encoder *encoder) DRM_ERROR("Timeout waiting for PLL lock deassertion\n"); }

-static void assert_bpp_mismatch(enum mipi_dsi_pixel_format fmt, int pipe_bpp) -{

• int bpp = mipi_dsi_pixel_format_to_bpp(fmt);
• WARN(bpp != pipe_bpp,

•     "bpp match assertion failure (expected %d, current %d)\n",
  

•     bpp, pipe_bpp);
  

-}

-u32 vlv_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp, +u32 vlv_dsi_get_pclk(struct intel_encoder *encoder, struct intel_crtc_state *config) { struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);

• int bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format); u32 dsi_clock, pclk; u32 pll_ctl, pll_div; u32 m = 0, p = 0, n;

@@ -319,15 +311,12 @@ u32 vlv_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp,

dsi_clock = (m * refclk) / (p * n);

• /* pixel_format and pipe_bpp should agree */
• assert_bpp_mismatch(intel_dsi->pixel_format, pipe_bpp);
• pclk = DIV_ROUND_CLOSEST(dsi_clock * intel_dsi->lane_count, pipe_bpp);

• pclk = DIV_ROUND_CLOSEST(dsi_clock * intel_dsi->lane_count, bpp);

  return pclk;

}

-u32 bxt_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp, +u32 bxt_dsi_get_pclk(struct intel_encoder *encoder, struct intel_crtc_state *config) { u32 pclk; @@ -335,12 +324,7 @@ u32 bxt_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp, u32 dsi_ratio; struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

• /* Divide by zero */
• if (!pipe_bpp) {

• DRM_ERROR("Invalid BPP(0)\n");
  

• return 0;
  

• }

• int bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format);

  config->dsi_pll.ctrl = I915_READ(BXT_DSI_PLL_CTL);

@@ -348,10 +332,7 @@ u32 bxt_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp,

dsi_clk = (dsi_ratio * BXT_REF_CLOCK_KHZ) / 2;

• /* pixel_format and pipe_bpp should agree */
• assert_bpp_mismatch(intel_dsi->pixel_format, pipe_bpp);
• pclk = DIV_ROUND_CLOSEST(dsi_clk * intel_dsi->lane_count, pipe_bpp);

• pclk = DIV_ROUND_CLOSEST(dsi_clk * intel_dsi->lane_count, bpp);

  DRM_DEBUG_DRIVER("Calculated pclk=%u\n", pclk); return pclk;