[Intel-gfx] [PATCH 3/3] drm/i915/dp: intel_dp connector hook for VRR support

[Manasi Navare]

On Thu, Mar 19, 2020 at 12:14:28PM +0200, Jani Nikula wrote:
> On Tue, 17 Mar 2020, Manasi Navare <manasi.d.navare at intel.com> wrote:
> > This defines the get_vrr_support hook for intel DP connector
> > VRR support is set to true based on the DPCD ignore MSA and
> > EDID monitor range
> 
> Yeah... but what do you use it for?
>

Hi Jani,

My idea of adding the intel_dp->vrr_capable variable was to 
store the vrr capability in intel_dp for internal i915 use later when
we decide on vrr crtc states etc and configure the pipe etc for VRR modes.

I added this in a hook that gets called in the connector probe function
right after detect and get_modes() since thats when we will have parse
the EDID monitor range and populated that in drm_display_info struct.

This hook is also needed for us to then set the vrr capable property
for that connector.

But yes no that I rethink on of i actually need something in intel_dp
I feel that I can just get away with crtc_state->vrr_capable
that will be computed in atomic check based on the drm_display_info
and dpcd read there and in this hook we can just set the vrr capable property.

But that would mean duplicating this conditional code in atomic check.
What would be your suggestion?
Cache it here in intel_dp and just use this to set crtc_state->VRR values in modeset
or set it directly in atomic check?

In terms of reseting it, I can set this to 0 in intel_dp_detect(), if connector disconnected path
where i reset the dsc_dpcd and dp_compliance variables?

Regards
Manasi
 
> >
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Harry Wentland <harry.wentland at amd.com>
> > Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas at amd.com>
> > Cc: Aditya Swarup <aditya.swarup at intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  3 +++
> >  drivers/gpu/drm/i915/display/intel_dp.c       | 19 +++++++++++++++++++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 5e00e611f077..cd37ee6db1ff 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1353,6 +1353,9 @@ struct intel_dp {
> >  
> >  	/* Display stream compression testing */
> >  	bool force_dsc_en;
> > +
> > +	/* DP Variable refresh rate/ Adaptive sync support */
> > +	bool vrr_capable;
> 
> Only set, never read.
> 
> >  };
> >  
> >  enum lspcon_vendor {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 0a417cd2af2b..ccf5d868b5c1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5860,6 +5860,24 @@ static int intel_dp_get_modes(struct drm_connector *connector)
> >  	return 0;
> >  }
> >  
> > +static void intel_dp_get_vrr_support(struct drm_connector *connector)
> > +{
> > +	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
> > +	const struct drm_display_info *info = &connector->display_info;
> > +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > +
> > +	/*
> > +	 * DP Sink is capable of Variable refresh video timings if
> > +	 * Ignore MSA bit is set in DPCD.
> > +	 * EDID monitor range also should be atleast 10 for reasonable
> > +	 * Adaptive sync/ VRR end user experience.
> > +	 */
> > +	if (INTEL_GEN(dev_priv) >= 12 &&
> > +	    drm_dp_sink_is_capable_without_timing_msa(intel_dp->dpcd) &&
> > +	    info->monitor_range.max_vfreq - info->monitor_range.min_vfreq > 10)
> > +		intel_dp->vrr_capable = true;
> 
> So for now this is just a cached value for i915 use. I don't know what
> you'll need it for, but you also don't explain why it needs to be
> *cached* instead of having a helper to tell you based on the above
> data. You only ever set ->vrr_capable to true, but you never reset it
> back to false e.g. when the display is changed on the connector.
> 
> Furthermore, because of the placing of the hook call in the previous
> patch, this will only use whatever details ->get_modes gives you, not
> the fallback data.
> 
> BR,
> Jani.
> 
> 
> > +}
> > +
> >  static int
> >  intel_dp_connector_register(struct drm_connector *connector)
> >  {
> > @@ -6756,6 +6774,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
> >  	.get_modes = intel_dp_get_modes,
> >  	.mode_valid = intel_dp_mode_valid,
> >  	.atomic_check = intel_dp_connector_atomic_check,
> > +	.get_adaptive_sync_support = intel_dp_get_vrr_support,
> >  };
> >  
> >  static const struct drm_encoder_funcs intel_dp_enc_funcs = {
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center