[Intel-gfx] [PATCH 1/4] drm/i915/dsi: Fix pipe_bpp for handling for 6 bpc pixel-formats
Hans de Goede
hdegoede at redhat.com
Mon Jan 21 09:53:45 UTC 2019
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 at redhat.com>
>
> lgtm
> Reviewed-by: Ville Syrjälä <ville.syrjala at 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
>
More information about the Intel-gfx
mailing list