Hi, > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Jani > Nikula > Sent: perjantai 5. huhtikuuta 2019 9.35 > To: Hans de Goede <hdegoede@xxxxxxxxxx>; Joonas Lahtinen > <joonas.lahtinen@xxxxxxxxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Ville > Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx > Subject: 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@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. :( 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
