[PATCH 1/9] drm: Add variable refresh rate properties to DRM connector
Harry Wentland
harry.wentland at amd.com
Tue Sep 11 18:23:38 UTC 2018
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 at amd.com>
>>>>> ---
>>>>> 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
>>>
>
More information about the amd-gfx
mailing list