On Sat, 01 Dec 2018, Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx> > --- > 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; -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx