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

[PATCH v5 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 v4 ===

amd changes:

* Removed unused FreeSync functions from amdgpu

=== 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
https://lists.freedesktop.org/archives/dri-devel/2018-October/192874.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/amdgpu: Set FreeSync state using drm VRR properties

 Documentation/gpu/drm-kms.rst                 |   7 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |   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 +
 11 files changed, 257 insertions(+), 131 deletions(-)

-- 
2.19.1

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

[PATCH v5 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

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

[PATCH v5 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 v5 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 v5 4/4] drm/amdgpu: 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 notify_freesync and set_freesync_property functions are dropped
from amdgpu_display_funcs.

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>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |   7 -
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++---------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   7 +-
 3 files changed, 138 insertions(+), 131 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index b9e9e8b02fb7..0cbe867ec375 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -295,13 +295,6 @@ struct amdgpu_display_funcs {
 			      uint16_t connector_object_id,
 			      struct amdgpu_hpd *hpd,
 			      struct amdgpu_router *router);
-	/* it is used to enter or exit into free sync mode */
-	int (*notify_freesync)(struct drm_device *dev, void *data,
-			       struct drm_file *filp);
-	/* it is used to allow enablement of freesync mode */
-	int (*set_freesync_property)(struct drm_connector *connector,
-				     struct drm_property *property,
-				     uint64_t val);
 
 
 };
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 e224f23e2215..f6af388cc32d 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,
@@ -4905,20 +4918,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 =
@@ -4928,19 +4939,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,
@@ -5015,9 +5025,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;
@@ -5026,9 +5033,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;
 
@@ -5065,6 +5069,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 */
@@ -5142,7 +5148,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;
@@ -5407,12 +5415,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)
@@ -5564,14 +5569,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) {
@@ -5581,9 +5587,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);
@@ -5591,10 +5595,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
 	 */
@@ -5606,7 +5610,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++) {
@@ -5638,8 +5641,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

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

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

[Christian König]

Am 12.10.2018 um 18:44 schrieb 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.

I'm honestly still not convinced that a per CRTC property is actually 
the right approach.

What we should rather do instead is to implement a target timestamp for 
the page flip.

Regards,
Christian.

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

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

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

[Michel Dänzer]

On 2018-10-13 7:38 p.m., Christian König wrote:
> Am 12.10.2018 um 18:44 schrieb 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.
> 
> I'm honestly still not convinced that a per CRTC property is actually
> the right approach.
> 
> What we should rather do instead is to implement a target timestamp for
> the page flip.

You'd have to be more specific about how the latter could completely
replace the former. I don't see how it could.


-- 
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 v5 2/4] drm: Add vrr_enabled property to drm CRTC

[Christian König]

Am 15.10.2018 um 11:40 schrieb Michel Dänzer:
> On 2018-10-13 7:38 p.m., Christian König wrote:
>> Am 12.10.2018 um 18:44 schrieb 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.
>> I'm honestly still not convinced that a per CRTC property is actually
>> the right approach.
>>
>> What we should rather do instead is to implement a target timestamp for
>> the page flip.
> You'd have to be more specific about how the latter could completely
> replace the former. I don't see how it could.

Each flip request send by an application gets a timestamp when the flip 
should be displayed.

If I'm not completely mistaken we already have something like that in 
both DRI2 as well as DRI3.

So as far as I can see we only need to add an extra flag that those 
information are trust worth in the context of VRR as well.

If we don't set this flag we always get the always working fallback 
behavior, e.g. VRR is disabled and we have a fixed refresh rate.

If we set this flag and the timestamp is in the past we get the VRR 
behavior to display the next frame as soon as possible.

If we set this flag and specific a specific timestamp then we get the 
VRR behavior to display the frame as close as possible to the specified 
timestamp.

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

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

[Michel Dänzer]

On 2018-10-15 11:47 a.m., Christian König wrote:
> Am 15.10.2018 um 11:40 schrieb Michel Dänzer:
>> On 2018-10-13 7:38 p.m., Christian König wrote:
>>> Am 12.10.2018 um 18:44 schrieb 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.
>>> I'm honestly still not convinced that a per CRTC property is actually
>>> the right approach.
>>>
>>> What we should rather do instead is to implement a target timestamp for
>>> the page flip.
>> You'd have to be more specific about how the latter could completely
>> replace the former. I don't see how it could.
> 
> Each flip request send by an application gets a timestamp when the flip
> should be displayed.
> 
> If I'm not completely mistaken we already have something like that in
> both DRI2 as well as DRI3.

Certainly not DRI2, but for now we're not enabling VRR with that anyway.

While the X11 Present extension specifies PresentOptionUST for this,
support for it isn't implemented yet in xserver (as in setting the
option has no effect, the X server interprets the target value as an MSC
anyway).

So this couldn't work before the next major release of xserver, which
based on recent history could be at least about one year away.

In contrast, the CRTC property based solution for the gaming use-case
can work even with older xserver versions.


> So as far as I can see we only need to add an extra flag that those
> information are trust worth in the context of VRR as well.
> 
> If we don't set this flag we always get the always working fallback
> behavior, e.g. VRR is disabled and we have a fixed refresh rate.
> 
> If we set this flag and the timestamp is in the past we get the VRR
> behavior to display the next frame as soon as possible.
> 
> If we set this flag and specific a specific timestamp then we get the
> VRR behavior to display the frame as close as possible to the specified
> timestamp.

Apart from the above, another issue is that this would give direct
control to the client on whether or not VRR should be used. But we want
to allow the user to disable VRR even if a client wants to use it, via
an RandR output property. This requires that the Xorg driver can control
whether or not VRR can actually be used, via the CRTC property added by
this patch.


-- 
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 v5 2/4] drm: Add vrr_enabled property to drm CRTC

[Koenig, Christian]

Am 15.10.2018 um 12:06 schrieb Michel Dänzer:
> [SNIP]
> Apart from the above, another issue is that this would give direct
> control to the client on whether or not VRR should be used. But we want
> to allow the user to disable VRR even if a client wants to use it, via
> an RandR output property. This requires that the Xorg driver can control
> whether or not VRR can actually be used, via the CRTC property added by
> this patch.
Oh, that is a really good argument!

No objection from my side to this then,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Wentland, Harry]

On 2018-10-12 12:44 p.m., Nicholas Kazlauskas wrote:
> 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>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

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

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

[Wentland, Harry]

On 2018-10-12 12:44 p.m., Nicholas Kazlauskas wrote:
> 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>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

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

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

[Wentland, Harry]

On 2018-10-12 12:44 p.m., Nicholas Kazlauskas 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.
> + *
> + *	Support for variable refresh rate will depend on the "vrr_capable"
> + *	property exposed on the &drm_connector object.

We probably want to make it clear that this is a content hint. Maybe something like:

 * "vrr_enabled":
 *	Default &drm_crtc boolean property that notifies the driver that the
 *	content on the CRTC is suitable for variable refresh presentation.
 *	The driver will take that as a hint to enable variable refresh rate
 *	if the receiver supports it, i.e. the "vrr_capable" property on the
 *	&drm_connector_object is true.


Harry

> + */
> +
>  /**
>   * drm_connector_attach_vrr_capable_property - creates the
>   * vrr_capable property
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v5 4/4] drm/amdgpu: Set FreeSync state using drm VRR properties

[Wentland, Harry]

On 2018-10-12 12:44 p.m., 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 notify_freesync and set_freesync_property functions are dropped
> from amdgpu_display_funcs.
> 
> 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>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |   7 -
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++---------
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   7 +-
>  3 files changed, 138 insertions(+), 131 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index b9e9e8b02fb7..0cbe867ec375 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -295,13 +295,6 @@ struct amdgpu_display_funcs {
>  			      uint16_t connector_object_id,
>  			      struct amdgpu_hpd *hpd,
>  			      struct amdgpu_router *router);
> -	/* it is used to enter or exit into free sync mode */
> -	int (*notify_freesync)(struct drm_device *dev, void *data,
> -			       struct drm_file *filp);
> -	/* it is used to allow enablement of freesync mode */
> -	int (*set_freesync_property)(struct drm_connector *connector,
> -				     struct drm_property *property,
> -				     uint64_t val);
>  
>  
>  };
> 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 e224f23e2215..f6af388cc32d 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,
> @@ -4905,20 +4918,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 =
> @@ -4928,19 +4939,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,
> @@ -5015,9 +5025,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;
> @@ -5026,9 +5033,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;
>  
> @@ -5065,6 +5069,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 */
> @@ -5142,7 +5148,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;
> @@ -5407,12 +5415,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)
> @@ -5564,14 +5569,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) {
> @@ -5581,9 +5587,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);
> @@ -5591,10 +5595,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
>  	 */
> @@ -5606,7 +5610,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++) {
> @@ -5638,8 +5641,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;
>  };
>  
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

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

On Mon, 15 Oct 2018 12:06:52 +0200
Michel Dänzer <michel-otUistvHUpPR7s880joybQ@public.gmane.org> wrote:

> On 2018-10-15 11:47 a.m., Christian König wrote:
> > Am 15.10.2018 um 11:40 schrieb Michel Dänzer:  
> >> On 2018-10-13 7:38 p.m., Christian König wrote:  
> >>> Am 12.10.2018 um 18:44 schrieb 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.  
> >>> I'm honestly still not convinced that a per CRTC property is actually
> >>> the right approach.
> >>>
> >>> What we should rather do instead is to implement a target timestamp for
> >>> the page flip.  
> >> You'd have to be more specific about how the latter could completely
> >> replace the former. I don't see how it could.  
> > 
> > Each flip request send by an application gets a timestamp when the flip
> > should be displayed.
> > 
> > If I'm not completely mistaken we already have something like that in
> > both DRI2 as well as DRI3.  
> 
> Certainly not DRI2, but for now we're not enabling VRR with that anyway.
> 
> While the X11 Present extension specifies PresentOptionUST for this,
> support for it isn't implemented yet in xserver (as in setting the
> option has no effect, the X server interprets the target value as an MSC
> anyway).
> 
> So this couldn't work before the next major release of xserver, which
> based on recent history could be at least about one year away.

Hi

> In contrast, the CRTC property based solution for the gaming use-case
> can work even with older xserver versions.

This is probably the heaviest reason.

Coming up with a KMS UABI for target timestamps could get complicated.
Do you need a flip queue deeper than one? Do you need to be able to
cancel flips?

> > So as far as I can see we only need to add an extra flag that those
> > information are trust worth in the context of VRR as well.
> > 
> > If we don't set this flag we always get the always working fallback
> > behavior, e.g. VRR is disabled and we have a fixed refresh rate.
> > 
> > If we set this flag and the timestamp is in the past we get the VRR
> > behavior to display the next frame as soon as possible.
> > 
> > If we set this flag and specific a specific timestamp then we get the
> > VRR behavior to display the frame as close as possible to the specified
> > timestamp.  
> 
> Apart from the above, another issue is that this would give direct
> control to the client on whether or not VRR should be used. But we want
> to allow the user to disable VRR even if a client wants to use it, via
> an RandR output property. This requires that the Xorg driver can control
> whether or not VRR can actually be used, via the CRTC property added by
> this patch.

It would not imply direct control to clients. The target timestamps go
through the X server, the X server can mangle them or remove them
before calling KMS any way it wants. The X server can invent a RandR
property to disable/enable VRR.

One would need a video-DDX update the very minimum to start passing the
timestamps to the kernel, so there is no way VRR would be enabled
unwanted.


Thanks,
pq

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

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

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