[v2,2/3] drm: Add variable refresh property to DRM CRTC

Submitted by Kazlauskas, Nicholas on Sept. 24, 2018, 6:15 p.m.

Details

Message ID 20180924181537.12092-3-nicholas.kazlauskas@amd.com
State New
Headers show
Series "A DRM API for adaptive sync and variable refresh rate support" ( rev: 2 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Kazlauskas, Nicholas Sept. 24, 2018, 6:15 p.m.
Variable refresh rate algorithms have typically been enabled only
when the display is covered by a single source of content.

This patch introduces a new default CRTC property that helps
hint to the driver when the CRTC composition is suitable for variable
refresh rate algorithms. Userspace can set this property dynamically
as the composition changes.

Whether the variable refresh rate algorithms are active will still
depend on the CRTC being suitable and the connector being capable
and enabled by the user for variable refresh rate support.

It is worth noting that while the property is atomic it isn't filtered
from legacy userspace queries. This allows for Xorg userspace drivers
to implement support in non-atomic setups.

Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/drm_atomic_helper.c |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c   |  6 ++++++
 drivers/gpu/drm/drm_crtc.c          |  2 ++
 drivers/gpu/drm/drm_mode_config.c   |  6 ++++++
 include/drm/drm_crtc.h              | 13 +++++++++++++
 include/drm/drm_mode_config.h       |  8 ++++++++
 6 files changed, 36 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3cf1aa132778..b768d397a811 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3473,6 +3473,7 @@  void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 	state->planes_changed = false;
 	state->connectors_changed = false;
 	state->color_mgmt_changed = false;
+	state->variable_refresh_changed = false;
 	state->zpos_changed = false;
 	state->commit = NULL;
 	state->event = NULL;
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 0bb27a24a55c..32a4cf8a01c3 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -433,6 +433,10 @@  static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
 		drm_property_blob_put(mode);
 		return ret;
+	} else if (property == config->prop_variable_refresh) {
+		if (state->variable_refresh != val)
+			state->variable_refresh_changed = true;
+		state->variable_refresh = val;
 	} else if (property == config->degamma_lut_property) {
 		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->degamma_lut,
@@ -491,6 +495,8 @@  drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = state->active;
 	else if (property == config->prop_mode_id)
 		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
+	else if (property == config->prop_variable_refresh)
+		*val = state->variable_refresh;
 	else if (property == config->degamma_lut_property)
 		*val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
 	else if (property == config->ctm_property)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5f488aa80bcd..ca33d6fb90ac 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -340,6 +340,8 @@  int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
 		drm_object_attach_property(&crtc->base,
 					   config->prop_out_fence_ptr, 0);
+		drm_object_attach_property(&crtc->base,
+					   config->prop_variable_refresh, 0);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index ee80788f2c40..1287c161d65d 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -310,6 +310,12 @@  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,
+			"VARIABLE_REFRESH");
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_variable_refresh = prop;
+
 	prop = drm_property_create(dev,
 			DRM_MODE_PROP_BLOB,
 			"DEGAMMA_LUT", 0);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index b21437bc95bf..32b77f18ce6d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -168,6 +168,12 @@  struct drm_crtc_state {
 	 * drivers to steer the atomic commit control flow.
 	 */
 	bool color_mgmt_changed : 1;
+	/**
+	 * @variable_refresh_changed: Variable refresh support has changed
+	 * on the CRTC. Used by the atomic helpers and drivers to steer the
+	 * atomic commit control flow.
+	 */
+	bool variable_refresh_changed : 1;
 
 	/**
 	 * @no_vblank:
@@ -290,6 +296,13 @@  struct drm_crtc_state {
 	 */
 	u32 pageflip_flags;
 
+	/**
+	 * @variable_refresh:
+	 *
+	 * Indicates whether the CRTC is suitable for variable refresh rate.
+	 */
+	bool variable_refresh;
+
 	/**
 	 * @event:
 	 *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 928e4172a0bb..1290191cd96e 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -639,6 +639,14 @@  struct drm_mode_config {
 	 * connectors must be of and active must be set to disabled, too.
 	 */
 	struct drm_property *prop_mode_id;
+	/**
+	 * @prop_variable_refresh: Default atomic CRTC property to indicate
+	 * whether the CRTC is suitable for variable refresh rate support.
+	 *
+	 * This is only an indication of support, not whether variable
+	 * refresh is active on the CRTC.
+	 */
+	struct drm_property *prop_variable_refresh;
 
 	/**
 	 * @dvi_i_subconnector_property: Optional DVI-I property to

Comments

On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote:
> Variable refresh rate algorithms have typically been enabled only
> when the display is covered by a single source of content.
> 
> This patch introduces a new default CRTC property that helps
> hint to the driver when the CRTC composition is suitable for variable
> refresh rate algorithms. Userspace can set this property dynamically
> as the composition changes.
> 
> Whether the variable refresh rate algorithms are active will still
> depend on the CRTC being suitable and the connector being capable
> and enabled by the user for variable refresh rate support.
> 
> It is worth noting that while the property is atomic it isn't filtered
> from legacy userspace queries. This allows for Xorg userspace drivers
> to implement support in non-atomic setups.
> 
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c   |  6 ++++++
>  drivers/gpu/drm/drm_crtc.c          |  2 ++
>  drivers/gpu/drm/drm_mode_config.c   |  6 ++++++
>  include/drm/drm_crtc.h              | 13 +++++++++++++
>  include/drm/drm_mode_config.h       |  8 ++++++++
>  6 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 3cf1aa132778..b768d397a811 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  	state->planes_changed = false;
>  	state->connectors_changed = false;
>  	state->color_mgmt_changed = false;
> +	state->variable_refresh_changed = false;

Another bool just to check if one bool changed seems a bit excessive.
Is there a reason you can't directly check if the other bool changed?

Actually I don't understand why this per-crtc thing is being added at
all. You already have the prop on the connector. Why do we want to
make life more difficult by requiring the thing to be set on both the
crtc and connector?

>  	state->zpos_changed = false;
>  	state->commit = NULL;
>  	state->event = NULL;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 0bb27a24a55c..32a4cf8a01c3 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>  		drm_property_blob_put(mode);
>  		return ret;
> +	} else if (property == config->prop_variable_refresh) {
> +		if (state->variable_refresh != val)
> +			state->variable_refresh_changed = true;
> +		state->variable_refresh = val;
>  	} else if (property == config->degamma_lut_property) {
>  		ret = drm_atomic_replace_property_blob_from_id(dev,
>  					&state->degamma_lut,
> @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  		*val = state->active;
>  	else if (property == config->prop_mode_id)
>  		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
> +	else if (property == config->prop_variable_refresh)
> +		*val = state->variable_refresh;
>  	else if (property == config->degamma_lut_property)
>  		*val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
>  	else if (property == config->ctm_property)
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5f488aa80bcd..ca33d6fb90ac 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>  		drm_object_attach_property(&crtc->base,
>  					   config->prop_out_fence_ptr, 0);
> +		drm_object_attach_property(&crtc->base,
> +					   config->prop_variable_refresh, 0);
>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index ee80788f2c40..1287c161d65d 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -310,6 +310,12 @@ 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,
> +			"VARIABLE_REFRESH");
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_variable_refresh = prop;
> +
>  	prop = drm_property_create(dev,
>  			DRM_MODE_PROP_BLOB,
>  			"DEGAMMA_LUT", 0);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b21437bc95bf..32b77f18ce6d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -168,6 +168,12 @@ struct drm_crtc_state {
>  	 * drivers to steer the atomic commit control flow.
>  	 */
>  	bool color_mgmt_changed : 1;
> +	/**
> +	 * @variable_refresh_changed: Variable refresh support has changed
> +	 * on the CRTC. Used by the atomic helpers and drivers to steer the
> +	 * atomic commit control flow.
> +	 */
> +	bool variable_refresh_changed : 1;
>  
>  	/**
>  	 * @no_vblank:
> @@ -290,6 +296,13 @@ struct drm_crtc_state {
>  	 */
>  	u32 pageflip_flags;
>  
> +	/**
> +	 * @variable_refresh:
> +	 *
> +	 * Indicates whether the CRTC is suitable for variable refresh rate.
> +	 */
> +	bool variable_refresh;
> +
>  	/**
>  	 * @event:
>  	 *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 928e4172a0bb..1290191cd96e 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -639,6 +639,14 @@ struct drm_mode_config {
>  	 * connectors must be of and active must be set to disabled, too.
>  	 */
>  	struct drm_property *prop_mode_id;
> +	/**
> +	 * @prop_variable_refresh: Default atomic CRTC property to indicate
> +	 * whether the CRTC is suitable for variable refresh rate support.
> +	 *
> +	 * This is only an indication of support, not whether variable
> +	 * refresh is active on the CRTC.
> +	 */
> +	struct drm_property *prop_variable_refresh;
>  
>  	/**
>  	 * @dvi_i_subconnector_property: Optional DVI-I property to
> -- 
> 2.19.0
On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote:
>> Variable refresh rate algorithms have typically been enabled only
>> when the display is covered by a single source of content.
>>
>> This patch introduces a new default CRTC property that helps
>> hint to the driver when the CRTC composition is suitable for variable
>> refresh rate algorithms. Userspace can set this property dynamically
>> as the composition changes.
>>
>> Whether the variable refresh rate algorithms are active will still
>> depend on the CRTC being suitable and the connector being capable
>> and enabled by the user for variable refresh rate support.
>>
>> It is worth noting that while the property is atomic it isn't filtered
>> from legacy userspace queries. This allows for Xorg userspace drivers
>> to implement support in non-atomic setups.
>>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> ---
>>   drivers/gpu/drm/drm_atomic_helper.c |  1 +
>>   drivers/gpu/drm/drm_atomic_uapi.c   |  6 ++++++
>>   drivers/gpu/drm/drm_crtc.c          |  2 ++
>>   drivers/gpu/drm/drm_mode_config.c   |  6 ++++++
>>   include/drm/drm_crtc.h              | 13 +++++++++++++
>>   include/drm/drm_mode_config.h       |  8 ++++++++
>>   6 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 3cf1aa132778..b768d397a811 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>>   	state->planes_changed = false;
>>   	state->connectors_changed = false;
>>   	state->color_mgmt_changed = false;
>> +	state->variable_refresh_changed = false;
> 
> Another bool just to check if one bool changed seems a bit excessive.
> Is there a reason you can't directly check if the other bool changed?

It's nice for an atomic driver to be able to check if the property has 
changed to steer control flow.

The driver could just check if the old crtc variable refresh value isn't 
equal to the new one itself, but there's already precedent set to 
provide flags like these instead.

It also lets the driver change it as needed during atomic commits. 
You'll see many drivers making use of the other flags like 
connectors_changed, mode_changed, etc.

> 
> Actually I don't understand why this per-crtc thing is being added at
> all. You already have the prop on the connector. Why do we want to
> make life more difficult by requiring the thing to be set on both the
> crtc and connector?

It doesn't make much sense without both.

The user can globally enable or disable the variable_refresh_enabled on 
the connector. This is the user's preference.

What they don't control is the variable_refresh - the content hint that 
userspace specifies when the CRTC contents are suitable for enabling 
variable refresh features (like adaptive sync). This is userspace's 
preference.

When both preferences agree, only then will variable refresh features be 
enabled.

The reasoning for the split is because not all content is suitable for 
variable refresh. Desktop environments, web browsers, etc only typically 
flip when needed - which will result in display flickering.

The userspace integration as part of these patches demonstrates enabling 
variable_refresh on the CRTC selectively.

> 
>>   	state->zpos_changed = false;
>>   	state->commit = NULL;
>>   	state->event = NULL;
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 0bb27a24a55c..32a4cf8a01c3 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>   		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>   		drm_property_blob_put(mode);
>>   		return ret;
>> +	} else if (property == config->prop_variable_refresh) {
>> +		if (state->variable_refresh != val)
>> +			state->variable_refresh_changed = true;
>> +		state->variable_refresh = val;
>>   	} else if (property == config->degamma_lut_property) {
>>   		ret = drm_atomic_replace_property_blob_from_id(dev,
>>   					&state->degamma_lut,
>> @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>>   		*val = state->active;
>>   	else if (property == config->prop_mode_id)
>>   		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
>> +	else if (property == config->prop_variable_refresh)
>> +		*val = state->variable_refresh;
>>   	else if (property == config->degamma_lut_property)
>>   		*val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
>>   	else if (property == config->ctm_property)
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 5f488aa80bcd..ca33d6fb90ac 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>>   		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>>   		drm_object_attach_property(&crtc->base,
>>   					   config->prop_out_fence_ptr, 0);
>> +		drm_object_attach_property(&crtc->base,
>> +					   config->prop_variable_refresh, 0);
>>   	}
>>   
>>   	return 0;
>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>> index ee80788f2c40..1287c161d65d 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -310,6 +310,12 @@ 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,
>> +			"VARIABLE_REFRESH");
>> +	if (!prop)
>> +		return -ENOMEM;
>> +	dev->mode_config.prop_variable_refresh = prop;
>> +
>>   	prop = drm_property_create(dev,
>>   			DRM_MODE_PROP_BLOB,
>>   			"DEGAMMA_LUT", 0);
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index b21437bc95bf..32b77f18ce6d 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -168,6 +168,12 @@ struct drm_crtc_state {
>>   	 * drivers to steer the atomic commit control flow.
>>   	 */
>>   	bool color_mgmt_changed : 1;
>> +	/**
>> +	 * @variable_refresh_changed: Variable refresh support has changed
>> +	 * on the CRTC. Used by the atomic helpers and drivers to steer the
>> +	 * atomic commit control flow.
>> +	 */
>> +	bool variable_refresh_changed : 1;
>>   
>>   	/**
>>   	 * @no_vblank:
>> @@ -290,6 +296,13 @@ struct drm_crtc_state {
>>   	 */
>>   	u32 pageflip_flags;
>>   
>> +	/**
>> +	 * @variable_refresh:
>> +	 *
>> +	 * Indicates whether the CRTC is suitable for variable refresh rate.
>> +	 */
>> +	bool variable_refresh;
>> +
>>   	/**
>>   	 * @event:
>>   	 *
>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> index 928e4172a0bb..1290191cd96e 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -639,6 +639,14 @@ struct drm_mode_config {
>>   	 * connectors must be of and active must be set to disabled, too.
>>   	 */
>>   	struct drm_property *prop_mode_id;
>> +	/**
>> +	 * @prop_variable_refresh: Default atomic CRTC property to indicate
>> +	 * whether the CRTC is suitable for variable refresh rate support.
>> +	 *
>> +	 * This is only an indication of support, not whether variable
>> +	 * refresh is active on the CRTC.
>> +	 */
>> +	struct drm_property *prop_variable_refresh;
>>   
>>   	/**
>>   	 * @dvi_i_subconnector_property: Optional DVI-I property to
>> -- 
>> 2.19.0
> 

Nicholas Kazlauskas
On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
> > On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote:
> >> Variable refresh rate algorithms have typically been enabled only
> >> when the display is covered by a single source of content.
> >>
> >> This patch introduces a new default CRTC property that helps
> >> hint to the driver when the CRTC composition is suitable for variable
> >> refresh rate algorithms. Userspace can set this property dynamically
> >> as the composition changes.
> >>
> >> Whether the variable refresh rate algorithms are active will still
> >> depend on the CRTC being suitable and the connector being capable
> >> and enabled by the user for variable refresh rate support.
> >>
> >> It is worth noting that while the property is atomic it isn't filtered
> >> from legacy userspace queries. This allows for Xorg userspace drivers
> >> to implement support in non-atomic setups.
> >>
> >> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> >> ---
> >>   drivers/gpu/drm/drm_atomic_helper.c |  1 +
> >>   drivers/gpu/drm/drm_atomic_uapi.c   |  6 ++++++
> >>   drivers/gpu/drm/drm_crtc.c          |  2 ++
> >>   drivers/gpu/drm/drm_mode_config.c   |  6 ++++++
> >>   include/drm/drm_crtc.h              | 13 +++++++++++++
> >>   include/drm/drm_mode_config.h       |  8 ++++++++
> >>   6 files changed, 36 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 3cf1aa132778..b768d397a811 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> >>   	state->planes_changed = false;
> >>   	state->connectors_changed = false;
> >>   	state->color_mgmt_changed = false;
> >> +	state->variable_refresh_changed = false;
> > 
> > Another bool just to check if one bool changed seems a bit excessive.
> > Is there a reason you can't directly check if the other bool changed?
> 
> It's nice for an atomic driver to be able to check if the property has 
> changed to steer control flow.
> 
> The driver could just check if the old crtc variable refresh value isn't 
> equal to the new one itself, but there's already precedent set to 
> provide flags like these instead.

Generally the changed flags are for more complicated things. Not
sure we want to start adding them for every little thing. Though
I suppose active_changed blows a hole in that theory.

> 
> It also lets the driver change it as needed during atomic commits. 
> You'll see many drivers making use of the other flags like 
> connectors_changed, mode_changed, etc.
> 
> > 
> > Actually I don't understand why this per-crtc thing is being added at
> > all. You already have the prop on the connector. Why do we want to
> > make life more difficult by requiring the thing to be set on both the
> > crtc and connector?
> 
> It doesn't make much sense without both.
> 
> The user can globally enable or disable the variable_refresh_enabled on 
> the connector. This is the user's preference.
> 
> What they don't control is the variable_refresh - the content hint that 
> userspace specifies when the CRTC contents are suitable for enabling 
> variable refresh features (like adaptive sync). This is userspace's 
> preference.

By userspace I guess you mean the compositor/display server. I don't
really see why the kernel has to be involved like this in a userspace
policy matter. If the compositor doesn't think vrr is a good idea then
it could simply choose to disable the prop on the connector even when
requested by its clients.

> 
> When both preferences agree, only then will variable refresh features be 
> enabled.
> 
> The reasoning for the split is because not all content is suitable for 
> variable refresh. Desktop environments, web browsers, etc only typically 
> flip when needed - which will result in display flickering.
> 
> The userspace integration as part of these patches demonstrates enabling 
> variable_refresh on the CRTC selectively.
> 
> > 
> >>   	state->zpos_changed = false;
> >>   	state->commit = NULL;
> >>   	state->event = NULL;
> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index 0bb27a24a55c..32a4cf8a01c3 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >>   		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
> >>   		drm_property_blob_put(mode);
> >>   		return ret;
> >> +	} else if (property == config->prop_variable_refresh) {
> >> +		if (state->variable_refresh != val)
> >> +			state->variable_refresh_changed = true;
> >> +		state->variable_refresh = val;
> >>   	} else if (property == config->degamma_lut_property) {
> >>   		ret = drm_atomic_replace_property_blob_from_id(dev,
> >>   					&state->degamma_lut,
> >> @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> >>   		*val = state->active;
> >>   	else if (property == config->prop_mode_id)
> >>   		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
> >> +	else if (property == config->prop_variable_refresh)
> >> +		*val = state->variable_refresh;
> >>   	else if (property == config->degamma_lut_property)
> >>   		*val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
> >>   	else if (property == config->ctm_property)
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index 5f488aa80bcd..ca33d6fb90ac 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> >>   		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
> >>   		drm_object_attach_property(&crtc->base,
> >>   					   config->prop_out_fence_ptr, 0);
> >> +		drm_object_attach_property(&crtc->base,
> >> +					   config->prop_variable_refresh, 0);
> >>   	}
> >>   
> >>   	return 0;
> >> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> >> index ee80788f2c40..1287c161d65d 100644
> >> --- a/drivers/gpu/drm/drm_mode_config.c
> >> +++ b/drivers/gpu/drm/drm_mode_config.c
> >> @@ -310,6 +310,12 @@ 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,
> >> +			"VARIABLE_REFRESH");
> >> +	if (!prop)
> >> +		return -ENOMEM;
> >> +	dev->mode_config.prop_variable_refresh = prop;
> >> +
> >>   	prop = drm_property_create(dev,
> >>   			DRM_MODE_PROP_BLOB,
> >>   			"DEGAMMA_LUT", 0);
> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> index b21437bc95bf..32b77f18ce6d 100644
> >> --- a/include/drm/drm_crtc.h
> >> +++ b/include/drm/drm_crtc.h
> >> @@ -168,6 +168,12 @@ struct drm_crtc_state {
> >>   	 * drivers to steer the atomic commit control flow.
> >>   	 */
> >>   	bool color_mgmt_changed : 1;
> >> +	/**
> >> +	 * @variable_refresh_changed: Variable refresh support has changed
> >> +	 * on the CRTC. Used by the atomic helpers and drivers to steer the
> >> +	 * atomic commit control flow.
> >> +	 */
> >> +	bool variable_refresh_changed : 1;
> >>   
> >>   	/**
> >>   	 * @no_vblank:
> >> @@ -290,6 +296,13 @@ struct drm_crtc_state {
> >>   	 */
> >>   	u32 pageflip_flags;
> >>   
> >> +	/**
> >> +	 * @variable_refresh:
> >> +	 *
> >> +	 * Indicates whether the CRTC is suitable for variable refresh rate.
> >> +	 */
> >> +	bool variable_refresh;
> >> +
> >>   	/**
> >>   	 * @event:
> >>   	 *
> >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> >> index 928e4172a0bb..1290191cd96e 100644
> >> --- a/include/drm/drm_mode_config.h
> >> +++ b/include/drm/drm_mode_config.h
> >> @@ -639,6 +639,14 @@ struct drm_mode_config {
> >>   	 * connectors must be of and active must be set to disabled, too.
> >>   	 */
> >>   	struct drm_property *prop_mode_id;
> >> +	/**
> >> +	 * @prop_variable_refresh: Default atomic CRTC property to indicate
> >> +	 * whether the CRTC is suitable for variable refresh rate support.
> >> +	 *
> >> +	 * This is only an indication of support, not whether variable
> >> +	 * refresh is active on the CRTC.
> >> +	 */
> >> +	struct drm_property *prop_variable_refresh;
> >>   
> >>   	/**
> >>   	 * @dvi_i_subconnector_property: Optional DVI-I property to
> >> -- 
> >> 2.19.0
> > 
> 
> Nicholas Kazlauskas
On 2018-09-24 10:26 p.m., Ville Syrjälä wrote:
> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
>>>
>>> Actually I don't understand why this per-crtc thing is being added at
>>> all. You already have the prop on the connector. Why do we want to
>>> make life more difficult by requiring the thing to be set on both the
>>> crtc and connector?
>>
>> It doesn't make much sense without both.
>>
>> The user can globally enable or disable the variable_refresh_enabled on 
>> the connector. This is the user's preference.
>>
>> What they don't control is the variable_refresh - the content hint that 
>> userspace specifies when the CRTC contents are suitable for enabling 
>> variable refresh features (like adaptive sync). This is userspace's 
>> preference.
> 
> By userspace I guess you mean the compositor/display server.

Actually rather the application, see the corresponding Mesa and
xf86-video-amdgpu patches.


> I don't really see why the kernel has to be involved like this in a
> userspace policy matter. If the compositor doesn't think vrr is a good
> idea then it could simply choose to disable the prop on the connector
> even when requested by its clients.

Connector properties are exposed directly to X11 clients as RandR output
properties. With only the connector property, the user running e.g.

 xrandr --output <name> --set variable_refresh_enabled 1

would result in variable refresh being enabled regardless of whether a
variable refresh compatible client is currently using page flipping,
which can result in flickering or getting stuck at the minimum refresh rate.
On 09/24/2018 04:26 PM, Ville Syrjälä wrote:
> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
>>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote:
>>>> Variable refresh rate algorithms have typically been enabled only
>>>> when the display is covered by a single source of content.
>>>>
>>>> This patch introduces a new default CRTC property that helps
>>>> hint to the driver when the CRTC composition is suitable for variable
>>>> refresh rate algorithms. Userspace can set this property dynamically
>>>> as the composition changes.
>>>>
>>>> Whether the variable refresh rate algorithms are active will still
>>>> depend on the CRTC being suitable and the connector being capable
>>>> and enabled by the user for variable refresh rate support.
>>>>
>>>> It is worth noting that while the property is atomic it isn't filtered
>>>> from legacy userspace queries. This allows for Xorg userspace drivers
>>>> to implement support in non-atomic setups.
>>>>
>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_atomic_helper.c |  1 +
>>>>    drivers/gpu/drm/drm_atomic_uapi.c   |  6 ++++++
>>>>    drivers/gpu/drm/drm_crtc.c          |  2 ++
>>>>    drivers/gpu/drm/drm_mode_config.c   |  6 ++++++
>>>>    include/drm/drm_crtc.h              | 13 +++++++++++++
>>>>    include/drm/drm_mode_config.h       |  8 ++++++++
>>>>    6 files changed, 36 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 3cf1aa132778..b768d397a811 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>>>>    	state->planes_changed = false;
>>>>    	state->connectors_changed = false;
>>>>    	state->color_mgmt_changed = false;
>>>> +	state->variable_refresh_changed = false;
>>>
>>> Another bool just to check if one bool changed seems a bit excessive.
>>> Is there a reason you can't directly check if the other bool changed?
>>
>> It's nice for an atomic driver to be able to check if the property has
>> changed to steer control flow.
>>
>> The driver could just check if the old crtc variable refresh value isn't
>> equal to the new one itself, but there's already precedent set to
>> provide flags like these instead.
> 
> Generally the changed flags are for more complicated things. Not
> sure we want to start adding them for every little thing. Though
> I suppose active_changed blows a hole in that theory.
> 
>>
>> It also lets the driver change it as needed during atomic commits.
>> You'll see many drivers making use of the other flags like
>> connectors_changed, mode_changed, etc.
>>
>>>
>>> Actually I don't understand why this per-crtc thing is being added at
>>> all. You already have the prop on the connector. Why do we want to
>>> make life more difficult by requiring the thing to be set on both the
>>> crtc and connector?
>>
>> It doesn't make much sense without both.
>>
>> The user can globally enable or disable the variable_refresh_enabled on
>> the connector. This is the user's preference.
>>
>> What they don't control is the variable_refresh - the content hint that
>> userspace specifies when the CRTC contents are suitable for enabling
>> variable refresh features (like adaptive sync). This is userspace's
>> preference.
> 
> By userspace I guess you mean the compositor/display server. I don't
> really see why the kernel has to be involved like this in a userspace
> policy matter. If the compositor doesn't think vrr is a good idea then
> it could simply choose to disable the prop on the connector even when
> requested by its clients.

The properties are both a kernel and userspace API so I think it's 
important to think about the usecase and API usage for both.

In a DRM driver the variable refresh capability will be determined by 
connector type and the display that's connected. The driver needs a way 
to track this if it's going to be sending any sort of packet over the 
connector. The capability property provides a method for the driver to 
do this while also exposing this information to the userspace for querying.

The variable_refresh_enabled property exists because not all users will 
want this feature enabled. I think it makes sense to place this 
alongside the capability property on the connector because of their 
relation and because of the ease of access for the end user in existing 
userspace stacks - xrandr makes this easy.

The variable_refresh CRTC property exists for a few reasons.

Userspace needs a method via the atomic API to let a DRM driver know 
that it wants variable frame timing to be enabled. The connector enable 
property certainly satisfies this condition - but I think there are 
other to take into consideration here.

Whenever I mentioned variable refresh "features", what I really meant 
was operating in one of two modes:

(1) Letting the driver and hardware adjust refresh automatically based 
on the flip rate on a CRTC from a single application

(2) Setting a fixed frame duration based on the flip rate on a CRTC from 
a single application

In both cases the important thing to note is that they're both dependent 
on how often a CRTC is flipping. There's also the "requirement" for 
single application in both cases - if there are multiple applications 
issuing flips then the hardware can't determine the correct content 
rate. The resulting user experience is going to be negative as the 
monitor seemingly adjusts to a random rate.

With the existing kernelspace and userspace stacks a DRM driver can't 
reasonably understand whether a single application is flipping or not 
and what variable refresh "mode" to operate in - these both depend on 
what's happening in userspace.

These factors are decided in userspace when interfacing with a DRM CRTC. 
The userspace integration patches I've posted alongside this interface 
demonstrate this usage - checks are done against a CRTC to see if the 
application fully covers the surface and the automatic adjustment mode 
is only enabled for when flips are issued for the CRTC originating from 
that application.

Determining which connectors use the CRTC can certainly be done and the 
property could be changed there, but I don't think this logically 
follows from the API usage described above.

The reasoning behind this being a default property on the CRTC is that I 
don't think userspace should need to keep track of what's actually 
connected using the CRTC. A user can hotplug displays at will or 
enable/disable variable refresh support via their display's OSD. 
Capabilities can change and this is only something a driver really needs 
to concern themselves with - the driver is what sends the control 
packets to the hardware.

I probably could have gone into more detail about some of this in the 
cover letter itself for these patchsets. These patches were actually a 
connector only interface originally but evolved after developing the 
actual implementation.

> 
>>
>> When both preferences agree, only then will variable refresh features be
>> enabled.
>>
>> The reasoning for the split is because not all content is suitable for
>> variable refresh. Desktop environments, web browsers, etc only typically
>> flip when needed - which will result in display flickering.
>>
>> The userspace integration as part of these patches demonstrates enabling
>> variable_refresh on the CRTC selectively.
>>
>>>
>>>>    	state->zpos_changed = false;
>>>>    	state->commit = NULL;
>>>>    	state->event = NULL;
>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> index 0bb27a24a55c..32a4cf8a01c3 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>>>    		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>>>    		drm_property_blob_put(mode);
>>>>    		return ret;
>>>> +	} else if (property == config->prop_variable_refresh) {
>>>> +		if (state->variable_refresh != val)
>>>> +			state->variable_refresh_changed = true;
>>>> +		state->variable_refresh = val;
>>>>    	} else if (property == config->degamma_lut_property) {
>>>>    		ret = drm_atomic_replace_property_blob_from_id(dev,
>>>>    					&state->degamma_lut,
>>>> @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>>>>    		*val = state->active;
>>>>    	else if (property == config->prop_mode_id)
>>>>    		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
>>>> +	else if (property == config->prop_variable_refresh)
>>>> +		*val = state->variable_refresh;
>>>>    	else if (property == config->degamma_lut_property)
>>>>    		*val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
>>>>    	else if (property == config->ctm_property)
>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>> index 5f488aa80bcd..ca33d6fb90ac 100644
>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>> @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>>>>    		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>>>>    		drm_object_attach_property(&crtc->base,
>>>>    					   config->prop_out_fence_ptr, 0);
>>>> +		drm_object_attach_property(&crtc->base,
>>>> +					   config->prop_variable_refresh, 0);
>>>>    	}
>>>>    
>>>>    	return 0;
>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>>>> index ee80788f2c40..1287c161d65d 100644
>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>> @@ -310,6 +310,12 @@ 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,
>>>> +			"VARIABLE_REFRESH");
>>>> +	if (!prop)
>>>> +		return -ENOMEM;
>>>> +	dev->mode_config.prop_variable_refresh = prop;
>>>> +
>>>>    	prop = drm_property_create(dev,
>>>>    			DRM_MODE_PROP_BLOB,
>>>>    			"DEGAMMA_LUT", 0);
>>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>>> index b21437bc95bf..32b77f18ce6d 100644
>>>> --- a/include/drm/drm_crtc.h
>>>> +++ b/include/drm/drm_crtc.h
>>>> @@ -168,6 +168,12 @@ struct drm_crtc_state {
>>>>    	 * drivers to steer the atomic commit control flow.
>>>>    	 */
>>>>    	bool color_mgmt_changed : 1;
>>>> +	/**
>>>> +	 * @variable_refresh_changed: Variable refresh support has changed
>>>> +	 * on the CRTC. Used by the atomic helpers and drivers to steer the
>>>> +	 * atomic commit control flow.
>>>> +	 */
>>>> +	bool variable_refresh_changed : 1;
>>>>    
>>>>    	/**
>>>>    	 * @no_vblank:
>>>> @@ -290,6 +296,13 @@ struct drm_crtc_state {
>>>>    	 */
>>>>    	u32 pageflip_flags;
>>>>    
>>>> +	/**
>>>> +	 * @variable_refresh:
>>>> +	 *
>>>> +	 * Indicates whether the CRTC is suitable for variable refresh rate.
>>>> +	 */
>>>> +	bool variable_refresh;
>>>> +
>>>>    	/**
>>>>    	 * @event:
>>>>    	 *
>>>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>>>> index 928e4172a0bb..1290191cd96e 100644
>>>> --- a/include/drm/drm_mode_config.h
>>>> +++ b/include/drm/drm_mode_config.h
>>>> @@ -639,6 +639,14 @@ struct drm_mode_config {
>>>>    	 * connectors must be of and active must be set to disabled, too.
>>>>    	 */
>>>>    	struct drm_property *prop_mode_id;
>>>> +	/**
>>>> +	 * @prop_variable_refresh: Default atomic CRTC property to indicate
>>>> +	 * whether the CRTC is suitable for variable refresh rate support.
>>>> +	 *
>>>> +	 * This is only an indication of support, not whether variable
>>>> +	 * refresh is active on the CRTC.
>>>> +	 */
>>>> +	struct drm_property *prop_variable_refresh;
>>>>    
>>>>    	/**
>>>>    	 * @dvi_i_subconnector_property: Optional DVI-I property to
>>>> -- 
>>>> 2.19.0
>>>
>>
>> Nicholas Kazlauskas
> 

Nicholas Kazlauskas
On Tue, Sep 25, 2018 at 03:28:28PM +0200, Michel Dänzer wrote:
> On 2018-09-24 10:26 p.m., Ville Syrjälä wrote:
> > On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
> >> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
> >>>
> >>> Actually I don't understand why this per-crtc thing is being added at
> >>> all. You already have the prop on the connector. Why do we want to
> >>> make life more difficult by requiring the thing to be set on both the
> >>> crtc and connector?
> >>
> >> It doesn't make much sense without both.
> >>
> >> The user can globally enable or disable the variable_refresh_enabled on 
> >> the connector. This is the user's preference.
> >>
> >> What they don't control is the variable_refresh - the content hint that 
> >> userspace specifies when the CRTC contents are suitable for enabling 
> >> variable refresh features (like adaptive sync). This is userspace's 
> >> preference.
> > 
> > By userspace I guess you mean the compositor/display server.
> 
> Actually rather the application, see the corresponding Mesa and
> xf86-video-amdgpu patches.
> 
> 
> > I don't really see why the kernel has to be involved like this in a
> > userspace policy matter. If the compositor doesn't think vrr is a good
> > idea then it could simply choose to disable the prop on the connector
> > even when requested by its clients.
> 
> Connector properties are exposed directly to X11 clients as RandR output
> properties. With only the connector property, the user running e.g.
> 
>  xrandr --output <name> --set variable_refresh_enabled 1
> 
> would result in variable refresh being enabled regardless of whether a
> variable refresh compatible client is currently using page flipping,
> which can result in flickering or getting stuck at the minimum refresh rate.

in ddx:

configure_vrr()
{
	kms_vrr = client_vrr && is_vrr_a_good_idea();
	kms_setprop(kms_vrr);
}

set_prop()
{
	if (is_vrr_prop)
		configure_vrr();
	else
		kms_setprop();
}

other_stuff()
{
	/* some stuff */
	...

	if (vrr_related_stuff_changed)
		configure_vrr();
}

or something along those lines?
On 2018-09-25 4:04 p.m., Ville Syrjälä wrote:
> On Tue, Sep 25, 2018 at 03:28:28PM +0200, Michel Dänzer wrote:
>> On 2018-09-24 10:26 p.m., Ville Syrjälä wrote:
>>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
>>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
>>>>>
>>>>> Actually I don't understand why this per-crtc thing is being added at
>>>>> all. You already have the prop on the connector. Why do we want to
>>>>> make life more difficult by requiring the thing to be set on both the
>>>>> crtc and connector?
>>>>
>>>> It doesn't make much sense without both.
>>>>
>>>> The user can globally enable or disable the variable_refresh_enabled on 
>>>> the connector. This is the user's preference.
>>>>
>>>> What they don't control is the variable_refresh - the content hint that 
>>>> userspace specifies when the CRTC contents are suitable for enabling 
>>>> variable refresh features (like adaptive sync). This is userspace's 
>>>> preference.
>>>
>>> By userspace I guess you mean the compositor/display server.
>>
>> Actually rather the application, see the corresponding Mesa and
>> xf86-video-amdgpu patches.
>>
>>
>>> I don't really see why the kernel has to be involved like this in a
>>> userspace policy matter. If the compositor doesn't think vrr is a good
>>> idea then it could simply choose to disable the prop on the connector
>>> even when requested by its clients.
>>
>> Connector properties are exposed directly to X11 clients as RandR output
>> properties. With only the connector property, the user running e.g.
>>
>>  xrandr --output <name> --set variable_refresh_enabled 1
>>
>> would result in variable refresh being enabled regardless of whether a
>> variable refresh compatible client is currently using page flipping,
>> which can result in flickering or getting stuck at the minimum refresh rate.
> 
> in ddx:
> 
> configure_vrr()
> {
> 	kms_vrr = client_vrr && is_vrr_a_good_idea();
> 	kms_setprop(kms_vrr);
> }
> 
> set_prop()
> {
> 	if (is_vrr_prop)
> 		configure_vrr();
> 	else
> 		kms_setprop();
> }
> 
> other_stuff()
> {
> 	/* some stuff */
> 	...
> 
> 	if (vrr_related_stuff_changed)
> 		configure_vrr();
> }
> 
> or something along those lines?
> 

Sure, that's not the problem, which is that if the Xorg driver doesn't
actively hide the property from clients (e.g. because it's a currently
released version), we get the issue I described above.

Also, if the Xorg driver were to hide the connector property from
clients, there's no way for the user to completely disable variable
refresh for a display (unless the Xorg driver exposes another fake
property for that, which seems a bit silly).
On Tue, Sep 25, 2018 at 04:35:59PM +0200, Michel Dänzer wrote:
> On 2018-09-25 4:04 p.m., Ville Syrjälä wrote:
> > On Tue, Sep 25, 2018 at 03:28:28PM +0200, Michel Dänzer wrote:
> >> On 2018-09-24 10:26 p.m., Ville Syrjälä wrote:
> >>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
> >>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
> >>>>>
> >>>>> Actually I don't understand why this per-crtc thing is being added at
> >>>>> all. You already have the prop on the connector. Why do we want to
> >>>>> make life more difficult by requiring the thing to be set on both the
> >>>>> crtc and connector?
> >>>>
> >>>> It doesn't make much sense without both.
> >>>>
> >>>> The user can globally enable or disable the variable_refresh_enabled on 
> >>>> the connector. This is the user's preference.
> >>>>
> >>>> What they don't control is the variable_refresh - the content hint that 
> >>>> userspace specifies when the CRTC contents are suitable for enabling 
> >>>> variable refresh features (like adaptive sync). This is userspace's 
> >>>> preference.
> >>>
> >>> By userspace I guess you mean the compositor/display server.
> >>
> >> Actually rather the application, see the corresponding Mesa and
> >> xf86-video-amdgpu patches.
> >>
> >>
> >>> I don't really see why the kernel has to be involved like this in a
> >>> userspace policy matter. If the compositor doesn't think vrr is a good
> >>> idea then it could simply choose to disable the prop on the connector
> >>> even when requested by its clients.
> >>
> >> Connector properties are exposed directly to X11 clients as RandR output
> >> properties. With only the connector property, the user running e.g.
> >>
> >>  xrandr --output <name> --set variable_refresh_enabled 1
> >>
> >> would result in variable refresh being enabled regardless of whether a
> >> variable refresh compatible client is currently using page flipping,
> >> which can result in flickering or getting stuck at the minimum refresh rate.
> > 
> > in ddx:
> > 
> > configure_vrr()
> > {
> > 	kms_vrr = client_vrr && is_vrr_a_good_idea();
> > 	kms_setprop(kms_vrr);
> > }
> > 
> > set_prop()
> > {
> > 	if (is_vrr_prop)
> > 		configure_vrr();
> > 	else
> > 		kms_setprop();
> > }
> > 
> > other_stuff()
> > {
> > 	/* some stuff */
> > 	...
> > 
> > 	if (vrr_related_stuff_changed)
> > 		configure_vrr();
> > }
> > 
> > or something along those lines?
> > 
> 
> Sure, that's not the problem, which is that if the Xorg driver doesn't
> actively hide the property from clients (e.g. because it's a currently
> released version), we get the issue I described above.

That sounds more like what client caps were meant for. Or maybe
drop the connector vrr enable prop and just go with the crtc one?

> 
> Also, if the Xorg driver were to hide the connector property from
> clients, there's no way for the user to completely disable variable
> refresh for a display (unless the Xorg driver exposes another fake
> property for that, which seems a bit silly).
> 
> 
> -- 
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
On Tue, Sep 25, 2018 at 06:28:17PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 25, 2018 at 04:35:59PM +0200, Michel Dänzer wrote:
> > On 2018-09-25 4:04 p.m., Ville Syrjälä wrote:
> > > On Tue, Sep 25, 2018 at 03:28:28PM +0200, Michel Dänzer wrote:
> > >> On 2018-09-24 10:26 p.m., Ville Syrjälä wrote:
> > >>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
> > >>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
> > >>>>>
> > >>>>> Actually I don't understand why this per-crtc thing is being added at
> > >>>>> all. You already have the prop on the connector. Why do we want to
> > >>>>> make life more difficult by requiring the thing to be set on both the
> > >>>>> crtc and connector?
> > >>>>
> > >>>> It doesn't make much sense without both.
> > >>>>
> > >>>> The user can globally enable or disable the variable_refresh_enabled on 
> > >>>> the connector. This is the user's preference.
> > >>>>
> > >>>> What they don't control is the variable_refresh - the content hint that 
> > >>>> userspace specifies when the CRTC contents are suitable for enabling 
> > >>>> variable refresh features (like adaptive sync). This is userspace's 
> > >>>> preference.
> > >>>
> > >>> By userspace I guess you mean the compositor/display server.
> > >>
> > >> Actually rather the application, see the corresponding Mesa and
> > >> xf86-video-amdgpu patches.
> > >>
> > >>
> > >>> I don't really see why the kernel has to be involved like this in a
> > >>> userspace policy matter. If the compositor doesn't think vrr is a good
> > >>> idea then it could simply choose to disable the prop on the connector
> > >>> even when requested by its clients.
> > >>
> > >> Connector properties are exposed directly to X11 clients as RandR output
> > >> properties. With only the connector property, the user running e.g.
> > >>
> > >>  xrandr --output <name> --set variable_refresh_enabled 1
> > >>
> > >> would result in variable refresh being enabled regardless of whether a
> > >> variable refresh compatible client is currently using page flipping,
> > >> which can result in flickering or getting stuck at the minimum refresh rate.
> > > 
> > > in ddx:
> > > 
> > > configure_vrr()
> > > {
> > > 	kms_vrr = client_vrr && is_vrr_a_good_idea();
> > > 	kms_setprop(kms_vrr);
> > > }
> > > 
> > > set_prop()
> > > {
> > > 	if (is_vrr_prop)
> > > 		configure_vrr();
> > > 	else
> > > 		kms_setprop();
> > > }
> > > 
> > > other_stuff()
> > > {
> > > 	/* some stuff */
> > > 	...
> > > 
> > > 	if (vrr_related_stuff_changed)
> > > 		configure_vrr();
> > > }
> > > 
> > > or something along those lines?
> > > 
> > 
> > Sure, that's not the problem, which is that if the Xorg driver doesn't
> > actively hide the property from clients (e.g. because it's a currently
> > released version), we get the issue I described above.
> 
> That sounds more like what client caps were meant for. Or maybe
> drop the connector vrr enable prop and just go with the crtc one?
> 
> > Also, if the Xorg driver were to hide the connector property from
> > clients, there's no way for the user to completely disable variable
> > refresh for a display (unless the Xorg driver exposes another fake
> > property for that, which seems a bit silly).

Even easier solution: Only add the enable knob on the CRTC. The CRTC is
the place where we set the timing information (through the mode), not the
connector. Splitting timing information across crtc and connector doesn't
make sense to me, especially since you can have (in theory at least)
multiple connectors.

And once it's at the CRTC, you don't have the forward-compat issue
anymore, it's up to the DDX to decide how/where exactly to expose this
knob. Simplest (because no one loves to extend xproto) seems indeed to be
to add it as an xrandr connector prop. But just because that's the
reasonable solution at the xrandr level doesn't mean it's a great at idea
at the kms api level.
-Daniel
Hi,

I have a somewhat of my own view on what would be involved with VRR,
and I'd like to hear what you think of it. Comments inline.


On Tue, 25 Sep 2018 09:51:37 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:

> On 09/24/2018 04:26 PM, Ville Syrjälä wrote:
> > On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:  
> >> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:  
> >>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote:  
> >>>> Variable refresh rate algorithms have typically been enabled only
> >>>> when the display is covered by a single source of content.
> >>>>
> >>>> This patch introduces a new default CRTC property that helps
> >>>> hint to the driver when the CRTC composition is suitable for variable
> >>>> refresh rate algorithms. Userspace can set this property dynamically
> >>>> as the composition changes.
> >>>>
> >>>> Whether the variable refresh rate algorithms are active will still
> >>>> depend on the CRTC being suitable and the connector being capable
> >>>> and enabled by the user for variable refresh rate support.
> >>>>
> >>>> It is worth noting that while the property is atomic it isn't filtered
> >>>> from legacy userspace queries. This allows for Xorg userspace drivers
> >>>> to implement support in non-atomic setups.
> >>>>
> >>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

...

> >>> Actually I don't understand why this per-crtc thing is being added at
> >>> all. You already have the prop on the connector. Why do we want to
> >>> make life more difficult by requiring the thing to be set on both the
> >>> crtc and connector?  
> >>
> >> It doesn't make much sense without both.
> >>
> >> The user can globally enable or disable the variable_refresh_enabled on
> >> the connector. This is the user's preference.
> >>
> >> What they don't control is the variable_refresh - the content hint that
> >> userspace specifies when the CRTC contents are suitable for enabling
> >> variable refresh features (like adaptive sync). This is userspace's
> >> preference.  
> > 
> > By userspace I guess you mean the compositor/display server. I don't
> > really see why the kernel has to be involved like this in a userspace
> > policy matter. If the compositor doesn't think vrr is a good idea then
> > it could simply choose to disable the prop on the connector even when
> > requested by its clients.  
> 
> The properties are both a kernel and userspace API so I think it's 
> important to think about the usecase and API usage for both.
> 
> In a DRM driver the variable refresh capability will be determined by 
> connector type and the display that's connected. The driver needs a way 
> to track this if it's going to be sending any sort of packet over the 
> connector. The capability property provides a method for the driver to 
> do this while also exposing this information to the userspace for querying.
> 
> The variable_refresh_enabled property exists because not all users will 
> want this feature enabled. I think it makes sense to place this 
> alongside the capability property on the connector because of their 
> relation and because of the ease of access for the end user in existing 
> userspace stacks - xrandr makes this easy.

I'm not sure I understood your intention here. Do you mean that you
expect games (e.g. via Mesa) to toggle the connector property via RandR
to tell if they wish for VRR?

The RandR interface is for display configuration utilities, like the UIs
that desktop environments use to configure monitors. It should not be
used by applications like games to modify anything. I believe there are
lots of apps out there that enrage users by abusing RandR, but one
should not base an interface design on such abuse.

If games are not using RandR to communicate the wish for VRR, what do
they use? Does the X11 Present extension have something for it?

What is the application-facing API to request VRR, is it in GLX, EGL,
Vulkan, something else?

Btw. I would not expect xrandr to qualify as proper userspace to
validate new kernel UABI, like new properties. Did you have something
else for the userspace settable connector property?

> The variable_refresh CRTC property exists for a few reasons.
> 
> Userspace needs a method via the atomic API to let a DRM driver know 
> that it wants variable frame timing to be enabled. The connector enable 
> property certainly satisfies this condition - but I think there are 
> other to take into consideration here.

I agree that this CRTC property makes sense.

> Whenever I mentioned variable refresh "features", what I really meant 
> was operating in one of two modes:
> 
> (1) Letting the driver and hardware adjust refresh automatically based 
> on the flip rate on a CRTC from a single application
> 
> (2) Setting a fixed frame duration based on the flip rate on a CRTC from 
> a single application

I wonder if that's too much magic in the kernel... what would be wrong
with simply flipping ASAP when VRR is active?

How will userspace be able to predict coming flip opportunities if the
kernel does so much magic?

> In both cases the important thing to note is that they're both dependent 
> on how often a CRTC is flipping. There's also the "requirement" for 
> single application in both cases - if there are multiple applications 
> issuing flips then the hardware can't determine the correct content 
> rate. The resulting user experience is going to be negative as the 
> monitor seemingly adjusts to a random rate.
> 
> With the existing kernelspace and userspace stacks a DRM driver can't 
> reasonably understand whether a single application is flipping or not 
> and what variable refresh "mode" to operate in - these both depend on 
> what's happening in userspace.
> 
> These factors are decided in userspace when interfacing with a DRM CRTC. 
> The userspace integration patches I've posted alongside this interface 
> demonstrate this usage - checks are done against a CRTC to see if the 
> application fully covers the surface and the automatic adjustment mode 
> is only enabled for when flips are issued for the CRTC originating from 
> that application.
> 
> Determining which connectors use the CRTC can certainly be done and the 
> property could be changed there, but I don't think this logically 
> follows from the API usage described above.
> 
> The reasoning behind this being a default property on the CRTC is that I 
> don't think userspace should need to keep track of what's actually 
> connected using the CRTC. A user can hotplug displays at will or 
> enable/disable variable refresh support via their display's OSD. 
> Capabilities can change and this is only something a driver really needs 
> to concern themselves with - the driver is what sends the control 
> packets to the hardware.

Userspace must already track carefully what is connected and what is
driven through which CRTC, otherwise it cannot drive the KMS API
correctly.

> I probably could have gone into more detail about some of this in the 
> cover letter itself for these patchsets. These patches were actually a 
> connector only interface originally but evolved after developing the 
> actual implementation.
> 
> >   
> >>
> >> When both preferences agree, only then will variable refresh features be
> >> enabled.
> >>
> >> The reasoning for the split is because not all content is suitable for
> >> variable refresh. Desktop environments, web browsers, etc only typically
> >> flip when needed - which will result in display flickering.

Flickering? What do you mean?

> >>
> >> The userspace integration as part of these patches demonstrates enabling
> >> variable_refresh on the CRTC selectively.


I believe that VRR on X11 would probably depend on these bits:

1. Hardware capability
	Whether the monitor, the CRTC hardware, and the kernel driver
	all agree that VRR is possible.

2. User preference
	A toggle on the desktop environment's display settings
	application to allow or disallow VRR. After all, video mode
	configuration is here too, and ideally applications should not
	mess with the video mode directly.

3. Application preference
	Whether the application, e.g. a game, is prepared to deal with
	VRR and wishes it to be enabled.

4. X server scenegraph
	Which windows happen to be showing on the VRR-capable output
	and does that scenegraph allow e.g. flipping straight to client
	buffers instead of having the X server copy into framebuffers
	of its own.

You mentioned that VRR probably doesn't make sense if there are
multiple windows showing in the same output, so point 4 would cover
that. Also games etc. would aim to hit the direct scanout path to avoid
wasting time in the X server copy, so it seems that requiring flips to
client buffers would be reasonable for enabling VRR. Sounds like that
is exactly how you implemented it in xf86-video-amdgpu, isn't it?

Skimming through your proposal, it seems you have covered 3 out of 4
points. Did you or I miss something? You cannot have points 2 and 3
fight over the control of the same property.

How do you envision VRR to interact with X11 compositing managers?

I presume compositing managers aim for the direct scanout path in the X
server to avoid yet another copy there, right? Does point 4 need to
exclude the Composite Overlay Window (COW) from allowed VRR windows?


Given all the above, I would like to question the choice of trying to
accommodate X11 interfaces directly in the DRM UABI. There seems to be
quite many bits controlled by different processes and VRR should be
active only when they all agree. Therefore, if you want end user
applications to rely on straight connector property pass-through via
RandR, you end up with more properties than strictly necessary,
complicating the UABI.

In other words, I agree with the criticism here by Ville and Daniel.


On the other hand, in Wayland architecture, there is only one single
userspace process you need to consider in the DRM UABI: the display
server:
- There is no separate compositing manager process.
- There is no separate application to handle user preference (point 2);
  well, not in the sense that you would need to consider it in the DRM
  UABI, because it will go through the display server that is part of
  the desktop rather than bypassing the desktop.
- There is no DRM property pass-through to end-user applications,
  instead there will be dedicated protocol extensions to let the
  display server make the final call.

Therefore, it would be fine to have just two bits:

1. Hardware capability
	Whether the monitor, the CRTC hardware, and the kernel driver
	all agree that VRR is possible.

2. - 4. Whether userspace (i.e. the display server) wants to use VRR or
	not. This would be the CRTC property.

I also believe that it would be useful to expose vmin/vmax to userspace
as properties, so that display servers and apps can plan ahead on when
they will render. I suppose that can be left for later when someone
starts working on userspace taking advantage of it, so that the proper
userspace requirement for new UABI is fulfilled.


Thanks,
pq
On 10/05/2018 04:10 AM, Pekka Paalanen wrote:
> Hi,
> 
> I have a somewhat of my own view on what would be involved with VRR,
> and I'd like to hear what you think of it. Comments inline.
> 
> 
> On Tue, 25 Sep 2018 09:51:37 -0400
> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> 
>> On 09/24/2018 04:26 PM, Ville Syrjälä wrote:
>>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
>>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
>>>>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote:
>>>>>> Variable refresh rate algorithms have typically been enabled only
>>>>>> when the display is covered by a single source of content.
>>>>>>
>>>>>> This patch introduces a new default CRTC property that helps
>>>>>> hint to the driver when the CRTC composition is suitable for variable
>>>>>> refresh rate algorithms. Userspace can set this property dynamically
>>>>>> as the composition changes.
>>>>>>
>>>>>> Whether the variable refresh rate algorithms are active will still
>>>>>> depend on the CRTC being suitable and the connector being capable
>>>>>> and enabled by the user for variable refresh rate support.
>>>>>>
>>>>>> It is worth noting that while the property is atomic it isn't filtered
>>>>>> from legacy userspace queries. This allows for Xorg userspace drivers
>>>>>> to implement support in non-atomic setups.
>>>>>>
>>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> 
> ...
> 
>>>>> Actually I don't understand why this per-crtc thing is being added at
>>>>> all. You already have the prop on the connector. Why do we want to
>>>>> make life more difficult by requiring the thing to be set on both the
>>>>> crtc and connector?
>>>>
>>>> It doesn't make much sense without both.
>>>>
>>>> The user can globally enable or disable the variable_refresh_enabled on
>>>> the connector. This is the user's preference.
>>>>
>>>> What they don't control is the variable_refresh - the content hint that
>>>> userspace specifies when the CRTC contents are suitable for enabling
>>>> variable refresh features (like adaptive sync). This is userspace's
>>>> preference.
>>>
>>> By userspace I guess you mean the compositor/display server. I don't
>>> really see why the kernel has to be involved like this in a userspace
>>> policy matter. If the compositor doesn't think vrr is a good idea then
>>> it could simply choose to disable the prop on the connector even when
>>> requested by its clients.
>>
>> The properties are both a kernel and userspace API so I think it's
>> important to think about the usecase and API usage for both.
>>
>> In a DRM driver the variable refresh capability will be determined by
>> connector type and the display that's connected. The driver needs a way
>> to track this if it's going to be sending any sort of packet over the
>> connector. The capability property provides a method for the driver to
>> do this while also exposing this information to the userspace for querying.
>>
>> The variable_refresh_enabled property exists because not all users will
>> want this feature enabled. I think it makes sense to place this
>> alongside the capability property on the connector because of their
>> relation and because of the ease of access for the end user in existing
>> userspace stacks - xrandr makes this easy.
> 
> I'm not sure I understood your intention here. Do you mean that you
> expect games (e.g. via Mesa) to toggle the connector property via RandR
> to tell if they wish for VRR?
> 
> The RandR interface is for display configuration utilities, like the UIs
> that desktop environments use to configure monitors. It should not be
> used by applications like games to modify anything. I believe there are
> lots of apps out there that enrage users by abusing RandR, but one
> should not base an interface design on such abuse.
> 
> If games are not using RandR to communicate the wish for VRR, what do
> they use? Does the X11 Present extension have something for it?
> 
> What is the application-facing API to request VRR, is it in GLX, EGL,
> Vulkan, something else?
> 
> Btw. I would not expect xrandr to qualify as proper userspace to
> validate new kernel UABI, like new properties. Did you have something
> else for the userspace settable connector property?

The usecase I was considering with variable_refresh_enabled was a 
graphical UI in a settings app within desktop environment after they've 
checked variable_refresh_capable. I was thinking of this as a sort of 
"standard" way of configuring whether the user wants VRR enabled but 
you're right about xrandr not really demonstrating this.

Every desktop environment already has their own way of dealing with 
configuration and persisting settings so this doesn't bring any benefit 
in that regard.

I'm almost finished writing up v3s that should address the concerns that 
Ville, Daniel and you have raised regarding this. They drop the 
"variable_refresh_enabled" property on the connector.

> 
>> The variable_refresh CRTC property exists for a few reasons.
>>
>> Userspace needs a method via the atomic API to let a DRM driver know
>> that it wants variable frame timing to be enabled. The connector enable
>> property certainly satisfies this condition - but I think there are
>> other to take into consideration here.
> 
> I agree that this CRTC property makes sense.
> 
>> Whenever I mentioned variable refresh "features", what I really meant
>> was operating in one of two modes:
>>
>> (1) Letting the driver and hardware adjust refresh automatically based
>> on the flip rate on a CRTC from a single application
>>
>> (2) Setting a fixed frame duration based on the flip rate on a CRTC from
>> a single application
> 
> I wonder if that's too much magic in the kernel... what would be wrong
> with simply flipping ASAP when VRR is active?
> 
> How will userspace be able to predict coming flip opportunities if the
> kernel does so much magic?

The kernel driver doesn't need to do much more than let the hardware 
know the variable refresh range. The "magic" is performed by hardware.

Most games would like to render as fast as possible to deliver a more 
responsive and smoother image to the user. Many of these are also 
resource intensive and won't always be able to render at the fixed 
refresh rate of the panel (especially for higher refresh rates like 
144Hz). The user will experience stuttering if the game takes too long 
to render and misses the vblank window for the flip.

Dynamic VRR adjustment can resolve this problem. The hardware can lower 
the refresh rate and increase the vblank window in response to this so 
the user doesn't experience stuttering (or latency).

Userspace shouldn't predict anything.

> 
>> In both cases the important thing to note is that they're both dependent
>> on how often a CRTC is flipping. There's also the "requirement" for
>> single application in both cases - if there are multiple applications
>> issuing flips then the hardware can't determine the correct content
>> rate. The resulting user experience is going to be negative as the
>> monitor seemingly adjusts to a random rate.
>>
>> With the existing kernelspace and userspace stacks a DRM driver can't
>> reasonably understand whether a single application is flipping or not
>> and what variable refresh "mode" to operate in - these both depend on
>> what's happening in userspace.
>>
>> These factors are decided in userspace when interfacing with a DRM CRTC.
>> The userspace integration patches I've posted alongside this interface
>> demonstrate this usage - checks are done against a CRTC to see if the
>> application fully covers the surface and the automatic adjustment mode
>> is only enabled for when flips are issued for the CRTC originating from
>> that application.
>>
>> Determining which connectors use the CRTC can certainly be done and the
>> property could be changed there, but I don't think this logically
>> follows from the API usage described above.
>>
>> The reasoning behind this being a default property on the CRTC is that I
>> don't think userspace should need to keep track of what's actually
>> connected using the CRTC. A user can hotplug displays at will or
>> enable/disable variable refresh support via their display's OSD.
>> Capabilities can change and this is only something a driver really needs
>> to concern themselves with - the driver is what sends the control
>> packets to the hardware.
> 
> Userspace must already track carefully what is connected and what is
> driven through which CRTC, otherwise it cannot drive the KMS API
> correctly.
> 
>> I probably could have gone into more detail about some of this in the
>> cover letter itself for these patchsets. These patches were actually a
>> connector only interface originally but evolved after developing the
>> actual implementation.
>>
>>>    
>>>>
>>>> When both preferences agree, only then will variable refresh features be
>>>> enabled.
>>>>
>>>> The reasoning for the split is because not all content is suitable for
>>>> variable refresh. Desktop environments, web browsers, etc only typically
>>>> flip when needed - which will result in display flickering.
> 
> Flickering? What do you mean?

This is a property of how panels work.

The luminance for a panel will vary based on how long the vrefresh is. 
Since the vrefresh length is changing as part of VRR you're more likely 
to notice the difference in luminance the bigger the difference is.

The difference will be largest when switching from the min vrefresh to 
the max vrefresh duration.

Large differences can occur for applications that render on demand like 
a web browser (and why you wouldn't want VRR enabled for those). The 
hardware would continuously wait for a flip that isn't coming. Then if 
the user moves their cursor or the page updates it's going to happen 
"randomly" in that window and the hardware will adjust to that.

> 
>>>>
>>>> The userspace integration as part of these patches demonstrates enabling
>>>> variable_refresh on the CRTC selectively.
> 
> 
> I believe that VRR on X11 would probably depend on these bits:
> 
> 1. Hardware capability
> 	Whether the monitor, the CRTC hardware, and the kernel driver
> 	all agree that VRR is possible.
> 
> 2. User preference
> 	A toggle on the desktop environment's display settings
> 	application to allow or disallow VRR. After all, video mode
> 	configuration is here too, and ideally applications should not
> 	mess with the video mode directly.
> 
> 3. Application preference
> 	Whether the application, e.g. a game, is prepared to deal with
> 	VRR and wishes it to be enabled.
> 
> 4. X server scenegraph
> 	Which windows happen to be showing on the VRR-capable output
> 	and does that scenegraph allow e.g. flipping straight to client
> 	buffers instead of having the X server copy into framebuffers
> 	of its own.
> 
> You mentioned that VRR probably doesn't make sense if there are
> multiple windows showing in the same output, so point 4 would cover
> that. Also games etc. would aim to hit the direct scanout path to avoid
> wasting time in the X server copy, so it seems that requiring flips to
> client buffers would be reasonable for enabling VRR. Sounds like that
> is exactly how you implemented it in xf86-video-amdgpu, isn't it?
> 
> Skimming through your proposal, it seems you have covered 3 out of 4
> points. Did you or I miss something? You cannot have points 2 and 3
> fight over the control of the same property.
> 
> How do you envision VRR to interact with X11 compositing managers?
> 
> I presume compositing managers aim for the direct scanout path in the X
> server to avoid yet another copy there, right? Does point 4 need to
> exclude the Composite Overlay Window (COW) from allowed VRR windows?
> 
> 
> Given all the above, I would like to question the choice of trying to
> accommodate X11 interfaces directly in the DRM UABI. There seems to be
> quite many bits controlled by different processes and VRR should be
> active only when they all agree. Therefore, if you want end user
> applications to rely on straight connector property pass-through via
> RandR, you end up with more properties than strictly necessary,
> complicating the UABI.
> 
> In other words, I agree with the criticism here by Ville and Daniel

Your interpretation of the X11 stack is mostly the same as mine with 
minor differences for the 2-4.

For the sake of the explanation I'm actually going to discuss what's 
part of the plan for v3 here since it's a little simpler and should 
address your concerns.

There's a large library of existing games and no modification should be 
needed for them to support the dynamic VRR adjustment. They all share a 
common render stack with GL or Vulkan. So mesa can be leveraged to 
"mark" the applications as suitable for VRR - with a window property in 
this case. There are less applications that are unsuitable than there 
are suitable so a blacklist approach is done here - an opt-out approach 
for application preference. This covers 3.

User preference can be handled as part of the DDX driver with something 
like an X Option. Dropping the variable_refresh_enabled property in 
favor of this works. This covers 2.

For application suitability, the Present extension will be leveraged 
here since it covers the logical restrictions (single application, CRTC 
covered) for this problem. If the window is flipping via the extension 
then it's suitable. This covers 4.

The CRTC VRR enable property would then be set in the DDX driver when 
user preference and application suitability match.

Which then leads into 1 - it will only be enabled when the hardware is 
capable.

Most compositors function well under this stack. It will vary depending 
on compositor support for window unredirection to let the window flip 
via the Present extension. Mutter handles this without any configuration 
and kwin can be configured to work with these patches. Compton can be 
configured to support unredirection as well.

These (among others) are covered as part of the blacklist for the mesa 
patches. They do need to be explicitly excluded.

> 
> 
> On the other hand, in Wayland architecture, there is only one single
> userspace process you need to consider in the DRM UABI: the display
> server:
> - There is no separate compositing manager process.
> - There is no separate application to handle user preference (point 2);
>    well, not in the sense that you would need to consider it in the DRM
>    UABI, because it will go through the display server that is part of
>    the desktop rather than bypassing the desktop.
> - There is no DRM property pass-through to end-user applications,
>    instead there will be dedicated protocol extensions to let the
>    display server make the final call.
> 
> Therefore, it would be fine to have just two bits:
> 
> 1. Hardware capability
> 	Whether the monitor, the CRTC hardware, and the kernel driver
> 	all agree that VRR is possible.
> 
> 2. - 4. Whether userspace (i.e. the display server) wants to use VRR or
> 	not. This would be the CRTC property.

I did some prototyping using the atomic API directly and the way I was 
utilizing the properties was exactly like this. Even more reason to go 
with the two properties (connector capable, CRTC enable) I suppose.

> 
> I also believe that it would be useful to expose vmin/vmax to userspace
> as properties, so that display servers and apps can plan ahead on when
> they will render. I suppose that can be left for later when someone
> starts working on userspace taking advantage of it, so that the proper
> userspace requirement for new UABI is fulfilled.

Knowing the vmin/vmax could potentially be useful for testing but most 
applications shouldn't be trying to predict or adjust rendering based on 
these.

I agree with the discussion for this coming later.

> 
> 
> Thanks,
> pq
> 

Nicholas Kazlauskas
On 2018-10-05 6:21 p.m., Kazlauskas, Nicholas wrote:
> On 10/05/2018 04:10 AM, Pekka Paalanen wrote:
>>
>> 2. User preference
>>     A toggle on the desktop environment's display settings
>>     application to allow or disallow VRR. After all, video mode
>>     configuration is here too, and ideally applications should not
>>     mess with the video mode directly.
>
> [...]
>
> User preference can be handled as part of the DDX driver with something
> like an X Option. Dropping the variable_refresh_enabled property in
> favor of this works. This covers 2.

The Xorg driver can expose a RandR output property, even if the kernel
doesn't expose a corresponding connector property. See e.g. the TearFree
output property in xf86-video-amdgpu.


>> I also believe that it would be useful to expose vmin/vmax to userspace
>> as properties, so that display servers and apps can plan ahead on when
>> they will render. I suppose that can be left for later when someone
>> starts working on userspace taking advantage of it, so that the proper
>> userspace requirement for new UABI is fulfilled.
> 
> Knowing the vmin/vmax could potentially be useful for testing but most
> applications shouldn't be trying to predict or adjust rendering based on
> these.

FWIW, recent research indicates that this is necessary for perfectly
smooth animation without micro-stuttering, even with VRR.
On 10/05/2018 12:56 PM, Michel Dänzer wrote:
> On 2018-10-05 6:21 p.m., Kazlauskas, Nicholas wrote:
>> On 10/05/2018 04:10 AM, Pekka Paalanen wrote:
>>>
>>> 2. User preference
>>>      A toggle on the desktop environment's display settings
>>>      application to allow or disallow VRR. After all, video mode
>>>      configuration is here too, and ideally applications should not
>>>      mess with the video mode directly.
>>
>> [...]
>>
>> User preference can be handled as part of the DDX driver with something
>> like an X Option. Dropping the variable_refresh_enabled property in
>> favor of this works. This covers 2.
> 
> The Xorg driver can expose a RandR output property, even if the kernel
> doesn't expose a corresponding connector property. See e.g. the TearFree
> output property in xf86-video-amdgpu.

The new configuration property for the DDX side of things does pretty 
much that.

> 
> 
>>> I also believe that it would be useful to expose vmin/vmax to userspace
>>> as properties, so that display servers and apps can plan ahead on when
>>> they will render. I suppose that can be left for later when someone
>>> starts working on userspace taking advantage of it, so that the proper
>>> userspace requirement for new UABI is fulfilled.
>>
>> Knowing the vmin/vmax could potentially be useful for testing but most
>> applications shouldn't be trying to predict or adjust rendering based on
>> these.
> 
> FWIW, recent research indicates that this is necessary for perfectly
> smooth animation without micro-stuttering, even with VRR.
> 
> 

This was brought up in previous RFCs about this but I'm not really 
convinced by the research methodology and the results from those threads.

Targeting the max vrefresh of the display (which is known without 
additional properties) should be best for prediction since anything 
lower would be inherently less smooth by definition.

Nicholas Kazlauskas
On 2018-10-05 7:48 p.m., Kazlauskas, Nicholas wrote:
> On 10/05/2018 12:56 PM, Michel Dänzer wrote:
>> On 2018-10-05 6:21 p.m., Kazlauskas, Nicholas wrote:
>>> On 10/05/2018 04:10 AM, Pekka Paalanen wrote:
>>>>
>>>> I also believe that it would be useful to expose vmin/vmax to userspace
>>>> as properties, so that display servers and apps can plan ahead on when
>>>> they will render. I suppose that can be left for later when someone
>>>> starts working on userspace taking advantage of it, so that the proper
>>>> userspace requirement for new UABI is fulfilled.
>>>
>>> Knowing the vmin/vmax could potentially be useful for testing but most
>>> applications shouldn't be trying to predict or adjust rendering based on
>>> these.
>>
>> FWIW, recent research indicates that this is necessary for perfectly
>> smooth animation without micro-stuttering, even with VRR.
> 
> This was brought up in previous RFCs about this but I'm not really
> convinced by the research methodology and the results from those threads.
> 
> Targeting the max vrefresh of the display (which is known without
> additional properties) should be best for prediction since anything
> lower would be inherently less smooth by definition.

Smooth animation isn't only about frame-rate, it's also about the timing
of each frame's presentation and its contents being consistent with each
other[0]. To ensure this, the application has to know the constraints
for when the next frame can be presented, and it has to be able to
control when the frame is presented.


[0] One example of this is https://www.youtube.com/watch?v=V4fgYS38aQg .
When this video is played back at 60 fps, the upper sphere moves
smoothly, but the lower sphere sometimes jumps back and forth slightly
(it's pretty subtle, might take a while to notice), because its position
isn't always consistent with the frame's presentation timing.
On Fri, 5 Oct 2018 12:21:20 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:

> On 10/05/2018 04:10 AM, Pekka Paalanen wrote:
> > Hi,
> > 
> > I have a somewhat of my own view on what would be involved with VRR,
> > and I'd like to hear what you think of it. Comments inline.
> > 
> > 
> > On Tue, 25 Sep 2018 09:51:37 -0400
> > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> >   
> >> On 09/24/2018 04:26 PM, Ville Syrjälä wrote:  
> >>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:  
> >>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:  
> >>>>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote:  
> >>>>>> Variable refresh rate algorithms have typically been enabled only
> >>>>>> when the display is covered by a single source of content.
> >>>>>>
> >>>>>> This patch introduces a new default CRTC property that helps
> >>>>>> hint to the driver when the CRTC composition is suitable for variable
> >>>>>> refresh rate algorithms. Userspace can set this property dynamically
> >>>>>> as the composition changes.
> >>>>>>
> >>>>>> Whether the variable refresh rate algorithms are active will still
> >>>>>> depend on the CRTC being suitable and the connector being capable
> >>>>>> and enabled by the user for variable refresh rate support.
> >>>>>>
> >>>>>> It is worth noting that while the property is atomic it isn't filtered
> >>>>>> from legacy userspace queries. This allows for Xorg userspace drivers
> >>>>>> to implement support in non-atomic setups.
> >>>>>>
> >>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>  

...

> >> Whenever I mentioned variable refresh "features", what I really meant
> >> was operating in one of two modes:
> >>
> >> (1) Letting the driver and hardware adjust refresh automatically based
> >> on the flip rate on a CRTC from a single application
> >>
> >> (2) Setting a fixed frame duration based on the flip rate on a CRTC from
> >> a single application  
> > 
> > I wonder if that's too much magic in the kernel... what would be wrong
> > with simply flipping ASAP when VRR is active?
> > 
> > How will userspace be able to predict coming flip opportunities if the
> > kernel does so much magic?  
> 
> The kernel driver doesn't need to do much more than let the hardware 
> know the variable refresh range. The "magic" is performed by hardware.
> 
> Most games would like to render as fast as possible to deliver a more 
> responsive and smoother image to the user. Many of these are also 
> resource intensive and won't always be able to render at the fixed 
> refresh rate of the panel (especially for higher refresh rates like 
> 144Hz). The user will experience stuttering if the game takes too long 
> to render and misses the vblank window for the flip.
> 
> Dynamic VRR adjustment can resolve this problem. The hardware can lower 
> the refresh rate and increase the vblank window in response to this so 
> the user doesn't experience stuttering (or latency).
> 
> Userspace shouldn't predict anything.

...

> >>>> The reasoning for the split is because not all content is suitable for
> >>>> variable refresh. Desktop environments, web browsers, etc only typically
> >>>> flip when needed - which will result in display flickering.  
> > 
> > Flickering? What do you mean?  
> 
> This is a property of how panels work.
> 
> The luminance for a panel will vary based on how long the vrefresh is. 
> Since the vrefresh length is changing as part of VRR you're more likely 
> to notice the difference in luminance the bigger the difference is.
> 
> The difference will be largest when switching from the min vrefresh to 
> the max vrefresh duration.
> 
> Large differences can occur for applications that render on demand like 
> a web browser (and why you wouldn't want VRR enabled for those). The 
> hardware would continuously wait for a flip that isn't coming. Then if 
> the user moves their cursor or the page updates it's going to happen 
> "randomly" in that window and the hardware will adjust to that.

Hi Nicholas,

it seems I have very much mis-guessed what VRR aims to achieve, and the
effect on luminance sounds horrible.

People have worked for years to make display timings more explicit,
giving better control and predictability over them. It sounds like VRR
is not an improvement that allows new smarter software to take control
of timings even better. Instead, VRR seems to be a step backwards,
introducing more uncertainty into the timings. The expectation of a
fixed unknown refresh rate must be built into software for the software
to work reasonably while VRR is active.

From your comments I understood that the VRR hardware still very much
depends on a consistent refresh rate, except the hardware (not the
software!) can additionally slew the refresh rate over time. Abrupt
changes in frame timings must still be prevented, but I wasn't quite
sure if you meant the hardware will do that or if the software must do
that, since you are worried about on-demand updating applications
causing flickering.

Hence, VRR looks like a band-aid for old and simple applications that
use brute force (more fps, maximize the work load) in an attempt to
make things smoother and to reduce latency. There are lots of such
applications, so VRR has its place. It is my mistake of assuming it
could do more. Yes, I am disappointed to realize this.

I believe that future display servers and applications that actually
care about display timings will just opt out from VRR to gain better
predictability.

I also believe that you will need to blacklist applications like video
players whose developers have spent a great deal of effort in making a
smart scheduling algorithm for fixed rate display. I assume that a
smart sheduling algorithm that is based on prediciting the next display
time instant will interact poorly with VRR. A video player will need to
know if it is getting fixed rate or VRR to choose the appropriate
timing algorithm - wherther it needs to adapt to the display rate or
will the display rate adapt to it.

In summary, VRR is only good for exactly the use cases you listed, and
for other uses it can be harmful, just like you said.

Given the above, I think we are now very much on the same page about
what the KMS UABI should look like.


> Most compositors function well under this stack. It will vary
> depending on compositor support for window unredirection to let the
> window flip via the Present extension. Mutter handles this without
> any configuration and kwin can be configured to work with these
> patches. Compton can be configured to support unredirection as well.

You are thinking about the applications. What about the compositor
itself? Is the COW not hitting the Present direct scanout path,
triggering VRR, when what is actually showing is the desktop with
on-demand randomly updating windows?

> These (among others) are covered as part of the blacklist for the
> mesa patches. They do need to be explicitly excluded.

Right, you blacklist all compositing managers to prevent them from
regressing. That feels a bit ugly to me, needing exceptions to not
regress things, but maybe that's business as usual in Mesa?

Fortunately that problem with compositing managers won't exist with
Wayland.

In any case, there must also be a way for an application to explicitly
say if it supports VRR or not and to know if it gets VRR or not. Are
there no EGL or Vulkan extensions for that?


Thanks,
pq
On 10/10/2018 03:14 AM, Pekka Paalanen wrote:
> On Fri, 5 Oct 2018 12:21:20 -0400
> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> 
>> On 10/05/2018 04:10 AM, Pekka Paalanen wrote:
>>> Hi,
>>>
>>> I have a somewhat of my own view on what would be involved with VRR,
>>> and I'd like to hear what you think of it. Comments inline.
>>>
>>>
>>> On Tue, 25 Sep 2018 09:51:37 -0400
>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
>>>    
>>>> On 09/24/2018 04:26 PM, Ville Syrjälä wrote:
>>>>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
>>>>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
>>>>>>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote:
>>>>>>>> Variable refresh rate algorithms have typically been enabled only
>>>>>>>> when the display is covered by a single source of content.
>>>>>>>>
>>>>>>>> This patch introduces a new default CRTC property that helps
>>>>>>>> hint to the driver when the CRTC composition is suitable for variable
>>>>>>>> refresh rate algorithms. Userspace can set this property dynamically
>>>>>>>> as the composition changes.
>>>>>>>>
>>>>>>>> Whether the variable refresh rate algorithms are active will still
>>>>>>>> depend on the CRTC being suitable and the connector being capable
>>>>>>>> and enabled by the user for variable refresh rate support.
>>>>>>>>
>>>>>>>> It is worth noting that while the property is atomic it isn't filtered
>>>>>>>> from legacy userspace queries. This allows for Xorg userspace drivers
>>>>>>>> to implement support in non-atomic setups.
>>>>>>>>
>>>>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> 
> ...
> 
>>>> Whenever I mentioned variable refresh "features", what I really meant
>>>> was operating in one of two modes:
>>>>
>>>> (1) Letting the driver and hardware adjust refresh automatically based
>>>> on the flip rate on a CRTC from a single application
>>>>
>>>> (2) Setting a fixed frame duration based on the flip rate on a CRTC from
>>>> a single application
>>>
>>> I wonder if that's too much magic in the kernel... what would be wrong
>>> with simply flipping ASAP when VRR is active?
>>>
>>> How will userspace be able to predict coming flip opportunities if the
>>> kernel does so much magic?
>>
>> The kernel driver doesn't need to do much more than let the hardware
>> know the variable refresh range. The "magic" is performed by hardware.
>>
>> Most games would like to render as fast as possible to deliver a more
>> responsive and smoother image to the user. Many of these are also
>> resource intensive and won't always be able to render at the fixed
>> refresh rate of the panel (especially for higher refresh rates like
>> 144Hz). The user will experience stuttering if the game takes too long
>> to render and misses the vblank window for the flip.
>>
>> Dynamic VRR adjustment can resolve this problem. The hardware can lower
>> the refresh rate and increase the vblank window in response to this so
>> the user doesn't experience stuttering (or latency).
>>
>> Userspace shouldn't predict anything.
> 
> ...
> 
>>>>>> The reasoning for the split is because not all content is suitable for
>>>>>> variable refresh. Desktop environments, web browsers, etc only typically
>>>>>> flip when needed - which will result in display flickering.
>>>
>>> Flickering? What do you mean?
>>
>> This is a property of how panels work.
>>
>> The luminance for a panel will vary based on how long the vrefresh is.
>> Since the vrefresh length is changing as part of VRR you're more likely
>> to notice the difference in luminance the bigger the difference is.
>>
>> The difference will be largest when switching from the min vrefresh to
>> the max vrefresh duration.
>>
>> Large differences can occur for applications that render on demand like
>> a web browser (and why you wouldn't want VRR enabled for those). The
>> hardware would continuously wait for a flip that isn't coming. Then if
>> the user moves their cursor or the page updates it's going to happen
>> "randomly" in that window and the hardware will adjust to that.
> 
> Hi Nicholas,
> 
> it seems I have very much mis-guessed what VRR aims to achieve, and the
> effect on luminance sounds horrible.

It really depends on the panel characteristics - like the VRR range and 
panel brightness. Some panels can be particularly unpleasant but others 
are fine.

> 
> People have worked for years to make display timings more explicit,
> giving better control and predictability over them. It sounds like VRR
> is not an improvement that allows new smarter software to take control
> of timings even better. Instead, VRR seems to be a step backwards,
> introducing more uncertainty into the timings. The expectation of a
> fixed unknown refresh rate must be built into software for the software
> to work reasonably while VRR is active. >
>  From your comments I understood that the VRR hardware still very much
> depends on a consistent refresh rate, except the hardware (not the
> software!) can additionally slew the refresh rate over time. Abrupt
> changes in frame timings must still be prevented, but I wasn't quite
> sure if you meant the hardware will do that or if the software must do
> that, since you are worried about on-demand updating applications
> causing flickering.
> 
> Hence, VRR looks like a band-aid for old and simple applications that
> use brute force (more fps, maximize the work load) in an attempt to
> make things smoother and to reduce latency. There are lots of such
> applications, so VRR has its place. It is my mistake of assuming it
> could do more. Yes, I am disappointed to realize this.
> 
> I believe that future display servers and applications that actually
> care about display timings will just opt out from VRR to gain better
> predictability.
> 
> I also believe that you will need to blacklist applications like video
> players whose developers have spent a great deal of effort in making a
> smart scheduling algorithm for fixed rate display. I assume that a
> smart sheduling algorithm that is based on prediciting the next display
> time instant will interact poorly with VRR. A video player will need to
> know if it is getting fixed rate or VRR to choose the appropriate
> timing algorithm - wherther it needs to adapt to the display rate or
> will the display rate adapt to it.
> 
> In summary, VRR is only good for exactly the use cases you listed, and
> for other uses it can be harmful, just like you said.
> 
> Given the above, I think we are now very much on the same page about
> what the KMS UABI should look like.


The use-case for the dynamic adjustment mode is largely games, yes. But 
I don't think that's something to be entirely dismissive of.

The old and simple applications are the ones that would benefit the 
least from this because they probably run fairly well on new hardware.

Modern games benefit the most from this since they have high performance 
requirements. There's also a fair amount of these titles coming out in 
the last few years largely because of the cross-platform deployment 
capabilities in popular game engines and frameworks.

While I don't have actual numbers, I think that most modern games that 
come out on Linux are scalable when it comes to refresh rate. VRR panels 
with a "decent" range will see nearly all stuttering gone in practice 
with no negative impact to the experience at all. This also benefits 
games that render lower than the monitor's refresh all the time, like 
45Hz on a VRR monitor with a 40-60Hz range.

The dynamic adjustment also works as expected with implicit vsync on/off 
via GLX swap control. Lightweight games don't need to burn a ton of 
power if they're capable of rendering above the refresh rate of the 
monitor. Vsync on will still cap you at the mode's refresh as expected 
(like 60Hz).

The patches I've put out target this use case mostly because of the 
benefit for a relatively simple interface. VRR can and has been used in 
more ways that this, however.

An example usecase that differs from this would actually be video 
playback. The monitor's refresh rate likely doesn't align with the video 
content rate. An API that exposes direct control over VRR (like the 
range, refresh duration, presentation timestamp) could allow the 
application to specify the content rate directly to deliver a smoother 
playback experience. For example, if you had a 24fps video and the VRR 
range was 40-60Hz you could target 48Hz via some API here.

> 
> 
>> Most compositors function well under this stack. It will vary
>> depending on compositor support for window unredirection to let the
>> window flip via the Present extension. Mutter handles this without
>> any configuration and kwin can be configured to work with these
>> patches. Compton can be configured to support unredirection as well.
> 
> You are thinking about the applications. What about the compositor
> itself? Is the COW not hitting the Present direct scanout path,
> triggering VRR, when what is actually showing is the desktop with
> on-demand randomly updating windows?
> 
>> These (among others) are covered as part of the blacklist for the
>> mesa patches. They do need to be explicitly excluded.
> 
> Right, you blacklist all compositing managers to prevent them from
> regressing. That feels a bit ugly to me, needing exceptions to not
> regress things, but maybe that's business as usual in Mesa?
> 
> Fortunately that problem with compositing managers won't exist with
> Wayland.

Compositors do bring out the worst when it comes to flickering for VRR 
with the dynamic mode. A more explicit API may help here in the future.

I think the blacklist approach is still the best when compared with the 
alternatives. At least when it comes to supporting the vast majority of 
existing applications without any explicit configuration from the user 
or developers.

I would imagine you're right about a Wayland implementation being 
cleaner when dealing with some of these issues.

> 
> In any case, there must also be a way for an application to explicitly
> say if it supports VRR or not and to know if it gets VRR or not. Are
> there no EGL or Vulkan extensions for that?

I think this is an interesting idea given what's already out there in 
terms of swap and present control. There was some discussion about this 
and the blacklist on the mesa mailing list.

For the current implementation the easiest way to override the blacklist 
is via including an explicit driconf file for the application. A 
semi-recent change in mesa lets applications do this without overriding 
the global or user conf.

> 
> 
> Thanks,
> pq
> 

Nicholas Kazlauskas
On Wed, 10 Oct 2018 09:35:50 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:

> The patches I've put out target this use case mostly because of the 
> benefit for a relatively simple interface. VRR can and has been used in 
> more ways that this, however.
> 
> An example usecase that differs from this would actually be video 
> playback. The monitor's refresh rate likely doesn't align with the video 
> content rate. An API that exposes direct control over VRR (like the 
> range, refresh duration, presentation timestamp) could allow the 
> application to specify the content rate directly to deliver a smoother 
> playback experience. For example, if you had a 24fps video and the VRR 
> range was 40-60Hz you could target 48Hz via some API here.

The way that has been discussed to be implemented is that DRM page flips
would carry a target timestamp, which the driver will then meet as good
as it can. It is better to define an absolute target timestamp than a
frequency, because a timestamp can be used to synchronize with audio
and more. Mario Kleiner can tell you all about scientific use cases
that require accurate display timing, not just frequency.

A timestamp will naturally lend itself to arbitrary on-demand screen
updates as well.

However, userspace still needs to know if the target timestamp it is
contemplating on could realistically be met, so that e.g. a video
player can choose between showing video frames as is vs. needing to
interpolate for the presentation times it actually can achieve.

I suppose we agree on the above.

Btw. would a video player even need explicit controls if it knows the
display will adapt to the player's refresh rate? It could just use the
original video rate and from what I understood, the display will soon
end up refreshing at exactly that rate. The player can still use the
realized page flip timestamps to synchronize with audio, since it can
assume the refresh rate is stable and can predict the next few flips.
But, this would only work if the video player knows that VRR will adapt
to its rate.

While we are talking about predictability of page flips, Weston is
already relying on a prediction to reduce the desktop latency to
screen. However, it should be possible to modify Weston to implement
the kind of VRR support for fullscreen exclusive windows like you are
proposing just fine.

Just like the X11 window property you mentioned for marking windows as
eligible for VRR in Xorg, Wayland will need a similar protocol
extension.

I'm happy to see work being done for VRR.


Thanks,
pq
Am 12.10.2018 um 09:23 schrieb Pekka Paalanen:
> On Wed, 10 Oct 2018 09:35:50 -0400
> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
>
>> The patches I've put out target this use case mostly because of the
>> benefit for a relatively simple interface. VRR can and has been used in
>> more ways that this, however.
>>
>> An example usecase that differs from this would actually be video
>> playback. The monitor's refresh rate likely doesn't align with the video
>> content rate. An API that exposes direct control over VRR (like the
>> range, refresh duration, presentation timestamp) could allow the
>> application to specify the content rate directly to deliver a smoother
>> playback experience. For example, if you had a 24fps video and the VRR
>> range was 40-60Hz you could target 48Hz via some API here.
> The way that has been discussed to be implemented is that DRM page flips
> would carry a target timestamp, which the driver will then meet as good
> as it can. It is better to define an absolute target timestamp than a
> frequency, because a timestamp can be used to synchronize with audio
> and more. Mario Kleiner can tell you all about scientific use cases
> that require accurate display timing, not just frequency.

To summarize what information should be provided by the driver stack to 
make applications happy:

1. The minimum time a frame can be displayed, in other words the maximum 
frame rate.
2. The maximum time a frame can be displayed, in other words the minimum 
frame rate.
3. How much change of frame timing is allowed between frames to avoid 
luminescence flickering.

Number 1 and 2 can also be used to signal the availability of VRR to 
applications, e.g. if they are identical we don't support VRR at all.

Regards,
Christian.

>
> A timestamp will naturally lend itself to arbitrary on-demand screen
> updates as well.
>
> However, userspace still needs to know if the target timestamp it is
> contemplating on could realistically be met, so that e.g. a video
> player can choose between showing video frames as is vs. needing to
> interpolate for the presentation times it actually can achieve.
>
> I suppose we agree on the above.
>
> Btw. would a video player even need explicit controls if it knows the
> display will adapt to the player's refresh rate? It could just use the
> original video rate and from what I understood, the display will soon
> end up refreshing at exactly that rate. The player can still use the
> realized page flip timestamps to synchronize with audio, since it can
> assume the refresh rate is stable and can predict the next few flips.
> But, this would only work if the video player knows that VRR will adapt
> to its rate.
>
> While we are talking about predictability of page flips, Weston is
> already relying on a prediction to reduce the desktop latency to
> screen. However, it should be possible to modify Weston to implement
> the kind of VRR support for fullscreen exclusive windows like you are
> proposing just fine.
>
> Just like the X11 window property you mentioned for marking windows as
> eligible for VRR in Xorg, Wayland will need a similar protocol
> extension.
>
> I'm happy to see work being done for VRR.
>
>
> Thanks,
> pq
On Fri, 12 Oct 2018 07:35:57 +0000
"Koenig, Christian" <Christian.Koenig@amd.com> wrote:

> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen:
> > On Wed, 10 Oct 2018 09:35:50 -0400
> > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> >  
> >> The patches I've put out target this use case mostly because of the
> >> benefit for a relatively simple interface. VRR can and has been used in
> >> more ways that this, however.
> >>
> >> An example usecase that differs from this would actually be video
> >> playback. The monitor's refresh rate likely doesn't align with the video
> >> content rate. An API that exposes direct control over VRR (like the
> >> range, refresh duration, presentation timestamp) could allow the
> >> application to specify the content rate directly to deliver a smoother
> >> playback experience. For example, if you had a 24fps video and the VRR
> >> range was 40-60Hz you could target 48Hz via some API here.  
> > The way that has been discussed to be implemented is that DRM page flips
> > would carry a target timestamp, which the driver will then meet as good
> > as it can. It is better to define an absolute target timestamp than a
> > frequency, because a timestamp can be used to synchronize with audio
> > and more. Mario Kleiner can tell you all about scientific use cases
> > that require accurate display timing, not just frequency.  
> 
> To summarize what information should be provided by the driver stack to 
> make applications happy:
> 
> 1. The minimum time a frame can be displayed, in other words the maximum 
> frame rate.
> 2. The maximum time a frame can be displayed, in other words the minimum 
> frame rate.
> 3. How much change of frame timing is allowed between frames to avoid 
> luminescence flickering.
> 
> Number 1 and 2 can also be used to signal the availability of VRR to 
> applications, e.g. if they are identical we don't support VRR at all.

Hi Christian,

"the maximum time a frame can be displayed" is perhaps not an
unambiguous definition. A frame can be shown indefinitely in any case.
The CRTC will simply start scanning out the same frame again if there
is no new one. I understand what you want to say, but perhaps some
different words will be in order.

I do wonder if applications really want to know the maximum acceptable
slew rate in timings... maybe that should be left for the drivers to
apply. What I'm thinking is that we have the page flip timestamp with
the page flip events to tell when the new FB became active. That
information could be extended with a time range on when the very next
flip could take place. Applications are already computing that
prediction from the flip timestamp and fixed refresh rate, but it might
be nice to give them the driver's opinion explicitly. Maybe the
tolerable slew rate is not a constant.

Other than that, yes, it sounds fine to me.


Thanks,
pq
Am 12.10.2018 um 11:21 schrieb Pekka Paalanen:
> On Fri, 12 Oct 2018 07:35:57 +0000
> "Koenig, Christian" <Christian.Koenig@amd.com> wrote:
>
>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen:
>>> On Wed, 10 Oct 2018 09:35:50 -0400
>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
>>>   
>>>> The patches I've put out target this use case mostly because of the
>>>> benefit for a relatively simple interface. VRR can and has been used in
>>>> more ways that this, however.
>>>>
>>>> An example usecase that differs from this would actually be video
>>>> playback. The monitor's refresh rate likely doesn't align with the video
>>>> content rate. An API that exposes direct control over VRR (like the
>>>> range, refresh duration, presentation timestamp) could allow the
>>>> application to specify the content rate directly to deliver a smoother
>>>> playback experience. For example, if you had a 24fps video and the VRR
>>>> range was 40-60Hz you could target 48Hz via some API here.
>>> The way that has been discussed to be implemented is that DRM page flips
>>> would carry a target timestamp, which the driver will then meet as good
>>> as it can. It is better to define an absolute target timestamp than a
>>> frequency, because a timestamp can be used to synchronize with audio
>>> and more. Mario Kleiner can tell you all about scientific use cases
>>> that require accurate display timing, not just frequency.
>> To summarize what information should be provided by the driver stack to
>> make applications happy:
>>
>> 1. The minimum time a frame can be displayed, in other words the maximum
>> frame rate.
>> 2. The maximum time a frame can be displayed, in other words the minimum
>> frame rate.
>> 3. How much change of frame timing is allowed between frames to avoid
>> luminescence flickering.
>>
>> Number 1 and 2 can also be used to signal the availability of VRR to
>> applications, e.g. if they are identical we don't support VRR at all.
> Hi Christian,
>
> "the maximum time a frame can be displayed" is perhaps not an
> unambiguous definition. A frame can be shown indefinitely in any case.

Good point. Please also note that I'm not an expert on the display stuff 
in general.

My background comes more from implementing the VDPAU mediaplayer 
interface in mesa.

So just throwing some ideas in here from the point of view of an 
userspace developer which wants to write a media player :)

> The CRTC will simply start scanning out the same frame again if there
> is no new one. I understand what you want to say, but perhaps some
> different words will be in order.
>
> I do wonder if applications really want to know the maximum acceptable
> slew rate in timings... maybe that should be left for the drivers to
> apply. What I'm thinking is that we have the page flip timestamp with
> the page flip events to tell when the new FB became active. That
> information could be extended with a time range on when the very next
> flip could take place. Applications are already computing that
> prediction from the flip timestamp and fixed refresh rate, but it might
> be nice to give them the driver's opinion explicitly. Maybe the
> tolerable slew rate is not a constant.

Well it depends. I agree that the kernel should probably enforce the 
slew rate to avoid flickering for the user.

But it might be beneficial for the application to know things like this.

What the application definitely needs to know is when a frame was 
actually displayed. E.g. application says I want to display this at 
point X and at some point later the kernel returns saying the frame was 
displayed at point N where in the ideal case N=X.

Additional to that let's please use 64bit values and nanoseconds for 
every value we use here, and NOT fps, line numbers or similar :)

Regards,
Christian.

>
> Other than that, yes, it sounds fine to me.
>
>
> Thanks,
> pq
On 10/12/2018 07:20 AM, Koenig, Christian wrote:
> Am 12.10.2018 um 11:21 schrieb Pekka Paalanen:
>> On Fri, 12 Oct 2018 07:35:57 +0000
>> "Koenig, Christian" <Christian.Koenig@amd.com> wrote:
>>
>>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen:
>>>> On Wed, 10 Oct 2018 09:35:50 -0400
>>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
>>>>    
>>>>> The patches I've put out target this use case mostly because of the
>>>>> benefit for a relatively simple interface. VRR can and has been used in
>>>>> more ways that this, however.
>>>>>
>>>>> An example usecase that differs from this would actually be video
>>>>> playback. The monitor's refresh rate likely doesn't align with the video
>>>>> content rate. An API that exposes direct control over VRR (like the
>>>>> range, refresh duration, presentation timestamp) could allow the
>>>>> application to specify the content rate directly to deliver a smoother
>>>>> playback experience. For example, if you had a 24fps video and the VRR
>>>>> range was 40-60Hz you could target 48Hz via some API here.
>>>> The way that has been discussed to be implemented is that DRM page flips
>>>> would carry a target timestamp, which the driver will then meet as good
>>>> as it can. It is better to define an absolute target timestamp than a
>>>> frequency, because a timestamp can be used to synchronize with audio
>>>> and more. Mario Kleiner can tell you all about scientific use cases
>>>> that require accurate display timing, not just frequency.
>>> To summarize what information should be provided by the driver stack to
>>> make applications happy:
>>>
>>> 1. The minimum time a frame can be displayed, in other words the maximum
>>> frame rate.
>>> 2. The maximum time a frame can be displayed, in other words the minimum
>>> frame rate.
>>> 3. How much change of frame timing is allowed between frames to avoid
>>> luminescence flickering.
>>>
>>> Number 1 and 2 can also be used to signal the availability of VRR to
>>> applications, e.g. if they are identical we don't support VRR at all.
>> Hi Christian,
>>
>> "the maximum time a frame can be displayed" is perhaps not an
>> unambiguous definition. A frame can be shown indefinitely in any case.
> 
> Good point. Please also note that I'm not an expert on the display stuff
> in general.
> 
> My background comes more from implementing the VDPAU mediaplayer
> interface in mesa.
> 
> So just throwing some ideas in here from the point of view of an
> userspace developer which wants to write a media player :)
> 
>> The CRTC will simply start scanning out the same frame again if there
>> is no new one. I understand what you want to say, but perhaps some
>> different words will be in order.
>>
>> I do wonder if applications really want to know the maximum acceptable
>> slew rate in timings... maybe that should be left for the drivers to
>> apply. What I'm thinking is that we have the page flip timestamp with
>> the page flip events to tell when the new FB became active. That
>> information could be extended with a time range on when the very next
>> flip could take place. Applications are already computing that
>> prediction from the flip timestamp and fixed refresh rate, but it might
>> be nice to give them the driver's opinion explicitly. Maybe the
>> tolerable slew rate is not a constant.
> 
> Well it depends. I agree that the kernel should probably enforce the
> slew rate to avoid flickering for the user.
> 
> But it might be beneficial for the application to know things like this.
> 
> What the application definitely needs to know is when a frame was
> actually displayed. E.g. application says I want to display this at
> point X and at some point later the kernel returns saying the frame was
> displayed at point N where in the ideal case N=X.
> 
> Additional to that let's please use 64bit values and nanoseconds for
> every value we use here, and NOT fps, line numbers or similar :)
> 
> Regards,
> Christian.

Flickering will really depend on the panel itself. A wider ranger is
more likely to exhibit the issue, but factors like topology, pixel
density and other display technology can affect this.

It can be hard for a driver to guess at all of this. For many panels
it'd be difficult to notice unless it's consistently jumping between the
min and max range.

Opening up an API that restricts how much the driver can change the
refresh rate in a VRR scenario seems a bit extreme, but there's probably
some applications that could benefit from this. I'd certainly want to
see the actual use case first, though. This still feels more like a
driver policy to me.

I agree with the nanosecond based timestamp API making the most logical
sense here from an API perspective. This does overlap a little bit with
the target vblank property that's already on the CRTC, perhaps.

The target vblank could be determined based on the timestamp. The driver
is likely to exceed the target presentation timestamp by a fair bit if
it was to just do this, however. VRR could be used in this case to get
closer to the timestamp. A naive implementation could iterate over every
rate in the range and take the one with the minimum difference, for
example.

> 
>>
>> Other than that, yes, it sounds fine to me.
>>
>>
>> Thanks,
>> pq
> 

Nicholas Kazlauskas
On Fri, 12 Oct 2018 08:58:23 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:

> On 10/12/2018 07:20 AM, Koenig, Christian wrote:
> > Am 12.10.2018 um 11:21 schrieb Pekka Paalanen:  
> >> On Fri, 12 Oct 2018 07:35:57 +0000
> >> "Koenig, Christian" <Christian.Koenig@amd.com> wrote:
> >>  
> >>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen:  
> >>>> On Wed, 10 Oct 2018 09:35:50 -0400
> >>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> >>>>      
> >>>>> The patches I've put out target this use case mostly because of the
> >>>>> benefit for a relatively simple interface. VRR can and has been used in
> >>>>> more ways that this, however.
> >>>>>
> >>>>> An example usecase that differs from this would actually be video
> >>>>> playback. The monitor's refresh rate likely doesn't align with the video
> >>>>> content rate. An API that exposes direct control over VRR (like the
> >>>>> range, refresh duration, presentation timestamp) could allow the
> >>>>> application to specify the content rate directly to deliver a smoother
> >>>>> playback experience. For example, if you had a 24fps video and the VRR
> >>>>> range was 40-60Hz you could target 48Hz via some API here.  
> >>>> The way that has been discussed to be implemented is that DRM page flips
> >>>> would carry a target timestamp, which the driver will then meet as good
> >>>> as it can. It is better to define an absolute target timestamp than a
> >>>> frequency, because a timestamp can be used to synchronize with audio
> >>>> and more. Mario Kleiner can tell you all about scientific use cases
> >>>> that require accurate display timing, not just frequency.  
> >>> To summarize what information should be provided by the driver stack to
> >>> make applications happy:
> >>>
> >>> 1. The minimum time a frame can be displayed, in other words the maximum
> >>> frame rate.
> >>> 2. The maximum time a frame can be displayed, in other words the minimum
> >>> frame rate.
> >>> 3. How much change of frame timing is allowed between frames to avoid
> >>> luminescence flickering.
> >>>
> >>> Number 1 and 2 can also be used to signal the availability of VRR to
> >>> applications, e.g. if they are identical we don't support VRR at all.  
> >> Hi Christian,
> >>
> >> "the maximum time a frame can be displayed" is perhaps not an
> >> unambiguous definition. A frame can be shown indefinitely in any case.  
> > 
> > Good point. Please also note that I'm not an expert on the display stuff
> > in general.
> > 
> > My background comes more from implementing the VDPAU mediaplayer
> > interface in mesa.
> > 
> > So just throwing some ideas in here from the point of view of an
> > userspace developer which wants to write a media player :)

Excellent!

> >> The CRTC will simply start scanning out the same frame again if there
> >> is no new one. I understand what you want to say, but perhaps some
> >> different words will be in order.
> >>
> >> I do wonder if applications really want to know the maximum acceptable
> >> slew rate in timings... maybe that should be left for the drivers to
> >> apply. What I'm thinking is that we have the page flip timestamp with
> >> the page flip events to tell when the new FB became active. That
> >> information could be extended with a time range on when the very next
> >> flip could take place. Applications are already computing that
> >> prediction from the flip timestamp and fixed refresh rate, but it might
> >> be nice to give them the driver's opinion explicitly. Maybe the
> >> tolerable slew rate is not a constant.  
> > 
> > Well it depends. I agree that the kernel should probably enforce the
> > slew rate to avoid flickering for the user.

That's what I was hoping if the monitor hardware does not do that
already, but now it sounds like it's not possible. Another failed
assumption from my side.

> > But it might be beneficial for the application to know things like this.

Applications should know when they could likely flip, my question is
how to tell them. Is the acceptable slew rate a constant for a video
mode, or does it depend on the previous refresh interval.

> > 
> > What the application definitely needs to know is when a frame was
> > actually displayed. E.g. application says I want to display this at
> > point X and at some point later the kernel returns saying the frame was
> > displayed at point N where in the ideal case N=X.

You already get this timestamp with the DRM page flip events. We also
have Wayland and X11 extensions to get it to apps.

> > 
> > Additional to that let's please use 64bit values and nanoseconds for
> > every value we use here, and NOT fps, line numbers or similar :)

Seconded!

I'd really favour absolute timestamps even.

> > 
> > Regards,
> > Christian.  
> 
> Flickering will really depend on the panel itself. A wider ranger is
> more likely to exhibit the issue, but factors like topology, pixel
> density and other display technology can affect this.
> 
> It can be hard for a driver to guess at all of this. For many panels
> it'd be difficult to notice unless it's consistently jumping between the
> min and max range.

Do you mean that there is no way to know and get that information from
the monitor itself? Nothing in EDID that could even be used as a
default and quirk the monitors that got it wrong?

> Opening up an API that restricts how much the driver can change the
> refresh rate in a VRR scenario seems a bit extreme, but there's probably
> some applications that could benefit from this. I'd certainly want to
> see the actual use case first, though. This still feels more like a
> driver policy to me.

I don't think anyone suggested that, certainly I did not. The driver
should get that information from the monitor hardware so that it can
drive it correctly. If the hardware driver doesn't know, then how could
the DRM core or userspace know any better? My whole premise was that the
driver knows.

Sounds like VRR hardware was designed not only for a consistent refresh
rate but also temporary glitches being ok.

I suppose this will result in choosing a very pessimistic allowed slew
rate in the driver that covers 95% of VRR monitors and handle the rest
with quirks. That could still work.

> I agree with the nanosecond based timestamp API making the most logical
> sense here from an API perspective. This does overlap a little bit with
> the target vblank property that's already on the CRTC, perhaps.

KMS UABI already has a target vblank property defined, or are you
talking about your CRTC hardware?

Target vblank counter value makes even less sense with VRR than it ever
did with a fixed refresh rate. :-)

> The target vblank could be determined based on the timestamp. The driver
> is likely to exceed the target presentation timestamp by a fair bit if
> it was to just do this, however. VRR could be used in this case to get
> closer to the timestamp. A naive implementation could iterate over every
> rate in the range and take the one with the minimum difference, for
> example.

Could you elaborate on that, who could be doing what exactly to achieve
what?


Thanks,
pq
On 10/15/2018 09:57 AM, Pekka Paalanen wrote:
> On Fri, 12 Oct 2018 08:58:23 -0400
> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> 
>> On 10/12/2018 07:20 AM, Koenig, Christian wrote:
>>> Am 12.10.2018 um 11:21 schrieb Pekka Paalanen:
>>>> On Fri, 12 Oct 2018 07:35:57 +0000
>>>> "Koenig, Christian" <Christian.Koenig@amd.com> wrote:
>>>>   
>>>>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen:
>>>>>> On Wed, 10 Oct 2018 09:35:50 -0400
>>>>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
>>>>>>       
>>>>>>> The patches I've put out target this use case mostly because of the
>>>>>>> benefit for a relatively simple interface. VRR can and has been used in
>>>>>>> more ways that this, however.
>>>>>>>
>>>>>>> An example usecase that differs from this would actually be video
>>>>>>> playback. The monitor's refresh rate likely doesn't align with the video
>>>>>>> content rate. An API that exposes direct control over VRR (like the
>>>>>>> range, refresh duration, presentation timestamp) could allow the
>>>>>>> application to specify the content rate directly to deliver a smoother
>>>>>>> playback experience. For example, if you had a 24fps video and the VRR
>>>>>>> range was 40-60Hz you could target 48Hz via some API here.
>>>>>> The way that has been discussed to be implemented is that DRM page flips
>>>>>> would carry a target timestamp, which the driver will then meet as good
>>>>>> as it can. It is better to define an absolute target timestamp than a
>>>>>> frequency, because a timestamp can be used to synchronize with audio
>>>>>> and more. Mario Kleiner can tell you all about scientific use cases
>>>>>> that require accurate display timing, not just frequency.
>>>>> To summarize what information should be provided by the driver stack to
>>>>> make applications happy:
>>>>>
>>>>> 1. The minimum time a frame can be displayed, in other words the maximum
>>>>> frame rate.
>>>>> 2. The maximum time a frame can be displayed, in other words the minimum
>>>>> frame rate.
>>>>> 3. How much change of frame timing is allowed between frames to avoid
>>>>> luminescence flickering.
>>>>>
>>>>> Number 1 and 2 can also be used to signal the availability of VRR to
>>>>> applications, e.g. if they are identical we don't support VRR at all.
>>>> Hi Christian,
>>>>
>>>> "the maximum time a frame can be displayed" is perhaps not an
>>>> unambiguous definition. A frame can be shown indefinitely in any case.
>>>
>>> Good point. Please also note that I'm not an expert on the display stuff
>>> in general.
>>>
>>> My background comes more from implementing the VDPAU mediaplayer
>>> interface in mesa.
>>>
>>> So just throwing some ideas in here from the point of view of an
>>> userspace developer which wants to write a media player :)
> 
> Excellent!
> 
>>>> The CRTC will simply start scanning out the same frame again if there
>>>> is no new one. I understand what you want to say, but perhaps some
>>>> different words will be in order.
>>>>
>>>> I do wonder if applications really want to know the maximum acceptable
>>>> slew rate in timings... maybe that should be left for the drivers to
>>>> apply. What I'm thinking is that we have the page flip timestamp with
>>>> the page flip events to tell when the new FB became active. That
>>>> information could be extended with a time range on when the very next
>>>> flip could take place. Applications are already computing that
>>>> prediction from the flip timestamp and fixed refresh rate, but it might
>>>> be nice to give them the driver's opinion explicitly. Maybe the
>>>> tolerable slew rate is not a constant.
>>>
>>> Well it depends. I agree that the kernel should probably enforce the
>>> slew rate to avoid flickering for the user.
> 
> That's what I was hoping if the monitor hardware does not do that
> already, but now it sounds like it's not possible. Another failed
> assumption from my side.
> 
>>> But it might be beneficial for the application to know things like this.
> 
> Applications should know when they could likely flip, my question is
> how to tell them. Is the acceptable slew rate a constant for a video
> mode, or does it depend on the previous refresh interval.
> 
>>>
>>> What the application definitely needs to know is when a frame was
>>> actually displayed. E.g. application says I want to display this at
>>> point X and at some point later the kernel returns saying the frame was
>>> displayed at point N where in the ideal case N=X.
> 
> You already get this timestamp with the DRM page flip events. We also
> have Wayland and X11 extensions to get it to apps.
> 
>>>
>>> Additional to that let's please use 64bit values and nanoseconds for
>>> every value we use here, and NOT fps, line numbers or similar :)
> 
> Seconded!
> 
> I'd really favour absolute timestamps even.
> 
>>>
>>> Regards,
>>> Christian.
>>
>> Flickering will really depend on the panel itself. A wider ranger is
>> more likely to exhibit the issue, but factors like topology, pixel
>> density and other display technology can affect this.
>>
>> It can be hard for a driver to guess at all of this. For many panels
>> it'd be difficult to notice unless it's consistently jumping between the
>> min and max range.
> 
> Do you mean that there is no way to know and get that information from
> the monitor itself? Nothing in EDID that could even be used as a
> default and quirk the monitors that got it wrong?

The variable refresh range is exposed, but that's about it. There's 
certainly no simple algorithm you can feed monitor information into to 
determine how bad the luminance flickering is going to look to the user.

> 
>> Opening up an API that restricts how much the driver can change the
>> refresh rate in a VRR scenario seems a bit extreme, but there's probably
>> some applications that could benefit from this. I'd certainly want to
>> see the actual use case first, though. This still feels more like a
>> driver policy to me.
> 
> I don't think anyone suggested that, certainly I did not. The driver
> should get that information from the monitor hardware so that it can
> drive it correctly. If the hardware driver doesn't know, then how could
> the DRM core or userspace know any better? My whole premise was that the
> driver knows.

This was referring to the "How much change of frame timing is allowed 
between frames to avoid luminescence flickering" comment. I assumed that 
this implied restriction of the min/max VRR ranges from the driver.

> 
> Sounds like VRR hardware was designed not only for a consistent refresh
> rate but also temporary glitches being ok.
> 
> I suppose this will result in choosing a very pessimistic allowed slew
> rate in the driver that covers 95% of VRR monitors and handle the rest
> with quirks. That could still work.
> 
>> I agree with the nanosecond based timestamp API making the most logical
>> sense here from an API perspective. This does overlap a little bit with
>> the target vblank property that's already on the CRTC, perhaps.
> 
> KMS UABI already has a target vblank property defined, or are you
> talking about your CRTC hardware?
> 
> Target vblank counter value makes even less sense with VRR than it ever
> did with a fixed refresh rate. :-) >
>> The target vblank could be determined based on the timestamp. The driver
>> is likely to exceed the target presentation timestamp by a fair bit if
>> it was to just do this, however. VRR could be used in this case to get
>> closer to the timestamp. A naive implementation could iterate over every
>> rate in the range and take the one with the minimum difference, for
>> example.
> 
> Could you elaborate on that, who could be doing what exactly to achieve
> what?
> 
> 
> Thanks,
> pq
> 

These comments were mostly discussing about how a kernel driver might 
approach a target presentation timestamp.

A target presentation timestamp isn't related to VRR directly. Without 
VRR this could still be implemented by a kernel driver, but it would 
effectively be the client asking the driver to pick the right target 
vblank. There's already some support for this in DRM with the 
target_vblank CRTC property, but it isn't the easiest thing to use.

The driver can do much better at getting closer to the client's target 
presentation timestamp by adjusting the refresh rate accordingly on a 
VRR capable monitor.

Nicholas Kazlauskas
On Mon, 15 Oct 2018 12:02:14 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:

> On 10/15/2018 09:57 AM, Pekka Paalanen wrote:
> > On Fri, 12 Oct 2018 08:58:23 -0400
> > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> >   
> >> On 10/12/2018 07:20 AM, Koenig, Christian wrote:  
> >>> Am 12.10.2018 um 11:21 schrieb Pekka Paalanen:  
> >>>> On Fri, 12 Oct 2018 07:35:57 +0000
> >>>> "Koenig, Christian" <Christian.Koenig@amd.com> wrote:
> >>>>     
> >>>>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen:  
> >>>>>> On Wed, 10 Oct 2018 09:35:50 -0400
> >>>>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> >>>>>>         

> >> Flickering will really depend on the panel itself. A wider ranger is
> >> more likely to exhibit the issue, but factors like topology, pixel
> >> density and other display technology can affect this.
> >>
> >> It can be hard for a driver to guess at all of this. For many panels
> >> it'd be difficult to notice unless it's consistently jumping between the
> >> min and max range.  
> > 
> > Do you mean that there is no way to know and get that information from
> > the monitor itself? Nothing in EDID that could even be used as a
> > default and quirk the monitors that got it wrong?  
> 
> The variable refresh range is exposed, but that's about it. There's 
> certainly no simple algorithm you can feed monitor information into to 
> determine how bad the luminance flickering is going to look to the user.
> 
> >   
> >> Opening up an API that restricts how much the driver can change the
> >> refresh rate in a VRR scenario seems a bit extreme, but there's probably
> >> some applications that could benefit from this. I'd certainly want to
> >> see the actual use case first, though. This still feels more like a
> >> driver policy to me.  
> > 
> > I don't think anyone suggested that, certainly I did not. The driver
> > should get that information from the monitor hardware so that it can
> > drive it correctly. If the hardware driver doesn't know, then how could
> > the DRM core or userspace know any better? My whole premise was that the
> > driver knows.  
> 
> This was referring to the "How much change of frame timing is allowed 
> between frames to avoid luminescence flickering" comment. I assumed that 
> this implied restriction of the min/max VRR ranges from the driver.

Yes, exactly. The limits on the change of frame timings should be
enforced by the driver if the hardware does not do it already, so that
regardless of when userspace is scheduling flips, the monitor will
never flicker. IOW, even with on-demand rendering apps with totally
random frame timings, the kernel driver or the monitor hardware must
ensure the monitor will never luminance-flicker visibly.

Making good image quality (in this case not flickering) depend on
userspace doing something specific correctly without even a possibility
to know what "correct" is, is just silly in my opinion.

Too bad that ship sailed years ago, so we will need to have very
pessimistic kernel driver expectations on what slew rates are
acceptable. I do believe the kernel driver must enforce limits on slew
rate to prevent screen flickering if the hardware does not prevent it
already.

Besides, the kernel driver needs to know when to re-scan out the current
framebuffer in case userspace decides to take a pause, e.g. a temporary
stall in a game - compiling shaders or whatever. That should not result
in any flickering either, right? How will that be handled?

So, maybe it's not a bad idea to think of an UABI for changing the slew
rate limits. It is similar to color calibration: the defaults should be
ok to watch, but if one wants more correct color reproduction, one
needs to calibrate the monitor and load up color correction factors
through the driver. The luminance flickering should never be disturbing
by default, but maybe some people want to optimize further.

> > 
> > Sounds like VRR hardware was designed not only for a consistent refresh
> > rate but also temporary glitches being ok.
> > 
> > I suppose this will result in choosing a very pessimistic allowed slew
> > rate in the driver that covers 95% of VRR monitors and handle the rest
> > with quirks. That could still work.
> >   
> >> I agree with the nanosecond based timestamp API making the most logical
> >> sense here from an API perspective. This does overlap a little bit with
> >> the target vblank property that's already on the CRTC, perhaps.  
> > 
> > KMS UABI already has a target vblank property defined, or are you
> > talking about your CRTC hardware?
> > 
> > Target vblank counter value makes even less sense with VRR than it ever
> > did with a fixed refresh rate. :-) >  
> >> The target vblank could be determined based on the timestamp. The driver
> >> is likely to exceed the target presentation timestamp by a fair bit if
> >> it was to just do this, however. VRR could be used in this case to get
> >> closer to the timestamp. A naive implementation could iterate over every
> >> rate in the range and take the one with the minimum difference, for
> >> example.  
> > 
> > Could you elaborate on that, who could be doing what exactly to achieve
> > what?
> 
> These comments were mostly discussing about how a kernel driver might 
> approach a target presentation timestamp.
> 
> A target presentation timestamp isn't related to VRR directly. Without 
> VRR this could still be implemented by a kernel driver, but it would 
> effectively be the client asking the driver to pick the right target 
> vblank. There's already some support for this in DRM with the 
> target_vblank CRTC property, but it isn't the easiest thing to use.
> 
> The driver can do much better at getting closer to the client's target 
> presentation timestamp by adjusting the refresh rate accordingly on a 
> VRR capable monitor.

Right. An absolute timestamp should be easier to use than a target
vblank for userspace. On the kernel side it would avoid depending on
estimating vblank counts from a clock when vblank interrupts have been
opportunistically turned off, put to lower rate, or across modesets
that might mess up vblank counters. Also good for self-refreshing
panels, virtual outputs that get forwarded as video streams, etc.

We digress, though. Target timestamps or vblanks are not necessary for
the VRR feature, flip ASAP is what we have right now. The kernel driver
should adjust when ASAP is to avoid too high slew rate and luminance
flickering.


Thanks,
pq