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)); - 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;
The display engine has 2 dithering enable bits which both need to be set for dithering to happen, 1 in the PIPECONF register which is taken care of by i9xx_set_pipeconf() and a second bit at the encoder level.
The dsi code was not setting the encoder level dithering enable bit causing dithering to be disabled, this commit fixes this.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/vlv_dsi.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index c10def5efa22..c21cbfa9653c 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -711,6 +711,10 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder, LANE_CONFIGURATION_DUAL_LINK_B : LANE_CONFIGURATION_DUAL_LINK_A; } + + if (intel_dsi->pixel_format != MIPI_DSI_FMT_RGB888) + temp |= DITHERING_ENABLE; + /* assert ip_tg_enable signal */ I915_WRITE(port_ctrl, temp | DPI_ENABLE); POSTING_READ(port_ctrl);
On Sat, Dec 01, 2018 at 12:31:46PM +0100, Hans de Goede wrote:
The display engine has 2 dithering enable bits which both need to be set for dithering to happen, 1 in the PIPECONF register which is taken care of by i9xx_set_pipeconf() and a second bit at the encoder level.
The dsi code was not setting the encoder level dithering enable bit causing dithering to be disabled, this commit fixes this.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/i915/vlv_dsi.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index c10def5efa22..c21cbfa9653c 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -711,6 +711,10 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder, LANE_CONFIGURATION_DUAL_LINK_B : LANE_CONFIGURATION_DUAL_LINK_A; }
if (intel_dsi->pixel_format != MIPI_DSI_FMT_RGB888)
temp |= DITHERING_ENABLE;
The docs say this was only made to work in C0 stepping. Not sure any BYT-Ts were ever shipped with B2/3, nor am I sure if setting the bit would have any effect there. IMO let's just set the bit and hope for the best.
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
- /* assert ip_tg_enable signal */ I915_WRITE(port_ctrl, temp | DPI_ENABLE); POSTING_READ(port_ctrl);
-- 2.19.1
Hi,
On 15-01-19 15:55, Ville Syrjälä wrote:
On Sat, Dec 01, 2018 at 12:31:46PM +0100, Hans de Goede wrote:
The display engine has 2 dithering enable bits which both need to be set for dithering to happen, 1 in the PIPECONF register which is taken care of by i9xx_set_pipeconf() and a second bit at the encoder level.
The dsi code was not setting the encoder level dithering enable bit causing dithering to be disabled, this commit fixes this.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/i915/vlv_dsi.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index c10def5efa22..c21cbfa9653c 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -711,6 +711,10 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder, LANE_CONFIGURATION_DUAL_LINK_B : LANE_CONFIGURATION_DUAL_LINK_A; }
if (intel_dsi->pixel_format != MIPI_DSI_FMT_RGB888)
temp |= DITHERING_ENABLE;
The docs say this was only made to work in C0 stepping. Not sure any BYT-Ts were ever shipped with B2/3, nor am I sure if setting the bit would have any effect there. IMO let's just set the bit and hope for the best.
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
Thank you, I've pushed patches 1 and 2 of this series to dinq.
Regards,
Hans
On devices with a burst_mode_ratio which is not 100 (1:1), the pclk will have a different value then drm_display_mode.clock .
On a Prowise PT301 tablet where vbt.lfp_lvds_vbt_mode.clock is 66100 and burst_mode_ratio is 130 this leads to the following errors:
[drm:pipe_config_err [i915]] *ERROR* mismatch in pixel_rate (expected 66100, found 86458) [drm:pipe_config_err [i915]] *ERROR* mismatch in base.adjusted_mode.crtc_clock (expected 66100, found 86458) [drm:pipe_config_err [i915]] *ERROR* mismatch in port_clock (expected 66100, found 86458)
This commit makes intel_dsi_compute_config() set pipe_config.adjusted_mode.crtc_clock, taking the burst_mode_ratio into account fixing this.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/vlv_dsi.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index c21cbfa9653c..d72ccf557a9c 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -347,6 +347,10 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder, return false; }
+ adjusted_mode->crtc_clock = + DIV_ROUND_UP(adjusted_mode->crtc_clock * + intel_dsi->burst_mode_ratio, 100); + pipe_config->clock_set = true;
return true;
On Sat, Dec 01, 2018 at 12:31:47PM +0100, Hans de Goede wrote:
On devices with a burst_mode_ratio which is not 100 (1:1), the pclk will have a different value then drm_display_mode.clock .
On a Prowise PT301 tablet where vbt.lfp_lvds_vbt_mode.clock is 66100 and burst_mode_ratio is 130 this leads to the following errors:
[drm:pipe_config_err [i915]] *ERROR* mismatch in pixel_rate (expected 66100, found 86458) [drm:pipe_config_err [i915]] *ERROR* mismatch in base.adjusted_mode.crtc_clock (expected 66100, found 86458) [drm:pipe_config_err [i915]] *ERROR* mismatch in port_clock (expected 66100, found 86458)
This commit makes intel_dsi_compute_config() set pipe_config.adjusted_mode.crtc_clock, taking the burst_mode_ratio into account fixing this.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/i915/vlv_dsi.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index c21cbfa9653c..d72ccf557a9c 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -347,6 +347,10 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder, return false; }
- adjusted_mode->crtc_clock =
DIV_ROUND_UP(adjusted_mode->crtc_clock *
intel_dsi->burst_mode_ratio, 100);
Hmm. Won't this cause incorrect refresh rate to be used in eg. vblank timestmap calculations?
OTOH if the pipe is really fetching data at the higher burst rate then we should rather want to calculate the watermarks/cdclk based on that higher rate. We do have pixel_rate in the crtc state but atm that is computed in generic code. We might have to push that to be encoder specific to do this correctly...
pipe_config->clock_set = true;
return true;
-- 2.19.1
Hi,
On 15-01-19 16:00, Ville Syrjälä wrote:
On Sat, Dec 01, 2018 at 12:31:47PM +0100, Hans de Goede wrote:
On devices with a burst_mode_ratio which is not 100 (1:1), the pclk will have a different value then drm_display_mode.clock .
On a Prowise PT301 tablet where vbt.lfp_lvds_vbt_mode.clock is 66100 and burst_mode_ratio is 130 this leads to the following errors:
[drm:pipe_config_err [i915]] *ERROR* mismatch in pixel_rate (expected 66100, found 86458) [drm:pipe_config_err [i915]] *ERROR* mismatch in base.adjusted_mode.crtc_clock (expected 66100, found 86458) [drm:pipe_config_err [i915]] *ERROR* mismatch in port_clock (expected 66100, found 86458)
This commit makes intel_dsi_compute_config() set pipe_config.adjusted_mode.crtc_clock, taking the burst_mode_ratio into account fixing this.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/i915/vlv_dsi.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index c21cbfa9653c..d72ccf557a9c 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -347,6 +347,10 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder, return false; }
- adjusted_mode->crtc_clock =
DIV_ROUND_UP(adjusted_mode->crtc_clock *
intel_dsi->burst_mode_ratio, 100);
Hmm. Won't this cause incorrect refresh rate to be used in eg. vblank timestmap calculations?
I guess so.
Note that this patch does not change any values actually written to the hardware. It seems that devices which actually use the burst mode are quite rare (this is the first one encounter in probably over 40 different byt/cht devices I've tested).
I've a feeling that the entire pipeline is actually running at the higher rate and that the framerate really is 30% higher.
Looking at the code, it seems that what a burst_mode_ratio of 130'does is make all the values in the "modeline" except for the visual area 30% larger, which means that we are probably already messing up the vblank calculations anyways, since using either the uncorrected or the corrected clock is wrong when using htotal from the original modeline, as looking at txbyteclkhs we will use bigger values for all drm_display_mode values except for the active region.
I think that the right way to deal with this is to isolate the burst_ratio handling to intel_dsi_vbt.c and adjust the modeline coming from the VBT by multiplying the clock and all timing parameters (except h/vdisplay) there by the burst_ratio and then recalculating h/vtotal.
This should lead to getting the vblank timestamp stuff right and allows removing of burst_mode_ratio from all code except for the vbt code.
If that is too invasive, given that this setup is quite rate, then I suggest we just go with this patch. My main concern is fixing the WARN_ON. This patch successfully does that.
OTOH if the pipe is really fetching data at the higher burst rate then we should rather want to calculate the watermarks/cdclk based on that higher rate.
Right, the more I think about this, the more I believe calculating a new modeline correcting for burst_ratio inside the vbt code and dropping burst_mode_ratio handling everywhere else is the right thing to do.
Regards,
Hans
Hi,
On 21-01-19 15:26, Hans de Goede wrote:
Hi,
On 15-01-19 16:00, Ville Syrjälä wrote:
On Sat, Dec 01, 2018 at 12:31:47PM +0100, Hans de Goede wrote:
On devices with a burst_mode_ratio which is not 100 (1:1), the pclk will have a different value then drm_display_mode.clock .
On a Prowise PT301 tablet where vbt.lfp_lvds_vbt_mode.clock is 66100 and burst_mode_ratio is 130 this leads to the following errors:
[drm:pipe_config_err [i915]] *ERROR* mismatch in pixel_rate (expected 66100, found 86458) [drm:pipe_config_err [i915]] *ERROR* mismatch in base.adjusted_mode.crtc_clock (expected 66100, found 86458) [drm:pipe_config_err [i915]] *ERROR* mismatch in port_clock (expected 66100, found 86458)
This commit makes intel_dsi_compute_config() set pipe_config.adjusted_mode.crtc_clock, taking the burst_mode_ratio into account fixing this.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/i915/vlv_dsi.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index c21cbfa9653c..d72ccf557a9c 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -347,6 +347,10 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder, return false; } + adjusted_mode->crtc_clock = + DIV_ROUND_UP(adjusted_mode->crtc_clock * + intel_dsi->burst_mode_ratio, 100);
Hmm. Won't this cause incorrect refresh rate to be used in eg. vblank timestmap calculations?
I guess so.
Note that this patch does not change any values actually written to the hardware. It seems that devices which actually use the burst mode are quite rare (this is the first one encounter in probably over 40 different byt/cht devices I've tested).
I've a feeling that the entire pipeline is actually running at the higher rate and that the framerate really is 30% higher.
Looking at the code, it seems that what a burst_mode_ratio of 130'does is make all the values in the "modeline" except for the visual area 30% larger, which means that we are probably already messing up the vblank calculations anyways, since using either the uncorrected or the corrected clock is wrong when using htotal from the original modeline, as looking at txbyteclkhs we will use bigger values for all drm_display_mode values except for the active region.
I think that the right way to deal with this is to isolate the burst_ratio handling to intel_dsi_vbt.c and adjust the modeline coming from the VBT by multiplying the clock and all timing parameters (except h/vdisplay) there by the burst_ratio and then recalculating h/vtotal.
This should lead to getting the vblank timestamp stuff right and allows removing of burst_mode_ratio from all code except for the vbt code.
If that is too invasive, given that this setup is quite rate, then I suggest we just go with this patch. My main concern is fixing the WARN_ON. This patch successfully does that.
OTOH if the pipe is really fetching data at the higher burst rate then we should rather want to calculate the watermarks/cdclk based on that higher rate.
Right, the more I think about this, the more I believe calculating a new modeline correcting for burst_ratio inside the vbt code and dropping burst_mode_ratio handling everywhere else is the right thing to do.
Regards,
Hans
p.s.
The 4th patch in this series is independent of the others, it fixes a small bug (not freeing a resource) in an exit error path which I noticed. It would be great if someone can review the 4th patch then I can push that one too and then this patch will be the only unmerged patch from this series.
Regards,
Hans
If we exit vlv_dsi_init() because we failed to find a fixed_mode, then we've already called drm_connector_init() and we should call drm_connector_cleanup() to unregister the connector object.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/vlv_dsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index d72ccf557a9c..7ca5aafcdf93 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -1861,7 +1861,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
if (!fixed_mode) { DRM_DEBUG_KMS("no fixed mode\n"); - goto err; + goto err_cleanup_connector; }
connector->display_info.width_mm = fixed_mode->width_mm; @@ -1874,6 +1874,8 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
return;
+err_cleanup_connector: + drm_connector_cleanup(&intel_connector->base); err: drm_encoder_cleanup(&intel_encoder->base); kfree(intel_dsi);
On Sat, Dec 01, 2018 at 12:31:48PM +0100, Hans de Goede wrote:
If we exit vlv_dsi_init() because we failed to find a fixed_mode, then we've already called drm_connector_init() and we should call drm_connector_cleanup() to unregister the connector object.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/i915/vlv_dsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index d72ccf557a9c..7ca5aafcdf93 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -1861,7 +1861,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
if (!fixed_mode) { DRM_DEBUG_KMS("no fixed mode\n");
goto err;
goto err_cleanup_connector;
}
connector->display_info.width_mm = fixed_mode->width_mm;
@@ -1874,6 +1874,8 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
return;
+err_cleanup_connector:
- drm_connector_cleanup(&intel_connector->base);
err: drm_encoder_cleanup(&intel_encoder->base); kfree(intel_dsi); -- 2.19.1
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:
- 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.
- 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().
- 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);
} else {pclk = bxt_dsi_get_pclk(encoder, pipe_config);
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:
- 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.
- 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().
- 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);
} else {pclk = bxt_dsi_get_pclk(encoder, pipe_config);
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:
- 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.
- 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().
- 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);
} else {pclk = bxt_dsi_get_pclk(encoder, pipe_config);
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:
- 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.
- 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().
- 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);
} else {pclk = bxt_dsi_get_pclk(encoder, pipe_config);
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
dri-devel@lists.freedesktop.org