Re: [RFC V3 2/2] drm/i915/vrr: Set drm crtc vrr_enabled property

From: "Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>
To: "Navare, Manasi" <manasi.d.navare@intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [RFC V3 2/2] drm/i915/vrr: Set drm crtc vrr_enabled property
Date: Wed, 1 Jun 2022 16:01:35 +0530	[thread overview]
Message-ID: <d5e08967-6bdb-3f9c-b014-7f8e069c2aa4@intel.com> (raw)
In-Reply-To: <20220531171451.GB257110@mdnavare-mobl1.jf.intel.com>

On Tue-31-05-2022 10:44 pm, Navare, Manasi wrote:
> On Tue, May 17, 2022 at 12:56:36PM +0530, Bhanuprakash Modem wrote:
>> This function sets the vrr_enabled property for crtc based
>> on the platform support and the request from userspace.
>>
>> V2: Check for platform support before updating the prop.
>> V3: Don't attach vrr_enabled prop, as it is alreay attached.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vrr.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 396f2f994fa0..7263b35550de 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -160,6 +160,10 @@ void intel_vrr_enable(struct intel_encoder *encoder,
>>   	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>>   	u32 trans_vrr_ctl;
>>   
>> +	if (HAS_VRR(dev_priv))
>> +		drm_mode_crtc_set_vrr_enabled_property(crtc_state->uapi.crtc,
>> +						       crtc_state->vrr.enable);
> 
> But here if userspace has accidently set the CRTC vrr enabled prop to
> true without cheking VRR capabile prop, this will be true and here the
> driver will still not
> reset it. If that is the purpose of adding this then we should infact
> set this to false if !HAS_VRR

Yes, my intention is same as you mentioned. But this looks correct to 
me, because:

1) HAS_VRR() is a check to make sure that the driver supports VRR. If 
there is no VRR support on the driver, then there is no point to 
set/clear this prop.

2) crtc_state->vrr.enable is true If vrr_capable is set to true, else false.

So we can interpret above check as below

enable_VRR() {
	IF Driver supports VRR:
		IF VRR_CAPABLE:
			Set VRR_ENABLED;
		ELSE:
			Clear VRR_ENABLED;
	ELSE:
		Do nothing;
}

- Bhanu

> 
> Manasi
> 
>> +
>>   	if (!crtc_state->vrr.enable)
>>   		return;
>>   
>> @@ -211,6 +215,10 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
>>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>   	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
>>   
>> +	if (HAS_VRR(dev_priv))
>> +		drm_mode_crtc_set_vrr_enabled_property(old_crtc_state->uapi.crtc,
>> +						       false);
>> +
>>   	if (!old_crtc_state->vrr.enable)
>>   		return;
>>   
>> -- 
>> 2.35.1
>>