[Intel-gfx] [PATCH 1/4] drm/i915/dsi: Fix pipe_bpp for handling for 6 bpc pixel-formats
Saarinen, Jani
jani.saarinen at intel.com
Fri Apr 5 06:59:17 UTC 2019
Hi,
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Jani
> Nikula
> Sent: perjantai 5. huhtikuuta 2019 9.35
> To: Hans de Goede <hdegoede at redhat.com>; Joonas Lahtinen
> <joonas.lahtinen at linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>; Ville
> Syrjälä <ville.syrjala at linux.intel.com>
> Cc: intel-gfx <intel-gfx at lists.freedesktop.org>; dri-devel at 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 at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list