Re: [Intel-gfx] [PATCH 05/11] drm/i915/display/dp: Compute VRR state in atomic_check

["Navare, Manasi" <manasi.d.navare@intel.com>]

On Tue, Nov 10, 2020 at 12:47:46PM +0200, Jani Nikula wrote:
> On Thu, 22 Oct 2020, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > This forces a complete modeset if vrr drm crtc state goes
> > from enabled to disabled and vice versa.
> > This patch also computes vrr state variables from the mode timings
> > and based on the vrr property set by userspace as well as hardware's
> > vrr capability.
> >
> > v2:
> > *Rebase
> > v3:
> > * Vmin = max (vtotal, vmin) (Manasi)
> > v4:
> > * set crtc_state->vrr.enable = 0 for disable request
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c |  7 +++-
> >  drivers/gpu/drm/i915/display/intel_dp.c      |  1 +
> >  drivers/gpu/drm/i915/display/intel_vrr.c     | 38 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_vrr.h     |  2 ++
> >  4 files changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index f41b6f8b5618..f70cc3b2a1a4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14213,6 +14213,10 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> >  
> >  	PIPE_CONF_CHECK_I(mst_master_transcoder);
> >  
> > +	PIPE_CONF_CHECK_BOOL(vrr.enable);
> > +	PIPE_CONF_CHECK_I(vrr.vtotalmin);
> > +	PIPE_CONF_CHECK_I(vrr.vtotalmax);
> > +
> >  #undef PIPE_CONF_CHECK_X
> >  #undef PIPE_CONF_CHECK_I
> >  #undef PIPE_CONF_CHECK_BOOL
> > @@ -15202,7 +15206,8 @@ static int intel_atomic_check(struct drm_device *dev,
> >  
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> > -		if (new_crtc_state->inherited != old_crtc_state->inherited)
> > +		if (new_crtc_state->inherited != old_crtc_state->inherited ||
> > +		    new_crtc_state->uapi.vrr_enabled != old_crtc_state->uapi.vrr_enabled)
> 
> Somehow this feels like a really specific check to add considering the
> abstraction level of the function in general.

Should I then create a separate function to force a full modeset by setting mode changed 
if vrr_enabled changed?
And call that from intel_atomic_check() ?

> 
> >  			new_crtc_state->uapi.mode_changed = true;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 3794b8f35edc..3185c4ca523d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2752,6 +2752,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  	if (!HAS_DDI(dev_priv))
> >  		intel_dp_set_clock(encoder, pipe_config);
> >  
> > +	intel_vrr_compute_config(intel_dp, pipe_config);
> >  	intel_psr_compute_config(intel_dp, pipe_config);
> >  	intel_dp_drrs_compute_config(intel_dp, pipe_config, output_bpp,
> >  				     constant_n);
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 0c8a91fabb64..56114f535f94 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -26,3 +26,41 @@ bool intel_is_vrr_capable(struct drm_connector *connector)
> >  		info->monitor_range.max_vfreq - info->monitor_range.min_vfreq > 10;
> >  }
> >  
> > +void
> > +intel_vrr_compute_config(struct intel_dp *intel_dp,
> > +			 struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> > +	struct drm_connector *connector = &intel_connector->base;
> > +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> > +	const struct drm_display_info *info = &connector->display_info;
> > +
> > +	if (!intel_is_vrr_capable(connector))
> > +		return;
> > +
> > +	if (!crtc_state->uapi.vrr_enabled) {
> > +		drm_dbg(&dev_priv->drm,
> > +			"VRR disable requested by Userspace\n");
> 
> drm_dbg_kms, though is this useful information? Quite a bit of log spam
> I'd think.

Yea this one can probably remove

Manasi


> 
> > +		crtc_state->vrr.enable = false;
> > +		return;
> > +	}
> > +
> > +	crtc_state->vrr.enable = true;
> > +	crtc_state->vrr.vtotalmin =
> > +		max_t(u16, adjusted_mode->crtc_vtotal,
> > +		      DIV_ROUND_CLOSEST(adjusted_mode->crtc_clock * 1000,
> > +					adjusted_mode->crtc_htotal *
> > +					info->monitor_range.max_vfreq));
> > +	crtc_state->vrr.vtotalmax =
> > +		max_t(u16, adjusted_mode->crtc_vtotal,
> > +		      DIV_ROUND_UP(adjusted_mode->crtc_clock * 1000,
> > +				   adjusted_mode->crtc_htotal *
> > +				   info->monitor_range.min_vfreq));
> > +
> > +	drm_dbg(&dev_priv->drm,
> 
> drm_dbg_kms
> 
> > +		"VRR Config: Enable = %s Vtotal Min = %d Vtotal Max = %d\n",
> > +		 yesno(crtc_state->vrr.enable), crtc_state->vrr.vtotalmin,
> > +		 crtc_state->vrr.vtotalmax);
> > +}
> > +
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
> > index 755746c7525c..1e6fe8fe92ec 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> > @@ -15,5 +15,7 @@ struct intel_encoder;
> >  struct intel_dp;
> >  
> >  bool intel_is_vrr_capable(struct drm_connector *connector);
> > +void intel_vrr_compute_config(struct intel_dp *intel_dp,
> > +			      struct intel_crtc_state *crtc_state);
> >  
> >  #endif /* __INTEL_VRR_H__ */
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx