[Intel-gfx] [v4 3/7] i915/dp/fec: Check for FEC Support

[Srivatsa, Anusha]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>Sent: Wednesday, October 31, 2018 2:01 PM
>To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh at intel.com>;
>Jani Nikula <jani.nikula at linux.intel.com>; Navare, Manasi D
><manasi.d.navare at intel.com>; Pandiyan, Dhinakaran
><dhinakaran.pandiyan at intel.com>
>Subject: Re: [v4 3/7] i915/dp/fec: Check for FEC Support
>
>On Tue, Oct 30, 2018 at 05:45:13PM -0700, Anusha Srivatsa wrote:
>> For DP 1.4 and above, Display Stream compression can be enabled only
>> if Forward Error Correctin can be performed.
>>
>> Check if the sink supports FEC using the helper.
>>
>> v2: Mention External DP where ever FEC is mentioned in the code.Check
>> return status of dpcd reads. (Gaurav)
>> - Do regular mode check even if FEC is not supported. (manasi)
>>
>> v3: Do not perform any dpcd writes in the atomic check phase. (DK,
>> Manasi)
>>
>> v4: Use debug level logging for scenario where sink does not support a
>> feature. (DK)
>>
>> v5: Correct commit message. rebase.
>>
>> v6: pass single field instead of an array for helper function.
>> (manasi)
>>
>> Cc: Gaurav K Singh <gaurav.k.singh at intel.com>
>> 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>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> Reviewed-by: Manasi Navare <manasi.d.navare at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c index 8ae7cf3d4ee1..a344be555dd6
>> 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -680,7 +680,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,
>> @@ -690,7 +690,8 @@ intel_dp_mode_valid(struct drm_connector
>*connector,
>>  				intel_dp_dsc_get_slice_count(intel_dp,
>>  							     target_clock,
>>  							     mode->hdisplay);
>> -		}
>> +		} else
>> +			DRM_DEBUG_KMS("Sink device does not Support
>FEC\n");
>
>Please no debug prints in the mode_valid() that would spam a lot.
>
>Not enough context in the patch for me to see what happens if FEC isn't
>supported. Do we just check against the uncompressed limits?

In cases where we cannot do FEC for DP cases, we should be disabling DSC.
Combining the checking of FEC support (this patch) with the patch 4 of the series which checks for state and disables DSC suitably.

Anusha 
>>  	}
>>
>>  	if ((mode_rate > max_rate && !(dsc_max_output_bpp &&
>> dsc_slice_count)) || @@ -2063,6 +2064,13 @@ static bool
>intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>  	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
>>  		return false;
>>
>> +	/* DSC not supported if external DP sink does not support FEC */
>> +	if (!intel_dp_is_edp(intel_dp) &&
>> +	    !drm_dp_sink_supports_fec(intel_dp->fec_capable)) {
>
>Why aren't we checking the source capabilities here as well?
>	
>> +		DRM_DEBUG_KMS("Sink does not support Forward Error
>Correction,
>> +disabling Display Compression\n");
>
>"disabling Display Compression" is not true. We're rejecting the entire thing.
>
>> +		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");
>> --
>> 2.17.1
>
>--
>Ville Syrjälä
>Intel