Re: [Intel-gfx] [RFC V3 1/2] drm/vrr: Attach vrr_enabled property to the drm crtc

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

On Tue-31-05-2022 10:42 pm, Navare, Manasi wrote:
> On Tue, May 17, 2022 at 12:56:35PM +0530, Bhanuprakash Modem wrote:
>> Modern display hardware is capable of supporting variable refresh rates.
>> This patch introduces helpers to attach and set "vrr_enabled" property
>> on the crtc to allow userspace to query VRR enabled status on that crtc.
>>
>> Atomic drivers should attach this property to crtcs those are capable of
>> driving variable refresh rates using
>> drm_mode_crtc_attach_vrr_enabled_property().
> 
> We are not attaching the prop anymore, please remove this from the
> commit message.

Thanks for the reply,

I'll update this in next rev.

> 
>>
>> The value should be updated based on driver and hardware capability
>> by using drm_mode_crtc_set_vrr_enabled_property().
>>
>> V2: Use property flag as atomic
>> V3: Drop helper to attach vrr_enabled prop, since it is already
>> attached (Manasi)
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>> ---
>>   drivers/gpu/drm/drm_crtc.c        | 26 ++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_mode_config.c |  2 +-
>>   include/drm/drm_crtc.h            |  3 +++
>>   3 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 26a77a735905..8bb8b4bf4199 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -239,6 +239,9 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
>>    * 		Driver's default scaling filter
>>    * 	Nearest Neighbor:
>>    * 		Nearest Neighbor scaling filter
>> + * VRR_ENABLED:
>> + *	Atomic property for setting the VRR state of the CRTC.
>> + *	To enable the VRR on CRTC, user-space must set this property to 1.
> 
> This prop was primarily a userspace Write only and driver read only
> property which would be used only by the userspace to request VRR on
> that CRTC,
> 
> Are we now modifying this to be used as a bidirectional property to also
> indicate the status of VRR on that CRTC which will be updated by the
> driver?

Precisely YES.

> 
> We need to add this accordingly and update the DRM documentation and
> also get acks from other vendors since AMD and other folks mght be using
> this as a write only prop.

Sure, I'll update the documentation.

@Harry/@Nicholas, can you please comment on this approach?

I would like to modify "vrr_enabled" prop to be used as a bidirectional 
property. So, that userspace can request to enable the VRR and also to 
get the status of VRR on that CRTC.

IGT to validate the same:
https://patchwork.freedesktop.org/series/100539/

Example:
* If userspace has accidentally set the "vrr_enabled" prop to true 
without checking connector's "vrr_capable" prop, driver should reset 
this "vrr_enabled" property to false.

* Userspace can read "vrr_enabled" prop anytime to get the current 
status of VRR on that crtc.

- Bhanu

> 
> Manasi
> 
>>    */
>>   
>>   __printf(6, 0)
>> @@ -883,3 +886,26 @@ int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL(drm_crtc_create_scaling_filter_property);
>> +
>> +/**
>> + * drm_mode_crtc_set_vrr_enabled_property - sets the vrr enabled property for
>> + * a crtc.
>> + * @crtc: drm CRTC
>> + * @vrr_enabled: True to enable the VRR on CRTC
>> + *
>> + * Should be used by atomic drivers to update the VRR enabled status on a CRTC
>> + */
>> +void drm_mode_crtc_set_vrr_enabled_property(struct drm_crtc *crtc,
>> +					    bool vrr_enabled)
>> +{
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (!config->prop_vrr_enabled)
>> +		return;
>> +
>> +	drm_object_property_set_value(&crtc->base,
>> +				      config->prop_vrr_enabled,
>> +				      vrr_enabled);
>> +}
>> +EXPORT_SYMBOL(drm_mode_crtc_set_vrr_enabled_property);
>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>> index 37b4b9f0e468..b7cde73d5586 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -323,7 +323,7 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>>   		return -ENOMEM;
>>   	dev->mode_config.prop_mode_id = prop;
>>   
>> -	prop = drm_property_create_bool(dev, 0,
>> +	prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>>   			"VRR_ENABLED");
>>   	if (!prop)
>>   		return -ENOMEM;
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index a70baea0636c..906787398f40 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1333,4 +1333,7 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
>>   int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
>>   					    unsigned int supported_filters);
>>   
>> +void drm_mode_crtc_set_vrr_enabled_property(struct drm_crtc *crtc,
>> +					    bool vrr_enabled);
>> +
>>   #endif /* __DRM_CRTC_H__ */
>> -- 
>> 2.35.1
>>