On Wed, 20 Nov 2019, "Kulkarni, Vandita" <vandita.kulkarni@xxxxxxxxx> wrote:
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@xxxxxxxxx>
>> Sent: Friday, November 15, 2019 9:04 PM
>> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>; Kulkarni, Vandita
>> <vandita.kulkarni@xxxxxxxxx>; ville.syrjala@xxxxxxxxxxxxxxx
>> Subject: [PATCH v2 10/10] drm/i915/dsi: add support for DSC
>>
>> Enable DSC for DSI, if specified in VBT.
>>
>> This is now excessively dynamic, being enabled at compute config. I don't
>> expect us to need to switch between DSC and non-DSC for the same panel.
>> Cargo culting the DP DSC shows.
>>
>> Mode valid lacks a sensible implementation, as does get config.
>>
>> v3: take compressed bpp into account
>>
>> v2: Nuke conn_state->max_requested_bpc, it's not used on DSI
>>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>>
>> ---
>>
>> The burst mode stuff wrt DSC are still whatever, but at least we should be
>> taking DSC better into account in port clock calculations.
>> ---
>> drivers/gpu/drm/i915/display/icl_dsi.c | 113 ++++++++++++++++++++-----
>> 1 file changed, 94 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
>> b/drivers/gpu/drm/i915/display/icl_dsi.c
>> index d576f29cef75..dc87134f5c27 100644
>> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
>> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
>> @@ -34,6 +34,7 @@
>> #include "intel_ddi.h"
>> #include "intel_dsi.h"
>> #include "intel_panel.h"
>> +#include "intel_vdsc.h"
>>
>> static inline int header_credits_available(struct drm_i915_private *dev_priv,
>> enum transcoder dsi_trans)
>> @@ -302,17 +303,22 @@ static void configure_dual_link_mode(struct
>> intel_encoder *encoder, }
>>
>> /* aka DSI 8X clock */
>> -static int afe_clk(struct intel_encoder *encoder)
>> +static int afe_clk(struct intel_encoder *encoder,
>> + const struct intel_crtc_state *crtc_state)
>> {
>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>> int bpp;
>>
>> - bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format);
>> + if (crtc_state->dsc.compression_enable)
>> + bpp = crtc_state->dsc.compressed_bpp;
>> + else
>> + bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi-
>> >pixel_format);
>>
>> return DIV_ROUND_CLOSEST(intel_dsi->pclk * bpp, intel_dsi-
>> >lane_count); }
>>
>> -static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder)
>> +static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder,
>> + const struct intel_crtc_state
>> *crtc_state)
>> {
>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); @@ -
>> 320,7 +326,7 @@ static void gen11_dsi_program_esc_clk_div(struct
>> intel_encoder *encoder)
>> int afe_clk_khz;
>> u32 esc_clk_div_m;
>>
>> - afe_clk_khz = afe_clk(encoder);
>> + afe_clk_khz = afe_clk(encoder, crtc_state);
>> esc_clk_div_m = DIV_ROUND_UP(afe_clk_khz, DSI_MAX_ESC_CLK);
>>
>> for_each_dsi_port(port, intel_dsi->ports) { @@ -498,7 +504,9 @@
>> static void gen11_dsi_enable_ddi_buffer(struct intel_encoder *encoder)
>> }
>> }
>>
>> -static void gen11_dsi_setup_dphy_timings(struct intel_encoder *encoder)
>> +static void
>> +gen11_dsi_setup_dphy_timings(struct intel_encoder *encoder,
>> + const struct intel_crtc_state *crtc_state)
>> {
>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); @@ -
>> 539,7 +547,7 @@ static void gen11_dsi_setup_dphy_timings(struct
>> intel_encoder *encoder)
>> * leave all fields at HW default values.
>> */
>> if (IS_GEN(dev_priv, 11)) {
>> - if (afe_clk(encoder) <= 800000) {
>> + if (afe_clk(encoder, crtc_state) <= 800000) {
>> for_each_dsi_port(port, intel_dsi->ports) {
>> tmp =
>> I915_READ(DPHY_TA_TIMING_PARAM(port));
>> tmp &= ~TA_SURE_MASK;
>> @@ -649,7 +657,7 @@ gen11_dsi_configure_transcoder(struct intel_encoder
>> *encoder,
>> tmp |= EOTP_DISABLED;
>>
>> /* enable link calibration if freq > 1.5Gbps */
>> - if (afe_clk(encoder) >= 1500 * 1000) {
>> + if (afe_clk(encoder, pipe_config) >= 1500 * 1000) {
>> tmp &= ~LINK_CALIBRATION_MASK;
>> tmp |= CALIBRATION_ENABLED_INITIAL_ONLY;
>> }
>> @@ -915,7 +923,8 @@ static void gen11_dsi_enable_transcoder(struct
>> intel_encoder *encoder)
>> }
>> }
>>
>> -static void gen11_dsi_setup_timeouts(struct intel_encoder *encoder)
>> +static void gen11_dsi_setup_timeouts(struct intel_encoder *encoder,
>> + const struct intel_crtc_state *crtc_state)
>> {
>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); @@ -
>> 930,7 +939,7 @@ static void gen11_dsi_setup_timeouts(struct intel_encoder
>> *encoder)
>> * TIME_NS = (BYTE_CLK_COUNT * 8 * 10^6)/ Bitrate
>> * ESCAPE_CLK_COUNT = TIME_NS/ESC_CLK_NS
>> */
>> - divisor = intel_dsi_tlpx_ns(intel_dsi) * afe_clk(encoder) * 1000;
>> + divisor = intel_dsi_tlpx_ns(intel_dsi) * afe_clk(encoder, crtc_state)
>> +* 1000;
>> mul = 8 * 1000000;
>> hs_tx_timeout = DIV_ROUND_UP(intel_dsi->hs_tx_timeout * mul,
>> divisor);
>> @@ -966,7 +975,7 @@ static void gen11_dsi_setup_timeouts(struct
>> intel_encoder *encoder)
>>
>> static void
>> gen11_dsi_enable_port_and_phy(struct intel_encoder *encoder,
>> - const struct intel_crtc_state *pipe_config)
>> + const struct intel_crtc_state *crtc_state)
>> {
>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>
>> @@ -983,13 +992,13 @@ gen11_dsi_enable_port_and_phy(struct
>> intel_encoder *encoder,
>> gen11_dsi_enable_ddi_buffer(encoder);
>>
>> /* setup D-PHY timings */
>> - gen11_dsi_setup_dphy_timings(encoder);
>> + gen11_dsi_setup_dphy_timings(encoder, crtc_state);
>>
>> /* step 4h: setup DSI protocol timeouts */
>> - gen11_dsi_setup_timeouts(encoder);
>> + gen11_dsi_setup_timeouts(encoder, crtc_state);
>>
>> /* Step (4h, 4i, 4j, 4k): Configure transcoder */
>> - gen11_dsi_configure_transcoder(encoder, pipe_config);
>> + gen11_dsi_configure_transcoder(encoder, crtc_state);
>>
>> /* Step 4l: Gate DDI clocks */
>> if (IS_GEN(dev_priv, 11))
>> @@ -1036,14 +1045,14 @@ static void gen11_dsi_powerup_panel(struct
>> intel_encoder *encoder) }
>>
>> static void gen11_dsi_pre_pll_enable(struct intel_encoder *encoder,
>> - const struct intel_crtc_state *pipe_config,
>> + const struct intel_crtc_state *crtc_state,
>> const struct drm_connector_state
>> *conn_state) {
>> /* step2: enable IO power */
>> gen11_dsi_enable_io_power(encoder);
>>
>> /* step3: enable DSI PLL */
>> - gen11_dsi_program_esc_clk_div(encoder);
>> + gen11_dsi_program_esc_clk_div(encoder, crtc_state);
>> }
>>
>> static void gen11_dsi_pre_enable(struct intel_encoder *encoder, @@ -
>> 1061,6 +1070,9 @@ static void gen11_dsi_pre_enable(struct intel_encoder
>> *encoder,
>> /* step5: program and powerup panel */
>> gen11_dsi_powerup_panel(encoder);
>>
>> + /* FIXME: location? */
>> + intel_dsc_enable(encoder, pipe_config);
>> +
>> /* step6c: configure transcoder timings */
>> gen11_dsi_set_transcoder_timings(encoder, pipe_config);
>>
>> @@ -1222,6 +1234,13 @@ static void gen11_dsi_disable(struct
>> intel_encoder *encoder,
>> gen11_dsi_disable_io_power(encoder);
>> }
>>
>> +static enum drm_mode_status gen11_dsi_mode_valid(struct
>> drm_connector *connector,
>> + struct drm_display_mode
>> *mode)
>> +{
>> + /* FIXME: DSC? */
>> + return intel_dsi_mode_valid(connector, mode); }
>> +
>> static void gen11_dsi_get_timings(struct intel_encoder *encoder,
>> struct intel_crtc_state *pipe_config) { @@ -
>> 1269,6 +1288,53 @@ static void gen11_dsi_get_config(struct intel_encoder
>> *encoder,
>> pipe_config->pipe_bpp = bdw_get_pipemisc_bpp(crtc); }
>>
>> +static int gen11_dsi_dsc_compute_config(struct intel_encoder *encoder,
>> + struct intel_crtc_state *pipe_config) {
>> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> + struct drm_dsc_config *vdsc_cfg = &pipe_config->dsc.config;
>> + struct drm_display_mode *adjusted_mode = &pipe_config-
>> >hw.adjusted_mode;
>> + int dsc_max_bpc = INTEL_GEN(dev_priv) >= 12 ? 12 : 10;
>> + bool use_dsc;
>> + int ret;
>> +
>> + use_dsc = intel_bios_get_dsc_params(encoder, pipe_config,
>> dsc_max_bpc);
>> + if (!use_dsc)
>> + return 0;
>> +
>> + if (pipe_config->pipe_bpp < 8 * 3)
>> + return -EINVAL;
>> +
>> + if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) {
>> + if (pipe_config->dsc.slice_count > 1) {
>> + pipe_config->dsc.dsc_split = true;
>> + } else {
>> + DRM_DEBUG_KMS("Cannot split stream to use 2
>> VDSC instances\n");
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + vdsc_cfg->convert_rgb = false;
>> +
>> + ret = intel_dsc_compute_params(encoder, pipe_config);
>> + if (ret)
>> + return ret;
>> +
>> + /* DSI specific sanity checks on the common code */
>> + WARN_ON(vdsc_cfg->vbr_enable);
>> + WARN_ON(vdsc_cfg->pic_width % vdsc_cfg->slice_width);
>> + WARN_ON(vdsc_cfg->slice_height < 8);
>> + WARN_ON(vdsc_cfg->pic_height % vdsc_cfg->slice_height);
>> +
>> + ret = drm_dsc_compute_rc_parameters(vdsc_cfg);
>> + if (ret)
>> + return ret;
>> +
>> + pipe_config->dsc.compression_enable = true;
>> +
>> + return 0;
>> +}
>> +
>> static int gen11_dsi_compute_config(struct intel_encoder *encoder,
>> struct intel_crtc_state *pipe_config,
>> struct drm_connector_state *conn_state)
>> @@ -1300,7 +1366,11 @@ static int gen11_dsi_compute_config(struct
>> intel_encoder *encoder,
>> pipe_config->pipe_bpp = 18;
>
> TRANS_DSI_FUNC_CONF register needs to be updated with pixel format as
> compressed(0x6)
Absolutely, this was a bit of a facepalm for me, as well as the
horizontal timing changes. Thanks for catching.
On that note, burst mode is also broken for ICL DSI, but that's another
worry for another day.
> Should we be having pipe_config->output_format = compressed ?
No, I think we need the real value there for actual pre-DSC engine pipe
stuff.
I'll send a couple of more patches in reply to the cover letter for
testing. They'll need to be made part of the series better but I'm too
tired now.
BR,
Jani.
>
> Thanks,
> Vandita
>>
>> pipe_config->clock_set = true;
>> - pipe_config->port_clock = afe_clk(encoder) / 5;
>> +
>> + if (gen11_dsi_dsc_compute_config(encoder, pipe_config))
>> + DRM_DEBUG_KMS("Attempting to use DSC failed\n");
>> +
>> + pipe_config->port_clock = afe_clk(encoder, pipe_config) / 5;
>>
>> return 0;
>> }
>> @@ -1308,8 +1378,13 @@ static int gen11_dsi_compute_config(struct
>> intel_encoder *encoder, static void gen11_dsi_get_power_domains(struct
>> intel_encoder *encoder,
>> struct intel_crtc_state *crtc_state) {
>> - get_dsi_io_power_domains(to_i915(encoder->base.dev),
>> - enc_to_intel_dsi(&encoder->base));
>> + struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>> +
>> + get_dsi_io_power_domains(i915, enc_to_intel_dsi(&encoder-
>> >base));
>> +
>> + if (crtc_state->dsc.compression_enable)
>> + intel_display_power_get(i915,
>> +
>> intel_dsc_power_domain(crtc_state));
>> }
>>
>> static bool gen11_dsi_get_hw_state(struct intel_encoder *encoder, @@ -
>> 1379,7 +1454,7 @@ static const struct drm_connector_funcs
>> gen11_dsi_connector_funcs = {
>>
>> static const struct drm_connector_helper_funcs
>> gen11_dsi_connector_helper_funcs = {
>> .get_modes = intel_dsi_get_modes,
>> - .mode_valid = intel_dsi_mode_valid,
>> + .mode_valid = gen11_dsi_mode_valid,
>> .atomic_check = intel_digital_connector_atomic_check,
>> };
>>
>> --
>> 2.20.1
>
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
