[PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Nicholas Kazlauskas]

These patches are part of a proposed new interface for supporting variable refresh rate via DRM properties.

=== Changes from v1 ===

For drm:

* The variable_refresh_capable property is now flagged as DRM_MODE_PROP_IMMUTABLE

For drm/gpu/amd/display:

* Patches no longer pull in IOCTL/FreeSync refactoring code
* FreeSync enable/disable behavior has been modified to reflect changes in userspace behavior from xf86-video-amdgpu and mesa

=== Adaptive sync and variable refresh rate ===

Adaptive sync is part of the DisplayPort spec and allows for graphics adapters to drive displays with varying frame timings.

Variable refresh rate (VRR) is essentially the same, but defined for HDMI.

=== Use cases for variable refresh rate ===

Variable frame (flip) timings don't align well with fixed refresh rate displays. This results in stuttering, tearing and/or input lag. By adjusting the display refresh rate dynamically these issues can be reduced or eliminated.

However, not all content is suitable for dynamic refresh adaptation. Content that is flipped infrequently or at random intervals tends to fair poorly. Multiple clients trying to flip under the same screen can similarly interfere with prediction.

Userland needs a way to let the driver know when the content on the screen is suitable for variable refresh rate and if the user wishes to have the feature enabled.

=== DRM API to support variable refresh rates ===

This patch introduces a new API via atomic properties on the DRM connector and CRTC.

The connector has two new optional properties:

* bool variable_refresh_capable - set by the driver if the hardware is capable of supporting variable refresh tech

* bool variable_refresh_enabled - set by the user to enable variable refresh adjustment over the connector

The CRTC has one additional default property:

* bool variable_refresh - a content hint to the driver specifying that the CRTC contents are suitable for variable refresh adjustment

== Overview for DRM driver developers ===

Driver developers can attach the optional connector properties via drm_connector_attach_variable_refresh_properties on connectors that support variable refresh (typically DP or HDMI).

The variable_refresh_capable property should be managed as the output on the connector changes. The property is read only from userspace.

The variable_refresh_enabled property is intended to be a property controlled by userland as a global on/off switch for variable refresh technology. It should be checked before enabling variable refresh rate.

=== Overview for Userland developers ==

The variable_refresh property on the CRTC should be set to true when the CRTCs are suitable for variable refresh rate. In practice this is probably an application like a game - a single window that covers the whole CRTC surface and is the only client issuing flips.

To demonstrate the suitability of the API for variable refresh and dynamic adaptation there are additional patches using this API that implement adaptive variable refresh across kernel and userland projects:

* DRM (dri-devel)
* amdgpu DRM kernel driver (amd-gfx)
* xf86-video-amdgpu (amd-gfx)
* mesa (mesa-dev)

These patches enable adaptive variable refresh on X for AMD hardware provided that the user sets the variable_refresh_enabled property to true on supported connectors (ie. using xrandr --set-prop).

The patches have been tested as working on upstream userland with the GNOME desktop environment under a single monitor setup. They also work on KDE in single monitor setup if the compositor is disabled.

The patches require that the application window can issue screen flips via the Present extension to xf86-video-amdgpu. Due to Present extension limitations some desktop environments and multi-monitor setups are currently not compatible.

Full implementation details for these changes can be reviewed in their respective mailing lists.

=== Previous discussions ===

These patches are based upon feedback from patches and feedback from two previous threads on the subject which are linked below for reference:

https://lists.freedesktop.org/archives/amd-gfx/2018-April/021047.html
https://lists.freedesktop.org/archives/dri-devel/2017-October/155207.html
https://lists.freedesktop.org/archives/dri-devel/2018-September/189404.html

Nicholas Kazlauskas

Nicholas Kazlauskas (3):
  drm: Add variable refresh rate properties to connector
  drm: Add variable refresh property to DRM CRTC
  drm/amd/display: Set FreeSync state using DRM VRR properties

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
 drivers/gpu/drm/drm_atomic_helper.c           |   1 +
 drivers/gpu/drm/drm_atomic_uapi.c             |  12 +
 drivers/gpu/drm/drm_connector.c               |  35 +++
 drivers/gpu/drm/drm_crtc.c                    |   2 +
 drivers/gpu/drm/drm_mode_config.c             |   6 +
 include/drm/drm_connector.h                   |  27 ++
 include/drm/drm_crtc.h                        |  13 +
 include/drm/drm_mode_config.h                 |   8 +
 10 files changed, 225 insertions(+), 117 deletions(-)

-- 
2.19.0

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

[PATCH v2 1/3] drm: Add variable refresh rate properties to connector

[Nicholas Kazlauskas]

Modern display hardware is capable of supporting variable refresh rates
and adaptive sync technologies. The properties for querying and
controlling these features should be exposed on the DRM connector.

This patch introduces two new properties for variable refresh rate
support:

- variable_refresh_capable
- variable_refresh_enabled

These are optional properties that can be added to a DRM connector
dynamically by using drm_connector_attach_variable_refresh_properties.

DRM drivers should set variable_refresh_capable as applicable for
their hardware. The property variable_refresh_enabled is a userspace
controlled option.

Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  6 ++++++
 drivers/gpu/drm/drm_connector.c   | 35 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 27 ++++++++++++++++++++++++
 3 files changed, 68 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d5b7f315098c..0bb27a24a55c 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -723,6 +723,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 		state->content_type = val;
 	} else if (property == connector->scaling_mode_property) {
 		state->scaling_mode = val;
+	} else if (property == connector->variable_refresh_enabled_property) {
+		state->variable_refresh_enabled = val;
 	} else if (property == connector->content_protection_property) {
 		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
 			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
@@ -797,6 +799,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->content_type;
 	} else if (property == connector->scaling_mode_property) {
 		*val = state->scaling_mode;
+	} else if (property == connector->variable_refresh_capable_property) {
+		*val = state->variable_refresh_capable;
+	} else if (property == connector->variable_refresh_enabled_property) {
+		*val = state->variable_refresh_enabled;
 	} else if (property == connector->content_protection_property) {
 		*val = state->content_protection;
 	} else if (property == config->writeback_fb_id_property) {
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1e40e5decbe9..fc1732639bd3 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1254,6 +1254,41 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
 
+/**
+ * drm_connector_attach_variable_refresh_properties - creates and attaches
+ * properties for connectors that support adaptive refresh
+ * @connector: connector to create adaptive refresh properties on
+ */
+int drm_connector_attach_variable_refresh_properties(
+	struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	if (!connector->variable_refresh_capable_property) {
+		prop = drm_property_create_bool(dev, DRM_MODE_PROP_IMMUTABLE,
+			"variable_refresh_capable");
+		if (!prop)
+			return -ENOMEM;
+
+		connector->variable_refresh_capable_property = prop;
+		drm_object_attach_property(&connector->base, prop, 0);
+	}
+
+	if (!connector->variable_refresh_enabled_property) {
+		prop = drm_property_create_bool(dev, 0,
+			"variable_refresh_enabled");
+		if (!prop)
+			return -ENOMEM;
+
+		connector->variable_refresh_enabled_property = prop;
+		drm_object_attach_property(&connector->base, prop, 0);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_variable_refresh_properties);
+
 /**
  * drm_connector_attach_scaling_mode_property - attach atomic scaling mode property
  * @connector: connector to attach scaling mode property on.
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 91a877fa00cb..e2c26842bd50 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -449,6 +449,18 @@ struct drm_connector_state {
 	 */
 	unsigned int content_protection;
 
+	/**
+	 * @variable_refresh_enabled: Connector property used to check
+	 * if variable refresh is supported on the device.
+	 */
+	bool variable_refresh_capable;
+
+	/**
+	 * @variable_refresh_enabled: Connector property used to check
+	 * if variable refresh is enabled.
+	 */
+	bool variable_refresh_enabled;
+
 	/**
 	 * @writeback_job: Writeback job for writeback connectors
 	 *
@@ -910,6 +922,19 @@ struct drm_connector {
 	 */
 	struct drm_property *scaling_mode_property;
 
+	/**
+	 * @variable_refresh_capable_property: Optional property for
+	 * querying hardware support for variable refresh.
+	 */
+	struct drm_property *variable_refresh_capable_property;
+
+	/**
+	 * @variable_refresh_enabled_property: Optional property for
+	 * enabling or disabling support for variable refresh
+	 * on the connector.
+	 */
+	struct drm_property *variable_refresh_enabled_property;
+
 	/**
 	 * @content_protection_property: DRM ENUM property for content
 	 * protection. See drm_connector_attach_content_protection_property().
@@ -1183,6 +1208,8 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 int drm_connector_attach_content_type_property(struct drm_connector *dev);
 int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
 					       u32 scaling_mode_mask);
+int drm_connector_attach_variable_refresh_properties(
+	struct drm_connector *connector);
 int drm_connector_attach_content_protection_property(
 		struct drm_connector *connector);
 int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
-- 
2.19.0

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

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

[Nicholas Kazlauskas]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/3] drm/amd/display: Set FreeSync state using DRM VRR properties

[Nicholas Kazlauskas]

The AMDGPU specific FreeSync properties and IOCTLs are dropped
from amdgpu_dm in favor of the DRM variable refresh properties.

The AMDGPU connector properties freesync_capable and freesync_enabled
are mostly direct mappings to the DRM variable_refresh_capable and
variable_refresh_enabled.

The AMDGPU CRTC freesync_enabled property requires special care to
handle correctly. The DRM variable_refresh property is a content hint
to the driver which is independent from actual hardware variable
refresh capability and user configuration.

To handle changes with this property it was easier to drop the forced
modeset on variable refresh change. This patch now performs the required
stream updates when planes are attached *or* flipped.

This is done for a few reasons:

(1) VRR stream updates can be done in the fast update path

(2) amdgpu_dm_atomic_check would have had to been hacked apart to check
    desired variable refresh state and capability before the CRTC
    disable pass.

(3) Performing VRR stream updates on-flip is needed for enabling BTR
    support.

FreeSync state on the stream is now tracked and compared to the previous
packet sent to the hardware.

It's worth noting that the current implementation treats
variable_refresh_enabled as a strict requirement for sending
the VRR enable *or* disable packet.

Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
 2 files changed, 121 insertions(+), 117 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 6c849e055038..0fddc2e66f49 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1683,72 +1683,6 @@ static void dm_bandwidth_update(struct amdgpu_device *adev)
 	/* TODO: implement later */
 }
 
-static int amdgpu_notify_freesync(struct drm_device *dev, void *data,
-				struct drm_file *filp)
-{
-	struct drm_atomic_state *state;
-	struct drm_modeset_acquire_ctx ctx;
-	struct drm_crtc *crtc;
-	struct drm_connector *connector;
-	struct drm_connector_state *old_con_state, *new_con_state;
-	int ret = 0;
-	uint8_t i;
-	bool enable = false;
-
-	drm_modeset_acquire_init(&ctx, 0);
-
-	state = drm_atomic_state_alloc(dev);
-	if (!state) {
-		ret = -ENOMEM;
-		goto out;
-	}
-	state->acquire_ctx = &ctx;
-
-retry:
-	drm_for_each_crtc(crtc, dev) {
-		ret = drm_atomic_add_affected_connectors(state, crtc);
-		if (ret)
-			goto fail;
-
-		/* TODO rework amdgpu_dm_commit_planes so we don't need this */
-		ret = drm_atomic_add_affected_planes(state, crtc);
-		if (ret)
-			goto fail;
-	}
-
-	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
-		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
-		struct drm_crtc_state *new_crtc_state;
-		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
-		struct dm_crtc_state *dm_new_crtc_state;
-
-		if (!acrtc) {
-			ASSERT(0);
-			continue;
-		}
-
-		new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
-		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
-
-		dm_new_crtc_state->freesync_enabled = enable;
-	}
-
-	ret = drm_atomic_commit(state);
-
-fail:
-	if (ret == -EDEADLK) {
-		drm_atomic_state_clear(state);
-		drm_modeset_backoff(&ctx);
-		goto retry;
-	}
-
-	drm_atomic_state_put(state);
-
-out:
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-	return ret;
-}
 
 static const struct amdgpu_display_funcs dm_display_funcs = {
 	.bandwidth_update = dm_bandwidth_update, /* called unconditionally */
@@ -1762,7 +1696,6 @@ static const struct amdgpu_display_funcs dm_display_funcs = {
 		dm_crtc_get_scanoutpos,/* called unconditionally */
 	.add_encoder = NULL, /* VBIOS parsing. DAL does it. */
 	.add_connector = NULL, /* VBIOS parsing. DAL does it. */
-	.notify_freesync = amdgpu_notify_freesync,
 
 };
 
@@ -2645,7 +2578,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 
 	update_stream_signal(stream);
 
-	if (dm_state && dm_state->freesync_capable)
+	if (dm_state && dm_state->base.variable_refresh_capable)
 		stream->ignore_msa_timing_param = true;
 finish:
 	if (sink && sink->sink_signal == SIGNAL_TYPE_VIRTUAL)
@@ -2713,9 +2646,10 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
 		dc_stream_retain(state->stream);
 	}
 
+	state->freesync_enabled = cur->freesync_enabled;
+	state->freesync_config = cur->freesync_config;
 	state->adjust = cur->adjust;
 	state->vrr_infopacket = cur->vrr_infopacket;
-	state->freesync_enabled = cur->freesync_enabled;
 
 	/* TODO Duplicate dc_stream after objects are stream object is flattened */
 
@@ -2933,9 +2867,6 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
 
 	__drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);
 
-	new_state->freesync_capable = state->freesync_capable;
-	new_state->freesync_enable = state->freesync_enable;
-
 	return &new_state->base;
 }
 
@@ -3681,6 +3612,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				adev->mode_info.underscan_vborder_property,
 				0);
 
+	if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
+	    connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
+		drm_connector_attach_variable_refresh_properties(
+			&aconnector->base);
+	}
 }
 
 static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
@@ -4057,6 +3993,63 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc)
 						 acrtc->crtc_id);
 }
 
+static void update_freesync_state_on_stream(
+	struct amdgpu_display_manager *dm,
+	struct dm_crtc_state *new_crtc_state,
+	struct dc_stream_state *new_stream)
+{
+	struct mod_vrr_params vrr = {0};
+	struct dc_info_packet vrr_infopacket = {0};
+	struct mod_freesync_config config = new_crtc_state->freesync_config;
+
+	if (!new_stream)
+		return;
+
+	if (new_crtc_state->freesync_enabled) {
+		config.state = new_crtc_state->base.variable_refresh ?
+			VRR_STATE_ACTIVE_VARIABLE :
+			VRR_STATE_INACTIVE;
+	}
+
+	mod_freesync_build_vrr_params(dm->freesync_module,
+				      new_stream,
+				      &config, &vrr);
+
+	mod_freesync_build_vrr_infopacket(
+		dm->freesync_module,
+		new_stream,
+		&vrr,
+		&vrr_infopacket);
+
+	new_crtc_state->freesync_timing_changed =
+		(memcmp(&new_crtc_state->adjust,
+			&vrr.adjust,
+			sizeof(vrr.adjust)) != 0);
+
+	new_crtc_state->freesync_vrr_info_changed =
+		(memcmp(&new_crtc_state->vrr_infopacket,
+		        &vrr_infopacket,
+		        sizeof(vrr_infopacket)) != 0);
+
+	new_crtc_state->adjust = vrr.adjust;
+	new_crtc_state->vrr_infopacket = vrr_infopacket;
+
+	new_stream->adjust = new_crtc_state->adjust;
+	new_stream->vrr_infopacket = vrr_infopacket;
+
+	if (new_crtc_state->freesync_vrr_info_changed)
+		DRM_DEBUG_KMS("Freesync VRR info changed: crtc_id=%d wanted=%d actual=%d",
+			(int)new_crtc_state->base.crtc->base.id,
+			(int)new_crtc_state->base.variable_refresh,
+			(int)vrr.state);
+
+	if (new_crtc_state->freesync_timing_changed)
+		DRM_DEBUG_KMS("Freesync timing changed: crtc_id=%d min=%d max=%d\n",
+			(int)new_crtc_state->base.crtc->base.id,
+			(int)vrr.adjust.v_total_min,
+			(int)vrr.adjust.v_total_max);
+}
+
 /*
  * Executes flip
  *
@@ -4078,6 +4071,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
 	struct dc_flip_addrs addr = { {0} };
 	/* TODO eliminate or rename surface_update */
 	struct dc_surface_update surface_updates[1] = { {0} };
+	struct dc_stream_update stream_update = {0};
 	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
 
 
@@ -4137,11 +4131,21 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
 	surface_updates->surface = dc_stream_get_status(acrtc_state->stream)->plane_states[0];
 	surface_updates->flip_addr = &addr;
 
+	if (acrtc_state->stream) {
+		if (acrtc_state->freesync_timing_changed)
+			stream_update.adjust =
+				&acrtc_state->stream->adjust;
+
+		if (acrtc_state->freesync_vrr_info_changed)
+			stream_update.vrr_infopacket =
+				&acrtc_state->stream->vrr_infopacket;
+	}
+
 	dc_commit_updates_for_stream(adev->dm.dc,
 					     surface_updates,
 					     1,
 					     acrtc_state->stream,
-					     NULL,
+					     &stream_update,
 					     &surface_updates->surface,
 					     state);
 
@@ -4201,10 +4205,10 @@ static bool commit_planes_to_stream(
 	stream_update->dst = dc_stream->dst;
 	stream_update->out_transfer_func = dc_stream->out_transfer_func;
 
-	if (dm_new_crtc_state->freesync_enabled != dm_old_crtc_state->freesync_enabled) {
+	if (dm_new_crtc_state->freesync_vrr_info_changed)
 		stream_update->vrr_infopacket = &dc_stream->vrr_infopacket;
+	if (dm_new_crtc_state->freesync_timing_changed)
 		stream_update->adjust = &dc_stream->adjust;
-	}
 
 	for (i = 0; i < new_plane_count; i++) {
 		updates[i].surface = plane_states[i];
@@ -4341,9 +4345,6 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
 		}
 
-		dc_stream_attach->adjust = acrtc_state->adjust;
-		dc_stream_attach->vrr_infopacket = acrtc_state->vrr_infopacket;
-
 		if (false == commit_planes_to_stream(dm->dc,
 							plane_states_constructed,
 							planes_count,
@@ -4515,6 +4516,17 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		}
 	}
 
+	/* Handle freesync changes. */
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, j) {
+		if (!new_crtc_state->active)
+			continue;
+
+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+
+		update_freesync_state_on_stream(dm,
+			dm_new_crtc_state, dm_new_crtc_state->stream);
+	}
+
 	/* Handle scaling and underscan changes*/
 	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
@@ -4547,9 +4559,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		WARN_ON(!status);
 		WARN_ON(!status->plane_count);
 
-		dm_new_crtc_state->stream->adjust = dm_new_crtc_state->adjust;
-		dm_new_crtc_state->stream->vrr_infopacket = dm_new_crtc_state->vrr_infopacket;
-
 		/*TODO How it works with MPO ?*/
 		if (!commit_planes_to_stream(
 				dm->dc,
@@ -4767,20 +4776,19 @@ static int do_aquire_global_lock(struct drm_device *dev,
 	return ret < 0 ? ret : 0;
 }
 
-void set_freesync_on_stream(struct amdgpu_display_manager *dm,
-			    struct dm_crtc_state *new_crtc_state,
-			    struct dm_connector_state *new_con_state,
-			    struct dc_stream_state *new_stream)
+static void get_freesync_config_for_crtc(
+	struct dm_crtc_state *new_crtc_state,
+	struct dm_connector_state *new_con_state)
 {
 	struct mod_freesync_config config = {0};
-	struct mod_vrr_params vrr = {0};
-	struct dc_info_packet vrr_infopacket = {0};
 	struct amdgpu_dm_connector *aconnector =
 			to_amdgpu_dm_connector(new_con_state->base.connector);
 
-	if (new_con_state->freesync_capable &&
-	    new_con_state->freesync_enable) {
-		config.state = new_crtc_state->freesync_enabled ?
+	new_crtc_state->freesync_enabled = false;
+
+	if (new_con_state->base.variable_refresh_capable &&
+	    new_con_state->base.variable_refresh_enabled) {
+		config.state = new_crtc_state->base.variable_refresh ?
 				VRR_STATE_ACTIVE_VARIABLE :
 				VRR_STATE_INACTIVE;
 		config.min_refresh_in_uhz =
@@ -4788,19 +4796,20 @@ void set_freesync_on_stream(struct amdgpu_display_manager *dm,
 		config.max_refresh_in_uhz =
 				aconnector->max_vfreq * 1000000;
 		config.vsif_supported = true;
-	}
 
-	mod_freesync_build_vrr_params(dm->freesync_module,
-				      new_stream,
-				      &config, &vrr);
+		new_crtc_state->freesync_enabled = true;
+	}
 
-	mod_freesync_build_vrr_infopacket(dm->freesync_module,
-					  new_stream,
-					  &vrr,
-					  &vrr_infopacket);
+	new_crtc_state->freesync_config = config;
+}
 
-	new_crtc_state->adjust = vrr.adjust;
-	new_crtc_state->vrr_infopacket = vrr_infopacket;
+static void reset_freesync_config_for_crtc(
+	struct dm_crtc_state *new_crtc_state)
+{
+	memset(&new_crtc_state->adjust, 0,
+			sizeof(new_crtc_state->adjust));
+	memset(&new_crtc_state->vrr_infopacket, 0,
+		sizeof(new_crtc_state->vrr_infopacket));
 }
 
 static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
@@ -4875,9 +4884,6 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 				break;
 			}
 
-			set_freesync_on_stream(dm, dm_new_crtc_state,
-					       dm_new_conn_state, new_stream);
-
 			if (dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
 			    dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
 				new_crtc_state->mode_changed = false;
@@ -4886,9 +4892,6 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 			}
 		}
 
-		if (dm_old_crtc_state->freesync_enabled != dm_new_crtc_state->freesync_enabled)
-			new_crtc_state->mode_changed = true;
-
 		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
 			goto next_crtc;
 
@@ -4925,6 +4928,8 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 			dc_stream_release(dm_old_crtc_state->stream);
 			dm_new_crtc_state->stream = NULL;
 
+			reset_freesync_config_for_crtc(dm_new_crtc_state);
+
 			*lock_and_validation_needed = true;
 
 		} else {/* Add stream for any updated/enabled CRTC */
@@ -5002,7 +5007,8 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 			amdgpu_dm_set_ctm(dm_new_crtc_state);
 		}
 
-
+		/* Update Freesync settings. */
+		get_freesync_config_for_crtc(dm_new_crtc_state, dm_new_conn_state);
 	}
 
 	return ret;
@@ -5267,12 +5273,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
 		goto fail;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-		struct dm_crtc_state *dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
-		struct dm_crtc_state *dm_old_crtc_state  = to_dm_crtc_state(old_crtc_state);
-
 		if (!drm_atomic_crtc_needs_modeset(new_crtc_state) &&
 		    !new_crtc_state->color_mgmt_changed &&
-		    (dm_old_crtc_state->freesync_enabled == dm_new_crtc_state->freesync_enabled))
+		    !new_crtc_state->variable_refresh_changed)
 			continue;
 
 		if (!new_crtc_state->enable)
@@ -5434,6 +5437,8 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 		return;
 	}
 
+	connector->state->variable_refresh_capable = false;
+
 	if (!edid) {
 		dm_con_state = to_dm_connector_state(connector->state);
 
@@ -5441,8 +5446,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 		amdgpu_dm_connector->max_vfreq = 0;
 		amdgpu_dm_connector->pixel_clock_mhz = 0;
 
-		dm_con_state->freesync_capable = false;
-		dm_con_state->freesync_enable = false;
 		return;
 	}
 
@@ -5466,7 +5469,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 						amdgpu_dm_connector);
 		}
 	}
-	dm_con_state->freesync_capable = false;
 	if (edid_check_required == true && (edid->version > 1 ||
 	   (edid->version == 1 && edid->revision > 1))) {
 		for (i = 0; i < 4; i++) {
@@ -5498,7 +5500,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 		if (amdgpu_dm_connector->max_vfreq -
 		    amdgpu_dm_connector->min_vfreq > 10) {
 
-			dm_con_state->freesync_capable = true;
+			connector->state->variable_refresh_capable = true;
 		}
 	}
 }
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index d4f1bdf93207..debde9899ba7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -187,7 +187,11 @@ struct dm_crtc_state {
 	int crc_skip_count;
 	bool crc_enabled;
 
+	bool freesync_timing_changed;
+	bool freesync_vrr_info_changed;
+
 	bool freesync_enabled;
+	struct mod_freesync_config freesync_config;
 	struct dc_crtc_timing_adjust adjust;
 	struct dc_info_packet vrr_infopacket;
 };
@@ -209,8 +213,6 @@ struct dm_connector_state {
 	uint8_t underscan_vborder;
 	uint8_t underscan_hborder;
 	bool underscan_enable;
-	bool freesync_enable;
-	bool freesync_capable;
 };
 
 #define to_dm_connector_state(x)\
-- 
2.19.0

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

Re: [PATCH v2 1/3] drm: Add variable refresh rate properties to connector

[Ville Syrjälä]

On Mon, Sep 24, 2018 at 02:15:35PM -0400, Nicholas Kazlauskas wrote:
> Modern display hardware is capable of supporting variable refresh rates
> and adaptive sync technologies. The properties for querying and
> controlling these features should be exposed on the DRM connector.
> 
> This patch introduces two new properties for variable refresh rate
> support:
> 
> - variable_refresh_capable
> - variable_refresh_enabled
> 
> These are optional properties that can be added to a DRM connector
> dynamically by using drm_connector_attach_variable_refresh_properties.
> 
> DRM drivers should set variable_refresh_capable as applicable for
> their hardware. The property variable_refresh_enabled is a userspace
> controlled option.
> 
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  6 ++++++
>  drivers/gpu/drm/drm_connector.c   | 35 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 27 ++++++++++++++++++++++++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d5b7f315098c..0bb27a24a55c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -723,6 +723,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		state->content_type = val;
>  	} else if (property == connector->scaling_mode_property) {
>  		state->scaling_mode = val;
> +	} else if (property == connector->variable_refresh_enabled_property) {
> +		state->variable_refresh_enabled = val;
>  	} else if (property == connector->content_protection_property) {
>  		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
>  			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
> @@ -797,6 +799,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->content_type;
>  	} else if (property == connector->scaling_mode_property) {
>  		*val = state->scaling_mode;
> +	} else if (property == connector->variable_refresh_capable_property) {
> +		*val = state->variable_refresh_capable;

Immutable props don't take this path. See
__drm_object_property_get_value().

> +	} else if (property == connector->variable_refresh_enabled_property) {
> +		*val = state->variable_refresh_enabled;
>  	} else if (property == connector->content_protection_property) {
>  		*val = state->content_protection;
>  	} else if (property == config->writeback_fb_id_property) {
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1e40e5decbe9..fc1732639bd3 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1254,6 +1254,41 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>  
> +/**
> + * drm_connector_attach_variable_refresh_properties - creates and attaches
> + * properties for connectors that support adaptive refresh
> + * @connector: connector to create adaptive refresh properties on
> + */
> +int drm_connector_attach_variable_refresh_properties(
> +	struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	if (!connector->variable_refresh_capable_property) {
> +		prop = drm_property_create_bool(dev, DRM_MODE_PROP_IMMUTABLE,
> +			"variable_refresh_capable");
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->variable_refresh_capable_property = prop;
> +		drm_object_attach_property(&connector->base, prop, 0);
> +	}
> +
> +	if (!connector->variable_refresh_enabled_property) {
> +		prop = drm_property_create_bool(dev, 0,
> +			"variable_refresh_enabled");
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->variable_refresh_enabled_property = prop;
> +		drm_object_attach_property(&connector->base, prop, 0);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_variable_refresh_properties);
> +
>  /**
>   * drm_connector_attach_scaling_mode_property - attach atomic scaling mode property
>   * @connector: connector to attach scaling mode property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 91a877fa00cb..e2c26842bd50 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -449,6 +449,18 @@ struct drm_connector_state {
>  	 */
>  	unsigned int content_protection;
>  
> +	/**
> +	 * @variable_refresh_enabled: Connector property used to check
> +	 * if variable refresh is supported on the device.
> +	 */
> +	bool variable_refresh_capable;
> +
> +	/**
> +	 * @variable_refresh_enabled: Connector property used to check
> +	 * if variable refresh is enabled.
> +	 */
> +	bool variable_refresh_enabled;
> +
>  	/**
>  	 * @writeback_job: Writeback job for writeback connectors
>  	 *
> @@ -910,6 +922,19 @@ struct drm_connector {
>  	 */
>  	struct drm_property *scaling_mode_property;
>  
> +	/**
> +	 * @variable_refresh_capable_property: Optional property for
> +	 * querying hardware support for variable refresh.
> +	 */
> +	struct drm_property *variable_refresh_capable_property;
> +
> +	/**
> +	 * @variable_refresh_enabled_property: Optional property for
> +	 * enabling or disabling support for variable refresh
> +	 * on the connector.
> +	 */
> +	struct drm_property *variable_refresh_enabled_property;
> +
>  	/**
>  	 * @content_protection_property: DRM ENUM property for content
>  	 * protection. See drm_connector_attach_content_protection_property().
> @@ -1183,6 +1208,8 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>  int drm_connector_attach_content_type_property(struct drm_connector *dev);
>  int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>  					       u32 scaling_mode_mask);
> +int drm_connector_attach_variable_refresh_properties(
> +	struct drm_connector *connector);
>  int drm_connector_attach_content_protection_property(
>  		struct drm_connector *connector);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> -- 
> 2.19.0

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Kazlauskas, Nicholas]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Kazlauskas, Nicholas]

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Ville Syrjälä]

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?

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: Add variable refresh rate properties to connector

[Daniel Vetter]

On Mon, Sep 24, 2018 at 09:32:11PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 24, 2018 at 02:15:35PM -0400, Nicholas Kazlauskas wrote:
> > Modern display hardware is capable of supporting variable refresh rates
> > and adaptive sync technologies. The properties for querying and
> > controlling these features should be exposed on the DRM connector.
> > 
> > This patch introduces two new properties for variable refresh rate
> > support:
> > 
> > - variable_refresh_capable
> > - variable_refresh_enabled
> > 
> > These are optional properties that can be added to a DRM connector
> > dynamically by using drm_connector_attach_variable_refresh_properties.
> > 
> > DRM drivers should set variable_refresh_capable as applicable for
> > their hardware. The property variable_refresh_enabled is a userspace
> > controlled option.
> > 
> > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  6 ++++++
> >  drivers/gpu/drm/drm_connector.c   | 35 +++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       | 27 ++++++++++++++++++++++++
> >  3 files changed, 68 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d5b7f315098c..0bb27a24a55c 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -723,6 +723,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  		state->content_type = val;
> >  	} else if (property == connector->scaling_mode_property) {
> >  		state->scaling_mode = val;
> > +	} else if (property == connector->variable_refresh_enabled_property) {
> > +		state->variable_refresh_enabled = val;
> >  	} else if (property == connector->content_protection_property) {
> >  		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> >  			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
> > @@ -797,6 +799,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >  		*val = state->content_type;
> >  	} else if (property == connector->scaling_mode_property) {
> >  		*val = state->scaling_mode;
> > +	} else if (property == connector->variable_refresh_capable_property) {
> > +		*val = state->variable_refresh_capable;
> 
> Immutable props don't take this path. See
> __drm_object_property_get_value().

Yeah I think an explicit set function (like we have for edid, link_status,
...) would be good here. Gives us at least a neat way to attach some more
kernel-doc.
-Daniel

> 
> > +	} else if (property == connector->variable_refresh_enabled_property) {
> > +		*val = state->variable_refresh_enabled;
> >  	} else if (property == connector->content_protection_property) {
> >  		*val = state->content_protection;
> >  	} else if (property == config->writeback_fb_id_property) {
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 1e40e5decbe9..fc1732639bd3 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1254,6 +1254,41 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
> >  }
> >  EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
> >  
> > +/**
> > + * drm_connector_attach_variable_refresh_properties - creates and attaches
> > + * properties for connectors that support adaptive refresh
> > + * @connector: connector to create adaptive refresh properties on
> > + */
> > +int drm_connector_attach_variable_refresh_properties(
> > +	struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_property *prop;
> > +
> > +	if (!connector->variable_refresh_capable_property) {
> > +		prop = drm_property_create_bool(dev, DRM_MODE_PROP_IMMUTABLE,
> > +			"variable_refresh_capable");
> > +		if (!prop)
> > +			return -ENOMEM;
> > +
> > +		connector->variable_refresh_capable_property = prop;
> > +		drm_object_attach_property(&connector->base, prop, 0);
> > +	}
> > +
> > +	if (!connector->variable_refresh_enabled_property) {
> > +		prop = drm_property_create_bool(dev, 0,
> > +			"variable_refresh_enabled");
> > +		if (!prop)
> > +			return -ENOMEM;
> > +
> > +		connector->variable_refresh_enabled_property = prop;
> > +		drm_object_attach_property(&connector->base, prop, 0);
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_connector_attach_variable_refresh_properties);
> > +
> >  /**
> >   * drm_connector_attach_scaling_mode_property - attach atomic scaling mode property
> >   * @connector: connector to attach scaling mode property on.
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 91a877fa00cb..e2c26842bd50 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -449,6 +449,18 @@ struct drm_connector_state {
> >  	 */
> >  	unsigned int content_protection;
> >  
> > +	/**
> > +	 * @variable_refresh_enabled: Connector property used to check
> > +	 * if variable refresh is supported on the device.
> > +	 */
> > +	bool variable_refresh_capable;
> > +
> > +	/**
> > +	 * @variable_refresh_enabled: Connector property used to check
> > +	 * if variable refresh is enabled.
> > +	 */
> > +	bool variable_refresh_enabled;
> > +
> >  	/**
> >  	 * @writeback_job: Writeback job for writeback connectors
> >  	 *
> > @@ -910,6 +922,19 @@ struct drm_connector {
> >  	 */
> >  	struct drm_property *scaling_mode_property;
> >  
> > +	/**
> > +	 * @variable_refresh_capable_property: Optional property for
> > +	 * querying hardware support for variable refresh.
> > +	 */
> > +	struct drm_property *variable_refresh_capable_property;
> > +
> > +	/**
> > +	 * @variable_refresh_enabled_property: Optional property for
> > +	 * enabling or disabling support for variable refresh
> > +	 * on the connector.
> > +	 */
> > +	struct drm_property *variable_refresh_enabled_property;
> > +
> >  	/**
> >  	 * @content_protection_property: DRM ENUM property for content
> >  	 * protection. See drm_connector_attach_content_protection_property().
> > @@ -1183,6 +1208,8 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev);
> >  int drm_connector_attach_content_type_property(struct drm_connector *dev);
> >  int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
> >  					       u32 scaling_mode_mask);
> > +int drm_connector_attach_variable_refresh_properties(
> > +	struct drm_connector *connector);
> >  int drm_connector_attach_content_protection_property(
> >  		struct drm_connector *connector);
> >  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> > -- 
> > 2.19.0
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Daniel Vetter]

On Mon, Sep 24, 2018 at 02:15:34PM -0400, Nicholas Kazlauskas wrote:
> These patches are part of a proposed new interface for supporting variable refresh rate via DRM properties.
> 
> === Changes from v1 ===
> 
> For drm:
> 
> * The variable_refresh_capable property is now flagged as DRM_MODE_PROP_IMMUTABLE
> 
> For drm/gpu/amd/display:
> 
> * Patches no longer pull in IOCTL/FreeSync refactoring code
> * FreeSync enable/disable behavior has been modified to reflect changes in userspace behavior from xf86-video-amdgpu and mesa
> 
> === Adaptive sync and variable refresh rate ===
> 
> Adaptive sync is part of the DisplayPort spec and allows for graphics adapters to drive displays with varying frame timings.
> 
> Variable refresh rate (VRR) is essentially the same, but defined for HDMI.
> 
> === Use cases for variable refresh rate ===
> 
> Variable frame (flip) timings don't align well with fixed refresh rate displays. This results in stuttering, tearing and/or input lag. By adjusting the display refresh rate dynamically these issues can be reduced or eliminated.
> 
> However, not all content is suitable for dynamic refresh adaptation. Content that is flipped infrequently or at random intervals tends to fair poorly. Multiple clients trying to flip under the same screen can similarly interfere with prediction.
> 
> Userland needs a way to let the driver know when the content on the screen is suitable for variable refresh rate and if the user wishes to have the feature enabled.
> 
> === DRM API to support variable refresh rates ===
> 
> This patch introduces a new API via atomic properties on the DRM connector and CRTC.
> 
> The connector has two new optional properties:
> 
> * bool variable_refresh_capable - set by the driver if the hardware is capable of supporting variable refresh tech
> 
> * bool variable_refresh_enabled - set by the user to enable variable refresh adjustment over the connector
> 
> The CRTC has one additional default property:
> 
> * bool variable_refresh - a content hint to the driver specifying that the CRTC contents are suitable for variable refresh adjustment
> 
> == Overview for DRM driver developers ===
> 
> Driver developers can attach the optional connector properties via drm_connector_attach_variable_refresh_properties on connectors that support variable refresh (typically DP or HDMI).
> 
> The variable_refresh_capable property should be managed as the output on the connector changes. The property is read only from userspace.
> 
> The variable_refresh_enabled property is intended to be a property controlled by userland as a global on/off switch for variable refresh technology. It should be checked before enabling variable refresh rate.
> 
> === Overview for Userland developers ==
> 
> The variable_refresh property on the CRTC should be set to true when the CRTCs are suitable for variable refresh rate. In practice this is probably an application like a game - a single window that covers the whole CRTC surface and is the only client issuing flips.
> 
> To demonstrate the suitability of the API for variable refresh and dynamic adaptation there are additional patches using this API that implement adaptive variable refresh across kernel and userland projects:
> 
> * DRM (dri-devel)
> * amdgpu DRM kernel driver (amd-gfx)
> * xf86-video-amdgpu (amd-gfx)
> * mesa (mesa-dev)
> 
> These patches enable adaptive variable refresh on X for AMD hardware provided that the user sets the variable_refresh_enabled property to true on supported connectors (ie. using xrandr --set-prop).
> 
> The patches have been tested as working on upstream userland with the GNOME desktop environment under a single monitor setup. They also work on KDE in single monitor setup if the compositor is disabled.
> 
> The patches require that the application window can issue screen flips via the Present extension to xf86-video-amdgpu. Due to Present extension limitations some desktop environments and multi-monitor setups are currently not compatible.
> 
> Full implementation details for these changes can be reviewed in their respective mailing lists.
> 
> === Previous discussions ===
> 
> These patches are based upon feedback from patches and feedback from two previous threads on the subject which are linked below for reference:
> 
> https://lists.freedesktop.org/archives/amd-gfx/2018-April/021047.html
> https://lists.freedesktop.org/archives/dri-devel/2017-October/155207.html
> https://lists.freedesktop.org/archives/dri-devel/2018-September/189404.html
> 
> Nicholas Kazlauskas
> 
> Nicholas Kazlauskas (3):
>   drm: Add variable refresh rate properties to connector
>   drm: Add variable refresh property to DRM CRTC
>   drm/amd/display: Set FreeSync state using DRM VRR properties

Please include Manasi manasi.d.navare@intel.com when resending, she's
working on this from our side.

Also some overview kernel-docs that document the uapi aspect of how the
prorties are driven should be included. Probably best if you add a new
"Variable Refresh Rate" section under

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-properties

with links to functions drivers should call to set up and everything. Best
practice is to stuff all that into a DOC: comment.

An igt testcase would be neat too.

Cheers, Daniel

> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
>  drivers/gpu/drm/drm_atomic_helper.c           |   1 +
>  drivers/gpu/drm/drm_atomic_uapi.c             |  12 +
>  drivers/gpu/drm/drm_connector.c               |  35 +++
>  drivers/gpu/drm/drm_crtc.c                    |   2 +
>  drivers/gpu/drm/drm_mode_config.c             |   6 +
>  include/drm/drm_connector.h                   |  27 ++
>  include/drm/drm_crtc.h                        |  13 +
>  include/drm/drm_mode_config.h                 |   8 +
>  10 files changed, 225 insertions(+), 117 deletions(-)
> 
> -- 
> 2.19.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Harry Wentland]

On 2018-10-01 03:15 AM, Daniel Vetter wrote:
> On Mon, Sep 24, 2018 at 02:15:34PM -0400, Nicholas Kazlauskas wrote:
>> These patches are part of a proposed new interface for supporting variable refresh rate via DRM properties.
>>
>> === Changes from v1 ===
>>
>> For drm:
>>
>> * The variable_refresh_capable property is now flagged as DRM_MODE_PROP_IMMUTABLE
>>
>> For drm/gpu/amd/display:
>>
>> * Patches no longer pull in IOCTL/FreeSync refactoring code
>> * FreeSync enable/disable behavior has been modified to reflect changes in userspace behavior from xf86-video-amdgpu and mesa
>>
>> === Adaptive sync and variable refresh rate ===
>>
>> Adaptive sync is part of the DisplayPort spec and allows for graphics adapters to drive displays with varying frame timings.
>>
>> Variable refresh rate (VRR) is essentially the same, but defined for HDMI.
>>
>> === Use cases for variable refresh rate ===
>>
>> Variable frame (flip) timings don't align well with fixed refresh rate displays. This results in stuttering, tearing and/or input lag. By adjusting the display refresh rate dynamically these issues can be reduced or eliminated.
>>
>> However, not all content is suitable for dynamic refresh adaptation. Content that is flipped infrequently or at random intervals tends to fair poorly. Multiple clients trying to flip under the same screen can similarly interfere with prediction.
>>
>> Userland needs a way to let the driver know when the content on the screen is suitable for variable refresh rate and if the user wishes to have the feature enabled.
>>
>> === DRM API to support variable refresh rates ===
>>
>> This patch introduces a new API via atomic properties on the DRM connector and CRTC.
>>
>> The connector has two new optional properties:
>>
>> * bool variable_refresh_capable - set by the driver if the hardware is capable of supporting variable refresh tech
>>
>> * bool variable_refresh_enabled - set by the user to enable variable refresh adjustment over the connector
>>
>> The CRTC has one additional default property:
>>
>> * bool variable_refresh - a content hint to the driver specifying that the CRTC contents are suitable for variable refresh adjustment
>>
>> == Overview for DRM driver developers ===
>>
>> Driver developers can attach the optional connector properties via drm_connector_attach_variable_refresh_properties on connectors that support variable refresh (typically DP or HDMI).
>>
>> The variable_refresh_capable property should be managed as the output on the connector changes. The property is read only from userspace.
>>
>> The variable_refresh_enabled property is intended to be a property controlled by userland as a global on/off switch for variable refresh technology. It should be checked before enabling variable refresh rate.
>>
>> === Overview for Userland developers ==
>>
>> The variable_refresh property on the CRTC should be set to true when the CRTCs are suitable for variable refresh rate. In practice this is probably an application like a game - a single window that covers the whole CRTC surface and is the only client issuing flips.
>>
>> To demonstrate the suitability of the API for variable refresh and dynamic adaptation there are additional patches using this API that implement adaptive variable refresh across kernel and userland projects:
>>
>> * DRM (dri-devel)
>> * amdgpu DRM kernel driver (amd-gfx)
>> * xf86-video-amdgpu (amd-gfx)
>> * mesa (mesa-dev)
>>
>> These patches enable adaptive variable refresh on X for AMD hardware provided that the user sets the variable_refresh_enabled property to true on supported connectors (ie. using xrandr --set-prop).
>>
>> The patches have been tested as working on upstream userland with the GNOME desktop environment under a single monitor setup. They also work on KDE in single monitor setup if the compositor is disabled.
>>
>> The patches require that the application window can issue screen flips via the Present extension to xf86-video-amdgpu. Due to Present extension limitations some desktop environments and multi-monitor setups are currently not compatible.
>>
>> Full implementation details for these changes can be reviewed in their respective mailing lists.
>>
>> === Previous discussions ===
>>
>> These patches are based upon feedback from patches and feedback from two previous threads on the subject which are linked below for reference:
>>
>> https://lists.freedesktop.org/archives/amd-gfx/2018-April/021047.html
>> https://lists.freedesktop.org/archives/dri-devel/2017-October/155207.html
>> https://lists.freedesktop.org/archives/dri-devel/2018-September/189404.html
>>
>> Nicholas Kazlauskas
>>
>> Nicholas Kazlauskas (3):
>>   drm: Add variable refresh rate properties to connector
>>   drm: Add variable refresh property to DRM CRTC
>>   drm/amd/display: Set FreeSync state using DRM VRR properties
> 
> Please include Manasi manasi.d.navare@intel.com when resending, she's
> working on this from our side.
> 
> Also some overview kernel-docs that document the uapi aspect of how the
> prorties are driven should be included. Probably best if you add a new
> "Variable Refresh Rate" section under
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-properties
> 
> with links to functions drivers should call to set up and everything. Best
> practice is to stuff all that into a DOC: comment.
> 
> An igt testcase would be neat too.
> 

Thanks for bringing up docs and igt.

For igt if we expose a monitor's vmin/vmax through a debugfs we could then do vrr flips at different points within that range and measure the time until vblank notifications to check that they (roughly) align with the vtotal. Not sure how difficult that'd be with igt. Wonder if Arek has any ideas of a better approach.

Harry

> Cheers, Daniel
> 
>>
>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
>>  drivers/gpu/drm/drm_atomic_helper.c           |   1 +
>>  drivers/gpu/drm/drm_atomic_uapi.c             |  12 +
>>  drivers/gpu/drm/drm_connector.c               |  35 +++
>>  drivers/gpu/drm/drm_crtc.c                    |   2 +
>>  drivers/gpu/drm/drm_mode_config.c             |   6 +
>>  include/drm/drm_connector.h                   |  27 ++
>>  include/drm/drm_crtc.h                        |  13 +
>>  include/drm/drm_mode_config.h                 |   8 +
>>  10 files changed, 225 insertions(+), 117 deletions(-)
>>
>> -- 
>> 2.19.0
>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Mike Lothian]

[-- Attachment #1.1: Type: text/plain, Size: 6792 bytes --]

Hi

I'm curious to know whether this will/could work over PRIME

If the discrete card is rendering slower than the display's frequency could
the frequency be dropped on integrated display? I think there are laptops
that have freesync now

Cheers

Mike

On Mon, 1 Oct 2018 at 08:15 Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Sep 24, 2018 at 02:15:34PM -0400, Nicholas Kazlauskas wrote:
> > These patches are part of a proposed new interface for supporting
> variable refresh rate via DRM properties.
> >
> > === Changes from v1 ===
> >
> > For drm:
> >
> > * The variable_refresh_capable property is now flagged as
> DRM_MODE_PROP_IMMUTABLE
> >
> > For drm/gpu/amd/display:
> >
> > * Patches no longer pull in IOCTL/FreeSync refactoring code
> > * FreeSync enable/disable behavior has been modified to reflect changes
> in userspace behavior from xf86-video-amdgpu and mesa
> >
> > === Adaptive sync and variable refresh rate ===
> >
> > Adaptive sync is part of the DisplayPort spec and allows for graphics
> adapters to drive displays with varying frame timings.
> >
> > Variable refresh rate (VRR) is essentially the same, but defined for
> HDMI.
> >
> > === Use cases for variable refresh rate ===
> >
> > Variable frame (flip) timings don't align well with fixed refresh rate
> displays. This results in stuttering, tearing and/or input lag. By
> adjusting the display refresh rate dynamically these issues can be reduced
> or eliminated.
> >
> > However, not all content is suitable for dynamic refresh adaptation.
> Content that is flipped infrequently or at random intervals tends to fair
> poorly. Multiple clients trying to flip under the same screen can similarly
> interfere with prediction.
> >
> > Userland needs a way to let the driver know when the content on the
> screen is suitable for variable refresh rate and if the user wishes to have
> the feature enabled.
> >
> > === DRM API to support variable refresh rates ===
> >
> > This patch introduces a new API via atomic properties on the DRM
> connector and CRTC.
> >
> > The connector has two new optional properties:
> >
> > * bool variable_refresh_capable - set by the driver if the hardware is
> capable of supporting variable refresh tech
> >
> > * bool variable_refresh_enabled - set by the user to enable variable
> refresh adjustment over the connector
> >
> > The CRTC has one additional default property:
> >
> > * bool variable_refresh - a content hint to the driver specifying that
> the CRTC contents are suitable for variable refresh adjustment
> >
> > == Overview for DRM driver developers ===
> >
> > Driver developers can attach the optional connector properties via
> drm_connector_attach_variable_refresh_properties on connectors that support
> variable refresh (typically DP or HDMI).
> >
> > The variable_refresh_capable property should be managed as the output on
> the connector changes. The property is read only from userspace.
> >
> > The variable_refresh_enabled property is intended to be a property
> controlled by userland as a global on/off switch for variable refresh
> technology. It should be checked before enabling variable refresh rate.
> >
> > === Overview for Userland developers ==
> >
> > The variable_refresh property on the CRTC should be set to true when the
> CRTCs are suitable for variable refresh rate. In practice this is probably
> an application like a game - a single window that covers the whole CRTC
> surface and is the only client issuing flips.
> >
> > To demonstrate the suitability of the API for variable refresh and
> dynamic adaptation there are additional patches using this API that
> implement adaptive variable refresh across kernel and userland projects:
> >
> > * DRM (dri-devel)
> > * amdgpu DRM kernel driver (amd-gfx)
> > * xf86-video-amdgpu (amd-gfx)
> > * mesa (mesa-dev)
> >
> > These patches enable adaptive variable refresh on X for AMD hardware
> provided that the user sets the variable_refresh_enabled property to true
> on supported connectors (ie. using xrandr --set-prop).
> >
> > The patches have been tested as working on upstream userland with the
> GNOME desktop environment under a single monitor setup. They also work on
> KDE in single monitor setup if the compositor is disabled.
> >
> > The patches require that the application window can issue screen flips
> via the Present extension to xf86-video-amdgpu. Due to Present extension
> limitations some desktop environments and multi-monitor setups are
> currently not compatible.
> >
> > Full implementation details for these changes can be reviewed in their
> respective mailing lists.
> >
> > === Previous discussions ===
> >
> > These patches are based upon feedback from patches and feedback from two
> previous threads on the subject which are linked below for reference:
> >
> > https://lists.freedesktop.org/archives/amd-gfx/2018-April/021047.html
> >
> https://lists.freedesktop.org/archives/dri-devel/2017-October/155207.html
> >
> https://lists.freedesktop.org/archives/dri-devel/2018-September/189404.html
> >
> > Nicholas Kazlauskas
> >
> > Nicholas Kazlauskas (3):
> >   drm: Add variable refresh rate properties to connector
> >   drm: Add variable refresh property to DRM CRTC
> >   drm/amd/display: Set FreeSync state using DRM VRR properties
>
> Please include Manasi manasi.d.navare@intel.com when resending, she's
> working on this from our side.
>
> Also some overview kernel-docs that document the uapi aspect of how the
> prorties are driven should be included. Probably best if you add a new
> "Variable Refresh Rate" section under
>
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-properties
>
> with links to functions drivers should call to set up and everything. Best
> practice is to stuff all that into a DOC: comment.
>
> An igt testcase would be neat too.
>
> Cheers, Daniel
>
> >
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
> >  drivers/gpu/drm/drm_atomic_helper.c           |   1 +
> >  drivers/gpu/drm/drm_atomic_uapi.c             |  12 +
> >  drivers/gpu/drm/drm_connector.c               |  35 +++
> >  drivers/gpu/drm/drm_crtc.c                    |   2 +
> >  drivers/gpu/drm/drm_mode_config.c             |   6 +
> >  include/drm/drm_connector.h                   |  27 ++
> >  include/drm/drm_crtc.h                        |  13 +
> >  include/drm/drm_mode_config.h                 |   8 +
> >  10 files changed, 225 insertions(+), 117 deletions(-)
> >
> > --
> > 2.19.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 8665 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Daniel Vetter]

On Tue, Oct 02, 2018 at 10:49:17AM -0400, Harry Wentland wrote:
> 
> 
> On 2018-10-01 03:15 AM, Daniel Vetter wrote:
> > On Mon, Sep 24, 2018 at 02:15:34PM -0400, Nicholas Kazlauskas wrote:
> >> These patches are part of a proposed new interface for supporting variable refresh rate via DRM properties.
> >>
> >> === Changes from v1 ===
> >>
> >> For drm:
> >>
> >> * The variable_refresh_capable property is now flagged as DRM_MODE_PROP_IMMUTABLE
> >>
> >> For drm/gpu/amd/display:
> >>
> >> * Patches no longer pull in IOCTL/FreeSync refactoring code
> >> * FreeSync enable/disable behavior has been modified to reflect changes in userspace behavior from xf86-video-amdgpu and mesa
> >>
> >> === Adaptive sync and variable refresh rate ===
> >>
> >> Adaptive sync is part of the DisplayPort spec and allows for graphics adapters to drive displays with varying frame timings.
> >>
> >> Variable refresh rate (VRR) is essentially the same, but defined for HDMI.
> >>
> >> === Use cases for variable refresh rate ===
> >>
> >> Variable frame (flip) timings don't align well with fixed refresh rate displays. This results in stuttering, tearing and/or input lag. By adjusting the display refresh rate dynamically these issues can be reduced or eliminated.
> >>
> >> However, not all content is suitable for dynamic refresh adaptation. Content that is flipped infrequently or at random intervals tends to fair poorly. Multiple clients trying to flip under the same screen can similarly interfere with prediction.
> >>
> >> Userland needs a way to let the driver know when the content on the screen is suitable for variable refresh rate and if the user wishes to have the feature enabled.
> >>
> >> === DRM API to support variable refresh rates ===
> >>
> >> This patch introduces a new API via atomic properties on the DRM connector and CRTC.
> >>
> >> The connector has two new optional properties:
> >>
> >> * bool variable_refresh_capable - set by the driver if the hardware is capable of supporting variable refresh tech
> >>
> >> * bool variable_refresh_enabled - set by the user to enable variable refresh adjustment over the connector
> >>
> >> The CRTC has one additional default property:
> >>
> >> * bool variable_refresh - a content hint to the driver specifying that the CRTC contents are suitable for variable refresh adjustment
> >>
> >> == Overview for DRM driver developers ===
> >>
> >> Driver developers can attach the optional connector properties via drm_connector_attach_variable_refresh_properties on connectors that support variable refresh (typically DP or HDMI).
> >>
> >> The variable_refresh_capable property should be managed as the output on the connector changes. The property is read only from userspace.
> >>
> >> The variable_refresh_enabled property is intended to be a property controlled by userland as a global on/off switch for variable refresh technology. It should be checked before enabling variable refresh rate.
> >>
> >> === Overview for Userland developers ==
> >>
> >> The variable_refresh property on the CRTC should be set to true when the CRTCs are suitable for variable refresh rate. In practice this is probably an application like a game - a single window that covers the whole CRTC surface and is the only client issuing flips.
> >>
> >> To demonstrate the suitability of the API for variable refresh and dynamic adaptation there are additional patches using this API that implement adaptive variable refresh across kernel and userland projects:
> >>
> >> * DRM (dri-devel)
> >> * amdgpu DRM kernel driver (amd-gfx)
> >> * xf86-video-amdgpu (amd-gfx)
> >> * mesa (mesa-dev)
> >>
> >> These patches enable adaptive variable refresh on X for AMD hardware provided that the user sets the variable_refresh_enabled property to true on supported connectors (ie. using xrandr --set-prop).
> >>
> >> The patches have been tested as working on upstream userland with the GNOME desktop environment under a single monitor setup. They also work on KDE in single monitor setup if the compositor is disabled.
> >>
> >> The patches require that the application window can issue screen flips via the Present extension to xf86-video-amdgpu. Due to Present extension limitations some desktop environments and multi-monitor setups are currently not compatible.
> >>
> >> Full implementation details for these changes can be reviewed in their respective mailing lists.
> >>
> >> === Previous discussions ===
> >>
> >> These patches are based upon feedback from patches and feedback from two previous threads on the subject which are linked below for reference:
> >>
> >> https://lists.freedesktop.org/archives/amd-gfx/2018-April/021047.html
> >> https://lists.freedesktop.org/archives/dri-devel/2017-October/155207.html
> >> https://lists.freedesktop.org/archives/dri-devel/2018-September/189404.html
> >>
> >> Nicholas Kazlauskas
> >>
> >> Nicholas Kazlauskas (3):
> >>   drm: Add variable refresh rate properties to connector
> >>   drm: Add variable refresh property to DRM CRTC
> >>   drm/amd/display: Set FreeSync state using DRM VRR properties
> > 
> > Please include Manasi manasi.d.navare@intel.com when resending, she's
> > working on this from our side.
> > 
> > Also some overview kernel-docs that document the uapi aspect of how the
> > prorties are driven should be included. Probably best if you add a new
> > "Variable Refresh Rate" section under
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-properties
> > 
> > with links to functions drivers should call to set up and everything. Best
> > practice is to stuff all that into a DOC: comment.
> > 
> > An igt testcase would be neat too.
> > 
> 
> Thanks for bringing up docs and igt.
> 
> For igt if we expose a monitor's vmin/vmax through a debugfs we could
> then do vrr flips at different points within that range and measure the
> time until vblank notifications to check that they (roughly) align with
> the vtotal. Not sure how difficult that'd be with igt. Wonder if Arek
> has any ideas of a better approach.

We can do much better:

1. allocate a bunch of buffers
2. share with vgem
3. use vgem fake fences to perfectly control the "rendering" of your fake
workload
4. flip away, and check that vrr does what it's supposed to do.

Even more evil than vrr, this would test prime+vrr :-)

Plan b), but only works on intel: Use the magic spinning batch support we
have for i915.ko, where a dword write from the cpu stops the spinning, and
hence allows you to control how long the "rendering" takes very
accurately.

Neither needs driver hacks or debugfs.

Cheers, Daniel
> 
> Harry
> 
> > Cheers, Daniel
> > 
> >>
> >>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
> >>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
> >>  drivers/gpu/drm/drm_atomic_helper.c           |   1 +
> >>  drivers/gpu/drm/drm_atomic_uapi.c             |  12 +
> >>  drivers/gpu/drm/drm_connector.c               |  35 +++
> >>  drivers/gpu/drm/drm_crtc.c                    |   2 +
> >>  drivers/gpu/drm/drm_mode_config.c             |   6 +
> >>  include/drm/drm_connector.h                   |  27 ++
> >>  include/drm/drm_crtc.h                        |  13 +
> >>  include/drm/drm_mode_config.h                 |   8 +
> >>  10 files changed, 225 insertions(+), 117 deletions(-)
> >>
> >> -- 
> >> 2.19.0
> >>
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Manasi Navare]

On Wed, Oct 03, 2018 at 10:41:20AM +0200, Daniel Vetter wrote:
> On Tue, Oct 02, 2018 at 10:49:17AM -0400, Harry Wentland wrote:
> > 
> > 
> > On 2018-10-01 03:15 AM, Daniel Vetter wrote:
> > > On Mon, Sep 24, 2018 at 02:15:34PM -0400, Nicholas Kazlauskas wrote:
> > >> These patches are part of a proposed new interface for supporting variable refresh rate via DRM properties.
> > >>
> > >> === Changes from v1 ===
> > >>
> > >> For drm:
> > >>
> > >> * The variable_refresh_capable property is now flagged as DRM_MODE_PROP_IMMUTABLE
> > >>
> > >> For drm/gpu/amd/display:
> > >>
> > >> * Patches no longer pull in IOCTL/FreeSync refactoring code
> > >> * FreeSync enable/disable behavior has been modified to reflect changes in userspace behavior from xf86-video-amdgpu and mesa
> > >>
> > >> === Adaptive sync and variable refresh rate ===
> > >>
> > >> Adaptive sync is part of the DisplayPort spec and allows for graphics adapters to drive displays with varying frame timings.
> > >>
> > >> Variable refresh rate (VRR) is essentially the same, but defined for HDMI.
> > >>
> > >> === Use cases for variable refresh rate ===
> > >>
> > >> Variable frame (flip) timings don't align well with fixed refresh rate displays. This results in stuttering, tearing and/or input lag. By adjusting the display refresh rate dynamically these issues can be reduced or eliminated.
> > >>
> > >> However, not all content is suitable for dynamic refresh adaptation. Content that is flipped infrequently or at random intervals tends to fair poorly. Multiple clients trying to flip under the same screen can similarly interfere with prediction.
> > >>
> > >> Userland needs a way to let the driver know when the content on the screen is suitable for variable refresh rate and if the user wishes to have the feature enabled.
> > >>
> > >> === DRM API to support variable refresh rates ===
> > >>
> > >> This patch introduces a new API via atomic properties on the DRM connector and CRTC.
> > >>
> > >> The connector has two new optional properties:
> > >>
> > >> * bool variable_refresh_capable - set by the driver if the hardware is capable of supporting variable refresh tech
> > >>
> > >> * bool variable_refresh_enabled - set by the user to enable variable refresh adjustment over the connector
> > >>
> > >> The CRTC has one additional default property:
> > >>
> > >> * bool variable_refresh - a content hint to the driver specifying that the CRTC contents are suitable for variable refresh adjustment
> > >>
> > >> == Overview for DRM driver developers ===
> > >>
> > >> Driver developers can attach the optional connector properties via drm_connector_attach_variable_refresh_properties on connectors that support variable refresh (typically DP or HDMI).
> > >>
> > >> The variable_refresh_capable property should be managed as the output on the connector changes. The property is read only from userspace.
> > >>
> > >> The variable_refresh_enabled property is intended to be a property controlled by userland as a global on/off switch for variable refresh technology. It should be checked before enabling variable refresh rate.
> > >>
> > >> === Overview for Userland developers ==
> > >>
> > >> The variable_refresh property on the CRTC should be set to true when the CRTCs are suitable for variable refresh rate. In practice this is probably an application like a game - a single window that covers the whole CRTC surface and is the only client issuing flips.
> > >>
> > >> To demonstrate the suitability of the API for variable refresh and dynamic adaptation there are additional patches using this API that implement adaptive variable refresh across kernel and userland projects:
> > >>
> > >> * DRM (dri-devel)
> > >> * amdgpu DRM kernel driver (amd-gfx)
> > >> * xf86-video-amdgpu (amd-gfx)
> > >> * mesa (mesa-dev)
> > >>
> > >> These patches enable adaptive variable refresh on X for AMD hardware provided that the user sets the variable_refresh_enabled property to true on supported connectors (ie. using xrandr --set-prop).
> > >>
> > >> The patches have been tested as working on upstream userland with the GNOME desktop environment under a single monitor setup. They also work on KDE in single monitor setup if the compositor is disabled.
> > >>
> > >> The patches require that the application window can issue screen flips via the Present extension to xf86-video-amdgpu. Due to Present extension limitations some desktop environments and multi-monitor setups are currently not compatible.
> > >>
> > >> Full implementation details for these changes can be reviewed in their respective mailing lists.
> > >>
> > >> === Previous discussions ===
> > >>
> > >> These patches are based upon feedback from patches and feedback from two previous threads on the subject which are linked below for reference:
> > >>
> > >> https://lists.freedesktop.org/archives/amd-gfx/2018-April/021047.html
> > >> https://lists.freedesktop.org/archives/dri-devel/2017-October/155207.html
> > >> https://lists.freedesktop.org/archives/dri-devel/2018-September/189404.html
> > >>
> > >> Nicholas Kazlauskas
> > >>
> > >> Nicholas Kazlauskas (3):
> > >>   drm: Add variable refresh rate properties to connector
> > >>   drm: Add variable refresh property to DRM CRTC
> > >>   drm/amd/display: Set FreeSync state using DRM VRR properties
> > > 
> > > Please include Manasi manasi.d.navare@intel.com when resending, she's
> > > working on this from our side.
> > > 
> > > Also some overview kernel-docs that document the uapi aspect of how the
> > > prorties are driven should be included. Probably best if you add a new
> > > "Variable Refresh Rate" section under
> > > 
> > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-properties
> > > 
> > > with links to functions drivers should call to set up and everything. Best
> > > practice is to stuff all that into a DOC: comment.
> > > 
> > > An igt testcase would be neat too.
> > > 
> > 
> > Thanks for bringing up docs and igt.
> > 
> > For igt if we expose a monitor's vmin/vmax through a debugfs we could
> > then do vrr flips at different points within that range and measure the
> > time until vblank notifications to check that they (roughly) align with
> > the vtotal. Not sure how difficult that'd be with igt. Wonder if Arek
> > has any ideas of a better approach.
> 
> We can do much better:
> 
> 1. allocate a bunch of buffers
> 2. share with vgem
> 3. use vgem fake fences to perfectly control the "rendering" of your fake
> workload
> 4. flip away, and check that vrr does what it's supposed to do.
> 
> Even more evil than vrr, this would test prime+vrr :-)
> 
> Plan b), but only works on intel: Use the magic spinning batch support we
> have for i915.ko, where a dword write from the cpu stops the spinning, and
> hence allows you to control how long the "rendering" takes very
> accurately.
> 
> Neither needs driver hacks or debugfs.
> 
> Cheers, Daniel

I feel like adding debugfs information in general about VRR capabilities
like the VRR range supported by the panel is a good idea to invoke VRR
for various refresh rates from IGT or real applications and also
to be able to do checks for whether the actual vblank happened within the maximum
blanking interval supported.

And we already parse that information in the driver to set the Vblank min and max values
for the registers so easy to expose that as well through debugfs.

Manasi

> > 
> > Harry
> > 
> > > Cheers, Daniel
> > > 
> > >>
> > >>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
> > >>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
> > >>  drivers/gpu/drm/drm_atomic_helper.c           |   1 +
> > >>  drivers/gpu/drm/drm_atomic_uapi.c             |  12 +
> > >>  drivers/gpu/drm/drm_connector.c               |  35 +++
> > >>  drivers/gpu/drm/drm_crtc.c                    |   2 +
> > >>  drivers/gpu/drm/drm_mode_config.c             |   6 +
> > >>  include/drm/drm_connector.h                   |  27 ++
> > >>  include/drm/drm_crtc.h                        |  13 +
> > >>  include/drm/drm_mode_config.h                 |   8 +
> > >>  10 files changed, 225 insertions(+), 117 deletions(-)
> > >>
> > >> -- 
> > >> 2.19.0
> > >>
> > > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 11380 bytes --]

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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Kazlauskas, Nicholas]

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Kazlauskas, Nicholas]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

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.

-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 7720 bytes --]

On Fri, 5 Oct 2018 12:21:20 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org> 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-5C7GfCeVMHo@public.gmane.org> 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-5C7GfCeVMHo@public.gmane.org>  

...

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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

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

[Kazlauskas, Nicholas]

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Harry Wentland]

On 2018-10-03 02:35 PM, Manasi Navare wrote:
> On Wed, Oct 03, 2018 at 10:41:20AM +0200, Daniel Vetter wrote:
>> On Tue, Oct 02, 2018 at 10:49:17AM -0400, Harry Wentland wrote:
>>>
>>>
>>> On 2018-10-01 03:15 AM, Daniel Vetter wrote:
>>>> On Mon, Sep 24, 2018 at 02:15:34PM -0400, Nicholas Kazlauskas wrote:
>>>>> These patches are part of a proposed new interface for supporting variable refresh rate via DRM properties.
>>>>>
>>>>> === Changes from v1 ===
>>>>>
>>>>> For drm:
>>>>>
>>>>> * The variable_refresh_capable property is now flagged as DRM_MODE_PROP_IMMUTABLE
>>>>>
>>>>> For drm/gpu/amd/display:
>>>>>
>>>>> * Patches no longer pull in IOCTL/FreeSync refactoring code
>>>>> * FreeSync enable/disable behavior has been modified to reflect changes in userspace behavior from xf86-video-amdgpu and mesa
>>>>>
>>>>> === Adaptive sync and variable refresh rate ===
>>>>>
>>>>> Adaptive sync is part of the DisplayPort spec and allows for graphics adapters to drive displays with varying frame timings.
>>>>>
>>>>> Variable refresh rate (VRR) is essentially the same, but defined for HDMI.
>>>>>
>>>>> === Use cases for variable refresh rate ===
>>>>>
>>>>> Variable frame (flip) timings don't align well with fixed refresh rate displays. This results in stuttering, tearing and/or input lag. By adjusting the display refresh rate dynamically these issues can be reduced or eliminated.
>>>>>
>>>>> However, not all content is suitable for dynamic refresh adaptation. Content that is flipped infrequently or at random intervals tends to fair poorly. Multiple clients trying to flip under the same screen can similarly interfere with prediction.
>>>>>
>>>>> Userland needs a way to let the driver know when the content on the screen is suitable for variable refresh rate and if the user wishes to have the feature enabled.
>>>>>
>>>>> === DRM API to support variable refresh rates ===
>>>>>
>>>>> This patch introduces a new API via atomic properties on the DRM connector and CRTC.
>>>>>
>>>>> The connector has two new optional properties:
>>>>>
>>>>> * bool variable_refresh_capable - set by the driver if the hardware is capable of supporting variable refresh tech
>>>>>
>>>>> * bool variable_refresh_enabled - set by the user to enable variable refresh adjustment over the connector
>>>>>
>>>>> The CRTC has one additional default property:
>>>>>
>>>>> * bool variable_refresh - a content hint to the driver specifying that the CRTC contents are suitable for variable refresh adjustment
>>>>>
>>>>> == Overview for DRM driver developers ===
>>>>>
>>>>> Driver developers can attach the optional connector properties via drm_connector_attach_variable_refresh_properties on connectors that support variable refresh (typically DP or HDMI).
>>>>>
>>>>> The variable_refresh_capable property should be managed as the output on the connector changes. The property is read only from userspace.
>>>>>
>>>>> The variable_refresh_enabled property is intended to be a property controlled by userland as a global on/off switch for variable refresh technology. It should be checked before enabling variable refresh rate.
>>>>>
>>>>> === Overview for Userland developers ==
>>>>>
>>>>> The variable_refresh property on the CRTC should be set to true when the CRTCs are suitable for variable refresh rate. In practice this is probably an application like a game - a single window that covers the whole CRTC surface and is the only client issuing flips.
>>>>>
>>>>> To demonstrate the suitability of the API for variable refresh and dynamic adaptation there are additional patches using this API that implement adaptive variable refresh across kernel and userland projects:
>>>>>
>>>>> * DRM (dri-devel)
>>>>> * amdgpu DRM kernel driver (amd-gfx)
>>>>> * xf86-video-amdgpu (amd-gfx)
>>>>> * mesa (mesa-dev)
>>>>>
>>>>> These patches enable adaptive variable refresh on X for AMD hardware provided that the user sets the variable_refresh_enabled property to true on supported connectors (ie. using xrandr --set-prop).
>>>>>
>>>>> The patches have been tested as working on upstream userland with the GNOME desktop environment under a single monitor setup. They also work on KDE in single monitor setup if the compositor is disabled.
>>>>>
>>>>> The patches require that the application window can issue screen flips via the Present extension to xf86-video-amdgpu. Due to Present extension limitations some desktop environments and multi-monitor setups are currently not compatible.
>>>>>
>>>>> Full implementation details for these changes can be reviewed in their respective mailing lists.
>>>>>
>>>>> === Previous discussions ===
>>>>>
>>>>> These patches are based upon feedback from patches and feedback from two previous threads on the subject which are linked below for reference:
>>>>>
>>>>> https://lists.freedesktop.org/archives/amd-gfx/2018-April/021047.html
>>>>> https://lists.freedesktop.org/archives/dri-devel/2017-October/155207.html
>>>>> https://lists.freedesktop.org/archives/dri-devel/2018-September/189404.html
>>>>>
>>>>> Nicholas Kazlauskas
>>>>>
>>>>> Nicholas Kazlauskas (3):
>>>>>   drm: Add variable refresh rate properties to connector
>>>>>   drm: Add variable refresh property to DRM CRTC
>>>>>   drm/amd/display: Set FreeSync state using DRM VRR properties
>>>>
>>>> Please include Manasi manasi.d.navare@intel.com when resending, she's
>>>> working on this from our side.
>>>>
>>>> Also some overview kernel-docs that document the uapi aspect of how the
>>>> prorties are driven should be included. Probably best if you add a new
>>>> "Variable Refresh Rate" section under
>>>>
>>>> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-properties
>>>>
>>>> with links to functions drivers should call to set up and everything. Best
>>>> practice is to stuff all that into a DOC: comment.
>>>>
>>>> An igt testcase would be neat too.
>>>>
>>>
>>> Thanks for bringing up docs and igt.
>>>
>>> For igt if we expose a monitor's vmin/vmax through a debugfs we could
>>> then do vrr flips at different points within that range and measure the
>>> time until vblank notifications to check that they (roughly) align with
>>> the vtotal. Not sure how difficult that'd be with igt. Wonder if Arek
>>> has any ideas of a better approach.
>>
>> We can do much better:
>>
>> 1. allocate a bunch of buffers
>> 2. share with vgem
>> 3. use vgem fake fences to perfectly control the "rendering" of your fake
>> workload
>> 4. flip away, and check that vrr does what it's supposed to do.
>>
>> Even more evil than vrr, this would test prime+vrr :-)
>>
>> Plan b), but only works on intel: Use the magic spinning batch support we
>> have for i915.ko, where a dword write from the cpu stops the spinning, and
>> hence allows you to control how long the "rendering" takes very
>> accurately.
>>
>> Neither needs driver hacks or debugfs.
>>
>> Cheers, Daniel
> 
> I feel like adding debugfs information in general about VRR capabilities
> like the VRR range supported by the panel is a good idea to invoke VRR
> for various refresh rates from IGT or real applications and also
> to be able to do checks for whether the actual vblank happened within the maximum
> blanking interval supported.
> 

I think it's good to have for testing and debugging purposes.

Harry

> And we already parse that information in the driver to set the Vblank min and max values
> for the registers so easy to expose that as well through debugfs.
> 
> Manasi
> 
>>>
>>> Harry
>>>
>>>> Cheers, Daniel
>>>>
>>>>>
>>>>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
>>>>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
>>>>>  drivers/gpu/drm/drm_atomic_helper.c           |   1 +
>>>>>  drivers/gpu/drm/drm_atomic_uapi.c             |  12 +
>>>>>  drivers/gpu/drm/drm_connector.c               |  35 +++
>>>>>  drivers/gpu/drm/drm_crtc.c                    |   2 +
>>>>>  drivers/gpu/drm/drm_mode_config.c             |   6 +
>>>>>  include/drm/drm_connector.h                   |  27 ++
>>>>>  include/drm/drm_crtc.h                        |  13 +
>>>>>  include/drm/drm_mode_config.h                 |   8 +
>>>>>  10 files changed, 225 insertions(+), 117 deletions(-)
>>>>>
>>>>> -- 
>>>>> 2.19.0
>>>>>
>>>>
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Harry Wentland]

On 2018-10-03 04:41 AM, Daniel Vetter wrote:
> On Tue, Oct 02, 2018 at 10:49:17AM -0400, Harry Wentland wrote:
>>
>>
>> On 2018-10-01 03:15 AM, Daniel Vetter wrote:
>>> On Mon, Sep 24, 2018 at 02:15:34PM -0400, Nicholas Kazlauskas wrote:
>>>> These patches are part of a proposed new interface for supporting variable refresh rate via DRM properties.
>>>>
>>>> === Changes from v1 ===
>>>>
>>>> For drm:
>>>>
>>>> * The variable_refresh_capable property is now flagged as DRM_MODE_PROP_IMMUTABLE
>>>>
>>>> For drm/gpu/amd/display:
>>>>
>>>> * Patches no longer pull in IOCTL/FreeSync refactoring code
>>>> * FreeSync enable/disable behavior has been modified to reflect changes in userspace behavior from xf86-video-amdgpu and mesa
>>>>
>>>> === Adaptive sync and variable refresh rate ===
>>>>
>>>> Adaptive sync is part of the DisplayPort spec and allows for graphics adapters to drive displays with varying frame timings.
>>>>
>>>> Variable refresh rate (VRR) is essentially the same, but defined for HDMI.
>>>>
>>>> === Use cases for variable refresh rate ===
>>>>
>>>> Variable frame (flip) timings don't align well with fixed refresh rate displays. This results in stuttering, tearing and/or input lag. By adjusting the display refresh rate dynamically these issues can be reduced or eliminated.
>>>>
>>>> However, not all content is suitable for dynamic refresh adaptation. Content that is flipped infrequently or at random intervals tends to fair poorly. Multiple clients trying to flip under the same screen can similarly interfere with prediction.
>>>>
>>>> Userland needs a way to let the driver know when the content on the screen is suitable for variable refresh rate and if the user wishes to have the feature enabled.
>>>>
>>>> === DRM API to support variable refresh rates ===
>>>>
>>>> This patch introduces a new API via atomic properties on the DRM connector and CRTC.
>>>>
>>>> The connector has two new optional properties:
>>>>
>>>> * bool variable_refresh_capable - set by the driver if the hardware is capable of supporting variable refresh tech
>>>>
>>>> * bool variable_refresh_enabled - set by the user to enable variable refresh adjustment over the connector
>>>>
>>>> The CRTC has one additional default property:
>>>>
>>>> * bool variable_refresh - a content hint to the driver specifying that the CRTC contents are suitable for variable refresh adjustment
>>>>
>>>> == Overview for DRM driver developers ===
>>>>
>>>> Driver developers can attach the optional connector properties via drm_connector_attach_variable_refresh_properties on connectors that support variable refresh (typically DP or HDMI).
>>>>
>>>> The variable_refresh_capable property should be managed as the output on the connector changes. The property is read only from userspace.
>>>>
>>>> The variable_refresh_enabled property is intended to be a property controlled by userland as a global on/off switch for variable refresh technology. It should be checked before enabling variable refresh rate.
>>>>
>>>> === Overview for Userland developers ==
>>>>
>>>> The variable_refresh property on the CRTC should be set to true when the CRTCs are suitable for variable refresh rate. In practice this is probably an application like a game - a single window that covers the whole CRTC surface and is the only client issuing flips.
>>>>
>>>> To demonstrate the suitability of the API for variable refresh and dynamic adaptation there are additional patches using this API that implement adaptive variable refresh across kernel and userland projects:
>>>>
>>>> * DRM (dri-devel)
>>>> * amdgpu DRM kernel driver (amd-gfx)
>>>> * xf86-video-amdgpu (amd-gfx)
>>>> * mesa (mesa-dev)
>>>>
>>>> These patches enable adaptive variable refresh on X for AMD hardware provided that the user sets the variable_refresh_enabled property to true on supported connectors (ie. using xrandr --set-prop).
>>>>
>>>> The patches have been tested as working on upstream userland with the GNOME desktop environment under a single monitor setup. They also work on KDE in single monitor setup if the compositor is disabled.
>>>>
>>>> The patches require that the application window can issue screen flips via the Present extension to xf86-video-amdgpu. Due to Present extension limitations some desktop environments and multi-monitor setups are currently not compatible.
>>>>
>>>> Full implementation details for these changes can be reviewed in their respective mailing lists.
>>>>
>>>> === Previous discussions ===
>>>>
>>>> These patches are based upon feedback from patches and feedback from two previous threads on the subject which are linked below for reference:
>>>>
>>>> https://lists.freedesktop.org/archives/amd-gfx/2018-April/021047.html
>>>> https://lists.freedesktop.org/archives/dri-devel/2017-October/155207.html
>>>> https://lists.freedesktop.org/archives/dri-devel/2018-September/189404.html
>>>>
>>>> Nicholas Kazlauskas
>>>>
>>>> Nicholas Kazlauskas (3):
>>>>   drm: Add variable refresh rate properties to connector
>>>>   drm: Add variable refresh property to DRM CRTC
>>>>   drm/amd/display: Set FreeSync state using DRM VRR properties
>>>
>>> Please include Manasi manasi.d.navare@intel.com when resending, she's
>>> working on this from our side.
>>>
>>> Also some overview kernel-docs that document the uapi aspect of how the
>>> prorties are driven should be included. Probably best if you add a new
>>> "Variable Refresh Rate" section under
>>>
>>> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-properties
>>>
>>> with links to functions drivers should call to set up and everything. Best
>>> practice is to stuff all that into a DOC: comment.
>>>
>>> An igt testcase would be neat too.
>>>
>>
>> Thanks for bringing up docs and igt.
>>
>> For igt if we expose a monitor's vmin/vmax through a debugfs we could
>> then do vrr flips at different points within that range and measure the
>> time until vblank notifications to check that they (roughly) align with
>> the vtotal. Not sure how difficult that'd be with igt. Wonder if Arek
>> has any ideas of a better approach.
> 
> We can do much better:
> 
> 1. allocate a bunch of buffers
> 2. share with vgem
> 3. use vgem fake fences to perfectly control the "rendering" of your fake
> workload
> 4. flip away, and check that vrr does what it's supposed to do.
> 
> Even more evil than vrr, this would test prime+vrr :-)
> 

Not really familiar with vgem and prime but this sounds like a good approach.

> Plan b), but only works on intel: Use the magic spinning batch support we
> have for i915.ko, where a dword write from the cpu stops the spinning, and
> hence allows you to control how long the "rendering" takes very
> accurately.
> 

What's the "magic spinning batch support" and where can I find out more about it.

This could be very useful for VRR debug and testing.

Harry

> Neither needs driver hacks or debugfs.
> 
> Cheers, Daniel
>>
>> Harry
>>
>>> Cheers, Daniel
>>>
>>>>
>>>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
>>>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
>>>>  drivers/gpu/drm/drm_atomic_helper.c           |   1 +
>>>>  drivers/gpu/drm/drm_atomic_uapi.c             |  12 +
>>>>  drivers/gpu/drm/drm_connector.c               |  35 +++
>>>>  drivers/gpu/drm/drm_crtc.c                    |   2 +
>>>>  drivers/gpu/drm/drm_mode_config.c             |   6 +
>>>>  include/drm/drm_connector.h                   |  27 ++
>>>>  include/drm/drm_crtc.h                        |  13 +
>>>>  include/drm/drm_mode_config.h                 |   8 +
>>>>  10 files changed, 225 insertions(+), 117 deletions(-)
>>>>
>>>> -- 
>>>> 2.19.0
>>>>
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Harry Wentland]

On 2018-10-03 04:25 AM, Mike Lothian wrote:
> Hi
> 
> I'm curious to know whether this will/could work over PRIME
> 

I don't see why this shouldn't work over PRIME as long as the presenting GPU supports the new variable refresh rate API, but I know very little about prime, so maybe someone else can chime in and give a better opinion.

Harry

> If the discrete card is rendering slower than the display's frequency could the frequency be dropped on integrated display? I think there are laptops that have freesync now
> 
> Cheers
> 
> Mike
> 
> On Mon, 1 Oct 2018 at 08:15 Daniel Vetter <daniel@ffwll.ch <mailto:daniel@ffwll.ch>> wrote:
> 
>     On Mon, Sep 24, 2018 at 02:15:34PM -0400, Nicholas Kazlauskas wrote:
>     > These patches are part of a proposed new interface for supporting variable refresh rate via DRM properties.
>     >
>     > === Changes from v1 ===
>     >
>     > For drm:
>     >
>     > * The variable_refresh_capable property is now flagged as DRM_MODE_PROP_IMMUTABLE
>     >
>     > For drm/gpu/amd/display:
>     >
>     > * Patches no longer pull in IOCTL/FreeSync refactoring code
>     > * FreeSync enable/disable behavior has been modified to reflect changes in userspace behavior from xf86-video-amdgpu and mesa
>     >
>     > === Adaptive sync and variable refresh rate ===
>     >
>     > Adaptive sync is part of the DisplayPort spec and allows for graphics adapters to drive displays with varying frame timings.
>     >
>     > Variable refresh rate (VRR) is essentially the same, but defined for HDMI.
>     >
>     > === Use cases for variable refresh rate ===
>     >
>     > Variable frame (flip) timings don't align well with fixed refresh rate displays. This results in stuttering, tearing and/or input lag. By adjusting the display refresh rate dynamically these issues can be reduced or eliminated.
>     >
>     > However, not all content is suitable for dynamic refresh adaptation. Content that is flipped infrequently or at random intervals tends to fair poorly. Multiple clients trying to flip under the same screen can similarly interfere with prediction.
>     >
>     > Userland needs a way to let the driver know when the content on the screen is suitable for variable refresh rate and if the user wishes to have the feature enabled.
>     >
>     > === DRM API to support variable refresh rates ===
>     >
>     > This patch introduces a new API via atomic properties on the DRM connector and CRTC.
>     >
>     > The connector has two new optional properties:
>     >
>     > * bool variable_refresh_capable - set by the driver if the hardware is capable of supporting variable refresh tech
>     >
>     > * bool variable_refresh_enabled - set by the user to enable variable refresh adjustment over the connector
>     >
>     > The CRTC has one additional default property:
>     >
>     > * bool variable_refresh - a content hint to the driver specifying that the CRTC contents are suitable for variable refresh adjustment
>     >
>     > == Overview for DRM driver developers ===
>     >
>     > Driver developers can attach the optional connector properties via drm_connector_attach_variable_refresh_properties on connectors that support variable refresh (typically DP or HDMI).
>     >
>     > The variable_refresh_capable property should be managed as the output on the connector changes. The property is read only from userspace.
>     >
>     > The variable_refresh_enabled property is intended to be a property controlled by userland as a global on/off switch for variable refresh technology. It should be checked before enabling variable refresh rate.
>     >
>     > === Overview for Userland developers ==
>     >
>     > The variable_refresh property on the CRTC should be set to true when the CRTCs are suitable for variable refresh rate. In practice this is probably an application like a game - a single window that covers the whole CRTC surface and is the only client issuing flips.
>     >
>     > To demonstrate the suitability of the API for variable refresh and dynamic adaptation there are additional patches using this API that implement adaptive variable refresh across kernel and userland projects:
>     >
>     > * DRM (dri-devel)
>     > * amdgpu DRM kernel driver (amd-gfx)
>     > * xf86-video-amdgpu (amd-gfx)
>     > * mesa (mesa-dev)
>     >
>     > These patches enable adaptive variable refresh on X for AMD hardware provided that the user sets the variable_refresh_enabled property to true on supported connectors (ie. using xrandr --set-prop).
>     >
>     > The patches have been tested as working on upstream userland with the GNOME desktop environment under a single monitor setup. They also work on KDE in single monitor setup if the compositor is disabled.
>     >
>     > The patches require that the application window can issue screen flips via the Present extension to xf86-video-amdgpu. Due to Present extension limitations some desktop environments and multi-monitor setups are currently not compatible.
>     >
>     > Full implementation details for these changes can be reviewed in their respective mailing lists.
>     >
>     > === Previous discussions ===
>     >
>     > These patches are based upon feedback from patches and feedback from two previous threads on the subject which are linked below for reference:
>     >
>     > https://lists.freedesktop.org/archives/amd-gfx/2018-April/021047.html
>     > https://lists.freedesktop.org/archives/dri-devel/2017-October/155207.html
>     > https://lists.freedesktop.org/archives/dri-devel/2018-September/189404.html
>     >
>     > Nicholas Kazlauskas
>     >
>     > Nicholas Kazlauskas (3):
>     >   drm: Add variable refresh rate properties to connector
>     >   drm: Add variable refresh property to DRM CRTC
>     >   drm/amd/display: Set FreeSync state using DRM VRR properties
> 
>     Please include Manasi manasi.d.navare@intel.com <mailto:manasi.d.navare@intel.com> when resending, she's
>     working on this from our side.
> 
>     Also some overview kernel-docs that document the uapi aspect of how the
>     prorties are driven should be included. Probably best if you add a new
>     "Variable Refresh Rate" section under
> 
>     https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-properties
> 
>     with links to functions drivers should call to set up and everything. Best
>     practice is to stuff all that into a DOC: comment.
> 
>     An igt testcase would be neat too.
> 
>     Cheers, Daniel
> 
>     >
>     >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 232 +++++++++---------
>     >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   6 +-
>     >  drivers/gpu/drm/drm_atomic_helper.c           |   1 +
>     >  drivers/gpu/drm/drm_atomic_uapi.c             |  12 +
>     >  drivers/gpu/drm/drm_connector.c               |  35 +++
>     >  drivers/gpu/drm/drm_crtc.c                    |   2 +
>     >  drivers/gpu/drm/drm_mode_config.c             |   6 +
>     >  include/drm/drm_connector.h                   |  27 ++
>     >  include/drm/drm_crtc.h                        |  13 +
>     >  include/drm/drm_mode_config.h                 |   8 +
>     >  10 files changed, 225 insertions(+), 117 deletions(-)
>     >
>     > --
>     > 2.19.0
>     >
> 
>     -- 
>     Daniel Vetter
>     Software Engineer, Intel Corporation
>     http://blog.ffwll.ch
>     _______________________________________________
>     dri-devel mailing list
>     dri-devel@lists.freedesktop.org <mailto:dri-devel@lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 2648 bytes --]

On Wed, 10 Oct 2018 09:35:50 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org> 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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

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

[Koenig, Christian]

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

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

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Michel Dänzer]

On 2018-10-11 9:44 p.m., Harry Wentland wrote:
> On 2018-10-03 04:25 AM, Mike Lothian wrote:
>> 
>> I'm curious to know whether this will/could work over PRIME
> 
> I don't see why this shouldn't work over PRIME as long as the
> presenting GPU supports the new variable refresh rate API, but I know
> very little about prime, so maybe someone else can chime in and give
> a better opinion.

It won't work for displays connected to a secondary GPU, because those
aren't hooked up to the Present extension directly.

It can theoretically work with render offloading, if the primary GPU can
scan out directly from the shared pixmap. This is only possible with
current AMD APUs which support scatter/gather scanout (Carrizo and
newer?). One gotcha is that xf86-video-amdgpu currently doesn't allow
flipping between buffers with different tiling parameters (BTW Harry,
does that work with DC?), but that should be easy to fix.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Koenig, Christian]

Am 12.10.2018 um 10:26 schrieb Michel Dänzer:
> On 2018-10-11 9:44 p.m., Harry Wentland wrote:
>> On 2018-10-03 04:25 AM, Mike Lothian wrote:
>>> I'm curious to know whether this will/could work over PRIME
>> I don't see why this shouldn't work over PRIME as long as the
>> presenting GPU supports the new variable refresh rate API, but I know
>> very little about prime, so maybe someone else can chime in and give
>> a better opinion.
> It won't work for displays connected to a secondary GPU, because those
> aren't hooked up to the Present extension directly.
>
> It can theoretically work with render offloading, if the primary GPU can
> scan out directly from the shared pixmap. This is only possible with
> current AMD APUs which support scatter/gather scanout (Carrizo and
> newer?).

Actually only Carizzo and Stoney at the moment because this is buggy on 
Raven. Not sure if that is a pure software or a hardware problem.

Christian.

> One gotcha is that xf86-video-amdgpu currently doesn't allow
> flipping between buffers with different tiling parameters (BTW Harry,
> does that work with DC?), but that should be easy to fix.


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

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 2984 bytes --]

On Fri, 12 Oct 2018 07:35:57 +0000
"Koenig, Christian" <Christian.Koenig-5C7GfCeVMHo@public.gmane.org> wrote:

> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen:
> > On Wed, 10 Oct 2018 09:35:50 -0400
> > "Kazlauskas, Nicholas" <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org> 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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

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

[Koenig, Christian]

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

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

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

[Kazlauskas, Nicholas]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 7292 bytes --]

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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Kazlauskas, Nicholas]

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 6723 bytes --]

On Mon, 15 Oct 2018 12:02:14 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org> wrote:

> On 10/15/2018 09:57 AM, Pekka Paalanen wrote:
> > On Fri, 12 Oct 2018 08:58:23 -0400
> > "Kazlauskas, Nicholas" <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org> 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-5C7GfCeVMHo@public.gmane.org> wrote:
> >>>>     
> >>>>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen:  
> >>>>>> On Wed, 10 Oct 2018 09:35:50 -0400
> >>>>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org> 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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Wentland, Harry]

On 2018-10-12 4:31 a.m., Koenig, Christian wrote:
> Am 12.10.2018 um 10:26 schrieb Michel Dänzer:
>> On 2018-10-11 9:44 p.m., Harry Wentland wrote:
>>> On 2018-10-03 04:25 AM, Mike Lothian wrote:
>>>> I'm curious to know whether this will/could work over PRIME
>>> I don't see why this shouldn't work over PRIME as long as the
>>> presenting GPU supports the new variable refresh rate API, but I know
>>> very little about prime, so maybe someone else can chime in and give
>>> a better opinion.
>> It won't work for displays connected to a secondary GPU, because those
>> aren't hooked up to the Present extension directly.
>>
>> It can theoretically work with render offloading, if the primary GPU can
>> scan out directly from the shared pixmap. This is only possible with
>> current AMD APUs which support scatter/gather scanout (Carrizo and
>> newer?).
> 
> Actually only Carizzo and Stoney at the moment because this is buggy on 
> Raven. Not sure if that is a pure software or a hardware problem.
> 
> Christian.
> 
>> One gotcha is that xf86-video-amdgpu currently doesn't allow
>> flipping between buffers with different tiling parameters (BTW Harry,
>> does that work with DC?), but that should be easy to fix.
> 

Not currently. Fixable, but unfortunately not as easy as I'd hope. The good news is that I'm planning to rework that code so it would be easy to fix or should just happen on a per-flip basis.

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

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Michel Dänzer]

On 2018-10-25 7:57 p.m., Wentland, Harry wrote:
> On 2018-10-12 4:31 a.m., Koenig, Christian wrote:
>> Am 12.10.2018 um 10:26 schrieb Michel Dänzer:
>>> On 2018-10-11 9:44 p.m., Harry Wentland wrote:
>>>> On 2018-10-03 04:25 AM, Mike Lothian wrote:
>>>>> I'm curious to know whether this will/could work over PRIME
>>>> I don't see why this shouldn't work over PRIME as long as the 
>>>> presenting GPU supports the new variable refresh rate API, but
>>>> I know very little about prime, so maybe someone else can chime
>>>> in and give a better opinion.
>>> It won't work for displays connected to a secondary GPU, because
>>> those aren't hooked up to the Present extension directly.
>>> 
>>> It can theoretically work with render offloading, if the primary
>>> GPU can scan out directly from the shared pixmap. This is only
>>> possible with current AMD APUs which support scatter/gather
>>> scanout (Carrizo and newer?).
>> 
>> Actually only Carizzo and Stoney at the moment because this is
>> buggy on Raven. Not sure if that is a pure software or a hardware
>> problem.
>> 
>> Christian.
>> 
>>> One gotcha is that xf86-video-amdgpu currently doesn't allow 
>>> flipping between buffers with different tiling parameters (BTW
>>> Harry, does that work with DC?), but that should be easy to fix.
>> 
> 
> Not currently. Fixable, but unfortunately not as easy as I'd hope.
> The good news is that I'm planning to rework that code so it would be
> easy to fix or should just happen on a per-flip basis.

FWIW, I wouldn't spend any effort specifically on making this work. It's
not intended to work with the non-atomic flip ioctl at least, and it
definitely doesn't work without DC, so xf86-video-amdgpu will have to
handle it anyway.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx