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

["Kazlauskas, Nicholas" <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>]

On 09/11/2018 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@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 :)

I went back and checked and I guess you were right and most connector 
properties have this on them. Not sure what part of the code I was 
thinking back on.

> 
>> 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.

This might be a bug then since I don't use the --current flag (and it 
exhibits the same issue with it). I just tested this again with the 
DRM_MODE_PROP_IMMUTABLE flag set and it shows up as 0 for monitors that 
support it (even though it's set in kernelspace to 1).

> 
>>
>>>
>>>>
>>>>> +	} 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.
> 
>>
>>>>
>>>>>    	} 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
>>>
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx