Re: [v7 1/4] i915/dp/fec: Add fec_enable to the crtc state.

[Manasi Navare <manasi.d.navare@intel.com>]

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.syrjala@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx