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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Dec 10, 2021 at 10:50:09AM +0530, Nautiyal, Ankit K wrote:
> 
> On 10/15/2021 7:09 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@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@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.

I guess we should pull the FRL stuff into its own helper functions,
assuming there is something that can be shared between .mode_valid()
and .compute_config().

But looking at the FRL code it looks a bit sketchy. It doesn't seem
to account for any link bandwidth overhead from the 16b18b encoding
or whatever else overhead there is (the spec seems to have quite a
lot to say on this topic). Also it uses intel_dp_mode_min_output_bpp()
for the bandwidth calculation which seems wrong.
intel_dp_mode_min_output_bpp() deals with the DP side of the link
where min bpc can be as low as 6, but for the HDMI side min bpc
is always 8.

So looks to me like there's a bunch of stuff that needs fixing here.

-- 
Ville Syrjälä
Intel