>-----Original Message----- >From: Navare, Manasi D >Sent: Tuesday, November 6, 2018 6:40 PM >To: Srivatsa, Anusha <anusha.sriva...@intel.com> >Cc: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Ville >Syrjala ><ville.syrj...@linux.intel.com>; Jani Nikula <jani.nik...@linux.intel.com> >Subject: Re: [v7 1/4] i915/dp/fec: Add fec_enable to the crtc state. > >On Tue, Nov 06, 2018 at 04:31:19PM -0800, Anusha Srivatsa wrote: >> For DP 1.4 and above, Display Stream compression can be enabled only >> if Forward Error Correctin can be performed. >> >> Add a crtc state for FEC. Currently, the state is determined by >> platform, DP and DSC being enabled. Moving forward we can use the >> state to have error correction on other scenarios too if needed. >> >> v2: >> - Control compression_enable with the fec_enable parameter in crtc >> state and with intel_dp_supports_fec() >> (Ville) >> >> - intel_dp_can_fec()/intel_dp_supports_fec()(manasi) >> >> v3: Check for FEC support along with setting crtc state. >> >> v4: add checks to intel_dp_source_supports_dsc.(manasi) >> - Move intel_dp_supports_fec() closer to >> intel_dp_supports_dsc() (Anusha) >> >> v5: Move fec check to intel_dp_supports_dsc(Ville) >> >> v6: Remove warning. rebase. >> >> v7: change crtc state to include DP sink and fec capability of >> source.(Manasi) >> >> Suggested-by: Ville Syrjala <ville.syrj...@linux.intel.com> >> Cc: dri-devel@lists.freedesktop.org >> Cc: Ville Syrjala <ville.syrj...@linux.intel.com> >> Cc: Jani Nikula <jani.nik...@linux.intel.com> >> Cc: Manasi Navare <manasi.d.nav...@intel.com> >> Signed-off-by: Anusha Srivatsa <anusha.sriva...@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 31 +++++++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/intel_drv.h | 3 +++ >> 2 files changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c index 73c00c5acf14..f764c45deaab >> 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -545,7 +545,7 @@ intel_dp_mode_valid(struct drm_connector >*connector, >> dsc_slice_count = >> drm_dp_dsc_sink_max_slice_count(intel_dp- >>dsc_dpcd, >> true); >> - } else { >> + } else if (drm_dp_sink_supports_fec(intel_dp->fec_capable)) { >> dsc_max_output_bpp = >> intel_dp_dsc_get_output_bpp(max_link_clock, >> max_lanes, >> @@ -1710,12 +1710,27 @@ struct link_config_limits { >> int min_bpp, max_bpp; >> }; >> >> +static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp, >> + const struct intel_crtc_state >*pipe_config) { >> + 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); >> + >> + return INTEL_GEN(dev_priv) >= 11 && pipe_config->cpu_transcoder != >> +TRANSCODER_A; } >> + >> +static bool intel_dp_supports_fec(struct intel_dp *intel_dp, >> + const struct intel_crtc_state *pipe_config) { >> + return intel_dp_source_supports_fec(intel_dp, pipe_config) && >> + drm_dp_sink_supports_fec(intel_dp->fec_capable); >> +} >> + >> 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 (!intel_dp_is_edp(intel_dp)) >> return false; > >No point in keeping this !edp condition here, thats supposed to go with FEC >check, move that to where fec_supports is added. > >> >> @@ -1726,6 +1741,9 @@ static bool intel_dp_source_supports_dsc(struct >> intel_dp *intel_dp, static bool intel_dp_supports_dsc(struct intel_dp >> *intel_dp, >> const struct intel_crtc_state *pipe_config) { >> + if (!pipe_config->fec_enable) >> + return false; > >I think its better to use intel_dp_supports_fec(intel_dp, pipe_config) && !edp >instead of pipe_config->fec_enable because pipe_config->fec_enable state will >be set only in dp_compute_config > >> + >> if (!intel_dp_source_supports_dsc(intel_dp, pipe_config) || >> !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) >> return false; >> @@ -1886,9 +1904,18 @@ static bool intel_dp_dsc_compute_config(struct >intel_dp *intel_dp, >> u16 dsc_max_output_bpp = 0; >> u8 dsc_dp_slice_count = 0; >> >> + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && >> + intel_dp_supports_fec(intel_dp, pipe_config); > >Why is pipe_config->fec_enable set in dsc_compute_config()? This state is >totally >indepenedent of dsc state and we are treating this as an independent feature. I >think the earlier place of setting this in intel_dp_compute_link_config() is >correct
How about, maybe adding the fec_enable state in intel_dp_compute_config(). Most of the pipe_config parameters are being set there. So, by the time we first check for dsc, we know if FEC support is there or not. >Manasi > >> + >> if (!intel_dp_supports_dsc(intel_dp, pipe_config)) >> return false; >> >> + /* DSC not supported if external DP sink does not support FEC */ >> + if (!pipe_config->fec_enable) { >> + DRM_DEBUG_KMS("Sink does not support Forward Error >Correction, disabling Display Compression\n"); >> + 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"); diff - >-git >> a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index dd22cdeaa673..997bea5fdf16 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -945,6 +945,9 @@ struct intel_crtc_state { >> u8 slice_count; >> } dsc_params; >> struct drm_dsc_config dp_dsc_cfg; >> + >> + /* Forward Error correction State */ >> + bool fec_enable; >> }; >> >> struct intel_crtc { >> -- >> 2.19.1 >> _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel