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

Thu Apr 28 11:51:28 UTC 2022

On Wed-27-04-2022 03:31 am, Navare, Manasi wrote:
> On Mon, Apr 25, 2022 at 12:16:11PM +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().
>>
>> 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
> 
> We already have userspace making us of the CRTC vrr_enabled property to
> enable VRR for the CRTC like in case of full screen gaming.
> 
> This can already be done through:
> drm_atomic_crtc_set_property call. Why do we need additonal helpers
> for setting the same per CRTC property?

Thanks for the Review, Manasi.

Yeah, I just realized that we are attaching "vrr_enabled" as part of 
crtc_init()

drm_crtc_init_with_planes() --> __drm_crtc_init_with_planes() --> 
drm_object_attach_property(vrr_enabled);

I'll drop the drm_mode_crtc_attach_vrr_enabled_property() helper.

> 
> This is a default CRTC property so it will be created annd attached for
> CRTC as per the DRM doc:
> "VRR_ENABLED":
>   *      Default &drm_crtc boolean property that notifies the driver that the
>   *      content on the CRTC is suitable for variable refresh rate presentation.
>   *      The driver will take this property as a hint to enable variable
>   *      refresh rate support if the receiver supports it, ie. if the
>   *      "vrr_capable" property is true on the &drm_connector object. The
>   *      vertical front porch duration will be extended until page-flip or
>   *      timeout when enabled.
> 
> Then we can use the atomic_crtc_set/get_property helpers to set it
> Am I missing some other use case here?

My intention is to set/clear "VRR_ENABLED" prop based on the driver and 
hardware capabilities. And user can request anytime to get the status of 
VRR on that CRTC.

Example:
Consider we have an Non-VRR panel is connected, but driver supports VRR 
(hence vrr_capable = 0)
  * Request from user-space to enable the VRR on CRTC.
  * Driver should take the decision to set/clear the "VRR_ENABLED".
  * Request from use-space to read back the "VRR_ENABLED". It must be 0, 
since "vrr_capable" is 0.

Without this series, in above scenario I am getting "VRR_ENABLED" as 1 
which is not true.

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

I think, still we need a helper to set/clear the prop 
drm_mode_crtc_set_vrr_enabled_property(). I am not sure that we can use 
atomic_crtc_set/get_property helpers here, as these helpers are used by 
atomic_ioctls only.

- Bhanu

> 
> Manasi
> 
>>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>> Cc: Manasi Navare <manasi.d.navare at intel.com>
>> Cc: Harry Wentland <harry.wentland at amd.com>
>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
>> ---
>>   drivers/gpu/drm/drm_crtc.c        | 44 +++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_mode_config.c |  2 +-
>>   include/drm/drm_crtc.h            |  4 +++
>>   3 files changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 26a77a735905..95b4a0c7ecb3 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -883,3 +883,47 @@ int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL(drm_crtc_create_scaling_filter_property);
>> +
>> +/**
>> + * drm_mode_crtc_attach_vrr_enabled_property - attaches the vrr_enabled property
>> + * @crtc: drm CRTC to attach the vrr_enabled property on.
>> + *
>> + * This is used by atomic drivers to add support for querying
>> + * VRR enabled status for a crtc.
>> + */
>> +void drm_mode_crtc_attach_vrr_enabled_property(struct drm_crtc *crtc)
>> +{
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (!config->prop_vrr_enabled)
>> +		return;
>> +
>> +	drm_object_attach_property(&crtc->base,
>> +				   config->prop_vrr_enabled,
>> +				   0);
>> +}
>> +EXPORT_SYMBOL(drm_mode_crtc_attach_vrr_enabled_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..bde657cb0f9e 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1333,4 +1333,8 @@ 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_attach_vrr_enabled_property(struct drm_crtc *crtc);
>> +void drm_mode_crtc_set_vrr_enabled_property(struct drm_crtc *crtc,
>> +					    bool vrr_enabled);
>> +
>>   #endif /* __DRM_CRTC_H__ */
>> -- 
>> 2.35.1
>>