[Intel-gfx] [PATCH 09/20] drm/i915/dp: Extract intel_dp_tmds_clock_valid()

[Nautiyal, Ankit K]

On 10/15/2021 7:09 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> We're currently duplicating the DFP min/max TMDS clock checks
> in .mode_valid() and .compute_config(). Extract a helper suitable
> for both use cases.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dp.c | 59 +++++++++++--------------
>   1 file changed, 26 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 45e4bf54e1de..b3b8e74fac9c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -781,6 +781,25 @@ static bool intel_dp_hdisplay_bad(struct drm_i915_private *dev_priv,
>   	return hdisplay == 4096 && !HAS_DDI(dev_priv);
>   }
>   
> +static enum drm_mode_status
> +intel_dp_tmds_clock_valid(struct intel_dp *intel_dp,
> +			  int clock, int bpc, bool ycbcr420_output)
> +{
> +	int tmds_clock;
> +
> +	tmds_clock = intel_hdmi_tmds_clock(clock, bpc, ycbcr420_output);
> +
> +	if (intel_dp->dfp.min_tmds_clock &&
> +	    tmds_clock < intel_dp->dfp.min_tmds_clock)
> +		return MODE_CLOCK_LOW;
> +
> +	if (intel_dp->dfp.max_tmds_clock &&
> +	    tmds_clock > intel_dp->dfp.max_tmds_clock)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}


This looks good to me, a common helper to check if the tmds clock 
calculated for the the bpc selected and 420 format is within the limits 
of the DFP tmds limitations.

There are however some HDMI2.1 protocol converters that support higher 
mode with Fixed Rate Link (where the TMDS clock lane is used as an 
additional lane with hdmi2.1 sinks)

In that case, we would need to skip the tmds check, as the TMDS clock 
will not be sufficient for modes that can be supported with FRL mode, 
and all those higher modes will get pruned.

These PCONs will have additional fields in DPCD caps for maximum FRL 
rate in Gbps (stored in dfp->max_frl_rate), which we can use to check if 
the mode rate would be supported, if FRL mode is used.

I was wondering if we add a similar check for this case or add another 
argument to this function "is_frl_mode" and have the bw check there.


Regards,

Ankit



> +
>   static enum drm_mode_status
>   intel_dp_mode_valid_downstream(struct intel_connector *connector,
>   			       const struct drm_display_mode *mode,
> @@ -788,7 +807,6 @@ intel_dp_mode_valid_downstream(struct intel_connector *connector,
>   {
>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
>   	const struct drm_display_info *info = &connector->base.display_info;
> -	int tmds_clock;
>   
>   	/* If PCON supports FRL MODE, check FRL bandwidth constraints */
>   	if (intel_dp->dfp.pcon_max_frl_bw) {
> @@ -814,17 +832,8 @@ intel_dp_mode_valid_downstream(struct intel_connector *connector,
>   		return MODE_CLOCK_HIGH;
>   
>   	/* Assume 8bpc for the DP++/HDMI/DVI TMDS clock check */
> -	tmds_clock = intel_hdmi_tmds_clock(target_clock, 8,
> -					   drm_mode_is_420_only(info, mode));
> -
> -	if (intel_dp->dfp.min_tmds_clock &&
> -	    tmds_clock < intel_dp->dfp.min_tmds_clock)
> -		return MODE_CLOCK_LOW;
> -	if (intel_dp->dfp.max_tmds_clock &&
> -	    tmds_clock > intel_dp->dfp.max_tmds_clock)
> -		return MODE_CLOCK_HIGH;
> -
> -	return MODE_OK;
> +	return intel_dp_tmds_clock_valid(intel_dp, target_clock, 8,
> +					 drm_mode_is_420_only(info, mode));
>   }
>   
>   static bool intel_dp_need_bigjoiner(struct intel_dp *intel_dp,
> @@ -1069,32 +1078,16 @@ static bool intel_dp_hdmi_ycbcr420(struct intel_dp *intel_dp,
>   		 intel_dp->dfp.ycbcr_444_to_420);
>   }
>   
> -static bool intel_dp_hdmi_tmds_clock_valid(struct intel_dp *intel_dp,
> -					   const struct intel_crtc_state *crtc_state, int bpc)
> -{
> -	int clock = crtc_state->hw.adjusted_mode.crtc_clock;
> -	int tmds_clock = intel_hdmi_tmds_clock(clock, bpc,
> -					       intel_dp_hdmi_ycbcr420(intel_dp, crtc_state));
> -
> -	if (intel_dp->dfp.min_tmds_clock &&
> -	    tmds_clock < intel_dp->dfp.min_tmds_clock)
> -		return false;
> -
> -	if (intel_dp->dfp.max_tmds_clock &&
> -	    tmds_clock > intel_dp->dfp.max_tmds_clock)
> -		return false;
> -
> -	return true;
> -}
> -
>   static bool intel_dp_hdmi_bpc_possible(struct intel_dp *intel_dp,
>   				       const struct intel_crtc_state *crtc_state,
>   				       int bpc)
>   {
> +	bool ycbcr420_output = intel_dp_hdmi_ycbcr420(intel_dp, crtc_state);
> +	int clock = crtc_state->hw.adjusted_mode.crtc_clock;
>   
> -	return intel_hdmi_bpc_possible(crtc_state, bpc, intel_dp->has_hdmi_sink,
> -				       intel_dp_hdmi_ycbcr420(intel_dp, crtc_state)) &&
> -		intel_dp_hdmi_tmds_clock_valid(intel_dp, crtc_state, bpc);
> +	return intel_hdmi_bpc_possible(crtc_state, bpc,
> +				       intel_dp->has_hdmi_sink, ycbcr420_output) &&
> +		intel_dp_tmds_clock_valid(intel_dp, clock, bpc, ycbcr420_output) == MODE_OK;
>   }
>   
>   static int intel_dp_max_bpp(struct intel_dp *intel_dp,