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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Manasi Navare <manasi.d.navare@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 05/11] drm/i915/display/dp: Compute VRR state in atomic_check
Date: Tue, 10 Nov 2020 12:47:46 +0200	[thread overview]
Message-ID: <87o8k5sd2l.fsf@intel.com> (raw)
In-Reply-To: <20201022222709.29386-6-manasi.d.navare@intel.com>

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.

>  			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.

> +		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