>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Tuesday, November 6, 2018 6:43 AM >To: Navare, Manasi D <manasi.d.navare@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Jani Nikula ><jani.nikula@xxxxxxxxxxxxxxx>; Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; >Singh, Gaurav K <gaurav.k.singh@xxxxxxxxx> >Subject: Re: [PATCH v8 07/19] drm/i915/dp: Compute DSC pipe config in atomic >check > >On Fri, Nov 02, 2018 at 07:09:03PM -0700, Manasi Navare wrote: >> On Fri, Nov 02, 2018 at 02:31:26PM -0700, Manasi Navare wrote: >> > DSC params like the enable, compressed bpp, slice count and >> > dsc_split are added to the intel_crtc_state. These parameters are >> > set based on the requested mode and available link parameters during >> > the pipe configuration in atomic check phase. >> > These values are then later used to populate the remaining DSC and >> > RC parameters before enbaling DSC in atomic commit. >> > >> > v11: >> > * Const crtc_state, reject DSC on DP without FEC (Ville) >> > * Dont set dsc_split to false (Ville) >> > v10: >> > * Add a helper for dp_dsc support (Ville) >> > * Set pipe_config to max bpp, link params for DSC for now (Ville) >> > * Compute bpp - use dp dsc support helper (Ville) >> > v9: >> > * Rebase on top of drm-tip that now uses fast_narrow config for edp >> > (Manasi) >> > v8: >> > * Check for DSC bpc not 0 (manasi) >> > >> > v7: >> > * Fix indentation in compute_m_n (Manasi) >> > >> > v6 (From Gaurav): >> > * Remove function call of intel_dp_compute_dsc_params() and invoke >> > intel_dp_compute_dsc_params() in the patch where it is defined to >> > fix compilation warning (Gaurav) >> > >> > v5: >> > Add drm_dsc_cfg in intel_crtc_state (Manasi) >> > >> > v4: >> > * Rebase on refactoring of intel_dp_compute_config on tip (Manasi) >> > * Add a comment why we need to check PSR while enabling DSC (Gaurav) >> > >> > v3: >> > * Check PPR > max_cdclock to use 2 VDSC instances (Ville) >> > >> > v2: >> > * Add if-else for eDP/DP (Gaurav) >> > >> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> >> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> >> > Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> >> > Cc: Gaurav K Singh <gaurav.k.singh@xxxxxxxxx> >> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx> >> > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> >> > Acked-by: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> >> > --- >> > drivers/gpu/drm/i915/intel_display.c | 20 +++- >> > drivers/gpu/drm/i915/intel_display.h | 3 +- >> > drivers/gpu/drm/i915/intel_dp.c | 167 ++++++++++++++++++++++++--- >> > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- >> > 4 files changed, 166 insertions(+), 26 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_display.c >> > b/drivers/gpu/drm/i915/intel_display.c >> > index b219d5858160..477e53c37353 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.c >> > +++ b/drivers/gpu/drm/i915/intel_display.c >> > @@ -6442,7 +6442,7 @@ static int ironlake_fdi_compute_config(struct >> > intel_crtc *intel_crtc, >> > >> > pipe_config->fdi_lanes = lane; >> > >> > - intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock, >> > + intel_link_compute_m_n(pipe_config->pipe_bpp, 0, lane, >> > +fdi_dotclock, >> > link_bw, &pipe_config->fdi_m_n, false); >> > >> > ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, >> > pipe_config); @@ -6679,17 +6679,25 @@ static void >> > compute_m_n(unsigned int m, unsigned int n, } >> > >> > void >> > -intel_link_compute_m_n(int bits_per_pixel, int nlanes, >> > +intel_link_compute_m_n(int bits_per_pixel, uint16_t compressed_bpp, >> > + int nlanes, >> > int pixel_clock, int link_clock, >> > struct intel_link_m_n *m_n, >> > bool constant_n) >> > { >> > m_n->tu = 64; >> > >> > - compute_m_n(bits_per_pixel * pixel_clock, >> > - link_clock * nlanes * 8, >> > - &m_n->gmch_m, &m_n->gmch_n, >> > - constant_n); >> > + /* For DSC, Data M/N calculation uses compressed BPP */ >> > + if (compressed_bpp) >> > + compute_m_n(compressed_bpp * pixel_clock, >> > + link_clock * nlanes * 8, >> > + &m_n->gmch_m, &m_n->gmch_n, >> > + constant_n); >> > + else >> > + compute_m_n(bits_per_pixel * pixel_clock, >> > + link_clock * nlanes * 8, >> > + &m_n->gmch_m, &m_n->gmch_n, >> > + constant_n); >> > >> > compute_m_n(pixel_clock, link_clock, >> > &m_n->link_m, &m_n->link_n, >> > diff --git a/drivers/gpu/drm/i915/intel_display.h >> > b/drivers/gpu/drm/i915/intel_display.h >> > index 5d50decbcbb5..b0b23e1e9392 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.h >> > +++ b/drivers/gpu/drm/i915/intel_display.h >> > @@ -407,7 +407,8 @@ struct intel_link_m_n { >> > (__i)++) \ >> > for_each_if(plane) >> > >> > -void intel_link_compute_m_n(int bpp, int nlanes, >> > +void intel_link_compute_m_n(int bpp, uint16_t compressed_bpp, >> > + int nlanes, >> > int pixel_clock, int link_clock, >> > struct intel_link_m_n *m_n, >> > bool constant_n); >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c >> > b/drivers/gpu/drm/i915/intel_dp.c index b39b4bda8e40..58326fc9d935 >> > 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -47,6 +47,8 @@ >> > >> > /* DP DSC small joiner has 2 FIFOs each of 640 x 6 bytes */ >> > #define DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER 61440 >> > +#define DP_DSC_MIN_SUPPORTED_BPC 8 >> > +#define DP_DSC_MAX_SUPPORTED_BPC 10 >> > >> > /* DP DSC throughput values used for slice count calculations KPixels/s */ >> > #define DP_DSC_PEAK_PIXEL_RATE 2720000 >> > @@ -1840,6 +1842,29 @@ struct link_config_limits { >> > int min_bpp, max_bpp; >> > }; >> > >> > +static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp, >> > + const struct intel_crtc_state >*pipe_config) { >> > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); >> > + >> > + /* FIXME: FEC needed for external DP until then reject DSC on DP */ If we want to check for FEC support in intel_dp_supports_dsc(), this FIXME should be moved from here. Anusha >> > + if (!intel_dp_is_edp(intel_dp)) >> > + return false; >> > + >> > + return INTEL_GEN(dev_priv) >= 10 && >> > + pipe_config->cpu_transcoder != TRANSCODER_A; } >> > + >> > +static bool intel_dp_supports_dsc(struct intel_dp *intel_dp, >> > + const struct intel_crtc_state *pipe_config) { >> > + if (!intel_dp_source_supports_dsc(intel_dp, pipe_config) || >> > + !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) >> > + return false; >> > + >> > + return true; >> > +} >> > + >> > static int intel_dp_compute_bpp(struct intel_dp *intel_dp, >> > struct intel_crtc_state *pipe_config) { @@ - >1863,6 +1888,15 @@ >> > static int intel_dp_compute_bpp(struct intel_dp *intel_dp, >> > } >> > } >> > >> > + /* If DSC is supported, use the max value reported by panel */ >> > + if (intel_dp_supports_dsc(intel_dp, pipe_config)) { >> > + bpc = min_t(u8, >> > + drm_dp_dsc_sink_max_color_depth(intel_dp- >>dsc_dpcd), >> > + DP_DSC_MAX_SUPPORTED_BPC); >> > + if (bpc) >> > + bpp = min(bpp, 3*bpc); >> >> Ville, the problem with chosing the min between existing bpp and 3 * >> bpc (computed from DSC dpcd) is that EDID based bpc might be lower and >> in case DSC is supported, the bpc value should be based on the dsc >> color depth and that should override the value obtained from EDID. >> This was confirmed from the VDSC programming documents obtained from >> HW architecture folks >> >> So i think it should still be if DSC supported use the bpc directly >> from min_t(u8, >> drm_dp_dsc_sink_max_color_depth(intel_dp- >>dsc_dpcd),DP_DSC_MAX_SUPPORT >> ED_BPC); > >Maybe > >bpc_min = something; >bpc_max = something_else; >bpp = clamp(bpp, bpc_min*3, bpc_max*3); > >Either that or we just return an error of the requested bpp is below the dsc >minimum. > >> >> Manasi >> >> > + } >> > + >> > return bpp; >> > } >> > >> > @@ -1974,6 +2008,80 @@ intel_dp_compute_link_config_fast(struct >intel_dp *intel_dp, >> > return false; >> > } >> > >> > +static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp, >> > + struct intel_crtc_state *pipe_config, >> > + const struct link_config_limits *limits) { >> > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); >> > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); >> > + struct drm_display_mode *adjusted_mode = &pipe_config- >>base.adjusted_mode; >> > + u16 dsc_max_output_bpp = 0; >> > + u8 dsc_dp_slice_count = 0; >> > + >> > + if (!intel_dp_supports_dsc(intel_dp, pipe_config)) >> > + return false; >> > + >> > + /* DSC not supported for DSC sink BPC < 8 */ >> > + if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) { >> > + DRM_DEBUG_KMS("No DSC support for less than 8bpc\n"); >> > + return false; >> > + } >> > + >> > + /* >> > + * For now enable DSC for max bpp, max link rate, max lane count. >> > + * Optimize this later for the minimum possible link rate/lane count >> > + * with DSC enabled for the requested mode. >> > + */ >> > + pipe_config->pipe_bpp = limits->max_bpp; >> > + pipe_config->port_clock = intel_dp->common_rates[limits->max_clock]; >> > + pipe_config->lane_count = limits->max_lane_count; >> > + >> > + if (intel_dp_is_edp(intel_dp)) { >> > + pipe_config->dsc_params.compressed_bpp = >> > + drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> >4; >> > + pipe_config->dsc_params.slice_count = >> > + drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd, >> > + true); >> > + } else { >> > + dsc_max_output_bpp = >> > + intel_dp_dsc_get_output_bpp(pipe_config->port_clock, >> > + pipe_config->lane_count, >> > + adjusted_mode->crtc_clock, >> > + adjusted_mode- >>crtc_hdisplay); >> > + dsc_dp_slice_count = >> > + intel_dp_dsc_get_slice_count(intel_dp, >> > + adjusted_mode->crtc_clock, >> > + adjusted_mode- >>crtc_hdisplay); >> > + if (!(dsc_max_output_bpp && dsc_dp_slice_count)) { >> > + DRM_DEBUG_KMS("Compressed BPP/Slice Count not >supported\n"); >> > + return false; >> > + } >> > + pipe_config->dsc_params.compressed_bpp = >dsc_max_output_bpp >> 4; >> > + pipe_config->dsc_params.slice_count = dsc_dp_slice_count; >> > + } >> > + /* >> > + * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate >> > + * is greater than the maximum Cdclock and if slice count is even >> > + * then we need to use 2 VDSC instances. >> > + */ >> > + if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) { >> > + if (pipe_config->dsc_params.slice_count > 1) { >> > + pipe_config->dsc_params.dsc_split = true; >> > + } else { >> > + DRM_DEBUG_KMS("Cannot split stream to use 2 VDSC >instances\n"); >> > + return false; >> > + } >> > + } >> > + pipe_config->dsc_params.compression_enable = true; >> > + DRM_DEBUG_KMS("DP DSC computed with Input Bpp = %d " >> > + "Compressed Bpp = %d Slice Count = %d\n", >> > + pipe_config->pipe_bpp, >> > + pipe_config->dsc_params.compressed_bpp, >> > + pipe_config->dsc_params.slice_count); >> > + >> > + return true; >> > +} >> > + >> > static bool >> > intel_dp_compute_link_config(struct intel_encoder *encoder, >> > struct intel_crtc_state *pipe_config) @@ -1982,6 >+2090,7 @@ >> > intel_dp_compute_link_config(struct intel_encoder *encoder, >> > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> > struct link_config_limits limits; >> > int common_len; >> > + bool ret = false; >> > >> > common_len = intel_dp_common_len_rate_limit(intel_dp, >> > intel_dp->max_link_rate); >@@ -1995,8 +2104,12 @@ >> > intel_dp_compute_link_config(struct intel_encoder *encoder, >> > limits.min_lane_count = 1; >> > limits.max_lane_count = intel_dp_max_lane_count(intel_dp); >> > >> > - limits.min_bpp = 6 * 3; >> > limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config); >> > + if (intel_dp_supports_dsc(intel_dp, pipe_config) && >> > + limits.max_bpp >= DP_DSC_MIN_SUPPORTED_BPC*3) >> > + limits.min_bpp = DP_DSC_MIN_SUPPORTED_BPC * 3; >> > + else >> > + limits.min_bpp = 6 * 3; >> > >> > if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) { >> > /* >> > @@ -2020,7 +2133,7 @@ intel_dp_compute_link_config(struct intel_encoder >*encoder, >> > intel_dp->common_rates[limits.max_clock], >> > limits.max_bpp, adjusted_mode->crtc_clock); >> > >> > - if (intel_dp_is_edp(intel_dp)) { >> > + if (intel_dp_is_edp(intel_dp)) >> > /* >> > * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4 >> > * section A.1: "It is recommended that the minimum number of >@@ >> > -2030,26 +2143,42 @@ intel_dp_compute_link_config(struct intel_encoder >*encoder, >> > * Note that we use the max clock and lane count for eDP 1.3 >and >> > * earlier, and fast vs. wide is irrelevant. >> > */ >> > - if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config, >> > - &limits)) >> > - return false; >> > - } else { >> > + ret = intel_dp_compute_link_config_fast(intel_dp, pipe_config, >> > + &limits); >> > + else >> > /* Optimize for slow and wide. */ >> > - if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, >> > - &limits)) >> > + ret = intel_dp_compute_link_config_wide(intel_dp, pipe_config, >> > + &limits); >> > + >> > + /* enable compression if the mode doesn't fit available BW */ >> > + if (!ret) { >> > + if (!intel_dp_dsc_compute_config(intel_dp, pipe_config, >> > + &limits)) >> > return false; >> > } >> > >> > - DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n", >> > - pipe_config->lane_count, pipe_config->port_clock, >> > - pipe_config->pipe_bpp); >> > + if (pipe_config->dsc_params.compression_enable) { >> > + DRM_DEBUG_KMS("DP lane count %d clock %d Input bpp %d >Compressed bpp %d\n", >> > + pipe_config->lane_count, pipe_config->port_clock, >> > + pipe_config->pipe_bpp, >> > + pipe_config->dsc_params.compressed_bpp); >> > >> > - DRM_DEBUG_KMS("DP link rate required %i available %i\n", >> > - intel_dp_link_required(adjusted_mode->crtc_clock, >> > - pipe_config->pipe_bpp), >> > - intel_dp_max_data_rate(pipe_config->port_clock, >> > - pipe_config->lane_count)); >> > + DRM_DEBUG_KMS("DP link rate required %i available %i\n", >> > + intel_dp_link_required(adjusted_mode->crtc_clock, >> > + pipe_config- >>dsc_params.compressed_bpp), >> > + intel_dp_max_data_rate(pipe_config->port_clock, >> > + pipe_config->lane_count)); >> > + } else { >> > + DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n", >> > + pipe_config->lane_count, pipe_config->port_clock, >> > + pipe_config->pipe_bpp); >> > >> > + DRM_DEBUG_KMS("DP link rate required %i available %i\n", >> > + intel_dp_link_required(adjusted_mode->crtc_clock, >> > + pipe_config->pipe_bpp), >> > + intel_dp_max_data_rate(pipe_config->port_clock, >> > + pipe_config->lane_count)); >> > + } >> > return true; >> > } >> > >> > @@ -2133,7 +2262,9 @@ intel_dp_compute_config(struct intel_encoder >*encoder, >> > intel_conn_state->broadcast_rgb == >INTEL_BROADCAST_RGB_LIMITED; >> > } >> > >> > - intel_link_compute_m_n(pipe_config->pipe_bpp, pipe_config- >>lane_count, >> > + intel_link_compute_m_n(pipe_config->pipe_bpp, >> > + pipe_config->dsc_params.compressed_bpp, >> > + pipe_config->lane_count, >> > adjusted_mode->crtc_clock, >> > pipe_config->port_clock, >> > &pipe_config->dp_m_n, >> > @@ -2142,7 +2273,7 @@ intel_dp_compute_config(struct intel_encoder >*encoder, >> > if (intel_connector->panel.downclock_mode != NULL && >> > dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) { >> > pipe_config->has_drrs = true; >> > - intel_link_compute_m_n(pipe_config->pipe_bpp, >> > + intel_link_compute_m_n(pipe_config->pipe_bpp, 0, >> > pipe_config->lane_count, >> > intel_connector- >>panel.downclock_mode->clock, >> > pipe_config->port_clock, diff --git >> > a/drivers/gpu/drm/i915/intel_dp_mst.c >> > b/drivers/gpu/drm/i915/intel_dp_mst.c >> > index 8b71d64ebd9d..e02caedd443c 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c >> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c >> > @@ -90,7 +90,7 @@ static bool intel_dp_mst_compute_config(struct >intel_encoder *encoder, >> > } >> > } >> > >> > - intel_link_compute_m_n(bpp, lane_count, >> > + intel_link_compute_m_n(bpp, 0, lane_count, >> > adjusted_mode->crtc_clock, >> > pipe_config->port_clock, >> > &pipe_config->dp_m_n, >> > -- >> > 2.18.0 >> > > >-- >Ville Syrjälä >Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
