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

[Srivatsa, Anusha]

>-----Original Message-----
>From: Navare, Manasi D
>Sent: Tuesday, November 6, 2018 6:40 PM
>To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Ville Syrjala
><ville.syrjala at linux.intel.com>; Jani Nikula <jani.nikula at 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.syrjala at linux.intel.com>
>> Cc: dri-devel at lists.freedesktop.org
>> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> Cc: Manasi Navare <manasi.d.navare at intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa at 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
>>