| Message ID | 20181201113148.23184-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/dsi: Fix pipe_bpp for handling for 6 bpc pixel-formats | expand |
On Sat, Dec 01, 2018 at 12:31:45PM +0100, Hans de Goede 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> lgtm Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> That said, I think we could make everything less confusing by doing something like this: compute_config() { port_clock = bitrate; } get_config() { port_clock = readout from pll; crtc_clock = derive from port_clock; } > --- > 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)); > - 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; > -- > 2.19.1
Hi, On 15-01-19 15:51, Ville Syrjälä wrote: > On Sat, Dec 01, 2018 at 12:31:45PM +0100, Hans de Goede 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> > > lgtm > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thank you. > That said, I think we could make everything less confusing by doing > something like this: > > compute_config() { > port_clock = bitrate; > } > > get_config() { > port_clock = readout from pll; > crtc_clock = derive from port_clock; > } Currently the code assumes that port_clock == crtc_clock, if make port_clock reflect the actual pll clock, without compensating for number of lanes and bpp, I think we need to make changes in more places. Regards, Hans > >> --- >> 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)); >> - 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; >> -- >> 2.19.1 >
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;
Hi, > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani > Nikula > Sent: perjantai 5. huhtikuuta 2019 9.35 > To: Hans de Goede <hdegoede@redhat.com>; Joonas Lahtinen > <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Ville > Syrjälä <ville.syrjala@linux.intel.com> > Cc: intel-gfx <intel-gfx@lists.freedesktop.org>; dri-devel@lists.freedesktop.org > Subject: Re: [Intel-gfx] [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. :( Indeed: https://patchwork.freedesktop.org/series/53352/ > > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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)); - 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;
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(-)