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

[PATCH v4 0/4] 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 v3 ===

drm changes:

* Docstring and formatting fixes

amd/display changes:

* Updated commit message and debug statements

=== Changes from v2 ===

The interface has changed substantially from the last revision and the cover letter has been updated accordingly.

drm changes:

* Most "variable_refresh" prefixes in code have been shortened to just "vrr". This was motivated after changes to the interface had function names close to 80 characters long. Comments from the mailing list were already shortening these in discussion as well.
* Documentation for "Variable Refresh" has been added to the KMS properties subsection for DRM driver developers.
* The drm_connector property "variable_refresh_capable" has been renamed to "vrr_capable".
* The drm_crtc property "variable_refresh" has been been renamed "vrr_enabled" to better reflect its usage.
* The drm_connector "variable_refresh_enabled" property has been removed. Managing this is now up to userspace when it sets "vrr_enabled".
* The "vrr_capable" property no longer has any state in drm_connector_state associated with it. The value can now be updated with the drm_connector_set_vrr_capable_property() function. This better matches the immutable flag on the property.
* The "variable_refresh_changed" flag has been removed from atomic helpers based on feedback from the mailing list and updated interface usage in the amd kernel driver.

amd/display changes:

* Updated interface usage based on the new properties
* Updated VRR infopacket handling based on new xf86-video-amdgpu usage

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

Adaptive sync is part of the DisplayPort specification 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. The user may experience "flickering" from differences in panel luminance at different refresh rates. Content that flips at an infrequent rate or demand is more likely to cause large changes in refresh rate that result in flickering.

Userland needs a way to let the driver know when the screen content is suitable for variable refresh rates.

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

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

The drm_connector has one new optional boolean property:

* bool vrr_capable - set by the driver if the hardware is capable of supporting variable refresh rates

The drm_crtc has one new default boolean property:

* bool vrr_enabled - set by userspace indicating that variable refresh rate should be enabled

== Overview for DRM driver developers ===

Driver developers can attach the "vrr_capable" property by calling drm_connector_attach_vrr_capable_property(). This should be done on connectors that could be capable of supporting variable refresh rates (such as DP or HDMI).

The "vrr_capable" can then be updated accordingly by calling drm_connector_set_vrr_capable_property().

The "vrr_enabled" property can be checked from the drm_crtc_state object.

=== Overview for Userland developers ==

The "vrr_enabled" property on the CRTC should be set to true when the CRTC is suitable for variable refresh rates.

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 (https://gitlab.freedesktop.org/xorg/driver/xf86-video-amdgpu)
* mesa (mesa-dev)

These patches enable adaptive variable refresh on X for AMD hardware. Support for variable refresh is disabled by default in xf86-video-amdgpu and will require additional user configuration.

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 a single monitor setup with the compositor disabled.

The patches require that suitable applications flip via the Present extension to xf86-video-amdgpu for the entire Screen. Some compositors may interfere with this if they are unable to unredirect the window.

Full implementation details for these changes can be reviewed in their respective mailing lists and the xf86-video-amdgpu GitLab.

=== Previous discussions ===

These patches are based upon feedback from previous threads on the subject. These 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.htm
https://lists.freedesktop.org/archives/dri-devel/2018-September/190910.html
https://lists.freedesktop.org/archives/dri-devel/2018-October/192211.html

Nicholas Kazlauskas (4):
  drm: Add vrr_capable property to the drm connector
  drm: Add vrr_enabled property to drm CRTC
  drm: Document variable refresh properties
  drm/amd/display: Set FreeSync state using drm VRR properties

 Documentation/gpu/drm-kms.rst                 |   7 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++---------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   7 +-
 drivers/gpu/drm/drm_atomic_uapi.c             |   4 +
 drivers/gpu/drm/drm_connector.c               |  71 +++++
 drivers/gpu/drm/drm_crtc.c                    |   2 +
 drivers/gpu/drm/drm_mode_config.c             |   6 +
 include/drm/drm_connector.h                   |  15 ++
 include/drm/drm_crtc.h                        |   9 +
 include/drm/drm_mode_config.h                 |   5 +
 10 files changed, 257 insertions(+), 124 deletions(-)

-- 
2.19.1

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

[PATCH v4 1/4] drm: Add vrr_capable property to the drm connector

[Nicholas Kazlauskas]

Modern display hardware is capable of supporting variable refresh rates.
This patch introduces the "vrr_capable" property on the connector to
allow userspace to query support for variable refresh rates.

Atomic drivers should attach this property to connectors that are
capable of driving variable refresh rates using
drm_connector_attach_vrr_capable_property().

The value should be updated based on driver and hardware capabiltiy
by using drm_connector_set_vrr_capable_property().

Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/drm_connector.c | 49 +++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h     | 15 ++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1e40e5decbe9..f0deeb7298d0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1254,6 +1254,37 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
 
+/**
+ * drm_connector_attach_vrr_capable_property - creates the
+ * vrr_capable property
+ * @connector: connector to create the vrr_capable property on.
+ *
+ * This is used by atomic drivers to add support for querying
+ * variable refresh rate capability for a connector.
+ *
+ * Returns:
+ * Zero on success, negative errono on failure.
+ */
+int drm_connector_attach_vrr_capable_property(
+	struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	if (!connector->vrr_capable_property) {
+		prop = drm_property_create_bool(dev, DRM_MODE_PROP_IMMUTABLE,
+			"vrr_capable");
+		if (!prop)
+			return -ENOMEM;
+
+		connector->vrr_capable_property = prop;
+		drm_object_attach_property(&connector->base, prop, 0);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_vrr_capable_property);
+
 /**
  * drm_connector_attach_scaling_mode_property - attach atomic scaling mode property
  * @connector: connector to attach scaling mode property on.
@@ -1582,6 +1613,24 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_set_link_status_property);
 
+/**
+ * drm_connector_set_vrr_capable_property - sets the variable refresh rate
+ * capable property for a connector
+ * @connector: drm connector
+ * @capable: True if the connector is variable refresh rate capable
+ *
+ * Should be used by atomic drivers to update the indicated support for
+ * variable refresh rate over a connector.
+ */
+void drm_connector_set_vrr_capable_property(
+		struct drm_connector *connector, bool capable)
+{
+	drm_object_property_set_value(&connector->base,
+				      connector->vrr_capable_property,
+				      capable);
+}
+EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
+
 /**
  * drm_connector_init_panel_orientation_property -
  *	initialize the connecters panel_orientation property
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 91a877fa00cb..b2263005234a 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -910,6 +910,17 @@ struct drm_connector {
 	 */
 	struct drm_property *scaling_mode_property;
 
+	/**
+	 * @vrr_capable_property: Optional property to help userspace
+	 * query hardware support for variable refresh rate on a connector.
+	 * connector. Drivers can add the property to a connector by
+	 * calling drm_connector_attach_vrr_capable_property().
+	 *
+	 * This should be updated only by calling
+	 * drm_connector_set_vrr_capable_property().
+	 */
+	struct drm_property *vrr_capable_property;
+
 	/**
 	 * @content_protection_property: DRM ENUM property for content
 	 * protection. See drm_connector_attach_content_protection_property().
@@ -1183,6 +1194,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_vrr_capable_property(
+		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);
@@ -1199,6 +1212,8 @@ int drm_connector_update_edid_property(struct drm_connector *connector,
 				       const struct edid *edid);
 void drm_connector_set_link_status_property(struct drm_connector *connector,
 					    uint64_t link_status);
+void drm_connector_set_vrr_capable_property(
+		struct drm_connector *connector, bool capable);
 int drm_connector_init_panel_orientation_property(
 	struct drm_connector *connector, int width, int height);
 
-- 
2.19.1

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

[PATCH v4 2/4] drm: Add vrr_enabled property to drm CRTC

[Nicholas Kazlauskas]

This patch introduces the 'vrr_enabled' CRTC property to allow
dynamic control over variable refresh rate support for a CRTC.

This property should be treated like a content hint to the driver -
if the hardware or driver is not capable of driving variable refresh
timings then this is not considered an error.

Capability for variable refresh rate support should be determined
by querying the vrr_capable drm connector property.

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

Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
 drivers/gpu/drm/drm_crtc.c        | 2 ++
 drivers/gpu/drm/drm_mode_config.c | 6 ++++++
 include/drm/drm_crtc.h            | 9 +++++++++
 include/drm/drm_mode_config.h     | 5 +++++
 5 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d5b7f315098c..eec396a57b88 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -433,6 +433,8 @@ 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_vrr_enabled) {
+		state->vrr_enabled = val;
 	} else if (property == config->degamma_lut_property) {
 		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->degamma_lut,
@@ -491,6 +493,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_vrr_enabled)
+		*val = state->vrr_enabled;
 	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..e4eb2c897ff4 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_vrr_enabled, 0);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index ee80788f2c40..5670c67f28d4 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,
+			"VRR_ENABLED");
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_vrr_enabled = 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..39c3900aab3c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -290,6 +290,15 @@ struct drm_crtc_state {
 	 */
 	u32 pageflip_flags;
 
+	/**
+	 * @vrr_enabled:
+	 *
+	 * Indicates if variable refresh rate should be enabled for the CRTC.
+	 * Support for the requested vrr state will depend on driver and
+	 * hardware capabiltiy - lacking support is not treated as failure.
+	 */
+	bool vrr_enabled;
+
 	/**
 	 * @event:
 	 *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 928e4172a0bb..49f2fcfdb5fc 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -639,6 +639,11 @@ struct drm_mode_config {
 	 * connectors must be of and active must be set to disabled, too.
 	 */
 	struct drm_property *prop_mode_id;
+	/**
+	 * @prop_vrr_enabled: Default atomic CRTC property to indicate
+	 * whether variable refresh rate should be enabled on the CRTC.
+	 */
+	struct drm_property *prop_vrr_enabled;
 
 	/**
 	 * @dvi_i_subconnector_property: Optional DVI-I property to
-- 
2.19.1

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

[PATCH v4 3/4] drm: Document variable refresh properties

[Nicholas Kazlauskas]

These include the drm_connector 'vrr_capable' and the drm_crtc
'vrr_enabled' properties.

Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 Documentation/gpu/drm-kms.rst   |  7 +++++++
 drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 4b1501b4835b..8da2a178cf85 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -575,6 +575,13 @@ Explicit Fencing Properties
 .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
    :doc: explicit fencing properties
 
+
+Variable Refresh Properties
+---------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_connector.c
+   :doc: Variable refresh properties
+
 Existing KMS Properties
 -----------------------
 
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index f0deeb7298d0..2a12853ca917 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
 
+/**
+ * DOC: Variable refresh properties
+ *
+ * Variable refresh rate control is supported via properties on the
+ * &drm_connector and &drm_crtc objects.
+ *
+ * "vrr_capable":
+ *	Optional &drm_connector boolean property that drivers should attach
+ *	with drm_connector_attach_vrr_capable_property() on connectors that
+ *	could support variable refresh rates. Drivers should update the
+ *	property value by calling drm_connector_set_vrr_capable_property().
+ *
+ *	Absence of the property should indicate absence of support.
+ *
+ * "vrr_enabled":
+ *	Default &drm_crtc boolean property that notifies the driver that the
+ *	variable refresh rate adjustment should be enabled for the CRTC.
+ *
+ *	Support for variable refresh rate will depend on the "vrr_capable"
+ *	property exposed on the &drm_connector object.
+ */
+
 /**
  * drm_connector_attach_vrr_capable_property - creates the
  * vrr_capable property
-- 
2.19.1

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

[PATCH v4 4/4] drm/amd/display: Set FreeSync state using drm VRR properties

[Nicholas Kazlauskas]

Support for AMDGPU specific FreeSync properties and ioctls are dropped
from amdgpu_dm in favor of supporting drm variable refresh rate
properties.

The drm vrr_capable property is now attached to any DP/HDMI connector.
Its value is updated accordingly to the connector's FreeSync capabiltiy.

The freesync_enable logic and ioctl control has has been dropped in
favor of utilizing the vrr_enabled on the drm CRTC. This allows for more
fine grained atomic control over which CRTCs should support variable
refresh rate.

To handle state changes for vrr_enabled it was easiest to drop the
forced modeset on freesync_enabled change. This patch now performs the
required stream updates when planes are 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 need to be 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.

VRR packets and timing adjustments are now tracked and compared to
previous values sent to the hardware.

Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++---------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   7 +-
 2 files changed, 138 insertions(+), 124 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 6a2342d72742..d5de4b91e144 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1802,72 +1802,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 */
@@ -1881,7 +1815,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,
 
 };
 
@@ -2834,7 +2767,8 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	state->adjust = cur->adjust;
 	state->vrr_infopacket = cur->vrr_infopacket;
-	state->freesync_enabled = cur->freesync_enabled;
+	state->vrr_supported = cur->vrr_supported;
+	state->freesync_config = cur->freesync_config;
 
 	/* TODO Duplicate dc_stream after objects are stream object is flattened */
 
@@ -3053,8 +2987,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;
 }
 
@@ -3800,6 +3732,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_vrr_capable_property(
+			&aconnector->base);
+	}
 }
 
 static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
@@ -4176,6 +4113,77 @@ 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;
+
+	/*
+	 * TODO: Determine why min/max totals and vrefresh can be 0 here.
+	 * For now it's sufficient to just guard against these conditions.
+	 */
+
+	if (!new_stream->timing.h_total || !new_stream->timing.v_total)
+		return;
+
+	if (new_crtc_state->vrr_supported &&
+	    config.min_refresh_in_uhz &&
+	    config.max_refresh_in_uhz) {
+		config.state = new_crtc_state->base.vrr_enabled ?
+			VRR_STATE_ACTIVE_VARIABLE :
+			VRR_STATE_INACTIVE;
+	} else {
+		config.state = VRR_STATE_UNSUPPORTED;
+	}
+
+	mod_freesync_build_vrr_params(dm->freesync_module,
+				      new_stream,
+				      &config, &vrr);
+
+	mod_freesync_build_vrr_infopacket(
+		dm->freesync_module,
+		new_stream,
+		&vrr,
+		packet_type_vrr,
+		transfer_func_unknown,
+		&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("VRR packet update: crtc=%u enabled=%d state=%d",
+			      new_crtc_state->base.crtc->base.id,
+			      (int)new_crtc_state->base.vrr_enabled,
+			      (int)vrr.state);
+
+	if (new_crtc_state->freesync_timing_changed)
+		DRM_DEBUG_KMS("VRR timing update: crtc=%u min=%u max=%u\n",
+			      new_crtc_state->base.crtc->base.id,
+			      vrr.adjust.v_total_min,
+			      vrr.adjust.v_total_max);
+}
+
 /*
  * Executes flip
  *
@@ -4197,6 +4205,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);
 	struct dc_stream_status *stream_status;
 
@@ -4269,11 +4278,26 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
 	}
 	surface_updates->flip_addr = &addr;
 
+	if (acrtc_state->stream) {
+		update_freesync_state_on_stream(
+			&adev->dm,
+			acrtc_state,
+			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);
 
@@ -4333,11 +4357,6 @@ 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) {
-		stream_update->vrr_infopacket = &dc_stream->vrr_infopacket;
-		stream_update->adjust = &dc_stream->adjust;
-	}
-
 	for (i = 0; i < new_plane_count; i++) {
 		updates[i].surface = plane_states[i];
 		updates[i].gamma =
@@ -4473,9 +4492,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,
@@ -4679,9 +4695,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,
@@ -4899,20 +4912,18 @@ 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->vrr_supported = new_con_state->freesync_capable;
+
+	if (new_con_state->freesync_capable) {
+		config.state = new_crtc_state->base.vrr_enabled ?
 				VRR_STATE_ACTIVE_VARIABLE :
 				VRR_STATE_INACTIVE;
 		config.min_refresh_in_uhz =
@@ -4922,19 +4933,18 @@ void set_freesync_on_stream(struct amdgpu_display_manager *dm,
 		config.vsif_supported = true;
 	}
 
-	mod_freesync_build_vrr_params(dm->freesync_module,
-				      new_stream,
-				      &config, &vrr);
+	new_crtc_state->freesync_config = config;
+}
 
-	mod_freesync_build_vrr_infopacket(dm->freesync_module,
-					  new_stream,
-					  &vrr,
-					  packet_type_fs1,
-					  NULL,
-					  &vrr_infopacket);
+static void reset_freesync_config_for_crtc(
+	struct dm_crtc_state *new_crtc_state)
+{
+	new_crtc_state->vrr_supported = false;
 
-	new_crtc_state->adjust = vrr.adjust;
-	new_crtc_state->vrr_infopacket = vrr_infopacket;
+	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,
@@ -5009,9 +5019,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;
@@ -5020,9 +5027,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;
 
@@ -5059,6 +5063,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 */
@@ -5136,7 +5142,9 @@ 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;
@@ -5401,12 +5409,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->vrr_enabled)
 			continue;
 
 		if (!new_crtc_state->enable)
@@ -5558,14 +5563,15 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 	struct detailed_data_monitor_range *range;
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
 			to_amdgpu_dm_connector(connector);
-	struct dm_connector_state *dm_con_state;
+	struct dm_connector_state *dm_con_state = NULL;
 
 	struct drm_device *dev = connector->dev;
 	struct amdgpu_device *adev = dev->dev_private;
+	bool freesync_capable = false;
 
 	if (!connector->state) {
 		DRM_ERROR("%s - Connector has no state", __func__);
-		return;
+		goto update;
 	}
 
 	if (!edid) {
@@ -5575,9 +5581,7 @@ 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;
+		goto update;
 	}
 
 	dm_con_state = to_dm_connector_state(connector->state);
@@ -5585,10 +5589,10 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 	edid_check_required = false;
 	if (!amdgpu_dm_connector->dc_sink) {
 		DRM_ERROR("dc_sink NULL, could not add free_sync module.\n");
-		return;
+		goto update;
 	}
 	if (!adev->dm.freesync_module)
-		return;
+		goto update;
 	/*
 	 * if edid non zero restrict freesync only for dp and edp
 	 */
@@ -5600,7 +5604,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++) {
@@ -5632,8 +5635,16 @@ 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;
+			freesync_capable = true;
 		}
 	}
+
+update:
+	if (dm_con_state)
+		dm_con_state->freesync_capable = freesync_capable;
+
+	if (connector->vrr_capable_property)
+		drm_connector_set_vrr_capable_property(connector,
+						       freesync_capable);
 }
 
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 978b34a5011c..a5aaf8b08968 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -185,7 +185,11 @@ struct dm_crtc_state {
 	int crc_skip_count;
 	bool crc_enabled;
 
-	bool freesync_enabled;
+	bool freesync_timing_changed;
+	bool freesync_vrr_info_changed;
+
+	bool vrr_supported;
+	struct mod_freesync_config freesync_config;
 	struct dc_crtc_timing_adjust adjust;
 	struct dc_info_packet vrr_infopacket;
 };
@@ -207,7 +211,6 @@ struct dm_connector_state {
 	uint8_t underscan_vborder;
 	uint8_t underscan_hborder;
 	bool underscan_enable;
-	bool freesync_enable;
 	bool freesync_capable;
 };
 
-- 
2.19.1

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

Re: [PATCH v4 4/4] drm/amd/display: Set FreeSync state using drm VRR properties

[Harry Wentland]

On 2018-10-11 12:39 PM, Nicholas Kazlauskas wrote:
> Support for AMDGPU specific FreeSync properties and ioctls are dropped
> from amdgpu_dm in favor of supporting drm variable refresh rate
> properties.
> 
> The drm vrr_capable property is now attached to any DP/HDMI connector.
> Its value is updated accordingly to the connector's FreeSync capabiltiy.
> 
> The freesync_enable logic and ioctl control has has been dropped in
> favor of utilizing the vrr_enabled on the drm CRTC. This allows for more
> fine grained atomic control over which CRTCs should support variable
> refresh rate.
> 
> To handle state changes for vrr_enabled it was easiest to drop the
> forced modeset on freesync_enabled change. This patch now performs the
> required stream updates when planes are 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 need to be 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.
> 
> VRR packets and timing adjustments are now tracked and compared to
> previous values sent to the hardware.
> 
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++---------
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   7 +-
>  2 files changed, 138 insertions(+), 124 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 6a2342d72742..d5de4b91e144 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1802,72 +1802,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 */
> @@ -1881,7 +1815,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,

Please also drop from amdgpu_display_funcs definition in amdgpu_mode.h. Might want to drop the set_freesync_property from there as well.

>  
>  };
>  
> @@ -2834,7 +2767,8 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>  	state->adjust = cur->adjust;
>  	state->vrr_infopacket = cur->vrr_infopacket;
> -	state->freesync_enabled = cur->freesync_enabled;
> +	state->vrr_supported = cur->vrr_supported;
> +	state->freesync_config = cur->freesync_config;
>  
>  	/* TODO Duplicate dc_stream after objects are stream object is flattened */
>  
> @@ -3053,8 +2987,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;
>  }
>  
> @@ -3800,6 +3732,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_vrr_capable_property(
> +			&aconnector->base);
> +	}
>  }
>  
>  static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
> @@ -4176,6 +4113,77 @@ 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;
> +
> +	/*
> +	 * TODO: Determine why min/max totals and vrefresh can be 0 here.
> +	 * For now it's sufficient to just guard against these conditions.
> +	 */
> +
> +	if (!new_stream->timing.h_total || !new_stream->timing.v_total)
> +		return;
> +
> +	if (new_crtc_state->vrr_supported &&
> +	    config.min_refresh_in_uhz &&
> +	    config.max_refresh_in_uhz) {
> +		config.state = new_crtc_state->base.vrr_enabled ?
> +			VRR_STATE_ACTIVE_VARIABLE :
> +			VRR_STATE_INACTIVE;
> +	} else {
> +		config.state = VRR_STATE_UNSUPPORTED;
> +	}
> +
> +	mod_freesync_build_vrr_params(dm->freesync_module,
> +				      new_stream,
> +				      &config, &vrr);
> +
> +	mod_freesync_build_vrr_infopacket(
> +		dm->freesync_module,
> +		new_stream,
> +		&vrr,
> +		packet_type_vrr,
> +		transfer_func_unknown,
> +		&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("VRR packet update: crtc=%u enabled=%d state=%d",
> +			      new_crtc_state->base.crtc->base.id,
> +			      (int)new_crtc_state->base.vrr_enabled,
> +			      (int)vrr.state);
> +
> +	if (new_crtc_state->freesync_timing_changed)
> +		DRM_DEBUG_KMS("VRR timing update: crtc=%u min=%u max=%u\n",
> +			      new_crtc_state->base.crtc->base.id,
> +			      vrr.adjust.v_total_min,
> +			      vrr.adjust.v_total_max);
> +}
> +
>  /*
>   * Executes flip
>   *
> @@ -4197,6 +4205,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);
>  	struct dc_stream_status *stream_status;
>  
> @@ -4269,11 +4278,26 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>  	}
>  	surface_updates->flip_addr = &addr;
>  
> +	if (acrtc_state->stream) {
> +		update_freesync_state_on_stream(
> +			&adev->dm,
> +			acrtc_state,
> +			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;
> +	}
> +

For consistency we might also want to do this in commit_planes_to_stream before the call to dc_commit_updates_for_stream.

We should really merge the two codepaths for dc_commit_updates_for_stream one of these days.

>  	dc_commit_updates_for_stream(adev->dm.dc,
>  					     surface_updates,
>  					     1,
>  					     acrtc_state->stream,
> -					     NULL,
> +					     &stream_update,
>  					     &surface_updates->surface,
>  					     state);
>  
> @@ -4333,11 +4357,6 @@ 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) {
> -		stream_update->vrr_infopacket = &dc_stream->vrr_infopacket;
> -		stream_update->adjust = &dc_stream->adjust;
> -	}
> -
>  	for (i = 0; i < new_plane_count; i++) {
>  		updates[i].surface = plane_states[i];
>  		updates[i].gamma =
> @@ -4473,9 +4492,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,
> @@ -4679,9 +4695,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,
> @@ -4899,20 +4912,18 @@ 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->vrr_supported = new_con_state->freesync_capable;
> +
> +	if (new_con_state->freesync_capable) {
> +		config.state = new_crtc_state->base.vrr_enabled ?
>  				VRR_STATE_ACTIVE_VARIABLE :
>  				VRR_STATE_INACTIVE;
>  		config.min_refresh_in_uhz =
> @@ -4922,19 +4933,18 @@ void set_freesync_on_stream(struct amdgpu_display_manager *dm,
>  		config.vsif_supported = true;
>  	}
>  
> -	mod_freesync_build_vrr_params(dm->freesync_module,
> -				      new_stream,
> -				      &config, &vrr);
> +	new_crtc_state->freesync_config = config;
> +}
>  
> -	mod_freesync_build_vrr_infopacket(dm->freesync_module,
> -					  new_stream,
> -					  &vrr,
> -					  packet_type_fs1,
> -					  NULL,
> -					  &vrr_infopacket);
> +static void reset_freesync_config_for_crtc(
> +	struct dm_crtc_state *new_crtc_state)
> +{
> +	new_crtc_state->vrr_supported = false;
>  
> -	new_crtc_state->adjust = vrr.adjust;
> -	new_crtc_state->vrr_infopacket = vrr_infopacket;
> +	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,
> @@ -5009,9 +5019,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;
> @@ -5020,9 +5027,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;
>  
> @@ -5059,6 +5063,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 */
> @@ -5136,7 +5142,9 @@ 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;
> @@ -5401,12 +5409,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->vrr_enabled)
>  			continue;
>  
>  		if (!new_crtc_state->enable)
> @@ -5558,14 +5563,15 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  	struct detailed_data_monitor_range *range;
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
>  			to_amdgpu_dm_connector(connector);
> -	struct dm_connector_state *dm_con_state;
> +	struct dm_connector_state *dm_con_state = NULL;
>  
>  	struct drm_device *dev = connector->dev;
>  	struct amdgpu_device *adev = dev->dev_private;
> +	bool freesync_capable = false;
>  
>  	if (!connector->state) {
>  		DRM_ERROR("%s - Connector has no state", __func__);
> -		return;
> +		goto update;
>  	}
>  
>  	if (!edid) {
> @@ -5575,9 +5581,7 @@ 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;
> +		goto update;
>  	}
>  
>  	dm_con_state = to_dm_connector_state(connector->state);
> @@ -5585,10 +5589,10 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  	edid_check_required = false;
>  	if (!amdgpu_dm_connector->dc_sink) {
>  		DRM_ERROR("dc_sink NULL, could not add free_sync module.\n");
> -		return;
> +		goto update;
>  	}
>  	if (!adev->dm.freesync_module)
> -		return;
> +		goto update;
>  	/*
>  	 * if edid non zero restrict freesync only for dp and edp
>  	 */
> @@ -5600,7 +5604,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++) {
> @@ -5632,8 +5635,16 @@ 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;
> +			freesync_capable = true;
>  		}
>  	}
> +
> +update:
> +	if (dm_con_state)
> +		dm_con_state->freesync_capable = freesync_capable;
> +

Interesting. Looks like we could just grab this from the drm_connector property as this isn't something that changes with the state but DRM forces us to track it on the connector state as drm_object_property_get_value() throws a WARN_ON for atomic drivers. Not sure I like it but it's what we got, I guess.

Harry

> +	if (connector->vrr_capable_property)
> +		drm_connector_set_vrr_capable_property(connector,
> +						       freesync_capable);
>  }
>  
> 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 978b34a5011c..a5aaf8b08968 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -185,7 +185,11 @@ struct dm_crtc_state {
>  	int crc_skip_count;
>  	bool crc_enabled;
>  
> -	bool freesync_enabled;
> +	bool freesync_timing_changed;
> +	bool freesync_vrr_info_changed;
> +
> +	bool vrr_supported;
> +	struct mod_freesync_config freesync_config;
>  	struct dc_crtc_timing_adjust adjust;
>  	struct dc_info_packet vrr_infopacket;
>  };
> @@ -207,7 +211,6 @@ struct dm_connector_state {
>  	uint8_t underscan_vborder;
>  	uint8_t underscan_hborder;
>  	bool underscan_enable;
> -	bool freesync_enable;
>  	bool freesync_capable;
>  };
>  
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4 4/4] drm/amd/display: Set FreeSync state using drm VRR properties

[Kazlauskas, Nicholas]

On 10/11/2018 04:39 PM, Harry Wentland wrote:
> On 2018-10-11 12:39 PM, Nicholas Kazlauskas wrote:
>> Support for AMDGPU specific FreeSync properties and ioctls are dropped
>> from amdgpu_dm in favor of supporting drm variable refresh rate
>> properties.
>>
>> The drm vrr_capable property is now attached to any DP/HDMI connector.
>> Its value is updated accordingly to the connector's FreeSync capabiltiy.
>>
>> The freesync_enable logic and ioctl control has has been dropped in
>> favor of utilizing the vrr_enabled on the drm CRTC. This allows for more
>> fine grained atomic control over which CRTCs should support variable
>> refresh rate.
>>
>> To handle state changes for vrr_enabled it was easiest to drop the
>> forced modeset on freesync_enabled change. This patch now performs the
>> required stream updates when planes are 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 need to be 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.
>>
>> VRR packets and timing adjustments are now tracked and compared to
>> previous values sent to the hardware.
>>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++---------
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   7 +-
>>   2 files changed, 138 insertions(+), 124 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 6a2342d72742..d5de4b91e144 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -1802,72 +1802,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 */
>> @@ -1881,7 +1815,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,
> 
> Please also drop from amdgpu_display_funcs definition in amdgpu_mode.h. Might want to drop the set_freesync_property from there as well.

Oh right, I forgot about those. Will do.

> 
>>   
>>   };
>>   
>> @@ -2834,7 +2767,8 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>>   
>>   	state->adjust = cur->adjust;
>>   	state->vrr_infopacket = cur->vrr_infopacket;
>> -	state->freesync_enabled = cur->freesync_enabled;
>> +	state->vrr_supported = cur->vrr_supported;
>> +	state->freesync_config = cur->freesync_config;
>>   
>>   	/* TODO Duplicate dc_stream after objects are stream object is flattened */
>>   
>> @@ -3053,8 +2987,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;
>>   }
>>   
>> @@ -3800,6 +3732,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_vrr_capable_property(
>> +			&aconnector->base);
>> +	}
>>   }
>>   
>>   static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
>> @@ -4176,6 +4113,77 @@ 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;
>> +
>> +	/*
>> +	 * TODO: Determine why min/max totals and vrefresh can be 0 here.
>> +	 * For now it's sufficient to just guard against these conditions.
>> +	 */
>> +
>> +	if (!new_stream->timing.h_total || !new_stream->timing.v_total)
>> +		return;
>> +
>> +	if (new_crtc_state->vrr_supported &&
>> +	    config.min_refresh_in_uhz &&
>> +	    config.max_refresh_in_uhz) {
>> +		config.state = new_crtc_state->base.vrr_enabled ?
>> +			VRR_STATE_ACTIVE_VARIABLE :
>> +			VRR_STATE_INACTIVE;
>> +	} else {
>> +		config.state = VRR_STATE_UNSUPPORTED;
>> +	}
>> +
>> +	mod_freesync_build_vrr_params(dm->freesync_module,
>> +				      new_stream,
>> +				      &config, &vrr);
>> +
>> +	mod_freesync_build_vrr_infopacket(
>> +		dm->freesync_module,
>> +		new_stream,
>> +		&vrr,
>> +		packet_type_vrr,
>> +		transfer_func_unknown,
>> +		&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("VRR packet update: crtc=%u enabled=%d state=%d",
>> +			      new_crtc_state->base.crtc->base.id,
>> +			      (int)new_crtc_state->base.vrr_enabled,
>> +			      (int)vrr.state);
>> +
>> +	if (new_crtc_state->freesync_timing_changed)
>> +		DRM_DEBUG_KMS("VRR timing update: crtc=%u min=%u max=%u\n",
>> +			      new_crtc_state->base.crtc->base.id,
>> +			      vrr.adjust.v_total_min,
>> +			      vrr.adjust.v_total_max);
>> +}
>> +
>>   /*
>>    * Executes flip
>>    *
>> @@ -4197,6 +4205,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);
>>   	struct dc_stream_status *stream_status;
>>   
>> @@ -4269,11 +4278,26 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>>   	}
>>   	surface_updates->flip_addr = &addr;
>>   
>> +	if (acrtc_state->stream) {
>> +		update_freesync_state_on_stream(
>> +			&adev->dm,
>> +			acrtc_state,
>> +			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;
>> +	}
>> +
> 
> For consistency we might also want to do this in commit_planes_to_stream before the call to dc_commit_updates_for_stream.
> 
> We should really merge the two codepaths for dc_commit_updates_for_stream one of these days.

In a prior revision to this patch I actually did do this on both but 
reduced it to just the flip - the reason being that there's no point in 
enabling this when we're *not* flipping since it's needed for this to 
function properly.

The merge is probably a good idea down the line to cut down on code 
duplication even further.

> 
>>   	dc_commit_updates_for_stream(adev->dm.dc,
>>   					     surface_updates,
>>   					     1,
>>   					     acrtc_state->stream,
>> -					     NULL,
>> +					     &stream_update,
>>   					     &surface_updates->surface,
>>   					     state);
>>   
>> @@ -4333,11 +4357,6 @@ 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) {
>> -		stream_update->vrr_infopacket = &dc_stream->vrr_infopacket;
>> -		stream_update->adjust = &dc_stream->adjust;
>> -	}
>> -
>>   	for (i = 0; i < new_plane_count; i++) {
>>   		updates[i].surface = plane_states[i];
>>   		updates[i].gamma =
>> @@ -4473,9 +4492,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,
>> @@ -4679,9 +4695,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,
>> @@ -4899,20 +4912,18 @@ 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->vrr_supported = new_con_state->freesync_capable;
>> +
>> +	if (new_con_state->freesync_capable) {
>> +		config.state = new_crtc_state->base.vrr_enabled ?
>>   				VRR_STATE_ACTIVE_VARIABLE :
>>   				VRR_STATE_INACTIVE;
>>   		config.min_refresh_in_uhz =
>> @@ -4922,19 +4933,18 @@ void set_freesync_on_stream(struct amdgpu_display_manager *dm,
>>   		config.vsif_supported = true;
>>   	}
>>   
>> -	mod_freesync_build_vrr_params(dm->freesync_module,
>> -				      new_stream,
>> -				      &config, &vrr);
>> +	new_crtc_state->freesync_config = config;
>> +}
>>   
>> -	mod_freesync_build_vrr_infopacket(dm->freesync_module,
>> -					  new_stream,
>> -					  &vrr,
>> -					  packet_type_fs1,
>> -					  NULL,
>> -					  &vrr_infopacket);
>> +static void reset_freesync_config_for_crtc(
>> +	struct dm_crtc_state *new_crtc_state)
>> +{
>> +	new_crtc_state->vrr_supported = false;
>>   
>> -	new_crtc_state->adjust = vrr.adjust;
>> -	new_crtc_state->vrr_infopacket = vrr_infopacket;
>> +	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,
>> @@ -5009,9 +5019,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;
>> @@ -5020,9 +5027,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;
>>   
>> @@ -5059,6 +5063,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 */
>> @@ -5136,7 +5142,9 @@ 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;
>> @@ -5401,12 +5409,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->vrr_enabled)
>>   			continue;
>>   
>>   		if (!new_crtc_state->enable)
>> @@ -5558,14 +5563,15 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>>   	struct detailed_data_monitor_range *range;
>>   	struct amdgpu_dm_connector *amdgpu_dm_connector =
>>   			to_amdgpu_dm_connector(connector);
>> -	struct dm_connector_state *dm_con_state;
>> +	struct dm_connector_state *dm_con_state = NULL;
>>   
>>   	struct drm_device *dev = connector->dev;
>>   	struct amdgpu_device *adev = dev->dev_private;
>> +	bool freesync_capable = false;
>>   
>>   	if (!connector->state) {
>>   		DRM_ERROR("%s - Connector has no state", __func__);
>> -		return;
>> +		goto update;
>>   	}
>>   
>>   	if (!edid) {
>> @@ -5575,9 +5581,7 @@ 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;
>> +		goto update;
>>   	}
>>   
>>   	dm_con_state = to_dm_connector_state(connector->state);
>> @@ -5585,10 +5589,10 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>>   	edid_check_required = false;
>>   	if (!amdgpu_dm_connector->dc_sink) {
>>   		DRM_ERROR("dc_sink NULL, could not add free_sync module.\n");
>> -		return;
>> +		goto update;
>>   	}
>>   	if (!adev->dm.freesync_module)
>> -		return;
>> +		goto update;
>>   	/*
>>   	 * if edid non zero restrict freesync only for dp and edp
>>   	 */
>> @@ -5600,7 +5604,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++) {
>> @@ -5632,8 +5635,16 @@ 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;
>> +			freesync_capable = true;
>>   		}
>>   	}
>> +
>> +update:
>> +	if (dm_con_state)
>> +		dm_con_state->freesync_capable = freesync_capable;
>> +
> 
> Interesting. Looks like we could just grab this from the drm_connector property as this isn't something that changes with the state but DRM forces us to track it on the connector state as drm_object_property_get_value() throws a WARN_ON for atomic drivers. Not sure I like it but it's what we got, I guess.
> 
> Harry

That was my original plan, but I think I understand the reasoning behind 
the separation of state and property.

I don't mind this approach too much because the value really feels like 
an implementation detail specific to our driver in this case.

It's not really describing whether the connector is actually capable or 
not, but whether it's fine to perform vrr updates later when we're 
updating the planes.

> 
>> +	if (connector->vrr_capable_property)
>> +		drm_connector_set_vrr_capable_property(connector,
>> +						       freesync_capable);
>>   }
>>   
>> 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 978b34a5011c..a5aaf8b08968 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -185,7 +185,11 @@ struct dm_crtc_state {
>>   	int crc_skip_count;
>>   	bool crc_enabled;
>>   
>> -	bool freesync_enabled;
>> +	bool freesync_timing_changed;
>> +	bool freesync_vrr_info_changed;
>> +
>> +	bool vrr_supported;
>> +	struct mod_freesync_config freesync_config;
>>   	struct dc_crtc_timing_adjust adjust;
>>   	struct dc_info_packet vrr_infopacket;
>>   };
>> @@ -207,7 +211,6 @@ struct dm_connector_state {
>>   	uint8_t underscan_vborder;
>>   	uint8_t underscan_hborder;
>>   	bool underscan_enable;
>> -	bool freesync_enable;
>>   	bool freesync_capable;
>>   };
>>   
>>

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

Re: [PATCH v4 4/4] drm/amd/display: Set FreeSync state using drm VRR properties

[Harry Wentland]

On 2018-10-11 04:56 PM, Kazlauskas, Nicholas wrote:
> On 10/11/2018 04:39 PM, Harry Wentland wrote:
>> On 2018-10-11 12:39 PM, Nicholas Kazlauskas wrote:
>>> Support for AMDGPU specific FreeSync properties and ioctls are dropped
>>> from amdgpu_dm in favor of supporting drm variable refresh rate
>>> properties.
>>>
>>> The drm vrr_capable property is now attached to any DP/HDMI connector.
>>> Its value is updated accordingly to the connector's FreeSync capabiltiy.
>>>
>>> The freesync_enable logic and ioctl control has has been dropped in
>>> favor of utilizing the vrr_enabled on the drm CRTC. This allows for more
>>> fine grained atomic control over which CRTCs should support variable
>>> refresh rate.
>>>
>>> To handle state changes for vrr_enabled it was easiest to drop the
>>> forced modeset on freesync_enabled change. This patch now performs the
>>> required stream updates when planes are 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 need to be 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.
>>>
>>> VRR packets and timing adjustments are now tracked and compared to
>>> previous values sent to the hardware.
>>>
>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>> ---
>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++---------
>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   7 +-
>>>   2 files changed, 138 insertions(+), 124 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 6a2342d72742..d5de4b91e144 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -1802,72 +1802,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 */
>>> @@ -1881,7 +1815,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,
>>
>> Please also drop from amdgpu_display_funcs definition in amdgpu_mode.h. Might want to drop the set_freesync_property from there as well.
> 
> Oh right, I forgot about those. Will do.
> 
>>
>>>     };
>>>   @@ -2834,7 +2767,8 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>>>         state->adjust = cur->adjust;
>>>       state->vrr_infopacket = cur->vrr_infopacket;
>>> -    state->freesync_enabled = cur->freesync_enabled;
>>> +    state->vrr_supported = cur->vrr_supported;
>>> +    state->freesync_config = cur->freesync_config;
>>>         /* TODO Duplicate dc_stream after objects are stream object is flattened */
>>>   @@ -3053,8 +2987,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;
>>>   }
>>>   @@ -3800,6 +3732,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_vrr_capable_property(
>>> +            &aconnector->base);
>>> +    }
>>>   }
>>>     static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
>>> @@ -4176,6 +4113,77 @@ 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;
>>> +
>>> +    /*
>>> +     * TODO: Determine why min/max totals and vrefresh can be 0 here.
>>> +     * For now it's sufficient to just guard against these conditions.
>>> +     */
>>> +
>>> +    if (!new_stream->timing.h_total || !new_stream->timing.v_total)
>>> +        return;
>>> +
>>> +    if (new_crtc_state->vrr_supported &&
>>> +        config.min_refresh_in_uhz &&
>>> +        config.max_refresh_in_uhz) {
>>> +        config.state = new_crtc_state->base.vrr_enabled ?
>>> +            VRR_STATE_ACTIVE_VARIABLE :
>>> +            VRR_STATE_INACTIVE;
>>> +    } else {
>>> +        config.state = VRR_STATE_UNSUPPORTED;
>>> +    }
>>> +
>>> +    mod_freesync_build_vrr_params(dm->freesync_module,
>>> +                      new_stream,
>>> +                      &config, &vrr);
>>> +
>>> +    mod_freesync_build_vrr_infopacket(
>>> +        dm->freesync_module,
>>> +        new_stream,
>>> +        &vrr,
>>> +        packet_type_vrr,
>>> +        transfer_func_unknown,
>>> +        &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("VRR packet update: crtc=%u enabled=%d state=%d",
>>> +                  new_crtc_state->base.crtc->base.id,
>>> +                  (int)new_crtc_state->base.vrr_enabled,
>>> +                  (int)vrr.state);
>>> +
>>> +    if (new_crtc_state->freesync_timing_changed)
>>> +        DRM_DEBUG_KMS("VRR timing update: crtc=%u min=%u max=%u\n",
>>> +                  new_crtc_state->base.crtc->base.id,
>>> +                  vrr.adjust.v_total_min,
>>> +                  vrr.adjust.v_total_max);
>>> +}
>>> +
>>>   /*
>>>    * Executes flip
>>>    *
>>> @@ -4197,6 +4205,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);
>>>       struct dc_stream_status *stream_status;
>>>   @@ -4269,11 +4278,26 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>>>       }
>>>       surface_updates->flip_addr = &addr;
>>>   +    if (acrtc_state->stream) {
>>> +        update_freesync_state_on_stream(
>>> +            &adev->dm,
>>> +            acrtc_state,
>>> +            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;
>>> +    }
>>> +
>>
>> For consistency we might also want to do this in commit_planes_to_stream before the call to dc_commit_updates_for_stream.
>>
>> We should really merge the two codepaths for dc_commit_updates_for_stream one of these days.
> 
> In a prior revision to this patch I actually did do this on both but reduced it to just the flip - the reason being that there's no point in enabling this when we're *not* flipping since it's needed for this to function properly.
> 
> The merge is probably a good idea down the line to cut down on code duplication even further.
> 

Sounds good.

Harry

>>
>>>       dc_commit_updates_for_stream(adev->dm.dc,
>>>                            surface_updates,
>>>                            1,
>>>                            acrtc_state->stream,
>>> -                         NULL,
>>> +                         &stream_update,
>>>                            &surface_updates->surface,
>>>                            state);
>>>   @@ -4333,11 +4357,6 @@ 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) {
>>> -        stream_update->vrr_infopacket = &dc_stream->vrr_infopacket;
>>> -        stream_update->adjust = &dc_stream->adjust;
>>> -    }
>>> -
>>>       for (i = 0; i < new_plane_count; i++) {
>>>           updates[i].surface = plane_states[i];
>>>           updates[i].gamma =
>>> @@ -4473,9 +4492,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,
>>> @@ -4679,9 +4695,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,
>>> @@ -4899,20 +4912,18 @@ 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->vrr_supported = new_con_state->freesync_capable;
>>> +
>>> +    if (new_con_state->freesync_capable) {
>>> +        config.state = new_crtc_state->base.vrr_enabled ?
>>>                   VRR_STATE_ACTIVE_VARIABLE :
>>>                   VRR_STATE_INACTIVE;
>>>           config.min_refresh_in_uhz =
>>> @@ -4922,19 +4933,18 @@ void set_freesync_on_stream(struct amdgpu_display_manager *dm,
>>>           config.vsif_supported = true;
>>>       }
>>>   -    mod_freesync_build_vrr_params(dm->freesync_module,
>>> -                      new_stream,
>>> -                      &config, &vrr);
>>> +    new_crtc_state->freesync_config = config;
>>> +}
>>>   -    mod_freesync_build_vrr_infopacket(dm->freesync_module,
>>> -                      new_stream,
>>> -                      &vrr,
>>> -                      packet_type_fs1,
>>> -                      NULL,
>>> -                      &vrr_infopacket);
>>> +static void reset_freesync_config_for_crtc(
>>> +    struct dm_crtc_state *new_crtc_state)
>>> +{
>>> +    new_crtc_state->vrr_supported = false;
>>>   -    new_crtc_state->adjust = vrr.adjust;
>>> -    new_crtc_state->vrr_infopacket = vrr_infopacket;
>>> +    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,
>>> @@ -5009,9 +5019,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;
>>> @@ -5020,9 +5027,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;
>>>   @@ -5059,6 +5063,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 */
>>> @@ -5136,7 +5142,9 @@ 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;
>>> @@ -5401,12 +5409,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->vrr_enabled)
>>>               continue;
>>>             if (!new_crtc_state->enable)
>>> @@ -5558,14 +5563,15 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>>>       struct detailed_data_monitor_range *range;
>>>       struct amdgpu_dm_connector *amdgpu_dm_connector =
>>>               to_amdgpu_dm_connector(connector);
>>> -    struct dm_connector_state *dm_con_state;
>>> +    struct dm_connector_state *dm_con_state = NULL;
>>>         struct drm_device *dev = connector->dev;
>>>       struct amdgpu_device *adev = dev->dev_private;
>>> +    bool freesync_capable = false;
>>>         if (!connector->state) {
>>>           DRM_ERROR("%s - Connector has no state", __func__);
>>> -        return;
>>> +        goto update;
>>>       }
>>>         if (!edid) {
>>> @@ -5575,9 +5581,7 @@ 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;
>>> +        goto update;
>>>       }
>>>         dm_con_state = to_dm_connector_state(connector->state);
>>> @@ -5585,10 +5589,10 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>>>       edid_check_required = false;
>>>       if (!amdgpu_dm_connector->dc_sink) {
>>>           DRM_ERROR("dc_sink NULL, could not add free_sync module.\n");
>>> -        return;
>>> +        goto update;
>>>       }
>>>       if (!adev->dm.freesync_module)
>>> -        return;
>>> +        goto update;
>>>       /*
>>>        * if edid non zero restrict freesync only for dp and edp
>>>        */
>>> @@ -5600,7 +5604,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++) {
>>> @@ -5632,8 +5635,16 @@ 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;
>>> +            freesync_capable = true;
>>>           }
>>>       }
>>> +
>>> +update:
>>> +    if (dm_con_state)
>>> +        dm_con_state->freesync_capable = freesync_capable;
>>> +
>>
>> Interesting. Looks like we could just grab this from the drm_connector property as this isn't something that changes with the state but DRM forces us to track it on the connector state as drm_object_property_get_value() throws a WARN_ON for atomic drivers. Not sure I like it but it's what we got, I guess.
>>
>> Harry
> 
> That was my original plan, but I think I understand the reasoning behind the separation of state and property.
> 
> I don't mind this approach too much because the value really feels like an implementation detail specific to our driver in this case.
> 
> It's not really describing whether the connector is actually capable or not, but whether it's fine to perform vrr updates later when we're updating the planes.
> 
>>
>>> +    if (connector->vrr_capable_property)
>>> +        drm_connector_set_vrr_capable_property(connector,
>>> +                               freesync_capable);
>>>   }
>>>   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 978b34a5011c..a5aaf8b08968 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> @@ -185,7 +185,11 @@ struct dm_crtc_state {
>>>       int crc_skip_count;
>>>       bool crc_enabled;
>>>   -    bool freesync_enabled;
>>> +    bool freesync_timing_changed;
>>> +    bool freesync_vrr_info_changed;
>>> +
>>> +    bool vrr_supported;
>>> +    struct mod_freesync_config freesync_config;
>>>       struct dc_crtc_timing_adjust adjust;
>>>       struct dc_info_packet vrr_infopacket;
>>>   };
>>> @@ -207,7 +211,6 @@ struct dm_connector_state {
>>>       uint8_t underscan_vborder;
>>>       uint8_t underscan_hborder;
>>>       bool underscan_enable;
>>> -    bool freesync_enable;
>>>       bool freesync_capable;
>>>   };
>>>  
> 
> Nicholas Kazlauskas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Pekka Paalanen]

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

On Thu, 11 Oct 2018 12:39:41 -0400
Nicholas Kazlauskas <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org> wrote:

> These include the drm_connector 'vrr_capable' and the drm_crtc
> 'vrr_enabled' properties.
> 
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
> ---
>  Documentation/gpu/drm-kms.rst   |  7 +++++++
>  drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 4b1501b4835b..8da2a178cf85 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -575,6 +575,13 @@ Explicit Fencing Properties
>  .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
>     :doc: explicit fencing properties
>  
> +
> +Variable Refresh Properties
> +---------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> +   :doc: Variable refresh properties
> +
>  Existing KMS Properties
>  -----------------------
>  
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index f0deeb7298d0..2a12853ca917 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>  
> +/**
> + * DOC: Variable refresh properties
> + *
> + * Variable refresh rate control is supported via properties on the
> + * &drm_connector and &drm_crtc objects.
> + *
> + * "vrr_capable":
> + *	Optional &drm_connector boolean property that drivers should attach
> + *	with drm_connector_attach_vrr_capable_property() on connectors that
> + *	could support variable refresh rates. Drivers should update the
> + *	property value by calling drm_connector_set_vrr_capable_property().
> + *
> + *	Absence of the property should indicate absence of support.
> + *
> + * "vrr_enabled":
> + *	Default &drm_crtc boolean property that notifies the driver that the
> + *	variable refresh rate adjustment should be enabled for the CRTC.

Hi,

where is the documentation that explains how drivers must implement
"variable refresh rate adjustment"?

What should and could userspace expect to get if it flicks this switch?

I also think the kernel documentation should include a description of
what VRR actually is and how it conceptually works as far as userspace
is concerned.

That is, the kernel documentation should describe what this thing does,
so that we avoid every driver implementing a different thing. For
example, one driver could prevent the luminance flickering itself by
tuning the timings while another driver might not do that, which means
that an application tested on the former driver will look just fine
while it is unbearable to watch on the latter.


Thanks,
pq

> + *
> + *	Support for variable refresh rate will depend on the "vrr_capable"
> + *	property exposed on the &drm_connector object.
> + */
> +
>  /**
>   * drm_connector_attach_vrr_capable_property - creates the
>   * vrr_capable property


[-- 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 v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> On Thu, 11 Oct 2018 12:39:41 -0400
> Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> wrote:
> 
>> These include the drm_connector 'vrr_capable' and the drm_crtc
>> 'vrr_enabled' properties.
>>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> ---
>>   Documentation/gpu/drm-kms.rst   |  7 +++++++
>>   drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
>> index 4b1501b4835b..8da2a178cf85 100644
>> --- a/Documentation/gpu/drm-kms.rst
>> +++ b/Documentation/gpu/drm-kms.rst
>> @@ -575,6 +575,13 @@ Explicit Fencing Properties
>>   .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
>>      :doc: explicit fencing properties
>>   
>> +
>> +Variable Refresh Properties
>> +---------------------------
>> +
>> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
>> +   :doc: Variable refresh properties
>> +
>>   Existing KMS Properties
>>   -----------------------
>>   
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index f0deeb7298d0..2a12853ca917 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>>   }
>>   EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>>   
>> +/**
>> + * DOC: Variable refresh properties
>> + *
>> + * Variable refresh rate control is supported via properties on the
>> + * &drm_connector and &drm_crtc objects.
>> + *
>> + * "vrr_capable":
>> + *	Optional &drm_connector boolean property that drivers should attach
>> + *	with drm_connector_attach_vrr_capable_property() on connectors that
>> + *	could support variable refresh rates. Drivers should update the
>> + *	property value by calling drm_connector_set_vrr_capable_property().
>> + *
>> + *	Absence of the property should indicate absence of support.
>> + *
>> + * "vrr_enabled":
>> + *	Default &drm_crtc boolean property that notifies the driver that the
>> + *	variable refresh rate adjustment should be enabled for the CRTC.
> 
> Hi,
> 
> where is the documentation that explains how drivers must implement
> "variable refresh rate adjustment"?
> 
> What should and could userspace expect to get if it flicks this switch?
> 
> I also think the kernel documentation should include a description of
> what VRR actually is and how it conceptually works as far as userspace
> is concerned.
> 
> That is, the kernel documentation should describe what this thing does,
> so that we avoid every driver implementing a different thing. For
> example, one driver could prevent the luminance flickering itself by
> tuning the timings while another driver might not do that, which means
> that an application tested on the former driver will look just fine
> while it is unbearable to watch on the latter.
> 
> 
> Thanks,
> pq

I felt it was best to leave this more on the vague side to not impose 
restrictions yet on what a driver must do.

If you think it's worth defining what the "baseline" expectation is for 
userspace, I would probably describe it as "utilizing the monitor's 
variable refresh rate to reduce stuttering or tearing that can occur 
during flipping". This is currently what the amdgpu driver has enabled 
for support. The implementation also isn't much more complex than just 
passing the variable refresh range to the hardware.

I wouldn't want every driver to be forced to implement some sort of 
luminance flickering by default. It's not noticeable on many panels and 
any tuning would inherently add latency to the output. It would probably 
be better left as a new property or a driver specific feature.

In general I would imagine that most future VRR features would end up as 
new properties. Anything that's purely software could be implemented as 
a drm helper that every driver can use. I think the target presentation 
timestamp feature is a good example for that.

> 
>> + *
>> + *	Support for variable refresh rate will depend on the "vrr_capable"
>> + *	property exposed on the &drm_connector object.
>> + */
>> +
>>   /**
>>    * drm_connector_attach_vrr_capable_property - creates the
>>    * vrr_capable property
> 

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Ville Syrjälä]

On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:
> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> > On Thu, 11 Oct 2018 12:39:41 -0400
> > Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> wrote:
> > 
> >> These include the drm_connector 'vrr_capable' and the drm_crtc
> >> 'vrr_enabled' properties.
> >>
> >> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> >> ---
> >>   Documentation/gpu/drm-kms.rst   |  7 +++++++
> >>   drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
> >>   2 files changed, 29 insertions(+)
> >>
> >> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> >> index 4b1501b4835b..8da2a178cf85 100644
> >> --- a/Documentation/gpu/drm-kms.rst
> >> +++ b/Documentation/gpu/drm-kms.rst
> >> @@ -575,6 +575,13 @@ Explicit Fencing Properties
> >>   .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
> >>      :doc: explicit fencing properties
> >>   
> >> +
> >> +Variable Refresh Properties
> >> +---------------------------
> >> +
> >> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> >> +   :doc: Variable refresh properties
> >> +
> >>   Existing KMS Properties
> >>   -----------------------
> >>   
> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >> index f0deeb7298d0..2a12853ca917 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
> >>   }
> >>   EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
> >>   
> >> +/**
> >> + * DOC: Variable refresh properties
> >> + *
> >> + * Variable refresh rate control is supported via properties on the
> >> + * &drm_connector and &drm_crtc objects.
> >> + *
> >> + * "vrr_capable":
> >> + *	Optional &drm_connector boolean property that drivers should attach
> >> + *	with drm_connector_attach_vrr_capable_property() on connectors that
> >> + *	could support variable refresh rates. Drivers should update the
> >> + *	property value by calling drm_connector_set_vrr_capable_property().
> >> + *
> >> + *	Absence of the property should indicate absence of support.
> >> + *
> >> + * "vrr_enabled":
> >> + *	Default &drm_crtc boolean property that notifies the driver that the
> >> + *	variable refresh rate adjustment should be enabled for the CRTC.
> > 
> > Hi,
> > 
> > where is the documentation that explains how drivers must implement
> > "variable refresh rate adjustment"?
> > 
> > What should and could userspace expect to get if it flicks this switch?
> > 
> > I also think the kernel documentation should include a description of
> > what VRR actually is and how it conceptually works as far as userspace
> > is concerned.
> > 
> > That is, the kernel documentation should describe what this thing does,
> > so that we avoid every driver implementing a different thing. For
> > example, one driver could prevent the luminance flickering itself by
> > tuning the timings while another driver might not do that, which means
> > that an application tested on the former driver will look just fine
> > while it is unbearable to watch on the latter.
> > 
> > 
> > Thanks,
> > pq
> 
> I felt it was best to leave this more on the vague side to not impose 
> restrictions yet on what a driver must do.
> 
> If you think it's worth defining what the "baseline" expectation is for 
> userspace, I would probably describe it as "utilizing the monitor's 
> variable refresh rate to reduce stuttering or tearing that can occur 
> during flipping". This is currently what the amdgpu driver has enabled 
> for support. The implementation also isn't much more complex than just 
> passing the variable refresh range to the hardware.
> 
> I wouldn't want every driver to be forced to implement some sort of 
> luminance flickering by default. It's not noticeable on many panels and 
> any tuning would inherently add latency to the output. It would probably 
> be better left as a new property or a driver specific feature.
> 
> In general I would imagine that most future VRR features would end up as 
> new properties. Anything that's purely software could be implemented as 
> a drm helper that every driver can use. I think the target presentation 
> timestamp feature is a good example for that.

Speaking of timestamps. What is the expected behaviour of vblank
timestamps when vrr is enabled?

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:
>> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
>>> On Thu, 11 Oct 2018 12:39:41 -0400
>>> Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> wrote:
>>>
>>>> These include the drm_connector 'vrr_capable' and the drm_crtc
>>>> 'vrr_enabled' properties.
>>>>
>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>>> ---
>>>>    Documentation/gpu/drm-kms.rst   |  7 +++++++
>>>>    drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
>>>>    2 files changed, 29 insertions(+)
>>>>
>>>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
>>>> index 4b1501b4835b..8da2a178cf85 100644
>>>> --- a/Documentation/gpu/drm-kms.rst
>>>> +++ b/Documentation/gpu/drm-kms.rst
>>>> @@ -575,6 +575,13 @@ Explicit Fencing Properties
>>>>    .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
>>>>       :doc: explicit fencing properties
>>>>    
>>>> +
>>>> +Variable Refresh Properties
>>>> +---------------------------
>>>> +
>>>> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
>>>> +   :doc: Variable refresh properties
>>>> +
>>>>    Existing KMS Properties
>>>>    -----------------------
>>>>    
>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>> index f0deeb7298d0..2a12853ca917 100644
>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>> @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>>>>    }
>>>>    EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>>>>    
>>>> +/**
>>>> + * DOC: Variable refresh properties
>>>> + *
>>>> + * Variable refresh rate control is supported via properties on the
>>>> + * &drm_connector and &drm_crtc objects.
>>>> + *
>>>> + * "vrr_capable":
>>>> + *	Optional &drm_connector boolean property that drivers should attach
>>>> + *	with drm_connector_attach_vrr_capable_property() on connectors that
>>>> + *	could support variable refresh rates. Drivers should update the
>>>> + *	property value by calling drm_connector_set_vrr_capable_property().
>>>> + *
>>>> + *	Absence of the property should indicate absence of support.
>>>> + *
>>>> + * "vrr_enabled":
>>>> + *	Default &drm_crtc boolean property that notifies the driver that the
>>>> + *	variable refresh rate adjustment should be enabled for the CRTC.
>>>
>>> Hi,
>>>
>>> where is the documentation that explains how drivers must implement
>>> "variable refresh rate adjustment"?
>>>
>>> What should and could userspace expect to get if it flicks this switch?
>>>
>>> I also think the kernel documentation should include a description of
>>> what VRR actually is and how it conceptually works as far as userspace
>>> is concerned.
>>>
>>> That is, the kernel documentation should describe what this thing does,
>>> so that we avoid every driver implementing a different thing. For
>>> example, one driver could prevent the luminance flickering itself by
>>> tuning the timings while another driver might not do that, which means
>>> that an application tested on the former driver will look just fine
>>> while it is unbearable to watch on the latter.
>>>
>>>
>>> Thanks,
>>> pq
>>
>> I felt it was best to leave this more on the vague side to not impose
>> restrictions yet on what a driver must do.
>>
>> If you think it's worth defining what the "baseline" expectation is for
>> userspace, I would probably describe it as "utilizing the monitor's
>> variable refresh rate to reduce stuttering or tearing that can occur
>> during flipping". This is currently what the amdgpu driver has enabled
>> for support. The implementation also isn't much more complex than just
>> passing the variable refresh range to the hardware.
>>
>> I wouldn't want every driver to be forced to implement some sort of
>> luminance flickering by default. It's not noticeable on many panels and
>> any tuning would inherently add latency to the output. It would probably
>> be better left as a new property or a driver specific feature.
>>
>> In general I would imagine that most future VRR features would end up as
>> new properties. Anything that's purely software could be implemented as
>> a drm helper that every driver can use. I think the target presentation
>> timestamp feature is a good example for that.
> 
> Speaking of timestamps. What is the expected behaviour of vblank
> timestamps when vrr is enabled?
>

When vrr is enabled the duration of the vertical front porch will be
extended until flip or timeout occurs. The vblank timestamp will vary
based on duration of the vertical front porch. The min/max duration for
the front porch can be specified by the driver via the min/max range.

No changes to vblank timestamping handling should be necessary to
accommodate variable refresh rate.

I think it's probably best to update the documentation for vrr_enable 
with some of the specifics I described above. That should help clarify 
userspace expectations as well.

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Ville Syrjälä]

On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> > On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:
> >> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> >>> On Thu, 11 Oct 2018 12:39:41 -0400
> >>> Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> wrote:
> >>>
> >>>> These include the drm_connector 'vrr_capable' and the drm_crtc
> >>>> 'vrr_enabled' properties.
> >>>>
> >>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> >>>> ---
> >>>>    Documentation/gpu/drm-kms.rst   |  7 +++++++
> >>>>    drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
> >>>>    2 files changed, 29 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> >>>> index 4b1501b4835b..8da2a178cf85 100644
> >>>> --- a/Documentation/gpu/drm-kms.rst
> >>>> +++ b/Documentation/gpu/drm-kms.rst
> >>>> @@ -575,6 +575,13 @@ Explicit Fencing Properties
> >>>>    .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
> >>>>       :doc: explicit fencing properties
> >>>>    
> >>>> +
> >>>> +Variable Refresh Properties
> >>>> +---------------------------
> >>>> +
> >>>> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> >>>> +   :doc: Variable refresh properties
> >>>> +
> >>>>    Existing KMS Properties
> >>>>    -----------------------
> >>>>    
> >>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >>>> index f0deeb7298d0..2a12853ca917 100644
> >>>> --- a/drivers/gpu/drm/drm_connector.c
> >>>> +++ b/drivers/gpu/drm/drm_connector.c
> >>>> @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
> >>>>    }
> >>>>    EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
> >>>>    
> >>>> +/**
> >>>> + * DOC: Variable refresh properties
> >>>> + *
> >>>> + * Variable refresh rate control is supported via properties on the
> >>>> + * &drm_connector and &drm_crtc objects.
> >>>> + *
> >>>> + * "vrr_capable":
> >>>> + *	Optional &drm_connector boolean property that drivers should attach
> >>>> + *	with drm_connector_attach_vrr_capable_property() on connectors that
> >>>> + *	could support variable refresh rates. Drivers should update the
> >>>> + *	property value by calling drm_connector_set_vrr_capable_property().
> >>>> + *
> >>>> + *	Absence of the property should indicate absence of support.
> >>>> + *
> >>>> + * "vrr_enabled":
> >>>> + *	Default &drm_crtc boolean property that notifies the driver that the
> >>>> + *	variable refresh rate adjustment should be enabled for the CRTC.
> >>>
> >>> Hi,
> >>>
> >>> where is the documentation that explains how drivers must implement
> >>> "variable refresh rate adjustment"?
> >>>
> >>> What should and could userspace expect to get if it flicks this switch?
> >>>
> >>> I also think the kernel documentation should include a description of
> >>> what VRR actually is and how it conceptually works as far as userspace
> >>> is concerned.
> >>>
> >>> That is, the kernel documentation should describe what this thing does,
> >>> so that we avoid every driver implementing a different thing. For
> >>> example, one driver could prevent the luminance flickering itself by
> >>> tuning the timings while another driver might not do that, which means
> >>> that an application tested on the former driver will look just fine
> >>> while it is unbearable to watch on the latter.
> >>>
> >>>
> >>> Thanks,
> >>> pq
> >>
> >> I felt it was best to leave this more on the vague side to not impose
> >> restrictions yet on what a driver must do.
> >>
> >> If you think it's worth defining what the "baseline" expectation is for
> >> userspace, I would probably describe it as "utilizing the monitor's
> >> variable refresh rate to reduce stuttering or tearing that can occur
> >> during flipping". This is currently what the amdgpu driver has enabled
> >> for support. The implementation also isn't much more complex than just
> >> passing the variable refresh range to the hardware.
> >>
> >> I wouldn't want every driver to be forced to implement some sort of
> >> luminance flickering by default. It's not noticeable on many panels and
> >> any tuning would inherently add latency to the output. It would probably
> >> be better left as a new property or a driver specific feature.
> >>
> >> In general I would imagine that most future VRR features would end up as
> >> new properties. Anything that's purely software could be implemented as
> >> a drm helper that every driver can use. I think the target presentation
> >> timestamp feature is a good example for that.
> > 
> > Speaking of timestamps. What is the expected behaviour of vblank
> > timestamps when vrr is enabled?
> >
> 
> When vrr is enabled the duration of the vertical front porch will be
> extended until flip or timeout occurs. The vblank timestamp will vary
> based on duration of the vertical front porch. The min/max duration for
> the front porch can be specified by the driver via the min/max range.
> 
> No changes to vblank timestamping handling should be necessary to
> accommodate variable refresh rate.

The problem is that the timestamp is supposed to correspond to the first
active pixel. And since we don't know how long the front porch will be
we can't realistically report the true value. So I guess just assuming
min front porch length is as good as anything else?

> 
> I think it's probably best to update the documentation for vrr_enable 
> with some of the specifics I described above. That should help clarify 
> userspace expectations as well.
> 
> 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 v4 3/4] drm: Document variable refresh properties

[Manasi Navare]

On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> > On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:
> >> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> >>> On Thu, 11 Oct 2018 12:39:41 -0400
> >>> Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> wrote:
> >>>
> >>>> These include the drm_connector 'vrr_capable' and the drm_crtc
> >>>> 'vrr_enabled' properties.
> >>>>
> >>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> >>>> ---
> >>>>    Documentation/gpu/drm-kms.rst   |  7 +++++++
> >>>>    drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
> >>>>    2 files changed, 29 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> >>>> index 4b1501b4835b..8da2a178cf85 100644
> >>>> --- a/Documentation/gpu/drm-kms.rst
> >>>> +++ b/Documentation/gpu/drm-kms.rst
> >>>> @@ -575,6 +575,13 @@ Explicit Fencing Properties
> >>>>    .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
> >>>>       :doc: explicit fencing properties
> >>>>    
> >>>> +
> >>>> +Variable Refresh Properties
> >>>> +---------------------------
> >>>> +
> >>>> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> >>>> +   :doc: Variable refresh properties
> >>>> +
> >>>>    Existing KMS Properties
> >>>>    -----------------------
> >>>>    
> >>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >>>> index f0deeb7298d0..2a12853ca917 100644
> >>>> --- a/drivers/gpu/drm/drm_connector.c
> >>>> +++ b/drivers/gpu/drm/drm_connector.c
> >>>> @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
> >>>>    }
> >>>>    EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
> >>>>    
> >>>> +/**
> >>>> + * DOC: Variable refresh properties
> >>>> + *
> >>>> + * Variable refresh rate control is supported via properties on the
> >>>> + * &drm_connector and &drm_crtc objects.
> >>>> + *
> >>>> + * "vrr_capable":
> >>>> + *	Optional &drm_connector boolean property that drivers should attach
> >>>> + *	with drm_connector_attach_vrr_capable_property() on connectors that
> >>>> + *	could support variable refresh rates. Drivers should update the
> >>>> + *	property value by calling drm_connector_set_vrr_capable_property().
> >>>> + *
> >>>> + *	Absence of the property should indicate absence of support.
> >>>> + *
> >>>> + * "vrr_enabled":
> >>>> + *	Default &drm_crtc boolean property that notifies the driver that the
> >>>> + *	variable refresh rate adjustment should be enabled for the CRTC.
> >>>
> >>> Hi,
> >>>
> >>> where is the documentation that explains how drivers must implement
> >>> "variable refresh rate adjustment"?
> >>>
> >>> What should and could userspace expect to get if it flicks this switch?
> >>>
> >>> I also think the kernel documentation should include a description of
> >>> what VRR actually is and how it conceptually works as far as userspace
> >>> is concerned.
> >>>
> >>> That is, the kernel documentation should describe what this thing does,
> >>> so that we avoid every driver implementing a different thing. For
> >>> example, one driver could prevent the luminance flickering itself by
> >>> tuning the timings while another driver might not do that, which means
> >>> that an application tested on the former driver will look just fine
> >>> while it is unbearable to watch on the latter.
> >>>
> >>>
> >>> Thanks,
> >>> pq
> >>
> >> I felt it was best to leave this more on the vague side to not impose
> >> restrictions yet on what a driver must do.
> >>
> >> If you think it's worth defining what the "baseline" expectation is for
> >> userspace, I would probably describe it as "utilizing the monitor's
> >> variable refresh rate to reduce stuttering or tearing that can occur
> >> during flipping". This is currently what the amdgpu driver has enabled

I would also mention that without VRR, the display engine processes the flips 
independently of the rendering speed which might result into stuttering or tearing.

Might also be worth giving a quick example with numbers in the documentation.
Something like: For Eg if the rendering speed is 40Hz, without VRR, display engine
will flip at constant 60Hz, which means that same frame will be displayed twice which
will be observed as stutter/repetition.
With VRR enabled, the vertical front porch will be extended and the flip will
be processed when its ready or at max blanking time resulting a smooth transition without repetition.

> >> for support. The implementation also isn't much more complex than just
> >> passing the variable refresh range to the hardware.
> >>
> >> I wouldn't want every driver to be forced to implement some sort of
> >> luminance flickering by default. It's not noticeable on many panels and
> >> any tuning would inherently add latency to the output. It would probably
> >> be better left as a new property or a driver specific feature.
> >>
> >> In general I would imagine that most future VRR features would end up as
> >> new properties. Anything that's purely software could be implemented as
> >> a drm helper that every driver can use. I think the target presentation
> >> timestamp feature is a good example for that.
> > 
> > Speaking of timestamps. What is the expected behaviour of vblank
> > timestamps when vrr is enabled?
> >
> 
> When vrr is enabled the duration of the vertical front porch will be
> extended until flip or timeout occurs. The vblank timestamp will vary
> based on duration of the vertical front porch. The min/max duration for
> the front porch can be specified by the driver via the min/max range.

This min max range that is read from monitor descriptor is only used to program
the HW registers right? Its not exposed to the userspace, correct?

Manasi

> 
> No changes to vblank timestamping handling should be necessary to
> accommodate variable refresh rate.
> 
> I think it's probably best to update the documentation for vrr_enable 
> with some of the specifics I described above. That should help clarify 
> userspace expectations as well.
> 
> Nicholas Kazlauskas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/26/18 3:13 PM, Manasi Navare wrote:
> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>> On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:
>>>> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
>>>>> On Thu, 11 Oct 2018 12:39:41 -0400
>>>>> Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> wrote:
>>>>>
>>>>>> These include the drm_connector 'vrr_capable' and the drm_crtc
>>>>>> 'vrr_enabled' properties.
>>>>>>
>>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>>>>> ---
>>>>>>     Documentation/gpu/drm-kms.rst   |  7 +++++++
>>>>>>     drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
>>>>>>     2 files changed, 29 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
>>>>>> index 4b1501b4835b..8da2a178cf85 100644
>>>>>> --- a/Documentation/gpu/drm-kms.rst
>>>>>> +++ b/Documentation/gpu/drm-kms.rst
>>>>>> @@ -575,6 +575,13 @@ Explicit Fencing Properties
>>>>>>     .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
>>>>>>        :doc: explicit fencing properties
>>>>>>     
>>>>>> +
>>>>>> +Variable Refresh Properties
>>>>>> +---------------------------
>>>>>> +
>>>>>> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
>>>>>> +   :doc: Variable refresh properties
>>>>>> +
>>>>>>     Existing KMS Properties
>>>>>>     -----------------------
>>>>>>     
>>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>>>> index f0deeb7298d0..2a12853ca917 100644
>>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>>> @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>>>>>>     }
>>>>>>     EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>>>>>>     
>>>>>> +/**
>>>>>> + * DOC: Variable refresh properties
>>>>>> + *
>>>>>> + * Variable refresh rate control is supported via properties on the
>>>>>> + * &drm_connector and &drm_crtc objects.
>>>>>> + *
>>>>>> + * "vrr_capable":
>>>>>> + *	Optional &drm_connector boolean property that drivers should attach
>>>>>> + *	with drm_connector_attach_vrr_capable_property() on connectors that
>>>>>> + *	could support variable refresh rates. Drivers should update the
>>>>>> + *	property value by calling drm_connector_set_vrr_capable_property().
>>>>>> + *
>>>>>> + *	Absence of the property should indicate absence of support.
>>>>>> + *
>>>>>> + * "vrr_enabled":
>>>>>> + *	Default &drm_crtc boolean property that notifies the driver that the
>>>>>> + *	variable refresh rate adjustment should be enabled for the CRTC.
>>>>>
>>>>> Hi,
>>>>>
>>>>> where is the documentation that explains how drivers must implement
>>>>> "variable refresh rate adjustment"?
>>>>>
>>>>> What should and could userspace expect to get if it flicks this switch?
>>>>>
>>>>> I also think the kernel documentation should include a description of
>>>>> what VRR actually is and how it conceptually works as far as userspace
>>>>> is concerned.
>>>>>
>>>>> That is, the kernel documentation should describe what this thing does,
>>>>> so that we avoid every driver implementing a different thing. For
>>>>> example, one driver could prevent the luminance flickering itself by
>>>>> tuning the timings while another driver might not do that, which means
>>>>> that an application tested on the former driver will look just fine
>>>>> while it is unbearable to watch on the latter.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> pq
>>>>
>>>> I felt it was best to leave this more on the vague side to not impose
>>>> restrictions yet on what a driver must do.
>>>>
>>>> If you think it's worth defining what the "baseline" expectation is for
>>>> userspace, I would probably describe it as "utilizing the monitor's
>>>> variable refresh rate to reduce stuttering or tearing that can occur
>>>> during flipping". This is currently what the amdgpu driver has enabled
> 
> I would also mention that without VRR, the display engine processes the flips
> independently of the rendering speed which might result into stuttering or tearing.
> 
> Might also be worth giving a quick example with numbers in the documentation.
> Something like: For Eg if the rendering speed is 40Hz, without VRR, display engine
> will flip at constant 60Hz, which means that same frame will be displayed twice which
> will be observed as stutter/repetition.
> With VRR enabled, the vertical front porch will be extended and the flip will
> be processed when its ready or at max blanking time resulting a smooth transition without repetition.

Good points about mentioning the problems it solves in the documentation.

> 
>>>> for support. The implementation also isn't much more complex than just
>>>> passing the variable refresh range to the hardware.
>>>>
>>>> I wouldn't want every driver to be forced to implement some sort of
>>>> luminance flickering by default. It's not noticeable on many panels and
>>>> any tuning would inherently add latency to the output. It would probably
>>>> be better left as a new property or a driver specific feature.
>>>>
>>>> In general I would imagine that most future VRR features would end up as
>>>> new properties. Anything that's purely software could be implemented as
>>>> a drm helper that every driver can use. I think the target presentation
>>>> timestamp feature is a good example for that.
>>>
>>> Speaking of timestamps. What is the expected behaviour of vblank
>>> timestamps when vrr is enabled?
>>>
>>
>> When vrr is enabled the duration of the vertical front porch will be
>> extended until flip or timeout occurs. The vblank timestamp will vary
>> based on duration of the vertical front porch. The min/max duration for
>> the front porch can be specified by the driver via the min/max range.
> 
> This min max range that is read from monitor descriptor is only used to program
> the HW registers right? Its not exposed to the userspace, correct?
> 
> Manasi

Currently the range isn't exposed to userspace.

I think I wouldn't mind having them for testing purposes but that can 
just be done via debugfs. They might make more sense as properties but I 
don't have any integration patches to utilize them in userspace.

> 
>>
>> No changes to vblank timestamping handling should be necessary to
>> accommodate variable refresh rate.
>>
>> I think it's probably best to update the documentation for vrr_enable
>> with some of the specifics I described above. That should help clarify
>> userspace expectations as well.
>>
>> Nicholas Kazlauskas

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Manasi Navare]

On Fri, Oct 26, 2018 at 08:06:11PM +0000, Kazlauskas, Nicholas wrote:
> On 10/26/18 3:13 PM, Manasi Navare wrote:
> > On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
> >> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> >>> On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:
> >>>> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> >>>>> On Thu, 11 Oct 2018 12:39:41 -0400
> >>>>> Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> wrote:
> >>>>>
> >>>>>> These include the drm_connector 'vrr_capable' and the drm_crtc
> >>>>>> 'vrr_enabled' properties.
> >>>>>>
> >>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> >>>>>> ---
> >>>>>>     Documentation/gpu/drm-kms.rst   |  7 +++++++
> >>>>>>     drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
> >>>>>>     2 files changed, 29 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> >>>>>> index 4b1501b4835b..8da2a178cf85 100644
> >>>>>> --- a/Documentation/gpu/drm-kms.rst
> >>>>>> +++ b/Documentation/gpu/drm-kms.rst
> >>>>>> @@ -575,6 +575,13 @@ Explicit Fencing Properties
> >>>>>>     .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
> >>>>>>        :doc: explicit fencing properties
> >>>>>>     
> >>>>>> +
> >>>>>> +Variable Refresh Properties
> >>>>>> +---------------------------
> >>>>>> +
> >>>>>> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> >>>>>> +   :doc: Variable refresh properties
> >>>>>> +
> >>>>>>     Existing KMS Properties
> >>>>>>     -----------------------
> >>>>>>     
> >>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >>>>>> index f0deeb7298d0..2a12853ca917 100644
> >>>>>> --- a/drivers/gpu/drm/drm_connector.c
> >>>>>> +++ b/drivers/gpu/drm/drm_connector.c
> >>>>>> @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
> >>>>>>     }
> >>>>>>     EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
> >>>>>>     
> >>>>>> +/**
> >>>>>> + * DOC: Variable refresh properties
> >>>>>> + *
> >>>>>> + * Variable refresh rate control is supported via properties on the
> >>>>>> + * &drm_connector and &drm_crtc objects.
> >>>>>> + *
> >>>>>> + * "vrr_capable":
> >>>>>> + *	Optional &drm_connector boolean property that drivers should attach
> >>>>>> + *	with drm_connector_attach_vrr_capable_property() on connectors that
> >>>>>> + *	could support variable refresh rates. Drivers should update the
> >>>>>> + *	property value by calling drm_connector_set_vrr_capable_property().
> >>>>>> + *
> >>>>>> + *	Absence of the property should indicate absence of support.
> >>>>>> + *
> >>>>>> + * "vrr_enabled":
> >>>>>> + *	Default &drm_crtc boolean property that notifies the driver that the
> >>>>>> + *	variable refresh rate adjustment should be enabled for the CRTC.
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> where is the documentation that explains how drivers must implement
> >>>>> "variable refresh rate adjustment"?
> >>>>>
> >>>>> What should and could userspace expect to get if it flicks this switch?
> >>>>>
> >>>>> I also think the kernel documentation should include a description of
> >>>>> what VRR actually is and how it conceptually works as far as userspace
> >>>>> is concerned.
> >>>>>
> >>>>> That is, the kernel documentation should describe what this thing does,
> >>>>> so that we avoid every driver implementing a different thing. For
> >>>>> example, one driver could prevent the luminance flickering itself by
> >>>>> tuning the timings while another driver might not do that, which means
> >>>>> that an application tested on the former driver will look just fine
> >>>>> while it is unbearable to watch on the latter.
> >>>>>
> >>>>>
> >>>>> Thanks,
> >>>>> pq
> >>>>
> >>>> I felt it was best to leave this more on the vague side to not impose
> >>>> restrictions yet on what a driver must do.
> >>>>
> >>>> If you think it's worth defining what the "baseline" expectation is for
> >>>> userspace, I would probably describe it as "utilizing the monitor's
> >>>> variable refresh rate to reduce stuttering or tearing that can occur
> >>>> during flipping". This is currently what the amdgpu driver has enabled
> > 
> > I would also mention that without VRR, the display engine processes the flips
> > independently of the rendering speed which might result into stuttering or tearing.
> > 
> > Might also be worth giving a quick example with numbers in the documentation.
> > Something like: For Eg if the rendering speed is 40Hz, without VRR, display engine
> > will flip at constant 60Hz, which means that same frame will be displayed twice which
> > will be observed as stutter/repetition.
> > With VRR enabled, the vertical front porch will be extended and the flip will
> > be processed when its ready or at max blanking time resulting a smooth transition without repetition.
> 
> Good points about mentioning the problems it solves in the documentation.
> 
> > 
> >>>> for support. The implementation also isn't much more complex than just
> >>>> passing the variable refresh range to the hardware.
> >>>>
> >>>> I wouldn't want every driver to be forced to implement some sort of
> >>>> luminance flickering by default. It's not noticeable on many panels and
> >>>> any tuning would inherently add latency to the output. It would probably
> >>>> be better left as a new property or a driver specific feature.
> >>>>
> >>>> In general I would imagine that most future VRR features would end up as
> >>>> new properties. Anything that's purely software could be implemented as
> >>>> a drm helper that every driver can use. I think the target presentation
> >>>> timestamp feature is a good example for that.
> >>>
> >>> Speaking of timestamps. What is the expected behaviour of vblank
> >>> timestamps when vrr is enabled?
> >>>
> >>
> >> When vrr is enabled the duration of the vertical front porch will be
> >> extended until flip or timeout occurs. The vblank timestamp will vary
> >> based on duration of the vertical front porch. The min/max duration for
> >> the front porch can be specified by the driver via the min/max range.
> > 
> > This min max range that is read from monitor descriptor is only used to program
> > the HW registers right? Its not exposed to the userspace, correct?
> > 
> > Manasi
> 
> Currently the range isn't exposed to userspace.
> 
> I think I wouldn't mind having them for testing purposes but that can 
> just be done via debugfs. They might make more sense as properties but I 
> don't have any integration patches to utilize them in userspace.
> 
> > 
> >>
> >> No changes to vblank timestamping handling should be necessary to
> >> accommodate variable refresh rate.
> >>
> >> I think it's probably best to update the documentation for vrr_enable
> >> with some of the specifics I described above. That should help clarify
> >> userspace expectations as well.
> >>
> >> Nicholas Kazlauskas

Yes debugfs for min, max Vrefresh range will be good.

Manasi

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Pekka Paalanen]

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

On Fri, 26 Oct 2018 17:34:15 +0000
"Kazlauskas, Nicholas" <Nicholas.Kazlauskas-5C7GfCeVMHo@public.gmane.org> wrote:

> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> > On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:  
> >> On 10/26/18 7:37 AM, Pekka Paalanen wrote:  

> >>> Hi,
> >>>
> >>> where is the documentation that explains how drivers must implement
> >>> "variable refresh rate adjustment"?
> >>>
> >>> What should and could userspace expect to get if it flicks this switch?
> >>>
> >>> I also think the kernel documentation should include a description of
> >>> what VRR actually is and how it conceptually works as far as userspace
> >>> is concerned.
> >>>
> >>> That is, the kernel documentation should describe what this thing does,
> >>> so that we avoid every driver implementing a different thing. For
> >>> example, one driver could prevent the luminance flickering itself by
> >>> tuning the timings while another driver might not do that, which means
> >>> that an application tested on the former driver will look just fine
> >>> while it is unbearable to watch on the latter.
> >>>
> >>>
> >>> Thanks,
> >>> pq  
> >>
> >> I felt it was best to leave this more on the vague side to not impose
> >> restrictions yet on what a driver must do.
> >>
> >> If you think it's worth defining what the "baseline" expectation is for
> >> userspace, I would probably describe it as "utilizing the monitor's
> >> variable refresh rate to reduce stuttering or tearing that can occur
> >> during flipping". This is currently what the amdgpu driver has enabled
> >> for support. The implementation also isn't much more complex than just
> >> passing the variable refresh range to the hardware.

Hi,

sorry, but that says nothing.

Also, tearing has nothing to do here. VRR reduces stuttering if
userspace is already tear-free. If userspace was driving the display in
a tearing fashion, VRR will not reduce or remove tearing, it just makes
it happen at different points and times. Tearing happens because
framebuffer flips are executed regardless of the refresh cycle, so
adjusting the refresh timings won't help.

However...

> >>
> >> I wouldn't want every driver to be forced to implement some sort of
> >> luminance flickering by default. It's not noticeable on many panels and
> >> any tuning would inherently add latency to the output. It would probably
> >> be better left as a new property or a driver specific feature.

The point is to give userspace guaranteed expectations. If the
expectation is that some displays might actually emit bad luminance
flickering, then userspace must always assume the worst and implement
the needed slewing algorithms to avoid it, even if it would be
unnecessary.

Userspace is farther away from the hardware than the drivers are, and
if userspace is required to implement luminance flickering avoidance,
that implementation must be done in all display servers and KMS apps
that might enable VRR. That would also call for a userspace hardware
database, so that people can set up quirks to enable/disable/adjust the
algorithms for specific hardware with the hopes that other users could
then have a good out-of-the-box experience. Instead, if possible, I
would like to see some guarantees from the kernel drivers that
userspace does not need to worry about luminance flickering.

Unless you would deem all hardware that can exhibit luminance
flickering as faulty and unsupported?

We need a baseline default expectation. You can modify that expectation
later with new properties, but I believe something needs to be defined
as the default. Even if the definition is really just "hardware takes
care of not flickering".

> >>
> >> In general I would imagine that most future VRR features would end up as
> >> new properties. Anything that's purely software could be implemented as
> >> a drm helper that every driver can use. I think the target presentation
> >> timestamp feature is a good example for that.  
> > 
> > Speaking of timestamps. What is the expected behaviour of vblank
> > timestamps when vrr is enabled?
> >  
> 
> When vrr is enabled the duration of the vertical front porch will be
> extended until flip or timeout occurs. The vblank timestamp will vary
> based on duration of the vertical front porch. The min/max duration for
> the front porch can be specified by the driver via the min/max range.

...This is actually useful information, it explains things. Do all VRR
hardware implementations fundamentally work like this?

With that definition, there is only more parameter to be exposed to
userspace in the future: what is the length of the timeout? No need to
expose maximum-refresh-freq because that information is already present
in the programmed video mode.

Btw. even this definition does not give any hint about problems like
the luminance flickering. If possible flickering is an issue that
cannot be simply ignored, then something should be documented about it.

Do you know of any other "hidden" issues that could require userspace
or drivers to be careful in how it will drive VRR?


Thanks,
pq

> No changes to vblank timestamping handling should be necessary to
> accommodate variable refresh rate.
> 
> I think it's probably best to update the documentation for vrr_enable 
> with some of the specifics I described above. That should help clarify 
> userspace expectations as well.
> 
> Nicholas Kazlauskas


[-- 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 v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/29/18 4:36 AM, Pekka Paalanen wrote:
> On Fri, 26 Oct 2018 17:34:15 +0000
> "Kazlauskas, Nicholas" <Nicholas.Kazlauskas@amd.com> wrote:
> 
>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>> On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:
>>>> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> 
>>>>> Hi,
>>>>>
>>>>> where is the documentation that explains how drivers must implement
>>>>> "variable refresh rate adjustment"?
>>>>>
>>>>> What should and could userspace expect to get if it flicks this switch?
>>>>>
>>>>> I also think the kernel documentation should include a description of
>>>>> what VRR actually is and how it conceptually works as far as userspace
>>>>> is concerned.
>>>>>
>>>>> That is, the kernel documentation should describe what this thing does,
>>>>> so that we avoid every driver implementing a different thing. For
>>>>> example, one driver could prevent the luminance flickering itself by
>>>>> tuning the timings while another driver might not do that, which means
>>>>> that an application tested on the former driver will look just fine
>>>>> while it is unbearable to watch on the latter.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> pq
>>>>
>>>> I felt it was best to leave this more on the vague side to not impose
>>>> restrictions yet on what a driver must do.
>>>>
>>>> If you think it's worth defining what the "baseline" expectation is for
>>>> userspace, I would probably describe it as "utilizing the monitor's
>>>> variable refresh rate to reduce stuttering or tearing that can occur
>>>> during flipping". This is currently what the amdgpu driver has enabled
>>>> for support. The implementation also isn't much more complex than just
>>>> passing the variable refresh range to the hardware.
> 
> Hi,
> 
> sorry, but that says nothing.
> 
> Also, tearing has nothing to do here. VRR reduces stuttering if
> userspace is already tear-free. If userspace was driving the display in
> a tearing fashion, VRR will not reduce or remove tearing, it just makes
> it happen at different points and times. Tearing happens because
> framebuffer flips are executed regardless of the refresh cycle, so
> adjusting the refresh timings won't help. >
> However...
> 
>>>>
>>>> I wouldn't want every driver to be forced to implement some sort of
>>>> luminance flickering by default. It's not noticeable on many panels and
>>>> any tuning would inherently add latency to the output. It would probably
>>>> be better left as a new property or a driver specific feature.
> 
> The point is to give userspace guaranteed expectations. If the
> expectation is that some displays might actually emit bad luminance
> flickering, then userspace must always assume the worst and implement
> the needed slewing algorithms to avoid it, even if it would be
> unnecessary.
> 
> Userspace is farther away from the hardware than the drivers are, and
> if userspace is required to implement luminance flickering avoidance,
> that implementation must be done in all display servers and KMS apps
> that might enable VRR. That would also call for a userspace hardware
> database, so that people can set up quirks to enable/disable/adjust the
> algorithms for specific hardware with the hopes that other users could
> then have a good out-of-the-box experience. Instead, if possible, I
> would like to see some guarantees from the kernel drivers that
> userspace does not need to worry about luminance flickering.
> 
> Unless you would deem all hardware that can exhibit luminance
> flickering as faulty and unsupported?
> 
> We need a baseline default expectation. You can modify that expectation
> later with new properties, but I believe something needs to be defined
> as the default. Even if the definition is really just "hardware takes
> care of not flickering".
> 
>>>>
>>>> In general I would imagine that most future VRR features would end up as
>>>> new properties. Anything that's purely software could be implemented as
>>>> a drm helper that every driver can use. I think the target presentation
>>>> timestamp feature is a good example for that.
>>>
>>> Speaking of timestamps. What is the expected behaviour of vblank
>>> timestamps when vrr is enabled?
>>>   
>>
>> When vrr is enabled the duration of the vertical front porch will be
>> extended until flip or timeout occurs. The vblank timestamp will vary
>> based on duration of the vertical front porch. The min/max duration for
>> the front porch can be specified by the driver via the min/max range.
> 
> ...This is actually useful information, it explains things. Do all VRR
> hardware implementations fundamentally work like this?
> 
> With that definition, there is only more parameter to be exposed to
> userspace in the future: what is the length of the timeout? No need to
> expose maximum-refresh-freq because that information is already present
> in the programmed video mode.
> 
> Btw. even this definition does not give any hint about problems like
> the luminance flickering. If possible flickering is an issue that
> cannot be simply ignored, then something should be documented about it.
> 
> Do you know of any other "hidden" issues that could require userspace
> or drivers to be careful in how it will drive VRR?
> 
> 
> Thanks,
> pq
There are two issues that can manifest when enabling variable refresh 
rate - "stuttering" and luminance flickering. Luminance flickering is
dependent on the panel, but stuttering will occur on anything (and will 
be worse on panels with a wider range). Both of these issues can occur 
for content that flips on demand or at random. The large and frequent 
fluctuations in flip times remove any sort of consistency for user input 
and rendering.

The stuttering is actually the primary reason the mesa patches exist
(with luminance flickering removal being a bonus). Whether variable 
refresh rate should be enabled or not will depend on whether the 
userspace content is appropriate or not.

A skewing algorithm would address both issues but would still make the
user experience worse for those applications. Using a fixed rate for 
consistency is likely still preferable for those applications.

This is why I'd prefer to define a baseline like:

"When the vrr_enable property is set to true for vrr_capable hardware 
the vertical front porch will be extended until either flip or timeout 
occurs.

The vertical front porch timing and timeout duration are determined by 
the driver based on the panel's reported minimum and maximum variable 
refresh range."

The patches don't presently expose the minimum/maximum range but this 
does guarantee bounds.

The inner bounds would then be determined by the driver to allow 
flexibility in implementing algorithms that can improve the user experience.

An example of such an algorithm would be low framerate compensation for 
flip rates below the range. The amdgpu driver has an implementation for 
this that can do frame doubling and tripling for monitors with an 
appropriate range.

You could also implement some sort of skewing for luminance flickering 
reduction for panels that really need it on top of this.

Future properties that utilize variable refresh rate would be compatible 
with this baseline - everything is based on the determination of the 
inner bounds on minimum and maximum refresh.

> 
>> No changes to vblank timestamping handling should be necessary to
>> accommodate variable refresh rate.
>>
>> I think it's probably best to update the documentation for vrr_enable
>> with some of the specifics I described above. That should help clarify
>> userspace expectations as well.
>>
>> Nicholas Kazlauskas
> 

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Michel Dänzer]

On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>>
>>> Speaking of timestamps. What is the expected behaviour of vblank
>>> timestamps when vrr is enabled?
>>
>> When vrr is enabled the duration of the vertical front porch will be
>> extended until flip or timeout occurs. The vblank timestamp will vary
>> based on duration of the vertical front porch. The min/max duration for
>> the front porch can be specified by the driver via the min/max range.
>>
>> No changes to vblank timestamping handling should be necessary to
>> accommodate variable refresh rate.
> 
> The problem is that the timestamp is supposed to correspond to the first
> active pixel. And since we don't know how long the front porch will be
> we can't realistically report the true value. So I guess just assuming
> min front porch length is as good as anything else?

That (and documenting that the timestamp corresponds to the earliest
possible first active pixel, not necessarily the actual one, with VRR)
might be good enough for the actual vblank event timestamps.


However, I'm not so sure about the timestamps of page flip completion
events. Those could be very misleading if the flip completes towards the
timeout, which could result in bad behaviour of applications which use
them for animation timing.

Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
:) in drm_crtc_send_vblank_event?


-- 
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 v4 3/4] drm: Document variable refresh properties

[Ville Syrjälä]

On Mon, Oct 29, 2018 at 05:37:49PM +0100, Michel Dänzer wrote:
> On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
> > On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
> >> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> >>>
> >>> Speaking of timestamps. What is the expected behaviour of vblank
> >>> timestamps when vrr is enabled?
> >>
> >> When vrr is enabled the duration of the vertical front porch will be
> >> extended until flip or timeout occurs. The vblank timestamp will vary
> >> based on duration of the vertical front porch. The min/max duration for
> >> the front porch can be specified by the driver via the min/max range.
> >>
> >> No changes to vblank timestamping handling should be necessary to
> >> accommodate variable refresh rate.
> > 
> > The problem is that the timestamp is supposed to correspond to the first
> > active pixel. And since we don't know how long the front porch will be
> > we can't realistically report the true value. So I guess just assuming
> > min front porch length is as good as anything else?
> 
> That (and documenting that the timestamp corresponds to the earliest
> possible first active pixel, not necessarily the actual one, with VRR)
> might be good enough for the actual vblank event timestamps.
> 
> 
> However, I'm not so sure about the timestamps of page flip completion
> events. Those could be very misleading if the flip completes towards the
> timeout, which could result in bad behaviour of applications which use
> them for animation timing.
> 
> Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
> :) in drm_crtc_send_vblank_event?

Hmm. Updated how? Whether it's a page flip event or vblank even we won't
know when the first active pixel will come. Although I suppose if
there is some kind of vrr slew rate limit we could at least account
for that to report a more correct "this is the earliest you migth be
able to see your frame" timestamp.

Oh, or are you actually saying that shceduling a new flip before the
timeout is actually going to latch that flip immediately? I figured
that the flip would get latched on the next start of vblank regardless,
and the act of scheduling a flip will just kick the hardware to start
scanning the previously latched frame earlier.

scanout A | ... vblank | scanout A | ... vblank | scanout B | ... vblank
                  ^                ^        ^               ^
                  |                |        flip C          latch C
                  flip B           latch B

vs.

scanout A | ... vblank | scanout B | ... vblank | scanout C | ... vblank
                  ^ ^                      ^ ^
                  | latch B                | latch C
                  flip B                   flip C

The latter would seem more like a tearing flip without the tearing.

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/29/18 2:03 PM, Ville Syrjälä wrote:
> On Mon, Oct 29, 2018 at 05:37:49PM +0100, Michel Dänzer wrote:
>> On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
>>> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>>>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>>>>
>>>>> Speaking of timestamps. What is the expected behaviour of vblank
>>>>> timestamps when vrr is enabled?
>>>>
>>>> When vrr is enabled the duration of the vertical front porch will be
>>>> extended until flip or timeout occurs. The vblank timestamp will vary
>>>> based on duration of the vertical front porch. The min/max duration for
>>>> the front porch can be specified by the driver via the min/max range.
>>>>
>>>> No changes to vblank timestamping handling should be necessary to
>>>> accommodate variable refresh rate.
>>>
>>> The problem is that the timestamp is supposed to correspond to the first
>>> active pixel. And since we don't know how long the front porch will be
>>> we can't realistically report the true value. So I guess just assuming
>>> min front porch length is as good as anything else?
>>
>> That (and documenting that the timestamp corresponds to the earliest
>> possible first active pixel, not necessarily the actual one, with VRR)
>> might be good enough for the actual vblank event timestamps.
>>
>>
>> However, I'm not so sure about the timestamps of page flip completion
>> events. Those could be very misleading if the flip completes towards the
>> timeout, which could result in bad behaviour of applications which use
>> them for animation timing.
>>
>> Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
>> :) in drm_crtc_send_vblank_event?
> 
> Hmm. Updated how? Whether it's a page flip event or vblank even we won't
> know when the first active pixel will come. Although I suppose if
> there is some kind of vrr slew rate limit we could at least account
> for that to report a more correct "this is the earliest you migth be
> able to see your frame" timestamp.
> 
> Oh, or are you actually saying that shceduling a new flip before the
> timeout is actually going to latch that flip immediately? I figured
> that the flip would get latched on the next start of vblank regardless,
> and the act of scheduling a flip will just kick the hardware to start
> scanning the previously latched frame earlier.
> 
> scanout A | ... vblank | scanout A | ... vblank | scanout B | ... vblank
>                    ^                ^        ^               ^
>                    |                |        flip C          latch C
>                    flip B           latch B
> 
> vs.
> 
> scanout A | ... vblank | scanout B | ... vblank | scanout C | ... vblank
>                    ^ ^                      ^ ^
>                    | latch B                | latch C
>                    flip B                   flip C
> 
> The latter would seem more like a tearing flip without the tearing.
> 

I'm not sure any of this is necessary.

The vblank timestamp is determined (at least if you're using the 
helpers) by using the scanout position.

The crtc_vtotal won't change when variable refresh rate is enabled. What 
does change is the behavior of the scanout position.

The vpos will continue going will beyond the end of crtc_vtotal up until 
whatever the vtotal would be for the minimum refresh rate. There's no 
clamping performed for these calculations so it ends up working as expected.

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Michel Dänzer]

On 2018-10-29 8:11 p.m., Kazlauskas, Nicholas wrote:
> On 10/29/18 2:03 PM, Ville Syrjälä wrote:
>> On Mon, Oct 29, 2018 at 05:37:49PM +0100, Michel Dänzer wrote:
>>> On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
>>>> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>>>>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>>>>>
>>>>>> Speaking of timestamps. What is the expected behaviour of vblank
>>>>>> timestamps when vrr is enabled?
>>>>>
>>>>> When vrr is enabled the duration of the vertical front porch will be
>>>>> extended until flip or timeout occurs. The vblank timestamp will vary
>>>>> based on duration of the vertical front porch. The min/max duration for
>>>>> the front porch can be specified by the driver via the min/max range.
>>>>>
>>>>> No changes to vblank timestamping handling should be necessary to
>>>>> accommodate variable refresh rate.
>>>>
>>>> The problem is that the timestamp is supposed to correspond to the first
>>>> active pixel. And since we don't know how long the front porch will be
>>>> we can't realistically report the true value. So I guess just assuming
>>>> min front porch length is as good as anything else?
>>>
>>> That (and documenting that the timestamp corresponds to the earliest
>>> possible first active pixel, not necessarily the actual one, with VRR)
>>> might be good enough for the actual vblank event timestamps.
>>>
>>>
>>> However, I'm not so sure about the timestamps of page flip completion
>>> events. Those could be very misleading if the flip completes towards the
>>> timeout, which could result in bad behaviour of applications which use
>>> them for animation timing.
>>>
>>> Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
>>> :) in drm_crtc_send_vblank_event?
>>
>> [...]
> 
> I'm not sure any of this is necessary.
> 
> The vblank timestamp is determined (at least if you're using the 
> helpers) by using the scanout position.
> 
> The crtc_vtotal won't change when variable refresh rate is enabled. What 
> does change is the behavior of the scanout position.
> 
> The vpos will continue going will beyond the end of crtc_vtotal up until 
> whatever the vtotal would be for the minimum refresh rate. There's no 
> clamping performed for these calculations so it ends up working as expected.

I'm afraid not.

amdgpu_display_get_crtc_scanoutpos returns how many lines after "the end
of vblank" (which is calculated based on crtc_vtotal) the current
scanout position is. drm_calc_vbltimestamp_from_scanoutpos converts that
to a time delta from "the end of vblank", and subtracts that from the
current time (when the scanout position was sampled). The result is the
timestamp corresponding to "the end of vblank" based on crtc_vtotal.


-- 
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 v4 3/4] drm: Document variable refresh properties

[Michel Dänzer]

On 2018-10-29 7:03 p.m., Ville Syrjälä wrote:
> On Mon, Oct 29, 2018 at 05:37:49PM +0100, Michel Dänzer wrote:
>> On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
>>> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>>>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>>>>
>>>>> Speaking of timestamps. What is the expected behaviour of vblank
>>>>> timestamps when vrr is enabled?
>>>>
>>>> When vrr is enabled the duration of the vertical front porch will be
>>>> extended until flip or timeout occurs. The vblank timestamp will vary
>>>> based on duration of the vertical front porch. The min/max duration for
>>>> the front porch can be specified by the driver via the min/max range.
>>>>
>>>> No changes to vblank timestamping handling should be necessary to
>>>> accommodate variable refresh rate.
>>>
>>> The problem is that the timestamp is supposed to correspond to the first
>>> active pixel. And since we don't know how long the front porch will be
>>> we can't realistically report the true value. So I guess just assuming
>>> min front porch length is as good as anything else?
>>
>> That (and documenting that the timestamp corresponds to the earliest
>> possible first active pixel, not necessarily the actual one, with VRR)
>> might be good enough for the actual vblank event timestamps.
>>
>>
>> However, I'm not so sure about the timestamps of page flip completion
>> events. Those could be very misleading if the flip completes towards the
>> timeout, which could result in bad behaviour of applications which use
>> them for animation timing.
>>
>> Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
>> :) in drm_crtc_send_vblank_event?
> 
> Hmm. Updated how? Whether it's a page flip event or vblank even we won't
> know when the first active pixel will come. Although I suppose if
> there is some kind of vrr slew rate limit we could at least account
> for that to report a more correct "this is the earliest you migth be
> able to see your frame" timestamp.
> 
> Oh, or are you actually saying that shceduling a new flip before the
> timeout is actually going to latch that flip immediately? I figured
> that the flip would get latched on the next start of vblank regardless,
> and the act of scheduling a flip will just kick the hardware to start
> scanning the previously latched frame earlier.
> 
> scanout A | ... vblank | scanout A | ... vblank | scanout B | ... vblank
>                   ^                ^        ^               ^
>                   |                |        flip C          latch C
>                   flip B           latch B

This would kind of defeat the point of VRR, wouldn't it? If a flip was
scheduled after the start of vblank, the vblank would always time out,
resulting in the minimum refresh rate.


> scanout A | ... vblank | scanout B | ... vblank | scanout C | ... vblank
>                   ^ ^                      ^ ^
>                   | latch B                | latch C
>                   flip B                   flip C

So this is what happens.

Regardless, when the flip is latched, AMD hardware generates a "pflip"
interrupt, and its handler calls drm_crtc_send_vblank_event (and in the
case of DC drm_crtc_accurate_vblank_count before that). So the time when
drm_crtc_send_vblank_event is called might be a good approximation of
when scanout of the next frame starts.

Another possibility might be to wait for the hardware vline counter to
wrap around to 0 before calling drm_crtc_accurate_vblank_count, then the
calculations should be based on 0 instead of crtc_vtotal.


-- 
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 v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/30/18 5:29 AM, Michel Dänzer wrote:
> On 2018-10-29 7:03 p.m., Ville Syrjälä wrote:
>> On Mon, Oct 29, 2018 at 05:37:49PM +0100, Michel Dänzer wrote:
>>> On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
>>>> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>>>>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>>>>>
>>>>>> Speaking of timestamps. What is the expected behaviour of vblank
>>>>>> timestamps when vrr is enabled?
>>>>>
>>>>> When vrr is enabled the duration of the vertical front porch will be
>>>>> extended until flip or timeout occurs. The vblank timestamp will vary
>>>>> based on duration of the vertical front porch. The min/max duration for
>>>>> the front porch can be specified by the driver via the min/max range.
>>>>>
>>>>> No changes to vblank timestamping handling should be necessary to
>>>>> accommodate variable refresh rate.
>>>>
>>>> The problem is that the timestamp is supposed to correspond to the first
>>>> active pixel. And since we don't know how long the front porch will be
>>>> we can't realistically report the true value. So I guess just assuming
>>>> min front porch length is as good as anything else?
>>>
>>> That (and documenting that the timestamp corresponds to the earliest
>>> possible first active pixel, not necessarily the actual one, with VRR)
>>> might be good enough for the actual vblank event timestamps.
>>>
>>>
>>> However, I'm not so sure about the timestamps of page flip completion
>>> events. Those could be very misleading if the flip completes towards the
>>> timeout, which could result in bad behaviour of applications which use
>>> them for animation timing.
>>>
>>> Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
>>> :) in drm_crtc_send_vblank_event?
>>
>> Hmm. Updated how? Whether it's a page flip event or vblank even we won't
>> know when the first active pixel will come. Although I suppose if
>> there is some kind of vrr slew rate limit we could at least account
>> for that to report a more correct "this is the earliest you migth be
>> able to see your frame" timestamp.
>>
>> Oh, or are you actually saying that shceduling a new flip before the
>> timeout is actually going to latch that flip immediately? I figured
>> that the flip would get latched on the next start of vblank regardless,
>> and the act of scheduling a flip will just kick the hardware to start
>> scanning the previously latched frame earlier.
>>
>> scanout A | ... vblank | scanout A | ... vblank | scanout B | ... vblank
>>                    ^                ^        ^               ^
>>                    |                |        flip C          latch C
>>                    flip B           latch B
> 
> This would kind of defeat the point of VRR, wouldn't it? If a flip was
> scheduled after the start of vblank, the vblank would always time out,
> resulting in the minimum refresh rate.
> 
> 
>> scanout A | ... vblank | scanout B | ... vblank | scanout C | ... vblank
>>                    ^ ^                      ^ ^
>>                    | latch B                | latch C
>>                    flip B                   flip C
> 
> So this is what happens.
> 
> Regardless, when the flip is latched, AMD hardware generates a "pflip"
> interrupt, and its handler calls drm_crtc_send_vblank_event (and in the
> case of DC drm_crtc_accurate_vblank_count before that). So the time when
> drm_crtc_send_vblank_event is called might be a good approximation of
> when scanout of the next frame starts.
> 
> Another possibility might be to wait for the hardware vline counter to
> wrap around to 0 before calling drm_crtc_accurate_vblank_count, then the
> calculations should be based on 0 instead of crtc_vtotal.
> 
> 


I understand the issue you're describing now. The timestamp is supposed 
to signify the end of the current vblank. The call to get scanout 
position is supposed to return the number of lines until scanout (a 
negative value) or the number of lines since the next scanout began (a 
positive value).

The amdgpu driver calculates the number of lines based on a hardware 
register status position which returns an increasing value from 0 that 
indicates the current vpos/hpos for the display.

For any vpos below vbl_start we know the value is correct since the next 
vblank hasn't begun yet. But for anythign about vbl_start it's probably 
wrong since it applies a corrective offset based on the fixed value of 
crtc_vtotal. It's even worse when the value is above crtc_vtotal since 
it'll be calculating the number of lines since the last scanout since 
it'll be a positive value.

So the issue becomes determining when the vfront porch will end.

When the flip address gets written the vfront porch will end at the 
start of the next line leaving only the back porch plus part of the 
line. But we don't know when the flip will occur, if at all. It hasn't 
occurred yet in this case.

Waiting for the wrap around to 0 might be the best choice here since 
there's no guarantee the flip will occur.

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/30/18 11:34 AM, Kazlauskas, Nicholas wrote:
> On 10/30/18 5:29 AM, Michel Dänzer wrote:
>> On 2018-10-29 7:03 p.m., Ville Syrjälä wrote:
>>> On Mon, Oct 29, 2018 at 05:37:49PM +0100, Michel Dänzer wrote:
>>>> On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
>>>>> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>>>>>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>>>>>>
>>>>>>> Speaking of timestamps. What is the expected behaviour of vblank
>>>>>>> timestamps when vrr is enabled?
>>>>>>
>>>>>> When vrr is enabled the duration of the vertical front porch will be
>>>>>> extended until flip or timeout occurs. The vblank timestamp will vary
>>>>>> based on duration of the vertical front porch. The min/max duration for
>>>>>> the front porch can be specified by the driver via the min/max range.
>>>>>>
>>>>>> No changes to vblank timestamping handling should be necessary to
>>>>>> accommodate variable refresh rate.
>>>>>
>>>>> The problem is that the timestamp is supposed to correspond to the first
>>>>> active pixel. And since we don't know how long the front porch will be
>>>>> we can't realistically report the true value. So I guess just assuming
>>>>> min front porch length is as good as anything else?
>>>>
>>>> That (and documenting that the timestamp corresponds to the earliest
>>>> possible first active pixel, not necessarily the actual one, with VRR)
>>>> might be good enough for the actual vblank event timestamps.
>>>>
>>>>
>>>> However, I'm not so sure about the timestamps of page flip completion
>>>> events. Those could be very misleading if the flip completes towards the
>>>> timeout, which could result in bad behaviour of applications which use
>>>> them for animation timing.
>>>>
>>>> Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
>>>> :) in drm_crtc_send_vblank_event?
>>>
>>> Hmm. Updated how? Whether it's a page flip event or vblank even we won't
>>> know when the first active pixel will come. Although I suppose if
>>> there is some kind of vrr slew rate limit we could at least account
>>> for that to report a more correct "this is the earliest you migth be
>>> able to see your frame" timestamp.
>>>
>>> Oh, or are you actually saying that shceduling a new flip before the
>>> timeout is actually going to latch that flip immediately? I figured
>>> that the flip would get latched on the next start of vblank regardless,
>>> and the act of scheduling a flip will just kick the hardware to start
>>> scanning the previously latched frame earlier.
>>>
>>> scanout A | ... vblank | scanout A | ... vblank | scanout B | ... vblank
>>>                     ^                ^        ^               ^
>>>                     |                |        flip C          latch C
>>>                     flip B           latch B
>>
>> This would kind of defeat the point of VRR, wouldn't it? If a flip was
>> scheduled after the start of vblank, the vblank would always time out,
>> resulting in the minimum refresh rate.
>>
>>
>>> scanout A | ... vblank | scanout B | ... vblank | scanout C | ... vblank
>>>                     ^ ^                      ^ ^
>>>                     | latch B                | latch C
>>>                     flip B                   flip C
>>
>> So this is what happens.
>>
>> Regardless, when the flip is latched, AMD hardware generates a "pflip"
>> interrupt, and its handler calls drm_crtc_send_vblank_event (and in the
>> case of DC drm_crtc_accurate_vblank_count before that). So the time when
>> drm_crtc_send_vblank_event is called might be a good approximation of
>> when scanout of the next frame starts.
>>
>> Another possibility might be to wait for the hardware vline counter to
>> wrap around to 0 before calling drm_crtc_accurate_vblank_count, then the
>> calculations should be based on 0 instead of crtc_vtotal.
>>
>>
> 
> 
> I understand the issue you're describing now. The timestamp is supposed
> to signify the end of the current vblank. The call to get scanout
> position is supposed to return the number of lines until scanout (a
> negative value) or the number of lines since the next scanout began (a
> positive value).
> 
> The amdgpu driver calculates the number of lines based on a hardware
> register status position which returns an increasing value from 0 that
> indicates the current vpos/hpos for the display.
> 
> For any vpos below vbl_start we know the value is correct since the next
> vblank hasn't begun yet. But for anythign about vbl_start it's probably
> wrong since it applies a corrective offset based on the fixed value of
> crtc_vtotal. It's even worse when the value is above crtc_vtotal since
> it'll be calculating the number of lines since the last scanout since
> it'll be a positive value.
> 
> So the issue becomes determining when the vfront porch will end.
> 
> When the flip address gets written the vfront porch will end at the
> start of the next line leaving only the back porch plus part of the
> line. But we don't know when the flip will occur, if at all. It hasn't
> occurred yet in this case.
> 
> Waiting for the wrap around to 0 might be the best choice here since
> there's no guarantee the flip will occur.
> 
> Nicholas Kazlauskas
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

I put some more thought into this and I don't think this is as bad as I 
had originally thought.

I think the vblank timestamp is supposed to be for the first active 
pixel of the next scanout. The usage of which is for clients to time 
their content/animation/etc.

The client likely doesn't care when they issue their flip, just that 
their content is matched for that vblank timestamp. The fixed refresh 
model works really well for that kind of application.

Utilizing variable refresh rate would be a mistake in that case since 
the client would then have to time based on when they flip which is a 
lot harder to "predict" precisely.

I did some more investigation into when amdgpu gets the scanout position 
and what values we get back out of it. The timestamp is updated shortly 
after the crtc irq vblank which is typically within a few lines of 
vbl_start. This makes sense, since we can provide the prediction 
timestamp early. Waiting for the vblank to wrap around to 0 doesn't 
really make sense here since that would mean we already hit timeout or 
the flip occurred - making the timestamp useless and probably burning a 
ton of CPU to do it.

What this means is that we're going to have to make some sort of 
prediction based on a fixed duration to have a reasonable result. I 
think assuming the min front porch duration (max refresh) is the logical 
approach here - in most cases this will be crtc_vtotal since it's the 
monitor's fastest refresh. For clients that flip early this will end up 
being the correct timestamp. No need to explicitly check for variable 
refresh enabled in the get scanout pos handler either.

There is one little caveat with get scanout pos with that approach, 
however. For usages other than the timestamp the number of lines until 
scanout will still be incorrect when going above crtc_vtotal. We can 
either assume the worst in this case (the max front porch duration, the 
min refresh/timeout) or clamp and just return the size of the back porch 
instead The vpos will look stalled, but the flip will likely happen 
sooner than the timeout in most cases so this will likely work out better.

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Michel Dänzer]

On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:
> On 10/30/18 11:34 AM, Kazlauskas, Nicholas wrote:
>> On 10/30/18 5:29 AM, Michel Dänzer wrote:
>>> On 2018-10-29 7:03 p.m., Ville Syrjälä wrote:
>>>> On Mon, Oct 29, 2018 at 05:37:49PM +0100, Michel Dänzer wrote:
>>>>> On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
>>>>>> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>>>>>>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>>>>>>>
>>>>>>>> Speaking of timestamps. What is the expected behaviour of vblank
>>>>>>>> timestamps when vrr is enabled?
>>>>>>>
>>>>>>> When vrr is enabled the duration of the vertical front porch will be
>>>>>>> extended until flip or timeout occurs. The vblank timestamp will vary
>>>>>>> based on duration of the vertical front porch. The min/max duration for
>>>>>>> the front porch can be specified by the driver via the min/max range.
>>>>>>>
>>>>>>> No changes to vblank timestamping handling should be necessary to
>>>>>>> accommodate variable refresh rate.
>>>>>>
>>>>>> The problem is that the timestamp is supposed to correspond to the first
>>>>>> active pixel. And since we don't know how long the front porch will be
>>>>>> we can't realistically report the true value. So I guess just assuming
>>>>>> min front porch length is as good as anything else?
>>>>>
>>>>> That (and documenting that the timestamp corresponds to the earliest
>>>>> possible first active pixel, not necessarily the actual one, with VRR)
>>>>> might be good enough for the actual vblank event timestamps.
>>>>>
>>>>>
>>>>> However, I'm not so sure about the timestamps of page flip completion
>>>>> events. Those could be very misleading if the flip completes towards the
>>>>> timeout, which could result in bad behaviour of applications which use
>>>>> them for animation timing.
>>>>>
>>>>> Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
>>>>> :) in drm_crtc_send_vblank_event?
>>>>
>>>> Hmm. Updated how? Whether it's a page flip event or vblank even we won't
>>>> know when the first active pixel will come. Although I suppose if
>>>> there is some kind of vrr slew rate limit we could at least account
>>>> for that to report a more correct "this is the earliest you migth be
>>>> able to see your frame" timestamp.
>>>>
>>>> Oh, or are you actually saying that shceduling a new flip before the
>>>> timeout is actually going to latch that flip immediately? I figured
>>>> that the flip would get latched on the next start of vblank regardless,
>>>> and the act of scheduling a flip will just kick the hardware to start
>>>> scanning the previously latched frame earlier.
>>>>
>>>> scanout A | ... vblank | scanout A | ... vblank | scanout B | ... vblank
>>>>                     ^                ^        ^               ^
>>>>                     |                |        flip C          latch C
>>>>                     flip B           latch B
>>>
>>> This would kind of defeat the point of VRR, wouldn't it? If a flip was
>>> scheduled after the start of vblank, the vblank would always time out,
>>> resulting in the minimum refresh rate.
>>>
>>>
>>>> scanout A | ... vblank | scanout B | ... vblank | scanout C | ... vblank
>>>>                     ^ ^                      ^ ^
>>>>                     | latch B                | latch C
>>>>                     flip B                   flip C
>>>
>>> So this is what happens.
>>>
>>> Regardless, when the flip is latched, AMD hardware generates a "pflip"
>>> interrupt, and its handler calls drm_crtc_send_vblank_event (and in the
>>> case of DC drm_crtc_accurate_vblank_count before that). So the time when
>>> drm_crtc_send_vblank_event is called might be a good approximation of
>>> when scanout of the next frame starts.
>>>
>>> Another possibility might be to wait for the hardware vline counter to
>>> wrap around to 0 before calling drm_crtc_accurate_vblank_count, then the
>>> calculations should be based on 0 instead of crtc_vtotal.
>>
>>
>> I understand the issue you're describing now. The timestamp is supposed
>> to signify the end of the current vblank. The call to get scanout
>> position is supposed to return the number of lines until scanout (a
>> negative value) or the number of lines since the next scanout began (a
>> positive value).
>>
>> The amdgpu driver calculates the number of lines based on a hardware
>> register status position which returns an increasing value from 0 that
>> indicates the current vpos/hpos for the display.
>>
>> For any vpos below vbl_start we know the value is correct since the next
>> vblank hasn't begun yet. But for anythign about vbl_start it's probably
>> wrong since it applies a corrective offset based on the fixed value of
>> crtc_vtotal. It's even worse when the value is above crtc_vtotal since
>> it'll be calculating the number of lines since the last scanout since
>> it'll be a positive value.
>>
>> So the issue becomes determining when the vfront porch will end.
>>
>> When the flip address gets written the vfront porch will end at the
>> start of the next line leaving only the back porch plus part of the
>> line. But we don't know when the flip will occur, if at all. It hasn't
>> occurred yet in this case.
>>
>> Waiting for the wrap around to 0 might be the best choice here since
>> there's no guarantee the flip will occur.
> 
> I put some more thought into this and I don't think this is as bad as I 
> had originally thought.
> 
> I think the vblank timestamp is supposed to be for the first active 
> pixel of the next scanout. The usage of which is for clients to time 
> their content/animation/etc.
> 
> The client likely doesn't care when they issue their flip, just that 
> their content is matched for that vblank timestamp. The fixed refresh 
> model works really well for that kind of application.
> 
> Utilizing variable refresh rate would be a mistake in that case since 
> the client would then have to time based on when they flip which is a 
> lot harder to "predict" precisely.

It's only a "mistake" as long as the timestamps are misleading. :) As
discussed before, accurate presentation timestamps are one requirement
for achieving perfectly smooth animation.


> I did some more investigation into when amdgpu gets the scanout position 
> and what values we get back out of it. The timestamp is updated shortly 
> after the crtc irq vblank which is typically within a few lines of 
> vbl_start. This makes sense, since we can provide the prediction 
> timestamp early. Waiting for the vblank to wrap around to 0 doesn't 
> really make sense here since that would mean we already hit timeout or 
> the flip occurred

Sounds like you're mixing up the two cases of "actual" vblank events
(triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
flip completion events (triggered by the PFLIP interrupt with our
hardware => drm_crtc_send_vblank_event).

Actual vblank events need to be delivered to userspace at the start of
vblank, so we indeed can't wait until the timestamp is accurate for
them. We just need to document the exact semantics of their timestamp
with VRR.

For page flip completion events though, the timestamp needs to be
accurate and correspond to when the flipped frame starts being scanned
out, otherwise we'll surely break at least some userspace relying on
this information.


-- 
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 v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/31/18 10:12 AM, Michel Dänzer wrote:
> On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:
>> On 10/30/18 11:34 AM, Kazlauskas, Nicholas wrote:
>>> On 10/30/18 5:29 AM, Michel Dänzer wrote:
>>>> On 2018-10-29 7:03 p.m., Ville Syrjälä wrote:
>>>>> On Mon, Oct 29, 2018 at 05:37:49PM +0100, Michel Dänzer wrote:
>>>>>> On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
>>>>>>> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>>>>>>>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>>>>>>>>
>>>>>>>>> Speaking of timestamps. What is the expected behaviour of vblank
>>>>>>>>> timestamps when vrr is enabled?
>>>>>>>>
>>>>>>>> When vrr is enabled the duration of the vertical front porch will be
>>>>>>>> extended until flip or timeout occurs. The vblank timestamp will vary
>>>>>>>> based on duration of the vertical front porch. The min/max duration for
>>>>>>>> the front porch can be specified by the driver via the min/max range.
>>>>>>>>
>>>>>>>> No changes to vblank timestamping handling should be necessary to
>>>>>>>> accommodate variable refresh rate.
>>>>>>>
>>>>>>> The problem is that the timestamp is supposed to correspond to the first
>>>>>>> active pixel. And since we don't know how long the front porch will be
>>>>>>> we can't realistically report the true value. So I guess just assuming
>>>>>>> min front porch length is as good as anything else?
>>>>>>
>>>>>> That (and documenting that the timestamp corresponds to the earliest
>>>>>> possible first active pixel, not necessarily the actual one, with VRR)
>>>>>> might be good enough for the actual vblank event timestamps.
>>>>>>
>>>>>>
>>>>>> However, I'm not so sure about the timestamps of page flip completion
>>>>>> events. Those could be very misleading if the flip completes towards the
>>>>>> timeout, which could result in bad behaviour of applications which use
>>>>>> them for animation timing.
>>>>>>
>>>>>> Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
>>>>>> :) in drm_crtc_send_vblank_event?
>>>>>
>>>>> Hmm. Updated how? Whether it's a page flip event or vblank even we won't
>>>>> know when the first active pixel will come. Although I suppose if
>>>>> there is some kind of vrr slew rate limit we could at least account
>>>>> for that to report a more correct "this is the earliest you migth be
>>>>> able to see your frame" timestamp.
>>>>>
>>>>> Oh, or are you actually saying that shceduling a new flip before the
>>>>> timeout is actually going to latch that flip immediately? I figured
>>>>> that the flip would get latched on the next start of vblank regardless,
>>>>> and the act of scheduling a flip will just kick the hardware to start
>>>>> scanning the previously latched frame earlier.
>>>>>
>>>>> scanout A | ... vblank | scanout A | ... vblank | scanout B | ... vblank
>>>>>                      ^                ^        ^               ^
>>>>>                      |                |        flip C          latch C
>>>>>                      flip B           latch B
>>>>
>>>> This would kind of defeat the point of VRR, wouldn't it? If a flip was
>>>> scheduled after the start of vblank, the vblank would always time out,
>>>> resulting in the minimum refresh rate.
>>>>
>>>>
>>>>> scanout A | ... vblank | scanout B | ... vblank | scanout C | ... vblank
>>>>>                      ^ ^                      ^ ^
>>>>>                      | latch B                | latch C
>>>>>                      flip B                   flip C
>>>>
>>>> So this is what happens.
>>>>
>>>> Regardless, when the flip is latched, AMD hardware generates a "pflip"
>>>> interrupt, and its handler calls drm_crtc_send_vblank_event (and in the
>>>> case of DC drm_crtc_accurate_vblank_count before that). So the time when
>>>> drm_crtc_send_vblank_event is called might be a good approximation of
>>>> when scanout of the next frame starts.
>>>>
>>>> Another possibility might be to wait for the hardware vline counter to
>>>> wrap around to 0 before calling drm_crtc_accurate_vblank_count, then the
>>>> calculations should be based on 0 instead of crtc_vtotal.
>>>
>>>
>>> I understand the issue you're describing now. The timestamp is supposed
>>> to signify the end of the current vblank. The call to get scanout
>>> position is supposed to return the number of lines until scanout (a
>>> negative value) or the number of lines since the next scanout began (a
>>> positive value).
>>>
>>> The amdgpu driver calculates the number of lines based on a hardware
>>> register status position which returns an increasing value from 0 that
>>> indicates the current vpos/hpos for the display.
>>>
>>> For any vpos below vbl_start we know the value is correct since the next
>>> vblank hasn't begun yet. But for anythign about vbl_start it's probably
>>> wrong since it applies a corrective offset based on the fixed value of
>>> crtc_vtotal. It's even worse when the value is above crtc_vtotal since
>>> it'll be calculating the number of lines since the last scanout since
>>> it'll be a positive value.
>>>
>>> So the issue becomes determining when the vfront porch will end.
>>>
>>> When the flip address gets written the vfront porch will end at the
>>> start of the next line leaving only the back porch plus part of the
>>> line. But we don't know when the flip will occur, if at all. It hasn't
>>> occurred yet in this case.
>>>
>>> Waiting for the wrap around to 0 might be the best choice here since
>>> there's no guarantee the flip will occur.
>>
>> I put some more thought into this and I don't think this is as bad as I
>> had originally thought.
>>
>> I think the vblank timestamp is supposed to be for the first active
>> pixel of the next scanout. The usage of which is for clients to time
>> their content/animation/etc.
>>
>> The client likely doesn't care when they issue their flip, just that
>> their content is matched for that vblank timestamp. The fixed refresh
>> model works really well for that kind of application.
>>
>> Utilizing variable refresh rate would be a mistake in that case since
>> the client would then have to time based on when they flip which is a
>> lot harder to "predict" precisely.
> 
> It's only a "mistake" as long as the timestamps are misleading. :) As
> discussed before, accurate presentation timestamps are one requirement
> for achieving perfectly smooth animation.

For most 3D games the game world/rendering can advance by an arbitrary 
timestep - and faster will be smoother, which is what the native mode 
would give you.

For fixed interval content/animation where you can't do that variable 
refresh rate could vastly improve the output. But like discussed before 
that would need a way to specify the interval/presentation timestamp to 
control that front porch duration. The timestamp will be misleading in 
any other case.

> 
> 
>> I did some more investigation into when amdgpu gets the scanout position
>> and what values we get back out of it. The timestamp is updated shortly
>> after the crtc irq vblank which is typically within a few lines of
>> vbl_start. This makes sense, since we can provide the prediction
>> timestamp early. Waiting for the vblank to wrap around to 0 doesn't
>> really make sense here since that would mean we already hit timeout or
>> the flip occurred
> 
> Sounds like you're mixing up the two cases of "actual" vblank events
> (triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
> flip completion events (triggered by the PFLIP interrupt with our
> hardware => drm_crtc_send_vblank_event).
> 
> Actual vblank events need to be delivered to userspace at the start of
> vblank, so we indeed can't wait until the timestamp is accurate for
> them. We just need to document the exact semantics of their timestamp
> with VRR.
> 
> For page flip completion events though, the timestamp needs to be
> accurate and correspond to when the flipped frame starts being scanned
> out, otherwise we'll surely break at least some userspace relying on
> this information.
> 
> 
Yeah, I was. I guess what's sent is the estimated vblank timestamp 
calculated at the start of the interrupt. And since that's just a guess 
rather than what's actually going to happen it's going to be wrong in a 
lot of cases.

I could see the wrap-around method working if the vblank timestamp was 
somehow updated in amdgpu or in drm_crtc_send_vblank_event. This would 
be a relatively simple fix but would break anything in userspace that 
relied on the timestamp for vblank interrupt and the flip completion 
being the same value. But I guess that's the case for any fix for this 
potential problem.

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Michel Dänzer]

On 2018-10-31 3:41 p.m., Kazlauskas, Nicholas wrote:
> On 10/31/18 10:12 AM, Michel Dänzer wrote:
>> On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:
>>> On 10/30/18 11:34 AM, Kazlauskas, Nicholas wrote:
>>>>
>>>> I understand the issue you're describing now. The timestamp is supposed
>>>> to signify the end of the current vblank. The call to get scanout
>>>> position is supposed to return the number of lines until scanout (a
>>>> negative value) or the number of lines since the next scanout began (a
>>>> positive value).
>>>>
>>>> The amdgpu driver calculates the number of lines based on a hardware
>>>> register status position which returns an increasing value from 0 that
>>>> indicates the current vpos/hpos for the display.
>>>>
>>>> For any vpos below vbl_start we know the value is correct since the next
>>>> vblank hasn't begun yet. But for anythign about vbl_start it's probably
>>>> wrong since it applies a corrective offset based on the fixed value of
>>>> crtc_vtotal. It's even worse when the value is above crtc_vtotal since
>>>> it'll be calculating the number of lines since the last scanout since
>>>> it'll be a positive value.
>>>>
>>>> So the issue becomes determining when the vfront porch will end.
>>>>
>>>> When the flip address gets written the vfront porch will end at the
>>>> start of the next line leaving only the back porch plus part of the
>>>> line. But we don't know when the flip will occur, if at all. It hasn't
>>>> occurred yet in this case.
>>>>
>>>> Waiting for the wrap around to 0 might be the best choice here since
>>>> there's no guarantee the flip will occur.
>>>
>>> I put some more thought into this and I don't think this is as bad as I
>>> had originally thought.
>>>
>>> I think the vblank timestamp is supposed to be for the first active
>>> pixel of the next scanout. The usage of which is for clients to time
>>> their content/animation/etc.
>>>
>>> The client likely doesn't care when they issue their flip, just that
>>> their content is matched for that vblank timestamp. The fixed refresh
>>> model works really well for that kind of application.
>>>
>>> Utilizing variable refresh rate would be a mistake in that case since
>>> the client would then have to time based on when they flip which is a
>>> lot harder to "predict" precisely.
>>
>> It's only a "mistake" as long as the timestamps are misleading. :) As
>> discussed before, accurate presentation timestamps are one requirement
>> for achieving perfectly smooth animation.
> 
> For most 3D games the game world/rendering can advance by an arbitrary 
> timestep - and faster will be smoother, which is what the native mode 
> would give you.
> 
> For fixed interval content/animation where you can't do that variable 
> refresh rate could vastly improve the output. But like discussed before 
> that would need a way to specify the interval/presentation timestamp to 
> control that front porch duration. The timestamp will be misleading in 
> any other case.

I don't agree that an accurate timestamp can ever be more "misleading"
than an inaccurate one. But yeah, accurate timestamps and time-based
presentation are two sides of the same coin which can buy the holy grail
of perfectly smooth animation. :)


>>> I did some more investigation into when amdgpu gets the scanout position
>>> and what values we get back out of it. The timestamp is updated shortly
>>> after the crtc irq vblank which is typically within a few lines of
>>> vbl_start. This makes sense, since we can provide the prediction
>>> timestamp early. Waiting for the vblank to wrap around to 0 doesn't
>>> really make sense here since that would mean we already hit timeout or
>>> the flip occurred
>>
>> Sounds like you're mixing up the two cases of "actual" vblank events
>> (triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
>> flip completion events (triggered by the PFLIP interrupt with our
>> hardware => drm_crtc_send_vblank_event).
>>
>> Actual vblank events need to be delivered to userspace at the start of
>> vblank, so we indeed can't wait until the timestamp is accurate for
>> them. We just need to document the exact semantics of their timestamp
>> with VRR.
>>
>> For page flip completion events though, the timestamp needs to be
>> accurate and correspond to when the flipped frame starts being scanned
>> out, otherwise we'll surely break at least some userspace relying on
>> this information.
>>
> Yeah, I was. I guess what's sent is the estimated vblank timestamp 
> calculated at the start of the interrupt.

s/interrupt/vblank/, yeah.


> And since that's just a guess rather than what's actually going to
> happen it's going to be wrong in a lot of cases.
> 
> I could see the wrap-around method working if the vblank timestamp was 
> somehow updated in amdgpu or in drm_crtc_send_vblank_event.

DC already calls drm_crtc_accurate_vblank_count before
drm_crtc_send_vblank_event, we "just" need to make sure that results in
an accurate timestamp.


> This would be a relatively simple fix but would break anything in
> userspace that relied on the timestamp for vblank interrupt and the
> flip completion being the same value.

Hmm, that's a good point. So while VRR is enabled, maybe it is safer to
defer delivery of vblank events until the accurate timestamp is known as
well, at least by default. If there is userspace which needs the events
earlier even with VRR but can live with the guessed timestamp, a flag or
something could be added for that.


-- 
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 v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/31/18 12:20 PM, Michel Dänzer wrote:
> On 2018-10-31 3:41 p.m., Kazlauskas, Nicholas wrote:
>> On 10/31/18 10:12 AM, Michel Dänzer wrote:
>>> On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:
>>>> On 10/30/18 11:34 AM, Kazlauskas, Nicholas wrote:
>>>>>
>>>>> I understand the issue you're describing now. The timestamp is supposed
>>>>> to signify the end of the current vblank. The call to get scanout
>>>>> position is supposed to return the number of lines until scanout (a
>>>>> negative value) or the number of lines since the next scanout began (a
>>>>> positive value).
>>>>>
>>>>> The amdgpu driver calculates the number of lines based on a hardware
>>>>> register status position which returns an increasing value from 0 that
>>>>> indicates the current vpos/hpos for the display.
>>>>>
>>>>> For any vpos below vbl_start we know the value is correct since the next
>>>>> vblank hasn't begun yet. But for anythign about vbl_start it's probably
>>>>> wrong since it applies a corrective offset based on the fixed value of
>>>>> crtc_vtotal. It's even worse when the value is above crtc_vtotal since
>>>>> it'll be calculating the number of lines since the last scanout since
>>>>> it'll be a positive value.
>>>>>
>>>>> So the issue becomes determining when the vfront porch will end.
>>>>>
>>>>> When the flip address gets written the vfront porch will end at the
>>>>> start of the next line leaving only the back porch plus part of the
>>>>> line. But we don't know when the flip will occur, if at all. It hasn't
>>>>> occurred yet in this case.
>>>>>
>>>>> Waiting for the wrap around to 0 might be the best choice here since
>>>>> there's no guarantee the flip will occur.
>>>>
>>>> I put some more thought into this and I don't think this is as bad as I
>>>> had originally thought.
>>>>
>>>> I think the vblank timestamp is supposed to be for the first active
>>>> pixel of the next scanout. The usage of which is for clients to time
>>>> their content/animation/etc.
>>>>
>>>> The client likely doesn't care when they issue their flip, just that
>>>> their content is matched for that vblank timestamp. The fixed refresh
>>>> model works really well for that kind of application.
>>>>
>>>> Utilizing variable refresh rate would be a mistake in that case since
>>>> the client would then have to time based on when they flip which is a
>>>> lot harder to "predict" precisely.
>>>
>>> It's only a "mistake" as long as the timestamps are misleading. :) As
>>> discussed before, accurate presentation timestamps are one requirement
>>> for achieving perfectly smooth animation.
>>
>> For most 3D games the game world/rendering can advance by an arbitrary
>> timestep - and faster will be smoother, which is what the native mode
>> would give you.
>>
>> For fixed interval content/animation where you can't do that variable
>> refresh rate could vastly improve the output. But like discussed before
>> that would need a way to specify the interval/presentation timestamp to
>> control that front porch duration. The timestamp will be misleading in
>> any other case.
> 
> I don't agree that an accurate timestamp can ever be more "misleading"
> than an inaccurate one. But yeah, accurate timestamps and time-based
> presentation are two sides of the same coin which can buy the holy grail
> of perfectly smooth animation. :)
> 
> 
>>>> I did some more investigation into when amdgpu gets the scanout position
>>>> and what values we get back out of it. The timestamp is updated shortly
>>>> after the crtc irq vblank which is typically within a few lines of
>>>> vbl_start. This makes sense, since we can provide the prediction
>>>> timestamp early. Waiting for the vblank to wrap around to 0 doesn't
>>>> really make sense here since that would mean we already hit timeout or
>>>> the flip occurred
>>>
>>> Sounds like you're mixing up the two cases of "actual" vblank events
>>> (triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
>>> flip completion events (triggered by the PFLIP interrupt with our
>>> hardware => drm_crtc_send_vblank_event).
>>>
>>> Actual vblank events need to be delivered to userspace at the start of
>>> vblank, so we indeed can't wait until the timestamp is accurate for
>>> them. We just need to document the exact semantics of their timestamp
>>> with VRR.
>>>
>>> For page flip completion events though, the timestamp needs to be
>>> accurate and correspond to when the flipped frame starts being scanned
>>> out, otherwise we'll surely break at least some userspace relying on
>>> this information.
>>>
>> Yeah, I was. I guess what's sent is the estimated vblank timestamp
>> calculated at the start of the interrupt.
> 
> s/interrupt/vblank/, yeah.
> 
> 
>> And since that's just a guess rather than what's actually going to
>> happen it's going to be wrong in a lot of cases.
>>
>> I could see the wrap-around method working if the vblank timestamp was
>> somehow updated in amdgpu or in drm_crtc_send_vblank_event.
> 
> DC already calls drm_crtc_accurate_vblank_count before
> drm_crtc_send_vblank_event, we "just" need to make sure that results in
> an accurate timestamp.
> 
> 
>> This would be a relatively simple fix but would break anything in
>> userspace that relied on the timestamp for vblank interrupt and the
>> flip completion being the same value.
> 
> Hmm, that's a good point. So while VRR is enabled, maybe it is safer to
> defer delivery of vblank events until the accurate timestamp is known as
> well, at least by default. If there is userspace which needs the events
> earlier even with VRR but can live with the guessed timestamp, a flag or
> something could be added for that.
> 
> 

I was under the impression that the vblank timestamp was reused but it's 
already going to differ since we call drm_crtc_accurate_vblank_count 
before drm_crtc_send_vblank_event. Thanks for pointing that out.

Since that works for updating timestamp what's left is making sure that 
it waits for the wrap around if it's above crtc_vtotal. It might make 
sense to add a new flag for this that's only used within 
amdgpu_get_crtc_scanout_position so the other call sites aren't affected.

There isn't a way to get an accurate timestamp with VRR enabled until 
after the flip happens. So deferring it kind of defeats the purpose of a 
client using it to make predictions before the flip for displaying their 
content.

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Pekka Paalanen]

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

On Wed, 31 Oct 2018 17:54:34 +0000
"Kazlauskas, Nicholas" <Nicholas.Kazlauskas@amd.com> wrote:

> On 10/31/18 12:20 PM, Michel Dänzer wrote:
> > On 2018-10-31 3:41 p.m., Kazlauskas, Nicholas wrote:  
> >> On 10/31/18 10:12 AM, Michel Dänzer wrote:  
> >>> On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:  
> >>>> On 10/30/18 11:34 AM, Kazlauskas, Nicholas wrote:  
> >>>>>
> >>>>> I understand the issue you're describing now. The timestamp is supposed
> >>>>> to signify the end of the current vblank. The call to get scanout
> >>>>> position is supposed to return the number of lines until scanout (a
> >>>>> negative value) or the number of lines since the next scanout began (a
> >>>>> positive value).
> >>>>>
> >>>>> The amdgpu driver calculates the number of lines based on a hardware
> >>>>> register status position which returns an increasing value from 0 that
> >>>>> indicates the current vpos/hpos for the display.
> >>>>>
> >>>>> For any vpos below vbl_start we know the value is correct since the next
> >>>>> vblank hasn't begun yet. But for anythign about vbl_start it's probably
> >>>>> wrong since it applies a corrective offset based on the fixed value of
> >>>>> crtc_vtotal. It's even worse when the value is above crtc_vtotal since
> >>>>> it'll be calculating the number of lines since the last scanout since
> >>>>> it'll be a positive value.
> >>>>>
> >>>>> So the issue becomes determining when the vfront porch will end.
> >>>>>
> >>>>> When the flip address gets written the vfront porch will end at the
> >>>>> start of the next line leaving only the back porch plus part of the
> >>>>> line. But we don't know when the flip will occur, if at all. It hasn't
> >>>>> occurred yet in this case.
> >>>>>
> >>>>> Waiting for the wrap around to 0 might be the best choice here since
> >>>>> there's no guarantee the flip will occur.  
> >>>>
> >>>> I put some more thought into this and I don't think this is as bad as I
> >>>> had originally thought.
> >>>>
> >>>> I think the vblank timestamp is supposed to be for the first active
> >>>> pixel of the next scanout. The usage of which is for clients to time
> >>>> their content/animation/etc.
> >>>>
> >>>> The client likely doesn't care when they issue their flip, just that
> >>>> their content is matched for that vblank timestamp. The fixed refresh
> >>>> model works really well for that kind of application.
> >>>>
> >>>> Utilizing variable refresh rate would be a mistake in that case since
> >>>> the client would then have to time based on when they flip which is a
> >>>> lot harder to "predict" precisely.  
> >>>
> >>> It's only a "mistake" as long as the timestamps are misleading. :) As
> >>> discussed before, accurate presentation timestamps are one requirement
> >>> for achieving perfectly smooth animation.  
> >>
> >> For most 3D games the game world/rendering can advance by an arbitrary
> >> timestep - and faster will be smoother, which is what the native mode
> >> would give you.
> >>
> >> For fixed interval content/animation where you can't do that variable
> >> refresh rate could vastly improve the output. But like discussed before
> >> that would need a way to specify the interval/presentation timestamp to
> >> control that front porch duration. The timestamp will be misleading in
> >> any other case.  
> > 
> > I don't agree that an accurate timestamp can ever be more "misleading"
> > than an inaccurate one. But yeah, accurate timestamps and time-based
> > presentation are two sides of the same coin which can buy the holy grail
> > of perfectly smooth animation. :)
> > 
> >   
> >>>> I did some more investigation into when amdgpu gets the scanout position
> >>>> and what values we get back out of it. The timestamp is updated shortly
> >>>> after the crtc irq vblank which is typically within a few lines of
> >>>> vbl_start. This makes sense, since we can provide the prediction
> >>>> timestamp early. Waiting for the vblank to wrap around to 0 doesn't
> >>>> really make sense here since that would mean we already hit timeout or
> >>>> the flip occurred  
> >>>
> >>> Sounds like you're mixing up the two cases of "actual" vblank events
> >>> (triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
> >>> flip completion events (triggered by the PFLIP interrupt with our
> >>> hardware => drm_crtc_send_vblank_event).
> >>>
> >>> Actual vblank events need to be delivered to userspace at the start of
> >>> vblank, so we indeed can't wait until the timestamp is accurate for
> >>> them. We just need to document the exact semantics of their timestamp
> >>> with VRR.
> >>>
> >>> For page flip completion events though, the timestamp needs to be
> >>> accurate and correspond to when the flipped frame starts being scanned
> >>> out, otherwise we'll surely break at least some userspace relying on
> >>> this information.
> >>>  
> >> Yeah, I was. I guess what's sent is the estimated vblank timestamp
> >> calculated at the start of the interrupt.  
> > 
> > s/interrupt/vblank/, yeah.
> > 
> >   
> >> And since that's just a guess rather than what's actually going to
> >> happen it's going to be wrong in a lot of cases.
> >>
> >> I could see the wrap-around method working if the vblank timestamp was
> >> somehow updated in amdgpu or in drm_crtc_send_vblank_event.  
> > 
> > DC already calls drm_crtc_accurate_vblank_count before
> > drm_crtc_send_vblank_event, we "just" need to make sure that results in
> > an accurate timestamp.
> > 
> >   
> >> This would be a relatively simple fix but would break anything in
> >> userspace that relied on the timestamp for vblank interrupt and the
> >> flip completion being the same value.  
> > 
> > Hmm, that's a good point. So while VRR is enabled, maybe it is safer to
> > defer delivery of vblank events until the accurate timestamp is known as
> > well, at least by default. If there is userspace which needs the events
> > earlier even with VRR but can live with the guessed timestamp, a flag or
> > something could be added for that.
> > 
> >   
> 
> I was under the impression that the vblank timestamp was reused but it's 
> already going to differ since we call drm_crtc_accurate_vblank_count 
> before drm_crtc_send_vblank_event. Thanks for pointing that out.
> 
> Since that works for updating timestamp what's left is making sure that 
> it waits for the wrap around if it's above crtc_vtotal. It might make 
> sense to add a new flag for this that's only used within 
> amdgpu_get_crtc_scanout_position so the other call sites aren't affected.
> 
> There isn't a way to get an accurate timestamp with VRR enabled until 
> after the flip happens. So deferring it kind of defeats the purpose of a 
> client using it to make predictions before the flip for displaying their 
> content.

Hi,

I don't think I understood half of the discussion above, but I'd like
to offer how Weston uses vblank timestamps and pageflip timestamps
today.

When Weston starts rendering "from idle", which means it has lost track
of the past flip timestamps since it has not been flipping, it uses
drmWaitVBlank() to query the latest flip time non-blocking:

	drmVBlank vbl = {
		.request.type = DRM_VBLANK_RELATIVE,
		.request.sequence = 0,
		.request.signal = 0,
	};

If that returns a valid answer, it is taken as the base timestamp. (If
it doesn't result in a valid timestamp, Weston makes a flip to the
current FB and uses the realized pageflip timestamp of that as the base
timestamp.)  The base timestamp is used to predict the next possible
flip time by adding one refresh period to it.

This obviously depends heavily on fixed refresh rate. However, if
userspace can know the VRR flip timeout length, I think it should be
possible to compute the next possible flip window being between:
base timestamp + minimum refresh period + { 0, timeout }.

Of course, the prediction of the next possible flip window is only an
estimate, so some inaccuracy is allowed. We still rely on the
after-the-fact feedback in the form of pageflip timestamps to be
accurate, so that we can potentially adjust the userspace timings to
eventually hit what we predict.

Implementing support for flip target timestamps would be merely an
addition to all the above, that should improve the sub-refresh-period
timing accuracy even further. But, to compute target timestamps,
userspace would still need to predict the flip window to avoid being
half a refresh period or more off on average.

Therefore, if at all possible, I would like you to set the vblank and
pageflip timestamp semantics such that this prediction is not
fundamentally broken.


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 v4 3/4] drm: Document variable refresh properties

[Michel Dänzer]

On 2018-10-31 6:54 p.m., Kazlauskas, Nicholas wrote:
> On 10/31/18 12:20 PM, Michel Dänzer wrote:
>> On 2018-10-31 3:41 p.m., Kazlauskas, Nicholas wrote:
>>> On 10/31/18 10:12 AM, Michel Dänzer wrote:
>>>> On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:
>>>>> On 10/30/18 11:34 AM, Kazlauskas, Nicholas wrote:
>>>>>>
>>>>>> I understand the issue you're describing now. The timestamp is supposed
>>>>>> to signify the end of the current vblank. The call to get scanout
>>>>>> position is supposed to return the number of lines until scanout (a
>>>>>> negative value) or the number of lines since the next scanout began (a
>>>>>> positive value).
>>>>>>
>>>>>> The amdgpu driver calculates the number of lines based on a hardware
>>>>>> register status position which returns an increasing value from 0 that
>>>>>> indicates the current vpos/hpos for the display.
>>>>>>
>>>>>> For any vpos below vbl_start we know the value is correct since the next
>>>>>> vblank hasn't begun yet. But for anythign about vbl_start it's probably
>>>>>> wrong since it applies a corrective offset based on the fixed value of
>>>>>> crtc_vtotal. It's even worse when the value is above crtc_vtotal since
>>>>>> it'll be calculating the number of lines since the last scanout since
>>>>>> it'll be a positive value.
>>>>>>
>>>>>> So the issue becomes determining when the vfront porch will end.
>>>>>>
>>>>>> When the flip address gets written the vfront porch will end at the
>>>>>> start of the next line leaving only the back porch plus part of the
>>>>>> line. But we don't know when the flip will occur, if at all. It hasn't
>>>>>> occurred yet in this case.
>>>>>>
>>>>>> Waiting for the wrap around to 0 might be the best choice here since
>>>>>> there's no guarantee the flip will occur.
>>>>>
>>>>> I put some more thought into this and I don't think this is as bad as I
>>>>> had originally thought.
>>>>>
>>>>> I think the vblank timestamp is supposed to be for the first active
>>>>> pixel of the next scanout. The usage of which is for clients to time
>>>>> their content/animation/etc.
>>>>>
>>>>> The client likely doesn't care when they issue their flip, just that
>>>>> their content is matched for that vblank timestamp. The fixed refresh
>>>>> model works really well for that kind of application.
>>>>>
>>>>> Utilizing variable refresh rate would be a mistake in that case since
>>>>> the client would then have to time based on when they flip which is a
>>>>> lot harder to "predict" precisely.
>>>>
>>>> It's only a "mistake" as long as the timestamps are misleading. :) As
>>>> discussed before, accurate presentation timestamps are one requirement
>>>> for achieving perfectly smooth animation.
>>>
>>> For most 3D games the game world/rendering can advance by an arbitrary
>>> timestep - and faster will be smoother, which is what the native mode
>>> would give you.
>>>
>>> For fixed interval content/animation where you can't do that variable
>>> refresh rate could vastly improve the output. But like discussed before
>>> that would need a way to specify the interval/presentation timestamp to
>>> control that front porch duration. The timestamp will be misleading in
>>> any other case.
>>
>> I don't agree that an accurate timestamp can ever be more "misleading"
>> than an inaccurate one. But yeah, accurate timestamps and time-based
>> presentation are two sides of the same coin which can buy the holy grail
>> of perfectly smooth animation. :)
>>
>>
>>>>> I did some more investigation into when amdgpu gets the scanout position
>>>>> and what values we get back out of it. The timestamp is updated shortly
>>>>> after the crtc irq vblank which is typically within a few lines of
>>>>> vbl_start. This makes sense, since we can provide the prediction
>>>>> timestamp early. Waiting for the vblank to wrap around to 0 doesn't
>>>>> really make sense here since that would mean we already hit timeout or
>>>>> the flip occurred
>>>>
>>>> Sounds like you're mixing up the two cases of "actual" vblank events
>>>> (triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
>>>> flip completion events (triggered by the PFLIP interrupt with our
>>>> hardware => drm_crtc_send_vblank_event).
>>>>
>>>> Actual vblank events need to be delivered to userspace at the start of
>>>> vblank, so we indeed can't wait until the timestamp is accurate for
>>>> them. We just need to document the exact semantics of their timestamp
>>>> with VRR.
>>>>
>>>> For page flip completion events though, the timestamp needs to be
>>>> accurate and correspond to when the flipped frame starts being scanned
>>>> out, otherwise we'll surely break at least some userspace relying on
>>>> this information.
>>>>
>>> Yeah, I was. I guess what's sent is the estimated vblank timestamp
>>> calculated at the start of the interrupt.
>>
>> s/interrupt/vblank/, yeah.
>>
>>
>>> And since that's just a guess rather than what's actually going to
>>> happen it's going to be wrong in a lot of cases.
>>>
>>> I could see the wrap-around method working if the vblank timestamp was
>>> somehow updated in amdgpu or in drm_crtc_send_vblank_event.
>>
>> DC already calls drm_crtc_accurate_vblank_count before
>> drm_crtc_send_vblank_event, we "just" need to make sure that results in
>> an accurate timestamp.
>>
>>
>>> This would be a relatively simple fix but would break anything in
>>> userspace that relied on the timestamp for vblank interrupt and the
>>> flip completion being the same value.
>>
>> Hmm, that's a good point. So while VRR is enabled, maybe it is safer to
>> defer delivery of vblank events until the accurate timestamp is known as
>> well, at least by default. If there is userspace which needs the events
>> earlier even with VRR but can live with the guessed timestamp, a flag or
>> something could be added for that.
> 
> I was under the impression that the vblank timestamp was reused but it's 
> already going to differ since we call drm_crtc_accurate_vblank_count 
> before drm_crtc_send_vblank_event. Thanks for pointing that out.
> 
> Since that works for updating timestamp what's left is making sure that 
> it waits for the wrap around if it's above crtc_vtotal. It might make 
> sense to add a new flag for this that's only used within 
> amdgpu_get_crtc_scanout_position so the other call sites aren't affected.
> 
> There isn't a way to get an accurate timestamp with VRR enabled until 
> after the flip happens. So deferring it kind of defeats the purpose of a 
> client using it to make predictions before the flip for displaying their 
> content.

That's generally not a valid use-case. The KMS API is defined such that
if userspace receives the vblank event for vblank period N and then
submits a page flip, the page flip cannot (normally[0]) take effect
before vblank period N+1.


There are mainly two valid use-cases for waiting for vblank:

1. Wait for vblank N, then submit page flip for vblank N+1.

2. Wait for vblank N, then try to do something before vblank N ends,
e.g. draw directly to the front buffer, as a poor man's way of avoiding
tearing or similar artifacts.


Use-case 1 is used by Xorg (both for Present and DRI2) and at least
Weston, but presumably most if not all Wayland compositors. From Pekka's
description of how Weston uses it (thanks!), it's clear that the
timestamp of vblank events does need to accurately correspond to the end
of the vblank period.

Use-case 2 is also used by Xorg, but only when not flipping, which rules
out VRR. Also, I'd argue that blocking waits for vblank are more
suitable for this use-case than vblank events.

Therefore, in summary I propose this is how it could work:

* Without VRR, timestamps continue to work the same way they do now.

* With VRR, delivery of both vblank and page flip completion events must
be deferred until the timestamp accurately corresponds to the actual end
of vblank. For blocking vblank waits, I'm not sure, but I'm leaning
towards continuing to return to userspace at the beginning of vblank by
default, even if the timestamp is inaccurate.

Deferring delivery of events should avoid busy-loops. Bonus points for
minimizing the number of interrupts needed. :)

Note that both blocking vblank waits and vblank events are currently
triggered from drm_(crtc_)handle_vblank, so some rework would be
required for this.

It's possible that there's other userspace which doesn't care about the
accuracy of the timestamps, but cares about getting events as early as
possible. In that case, flags might need to be added.


[0] Our drivers can complete the flip in the same vblank period under
some circumstances if userspace specifies it as the target using the
DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flag, but there's no
guarantee for it to work. And no other driver supports this yet. Without
those flags, the flip cannot complete in the same vblank period, or it
would break existing userspace using use-case 1 above.

-- 
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 v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 11/1/18 6:58 AM, Michel Dänzer wrote:
> On 2018-10-31 6:54 p.m., Kazlauskas, Nicholas wrote:
>> On 10/31/18 12:20 PM, Michel Dänzer wrote:
>>> On 2018-10-31 3:41 p.m., Kazlauskas, Nicholas wrote:
>>>> On 10/31/18 10:12 AM, Michel Dänzer wrote:
>>>>> On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:
>>>>>> On 10/30/18 11:34 AM, Kazlauskas, Nicholas wrote:
>>>>>>>
>>>>>>> I understand the issue you're describing now. The timestamp is supposed
>>>>>>> to signify the end of the current vblank. The call to get scanout
>>>>>>> position is supposed to return the number of lines until scanout (a
>>>>>>> negative value) or the number of lines since the next scanout began (a
>>>>>>> positive value).
>>>>>>>
>>>>>>> The amdgpu driver calculates the number of lines based on a hardware
>>>>>>> register status position which returns an increasing value from 0 that
>>>>>>> indicates the current vpos/hpos for the display.
>>>>>>>
>>>>>>> For any vpos below vbl_start we know the value is correct since the next
>>>>>>> vblank hasn't begun yet. But for anythign about vbl_start it's probably
>>>>>>> wrong since it applies a corrective offset based on the fixed value of
>>>>>>> crtc_vtotal. It's even worse when the value is above crtc_vtotal since
>>>>>>> it'll be calculating the number of lines since the last scanout since
>>>>>>> it'll be a positive value.
>>>>>>>
>>>>>>> So the issue becomes determining when the vfront porch will end.
>>>>>>>
>>>>>>> When the flip address gets written the vfront porch will end at the
>>>>>>> start of the next line leaving only the back porch plus part of the
>>>>>>> line. But we don't know when the flip will occur, if at all. It hasn't
>>>>>>> occurred yet in this case.
>>>>>>>
>>>>>>> Waiting for the wrap around to 0 might be the best choice here since
>>>>>>> there's no guarantee the flip will occur.
>>>>>>
>>>>>> I put some more thought into this and I don't think this is as bad as I
>>>>>> had originally thought.
>>>>>>
>>>>>> I think the vblank timestamp is supposed to be for the first active
>>>>>> pixel of the next scanout. The usage of which is for clients to time
>>>>>> their content/animation/etc.
>>>>>>
>>>>>> The client likely doesn't care when they issue their flip, just that
>>>>>> their content is matched for that vblank timestamp. The fixed refresh
>>>>>> model works really well for that kind of application.
>>>>>>
>>>>>> Utilizing variable refresh rate would be a mistake in that case since
>>>>>> the client would then have to time based on when they flip which is a
>>>>>> lot harder to "predict" precisely.
>>>>>
>>>>> It's only a "mistake" as long as the timestamps are misleading. :) As
>>>>> discussed before, accurate presentation timestamps are one requirement
>>>>> for achieving perfectly smooth animation.
>>>>
>>>> For most 3D games the game world/rendering can advance by an arbitrary
>>>> timestep - and faster will be smoother, which is what the native mode
>>>> would give you.
>>>>
>>>> For fixed interval content/animation where you can't do that variable
>>>> refresh rate could vastly improve the output. But like discussed before
>>>> that would need a way to specify the interval/presentation timestamp to
>>>> control that front porch duration. The timestamp will be misleading in
>>>> any other case.
>>>
>>> I don't agree that an accurate timestamp can ever be more "misleading"
>>> than an inaccurate one. But yeah, accurate timestamps and time-based
>>> presentation are two sides of the same coin which can buy the holy grail
>>> of perfectly smooth animation. :)
>>>
>>>
>>>>>> I did some more investigation into when amdgpu gets the scanout position
>>>>>> and what values we get back out of it. The timestamp is updated shortly
>>>>>> after the crtc irq vblank which is typically within a few lines of
>>>>>> vbl_start. This makes sense, since we can provide the prediction
>>>>>> timestamp early. Waiting for the vblank to wrap around to 0 doesn't
>>>>>> really make sense here since that would mean we already hit timeout or
>>>>>> the flip occurred
>>>>>
>>>>> Sounds like you're mixing up the two cases of "actual" vblank events
>>>>> (triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
>>>>> flip completion events (triggered by the PFLIP interrupt with our
>>>>> hardware => drm_crtc_send_vblank_event).
>>>>>
>>>>> Actual vblank events need to be delivered to userspace at the start of
>>>>> vblank, so we indeed can't wait until the timestamp is accurate for
>>>>> them. We just need to document the exact semantics of their timestamp
>>>>> with VRR.
>>>>>
>>>>> For page flip completion events though, the timestamp needs to be
>>>>> accurate and correspond to when the flipped frame starts being scanned
>>>>> out, otherwise we'll surely break at least some userspace relying on
>>>>> this information.
>>>>>
>>>> Yeah, I was. I guess what's sent is the estimated vblank timestamp
>>>> calculated at the start of the interrupt.
>>>
>>> s/interrupt/vblank/, yeah.
>>>
>>>
>>>> And since that's just a guess rather than what's actually going to
>>>> happen it's going to be wrong in a lot of cases.
>>>>
>>>> I could see the wrap-around method working if the vblank timestamp was
>>>> somehow updated in amdgpu or in drm_crtc_send_vblank_event.
>>>
>>> DC already calls drm_crtc_accurate_vblank_count before
>>> drm_crtc_send_vblank_event, we "just" need to make sure that results in
>>> an accurate timestamp.
>>>
>>>
>>>> This would be a relatively simple fix but would break anything in
>>>> userspace that relied on the timestamp for vblank interrupt and the
>>>> flip completion being the same value.
>>>
>>> Hmm, that's a good point. So while VRR is enabled, maybe it is safer to
>>> defer delivery of vblank events until the accurate timestamp is known as
>>> well, at least by default. If there is userspace which needs the events
>>> earlier even with VRR but can live with the guessed timestamp, a flag or
>>> something could be added for that.
>>
>> I was under the impression that the vblank timestamp was reused but it's
>> already going to differ since we call drm_crtc_accurate_vblank_count
>> before drm_crtc_send_vblank_event. Thanks for pointing that out.
>>
>> Since that works for updating timestamp what's left is making sure that
>> it waits for the wrap around if it's above crtc_vtotal. It might make
>> sense to add a new flag for this that's only used within
>> amdgpu_get_crtc_scanout_position so the other call sites aren't affected.
>>
>> There isn't a way to get an accurate timestamp with VRR enabled until
>> after the flip happens. So deferring it kind of defeats the purpose of a
>> client using it to make predictions before the flip for displaying their
>> content.
> 
> That's generally not a valid use-case. The KMS API is defined such that
> if userspace receives the vblank event for vblank period N and then
> submits a page flip, the page flip cannot (normally[0]) take effect
> before vblank period N+1.
> 
> 
> There are mainly two valid use-cases for waiting for vblank:
> 
> 1. Wait for vblank N, then submit page flip for vblank N+1.
> 
> 2. Wait for vblank N, then try to do something before vblank N ends,
> e.g. draw directly to the front buffer, as a poor man's way of avoiding
> tearing or similar artifacts.
> 
> 
> Use-case 1 is used by Xorg (both for Present and DRI2) and at least
> Weston, but presumably most if not all Wayland compositors. From Pekka's
> description of how Weston uses it (thanks!), it's clear that the
> timestamp of vblank events does need to accurately correspond to the end
> of the vblank period.
> 
> Use-case 2 is also used by Xorg, but only when not flipping, which rules
> out VRR. Also, I'd argue that blocking waits for vblank are more
> suitable for this use-case than vblank events.
> 
> Therefore, in summary I propose this is how it could work:
> 
> * Without VRR, timestamps continue to work the same way they do now.
> 
> * With VRR, delivery of both vblank and page flip completion events must
> be deferred until the timestamp accurately corresponds to the actual end
> of vblank. For blocking vblank waits, I'm not sure, but I'm leaning
> towards continuing to return to userspace at the beginning of vblank by
> default, even if the timestamp is inaccurate.
> 
> Deferring delivery of events should avoid busy-loops. Bonus points for
> minimizing the number of interrupts needed. :)
> 
> Note that both blocking vblank waits and vblank events are currently
> triggered from drm_(crtc_)handle_vblank, so some rework would be
> required for this.
> 
> It's possible that there's other userspace which doesn't care about the
> accuracy of the timestamps, but cares about getting events as early as
> possible. In that case, flags might need to be added.
> 
> 
> [0] Our drivers can complete the flip in the same vblank period under
> some circumstances if userspace specifies it as the target using the
> DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flag, but there's no
> guarantee for it to work. And no other driver supports this yet. Without
> those flags, the flip cannot complete in the same vblank period, or it
> would break existing userspace using use-case 1 above.
>
Deferring the pageflip timestamp is something we can do today in the 
driver with no impact on existing behavior/semantics. Since hpos is used 
in the calculation as well I think we don't need even need to wait for 
the roll-over necessarily.

Deferring the event that comes from drm_handle_vblank worries me though. 
The implementation for that would be sending that event on the roll-over 
at timeout or grouping it together on the pageflip. Those are pretty 
long delays for both.

To me it still makes more sense to just define the vblank timestamp 
coming out of drm_crtc_handle_vblank as an estimate when VRR is enabled. 
An estimation which ends up being the same value as what userspace gets 
today, the estimation for the current mode.

I think my opinion would be different if the timestamps were supposed to 
be the same value but that's incredibly unlikely given that they're 
offset from the ktime_get in most cases.

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

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Michel Dänzer]

On 2018-11-01 3:58 p.m., Kazlauskas, Nicholas wrote:
> On 11/1/18 6:58 AM, Michel Dänzer wrote:
>> On 2018-10-31 6:54 p.m., Kazlauskas, Nicholas wrote:
>>> On 10/31/18 12:20 PM, Michel Dänzer wrote:
>>>> On 2018-10-31 3:41 p.m., Kazlauskas, Nicholas wrote:
>>>>> On 10/31/18 10:12 AM, Michel Dänzer wrote:
>>>>>> On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:
>>>>>>>
>>>>>>> I did some more investigation into when amdgpu gets the scanout position
>>>>>>> and what values we get back out of it. The timestamp is updated shortly
>>>>>>> after the crtc irq vblank which is typically within a few lines of
>>>>>>> vbl_start. This makes sense, since we can provide the prediction
>>>>>>> timestamp early. Waiting for the vblank to wrap around to 0 doesn't
>>>>>>> really make sense here since that would mean we already hit timeout or
>>>>>>> the flip occurred
>>>>>>
>>>>>> Sounds like you're mixing up the two cases of "actual" vblank events
>>>>>> (triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
>>>>>> flip completion events (triggered by the PFLIP interrupt with our
>>>>>> hardware => drm_crtc_send_vblank_event).
>>>>>>
>>>>>> Actual vblank events need to be delivered to userspace at the start of
>>>>>> vblank, so we indeed can't wait until the timestamp is accurate for
>>>>>> them. We just need to document the exact semantics of their timestamp
>>>>>> with VRR.
>>>>>>
>>>>>> For page flip completion events though, the timestamp needs to be
>>>>>> accurate and correspond to when the flipped frame starts being scanned
>>>>>> out, otherwise we'll surely break at least some userspace relying on
>>>>>> this information.
>>>>>>
>>>>> Yeah, I was. I guess what's sent is the estimated vblank timestamp
>>>>> calculated at the start of the interrupt.
>>>>
>>>> s/interrupt/vblank/, yeah.
>>>>
>>>>
>>>>> And since that's just a guess rather than what's actually going to
>>>>> happen it's going to be wrong in a lot of cases.
>>>>>
>>>>> I could see the wrap-around method working if the vblank timestamp was
>>>>> somehow updated in amdgpu or in drm_crtc_send_vblank_event.
>>>>
>>>> DC already calls drm_crtc_accurate_vblank_count before
>>>> drm_crtc_send_vblank_event, we "just" need to make sure that results in
>>>> an accurate timestamp.
>>>>
>>>>
>>>>> This would be a relatively simple fix but would break anything in
>>>>> userspace that relied on the timestamp for vblank interrupt and the
>>>>> flip completion being the same value.
>>>>
>>>> Hmm, that's a good point. So while VRR is enabled, maybe it is safer to
>>>> defer delivery of vblank events until the accurate timestamp is known as
>>>> well, at least by default. If there is userspace which needs the events
>>>> earlier even with VRR but can live with the guessed timestamp, a flag or
>>>> something could be added for that.
>>>
>>> I was under the impression that the vblank timestamp was reused but it's
>>> already going to differ since we call drm_crtc_accurate_vblank_count
>>> before drm_crtc_send_vblank_event. Thanks for pointing that out.
>>>
>>> Since that works for updating timestamp what's left is making sure that
>>> it waits for the wrap around if it's above crtc_vtotal. It might make
>>> sense to add a new flag for this that's only used within
>>> amdgpu_get_crtc_scanout_position so the other call sites aren't affected.
>>>
>>> There isn't a way to get an accurate timestamp with VRR enabled until
>>> after the flip happens. So deferring it kind of defeats the purpose of a
>>> client using it to make predictions before the flip for displaying their
>>> content.
>>
>> That's generally not a valid use-case. The KMS API is defined such that
>> if userspace receives the vblank event for vblank period N and then
>> submits a page flip, the page flip cannot (normally[0]) take effect
>> before vblank period N+1.
>>
>>
>> There are mainly two valid use-cases for waiting for vblank:
>>
>> 1. Wait for vblank N, then submit page flip for vblank N+1.
>>
>> 2. Wait for vblank N, then try to do something before vblank N ends,
>> e.g. draw directly to the front buffer, as a poor man's way of avoiding
>> tearing or similar artifacts.
>>
>>
>> Use-case 1 is used by Xorg (both for Present and DRI2) and at least
>> Weston, but presumably most if not all Wayland compositors. From Pekka's
>> description of how Weston uses it (thanks!), it's clear that the
>> timestamp of vblank events does need to accurately correspond to the end
>> of the vblank period.
>>
>> Use-case 2 is also used by Xorg, but only when not flipping, which rules
>> out VRR. Also, I'd argue that blocking waits for vblank are more
>> suitable for this use-case than vblank events.
>>
>> Therefore, in summary I propose this is how it could work:
>>
>> * Without VRR, timestamps continue to work the same way they do now.
>>
>> * With VRR, delivery of both vblank and page flip completion events must
>> be deferred until the timestamp accurately corresponds to the actual end
>> of vblank. For blocking vblank waits, I'm not sure, but I'm leaning
>> towards continuing to return to userspace at the beginning of vblank by
>> default, even if the timestamp is inaccurate.
>>
>> Deferring delivery of events should avoid busy-loops. Bonus points for
>> minimizing the number of interrupts needed. :)
>>
>> Note that both blocking vblank waits and vblank events are currently
>> triggered from drm_(crtc_)handle_vblank, so some rework would be
>> required for this.
>>
>> It's possible that there's other userspace which doesn't care about the
>> accuracy of the timestamps, but cares about getting events as early as
>> possible. In that case, flags might need to be added.
>>
>>
>> [0] Our drivers can complete the flip in the same vblank period under
>> some circumstances if userspace specifies it as the target using the
>> DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flag, but there's no
>> guarantee for it to work. And no other driver supports this yet. Without
>> those flags, the flip cannot complete in the same vblank period, or it
>> would break existing userspace using use-case 1 above.
>>
> Deferring the pageflip timestamp is something we can do today in the 
> driver with no impact on existing behavior/semantics. Since hpos is used 
> in the calculation as well I think we don't need even need to wait for 
> the roll-over necessarily.

Not sure offhand how hpos helps for this, but I'll trust your word for
it. :)


> Deferring the event that comes from drm_handle_vblank worries me though. 
> The implementation for that would be sending that event on the roll-over 
> at timeout or grouping it together on the pageflip. Those are pretty 
> long delays for both.
> 
> To me it still makes more sense to just define the vblank timestamp 
> coming out of drm_crtc_handle_vblank as an estimate when VRR is enabled. 
> An estimation which ends up being the same value as what userspace gets 
> today, the estimation for the current mode.
> 
> I think my opinion would be different if the timestamps were supposed to 
> be the same value but that's incredibly unlikely given that they're 
> offset from the ktime_get in most cases.

Right now, without VRR, they're supposed to be the same within measuring
accuracy. If you're seeing bigger differences than that, can you provide
some examples? You might have discovered a bug.


Anyway, I re-read Pekka's description, and it sounds like Weston only
uses the vblank timestamp if no flip completed in the last vblank
period, which normally shouldn't happen with VRR. Otherwise it uses the
timestamp from the flip completion event.

So, it sounds like having different timestamps between the two cases
should be fine for Weston after all. Sorry for the confusion above.

So, I'm okay with trying to change only the timestamps of the flip
completion events. That'll certainly make things easier. :)


-- 
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 v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 11/1/18 1:05 PM, Michel Dänzer wrote:
> On 2018-11-01 3:58 p.m., Kazlauskas, Nicholas wrote:
>> On 11/1/18 6:58 AM, Michel Dänzer wrote:
>>> On 2018-10-31 6:54 p.m., Kazlauskas, Nicholas wrote:
>>>> On 10/31/18 12:20 PM, Michel Dänzer wrote:
>>>>> On 2018-10-31 3:41 p.m., Kazlauskas, Nicholas wrote:
>>>>>> On 10/31/18 10:12 AM, Michel Dänzer wrote:
>>>>>>> On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:
>>>>>>>>
>>>>>>>> I did some more investigation into when amdgpu gets the scanout position
>>>>>>>> and what values we get back out of it. The timestamp is updated shortly
>>>>>>>> after the crtc irq vblank which is typically within a few lines of
>>>>>>>> vbl_start. This makes sense, since we can provide the prediction
>>>>>>>> timestamp early. Waiting for the vblank to wrap around to 0 doesn't
>>>>>>>> really make sense here since that would mean we already hit timeout or
>>>>>>>> the flip occurred
>>>>>>>
>>>>>>> Sounds like you're mixing up the two cases of "actual" vblank events
>>>>>>> (triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
>>>>>>> flip completion events (triggered by the PFLIP interrupt with our
>>>>>>> hardware => drm_crtc_send_vblank_event).
>>>>>>>
>>>>>>> Actual vblank events need to be delivered to userspace at the start of
>>>>>>> vblank, so we indeed can't wait until the timestamp is accurate for
>>>>>>> them. We just need to document the exact semantics of their timestamp
>>>>>>> with VRR.
>>>>>>>
>>>>>>> For page flip completion events though, the timestamp needs to be
>>>>>>> accurate and correspond to when the flipped frame starts being scanned
>>>>>>> out, otherwise we'll surely break at least some userspace relying on
>>>>>>> this information.
>>>>>>>
>>>>>> Yeah, I was. I guess what's sent is the estimated vblank timestamp
>>>>>> calculated at the start of the interrupt.
>>>>>
>>>>> s/interrupt/vblank/, yeah.
>>>>>
>>>>>
>>>>>> And since that's just a guess rather than what's actually going to
>>>>>> happen it's going to be wrong in a lot of cases.
>>>>>>
>>>>>> I could see the wrap-around method working if the vblank timestamp was
>>>>>> somehow updated in amdgpu or in drm_crtc_send_vblank_event.
>>>>>
>>>>> DC already calls drm_crtc_accurate_vblank_count before
>>>>> drm_crtc_send_vblank_event, we "just" need to make sure that results in
>>>>> an accurate timestamp.
>>>>>
>>>>>
>>>>>> This would be a relatively simple fix but would break anything in
>>>>>> userspace that relied on the timestamp for vblank interrupt and the
>>>>>> flip completion being the same value.
>>>>>
>>>>> Hmm, that's a good point. So while VRR is enabled, maybe it is safer to
>>>>> defer delivery of vblank events until the accurate timestamp is known as
>>>>> well, at least by default. If there is userspace which needs the events
>>>>> earlier even with VRR but can live with the guessed timestamp, a flag or
>>>>> something could be added for that.
>>>>
>>>> I was under the impression that the vblank timestamp was reused but it's
>>>> already going to differ since we call drm_crtc_accurate_vblank_count
>>>> before drm_crtc_send_vblank_event. Thanks for pointing that out.
>>>>
>>>> Since that works for updating timestamp what's left is making sure that
>>>> it waits for the wrap around if it's above crtc_vtotal. It might make
>>>> sense to add a new flag for this that's only used within
>>>> amdgpu_get_crtc_scanout_position so the other call sites aren't affected.
>>>>
>>>> There isn't a way to get an accurate timestamp with VRR enabled until
>>>> after the flip happens. So deferring it kind of defeats the purpose of a
>>>> client using it to make predictions before the flip for displaying their
>>>> content.
>>>
>>> That's generally not a valid use-case. The KMS API is defined such that
>>> if userspace receives the vblank event for vblank period N and then
>>> submits a page flip, the page flip cannot (normally[0]) take effect
>>> before vblank period N+1.
>>>
>>>
>>> There are mainly two valid use-cases for waiting for vblank:
>>>
>>> 1. Wait for vblank N, then submit page flip for vblank N+1.
>>>
>>> 2. Wait for vblank N, then try to do something before vblank N ends,
>>> e.g. draw directly to the front buffer, as a poor man's way of avoiding
>>> tearing or similar artifacts.
>>>
>>>
>>> Use-case 1 is used by Xorg (both for Present and DRI2) and at least
>>> Weston, but presumably most if not all Wayland compositors. From Pekka's
>>> description of how Weston uses it (thanks!), it's clear that the
>>> timestamp of vblank events does need to accurately correspond to the end
>>> of the vblank period.
>>>
>>> Use-case 2 is also used by Xorg, but only when not flipping, which rules
>>> out VRR. Also, I'd argue that blocking waits for vblank are more
>>> suitable for this use-case than vblank events.
>>>
>>> Therefore, in summary I propose this is how it could work:
>>>
>>> * Without VRR, timestamps continue to work the same way they do now.
>>>
>>> * With VRR, delivery of both vblank and page flip completion events must
>>> be deferred until the timestamp accurately corresponds to the actual end
>>> of vblank. For blocking vblank waits, I'm not sure, but I'm leaning
>>> towards continuing to return to userspace at the beginning of vblank by
>>> default, even if the timestamp is inaccurate.
>>>
>>> Deferring delivery of events should avoid busy-loops. Bonus points for
>>> minimizing the number of interrupts needed. :)
>>>
>>> Note that both blocking vblank waits and vblank events are currently
>>> triggered from drm_(crtc_)handle_vblank, so some rework would be
>>> required for this.
>>>
>>> It's possible that there's other userspace which doesn't care about the
>>> accuracy of the timestamps, but cares about getting events as early as
>>> possible. In that case, flags might need to be added.
>>>
>>>
>>> [0] Our drivers can complete the flip in the same vblank period under
>>> some circumstances if userspace specifies it as the target using the
>>> DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flag, but there's no
>>> guarantee for it to work. And no other driver supports this yet. Without
>>> those flags, the flip cannot complete in the same vblank period, or it
>>> would break existing userspace using use-case 1 above.
>>>
>> Deferring the pageflip timestamp is something we can do today in the
>> driver with no impact on existing behavior/semantics. Since hpos is used
>> in the calculation as well I think we don't need even need to wait for
>> the roll-over necessarily.
> 
> Not sure offhand how hpos helps for this, but I'll trust your word for
> it. :)

If we're still in the vfront porch when on the pageflip irq timestamp 
query then it should be going over to the back porch on the next line.

So there's still the full back porch to go through, plus a little bit of 
the current line horizontally. That horizontal bit is included in the 
timestamp calculation already so it should be fine.

> 
> 
>> Deferring the event that comes from drm_handle_vblank worries me though.
>> The implementation for that would be sending that event on the roll-over
>> at timeout or grouping it together on the pageflip. Those are pretty
>> long delays for both.
>>
>> To me it still makes more sense to just define the vblank timestamp
>> coming out of drm_crtc_handle_vblank as an estimate when VRR is enabled.
>> An estimation which ends up being the same value as what userspace gets
>> today, the estimation for the current mode.
>>
>> I think my opinion would be different if the timestamps were supposed to
>> be the same value but that's incredibly unlikely given that they're
>> offset from the ktime_get in most cases.
> 
> Right now, without VRR, they're supposed to be the same within measuring
> accuracy. If you're seeing bigger differences than that, can you provide
> some examples? You might have discovered a bug.

There's a little bit of uncertainty since it's two separate calls to 
get_crtc_scanoutpos and two different calls to ktime_get for the end 
time. The numbers differ but not by a significant amount. I wouldn't 
consider it a bug at least.

> 
> 
> Anyway, I re-read Pekka's description, and it sounds like Weston only
> uses the vblank timestamp if no flip completed in the last vblank
> period, which normally shouldn't happen with VRR. Otherwise it uses the
> timestamp from the flip completion event.
> 
> So, it sounds like having different timestamps between the two cases
> should be fine for Weston after all. Sorry for the confusion above.
> 
> So, I'm okay with trying to change only the timestamps of the flip
> completion events. That'll certainly make things easier. :)
> 
> 

Sounds good.

The only change needed to handle this in the pageflip is a small bugfix 
for the case where vpos >= crtc_vtotal on the pageflip query. I'll 
include it as part of the next patch series with the updated documentation.

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