Re: [PATCH 1/9] drm: Add variable refresh rate properties to DRM connector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2018-09-11 01:56 PM, Ville Syrjälä wrote:
> On Tue, Sep 11, 2018 at 01:36:01PM -0400, Kazlauskas, Nicholas wrote:
>> On 09/11/2018 12:31 PM, Ville Syrjälä wrote:
>>> On Tue, Sep 11, 2018 at 07:22:43PM +0300, Ville Syrjälä wrote:
>>>> On Tue, Sep 11, 2018 at 12:13:25PM -0400, Nicholas Kazlauskas wrote:
>>>>> Modern monitor hardware is capable of supporting variable refresh rates
>>>>> and adaptive sync technologies. The properties for querying and
>>>>> controlling these features should be exposed on the DRM connector.
>>>>>
>>>>> This patch introduces two new properties for variable refresh rate
>>>>> support:
>>>>>
>>>>> - variable_refresh_capable
>>>>> - variable_refresh_enabled
>>>>>
>>>>> These are optional properties that can be added to a DRM connector
>>>>> dynamically by using drm_connector_attach_variable_refresh_properties.
>>>>>
>>>>> DRM drivers should set variable_refresh_capable as applicable for
>>>>> their hardware. The property variable_refresh_enabled as a userspace
>>>>> controlled option.
>>>>>
>>>>> Change-Id: I5f60f8b57534e1d3dacda4c64c6c9106b42f4439
>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_atomic.c    |  9 +++++++++
>>>>>   drivers/gpu/drm/drm_connector.c | 35 +++++++++++++++++++++++++++++++++
>>>>>   include/drm/drm_connector.h     | 27 +++++++++++++++++++++++++
>>>>>   3 files changed, 71 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>> index d0478abc01bd..2f89ab0fac87 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>> @@ -1403,6 +1403,11 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>>>>>   		state->content_type = val;
>>>>>   	} else if (property == connector->scaling_mode_property) {
>>>>>   		state->scaling_mode = val;
>>>>> +	} else if (property == connector->variable_refresh_capable_property) {
>>>>> +		DRM_DEBUG_KMS("only drivers can set variable_refresh_capable\n");
>>>>> +		return -EINVAL;
>>>>
>>>> Why is that needed? Why not just avoid exposing the vrr_enabled prop
>>>> when it's not supported?
>>>
>>> Ah, I guess you want to change the value after plugging in a new
>>> display. Yeah, if we don't want userspace to have to parse the EDID I
>>> guess we need this. But in that case it should be marked immutable,
>>> and then you don't need any of this atomic state related code for it.
>>
>> Yeah, it's for hotplugging that it works this way. The reason it's not 
>> marked as immutable is mostly convention - the existing driver 
>> write-only properties (with userspace queries) do the same thing.
>>
>> The immutable property flag is only used for non-connector properties. 
> 
> There are plenty of immutable connector properties: EDID, TILE,
> non-desktop, etc. I'm pretty sure there are more immutable connector
> properties than there are immutable properties on other types of
> objects. And considering immutable connector properties existed
> before other types of objects even had any properties I'd say
> it's well established by now :)
> 
>> During testing I also noticed that something likes to cache the initial 
>> default value for these immutable flagged properties for userspace 
>> queries - so when you call xrandr you only end up seeing the default 
>> value and not the current.
> 
> 'xrandr --current' is supposed to not probe hard for new stuff. Without
> the --current it may or may not result in a full probe depending on the
> implementation in the ddx and or the kms driver. A full probe should
> definitely happen after a hotplug. If it's not happening then there is
> a bug somewhere.
> 
>>
>>>
>>>>
>>>>> +	} else if (property == connector->variable_refresh_enabled_property) {
>>>>> +		state->variable_refresh_enabled = val;
>>>>
>>>> So one thing I already asked long ago is whether there would a point in
>>>> exposing some kind of min/max refresh rate range control. Never really
>>>> got an answer. Was somehting like that considered and rejected?
>>
>> For the initial patchset it seemed best to keep things simple and target 
>> the dynamic adaptation usecase with on/off toggle.
>>
>> The driver having access to the full range can enable additional 
>> functionality that can improve the user experience - a good example is 
>> low framerate compensation.
>>
>> Adding the properties to support setting the min/max could be done on 
>> top of this, however. Driver behavior would likely need to be defined 
>> for atomic check. The driver needs to verify with the monitor that the 
>> ranges are valid at some point - whether it silently fails or returns an 
>> error is an example of something to consider here.
>>
>> Do you have a specific usecase in mind that needs min/max exposed to 
>> userspace?
> 
> Not really. Some vague ideas about video playback and such, and maybe
> just for power saving. It was just an idea that occurred to me when
> thinking about this stuff. Although I suppose the compositor could
> always throttle the flip rate using other means. And in tne end it
> comes down to a policy question of how the compositor would decide
> what the allowed range is.
> 

Responding here to both your and Manasi's questions.

What good would exposing vmin/vmax do?

The gaming case is very well served by the properties Nicholas is introducing and the video playback case would be best served with something like a target_present_time or target_frame_duration. Exposing vmin/vmax might unnecessarily tie userland to DP/HDMI spec details and complicate an already complex problem.

Harry

>>
>>>>
>>>>>   	} else if (property == connector->content_protection_property) {
>>>>>   		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
>>>>>   			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
>>>>> @@ -1508,6 +1513,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>>>>>   		*val = state->content_type;
>>>>>   	} else if (property == connector->scaling_mode_property) {
>>>>>   		*val = state->scaling_mode;
>>>>> +	} else if (property == connector->variable_refresh_capable_property) {
>>>>> +		*val = state->variable_refresh_capable;
>>>>> +	} else if (property == connector->variable_refresh_enabled_property) {
>>>>> +		*val = state->variable_refresh_enabled;
>>>>>   	} else if (property == connector->content_protection_property) {
>>>>>   		*val = state->content_protection;
>>>>>   	} else if (property == config->writeback_fb_id_property) {
>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>>> index 6011d769d50b..37fb33fa77b9 100644
>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>> @@ -1250,6 +1250,41 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>>>>>   }
>>>>>   EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>>>>>   
>>>>> +/**
>>>>> + * drm_connector_attach_variable_refresh_properties - creates and attaches
>>>>> + * properties for connectors that support adaptive refresh
>>>>> + * @connector: connector to create adaptive refresh properties on
>>>>> + */
>>>>> +int drm_connector_attach_variable_refresh_properties(
>>>>> +	struct drm_connector *connector)
>>>>> +{
>>>>> +	struct drm_device *dev = connector->dev;
>>>>> +	struct drm_property *prop;
>>>>> +
>>>>> +	if (!connector->variable_refresh_capable_property) {
>>>>> +		prop = drm_property_create_bool(dev, 0,
>>>>> +			"variable_refresh_capable");
>>>>> +		if (!prop)
>>>>> +			return -ENOMEM;
>>>>> +
>>>>> +		connector->variable_refresh_capable_property = prop;
>>>>> +		drm_object_attach_property(&connector->base, prop, 0);
>>>>> +	}
>>>>> +
>>>>> +	if (!connector->variable_refresh_enabled_property) {
>>>>> +		prop = drm_property_create_bool(dev, 0,
>>>>> +			"variable_refresh_enabled");
>>>>> +		if (!prop)
>>>>> +			return -ENOMEM;
>>>>> +
>>>>> +		connector->variable_refresh_enabled_property = prop;
>>>>> +		drm_object_attach_property(&connector->base, prop, 0);
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_connector_attach_variable_refresh_properties);
>>>>> +
>>>>>   /**
>>>>>    * drm_connector_attach_scaling_mode_property - attach atomic scaling mode property
>>>>>    * @connector: connector to attach scaling mode property on.
>>>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>>>> index 97ea41dc678f..105a127e9191 100644
>>>>> --- a/include/drm/drm_connector.h
>>>>> +++ b/include/drm/drm_connector.h
>>>>> @@ -448,6 +448,18 @@ struct drm_connector_state {
>>>>>   	 */
>>>>>   	unsigned int content_protection;
>>>>>   
>>>>> +	/**
>>>>> +	 * @variable_refresh_enabled: Connector property used to check
>>>>> +	 * if variable refresh is supported on the device.
>>>>> +	 */
>>>>> +	bool variable_refresh_capable;
>>>>> +
>>>>> +	/**
>>>>> +	 * @variable_refresh_enabled: Connector property used to check
>>>>> +	 * if variable refresh is enabled.
>>>>> +	 */
>>>>> +	bool variable_refresh_enabled;
>>>>> +
>>>>>   	/**
>>>>>   	 * @writeback_job: Writeback job for writeback connectors
>>>>>   	 *
>>>>> @@ -909,6 +921,19 @@ struct drm_connector {
>>>>>   	 */
>>>>>   	struct drm_property *scaling_mode_property;
>>>>>   
>>>>> +	/**
>>>>> +	 * @variable_refresh_capable_property: Optional property for
>>>>> +	 * querying hardware support for variable refresh.
>>>>> +	 */
>>>>> +	struct drm_property *variable_refresh_capable_property;
>>>>> +
>>>>> +	/**
>>>>> +	 * @variable_refresh_enabled_property: Optional property for
>>>>> +	 * enabling or disabling support for variable refresh
>>>>> +	 * on the connector.
>>>>> +	 */
>>>>> +	struct drm_property *variable_refresh_enabled_property;
>>>>> +
>>>>>   	/**
>>>>>   	 * @content_protection_property: DRM ENUM property for content
>>>>>   	 * protection. See drm_connector_attach_content_protection_property().
>>>>> @@ -1182,6 +1207,8 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>>>>>   int drm_connector_attach_content_type_property(struct drm_connector *dev);
>>>>>   int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>>>>>   					       u32 scaling_mode_mask);
>>>>> +int drm_connector_attach_variable_refresh_properties(
>>>>> +	struct drm_connector *connector);
>>>>>   int drm_connector_attach_content_protection_property(
>>>>>   		struct drm_connector *connector);
>>>>>   int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>>>>> -- 
>>>>> 2.17.1
>>>>
>>>> -- 
>>>> Ville Syrjälä
>>>> Intel
>>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux