From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/4] drm/i915/dsi: Fix pipe_bpp for handling for 6 bpc pixel-formats
Date: Tue, 15 Jan 2019 16:51:20 +0200 [thread overview]
Message-ID: <20190115145120.GJ20097@intel.com> (raw)
In-Reply-To: <20181201113148.23184-1-hdegoede@redhat.com>
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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-01-15 14:51 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-01 11:31 [PATCH 1/4] drm/i915/dsi: Fix pipe_bpp for handling for 6 bpc pixel-formats Hans de Goede
2018-12-01 11:31 ` [PATCH 2/4] drm/i915/dsi: Enable dithering for 6 bpc panels Hans de Goede
2019-01-15 14:55 ` Ville Syrjälä
2019-01-21 9:54 ` Hans de Goede
2018-12-01 11:31 ` [PATCH 3/4] drm/i915/dsi: Adjust crtc_clock for burst_mode_ratio Hans de Goede
2019-01-15 15:00 ` Ville Syrjälä
2019-01-21 14:26 ` Hans de Goede
2019-01-21 15:30 ` Hans de Goede
2018-12-01 11:31 ` [PATCH 4/4] drm/i915/dsi: Call drm_connector_cleanup on vlv_dsi_init error exit path Hans de Goede
2019-01-25 21:05 ` Ville Syrjälä
2018-12-01 12:01 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915/dsi: Fix pipe_bpp for handling for 6 bpc pixel-formats Patchwork
2018-12-01 12:20 ` ✓ Fi.CI.BAT: success " Patchwork
2018-12-01 22:45 ` ✓ Fi.CI.IGT: " Patchwork
2019-01-15 14:51 ` Ville Syrjälä [this message]
2019-01-21 9:53 ` [PATCH 1/4] " Hans de Goede
2019-04-05 6:34 ` Jani Nikula
2019-04-05 6:59 ` [Intel-gfx] " Saarinen, Jani
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190115145120.GJ20097@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=hdegoede@redhat.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.