Hi,
This work from me and Simon adds support for DRM_MODE_PAGE_FLIP_ASYNC through
the atomic API. This feature is already available via the legacy API. The use
case is to be able to present a new frame immediately (or as soon as
possible), even if after missing a vblank. This might result in tearing, but
it's useful when a high framerate is desired, such as for gaming.
Differently from earlier versions, this one refuses to flip if any prop changes
for async flips. The idea is that the fast path of immediate page flips doesn't
play well with modeset changes, so only the fb_id can be changed.
Thanks,
André
- User-space patch: https://github.com/Plagman/gamescope/pull/595
- IGT tests: https://gitlab.freedesktop.org/andrealmeid/igt-gpu-tools/-/tree/atomic_async_page_flip
Changes from v7:
- Only accept flips to primary planes. If a driver support flips in different
planes, support will be added later.
v7: https://lore.kernel.org/dri-devel/[email protected]/
Changes from v6:
- Dropped the exception to allow MODE_ID changes (Simon)
- Clarify what happens when flipping with the same FB_ID (Pekka)
v6: https://lore.kernel.org/dri-devel/[email protected]/
Changes from v5:
- Add note in the docs that not every redundant attribute will result in no-op,
some might cause oversynchronization issues.
v5: https://lore.kernel.org/dri-devel/[email protected]/
Changes from v4:
- Documentation rewrote by Pekka Paalanen
v4: https://lore.kernel.org/dri-devel/[email protected]/
Changes from v3:
- Add new patch to reject prop changes
- Add a documentation clarifying the KMS atomic state set
v3: https://lore.kernel.org/dri-devel/[email protected]/
André Almeida (1):
drm: Refuse to async flip with atomic prop changes
Pekka Paalanen (1):
drm/doc: Define KMS atomic state set
Simon Ser (4):
drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
drm: introduce drm_mode_config.atomic_async_page_flip_not_supported
amd/display: indicate support for atomic async page-flips on DC
Documentation/gpu/drm-uapi.rst | 47 +++++++++++
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 1 +
drivers/gpu/drm/drm_atomic_uapi.c | 82 +++++++++++++++++--
drivers/gpu/drm/drm_crtc_internal.h | 2 +-
drivers/gpu/drm/drm_ioctl.c | 5 ++
drivers/gpu/drm/drm_mode_object.c | 2 +-
.../drm/i915/display/intel_display_driver.c | 1 +
drivers/gpu/drm/nouveau/nouveau_display.c | 1 +
include/drm/drm_mode_config.h | 11 +++
include/uapi/drm/drm.h | 10 ++-
include/uapi/drm/drm_mode.h | 9 ++
11 files changed, 162 insertions(+), 9 deletions(-)
--
2.42.0
Given that prop changes may lead to modesetting, which would defeat the
fast path of the async flip, refuse any atomic prop change for async
flips in atomic API. The only exception is the framebuffer ID to flip
to. Currently the only plane type supported is the primary one.
Reviewed-by: Simon Ser <[email protected]>
Signed-off-by: André Almeida <[email protected]>
---
v8: add a check for plane type, we can only flip primary planes
v7: drop the mode_id exception for prop changes
---
---
drivers/gpu/drm/drm_atomic_uapi.c | 54 +++++++++++++++++++++++++++--
drivers/gpu/drm/drm_crtc_internal.h | 2 +-
drivers/gpu/drm/drm_mode_object.c | 2 +-
3 files changed, 53 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a15121e75a0a..ebaa6413d5a0 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1006,13 +1006,28 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
return ret;
}
+static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
+ struct drm_property *prop)
+{
+ if (ret != 0 || old_val != prop_value) {
+ drm_dbg_atomic(prop->dev,
+ "[PROP:%d:%s] No prop can be changed during async flip\n",
+ prop->base.id, prop->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
int drm_atomic_set_property(struct drm_atomic_state *state,
struct drm_file *file_priv,
struct drm_mode_object *obj,
struct drm_property *prop,
- uint64_t prop_value)
+ uint64_t prop_value,
+ bool async_flip)
{
struct drm_mode_object *ref;
+ uint64_t old_val;
int ret;
if (!drm_property_change_valid_get(prop, prop_value, &ref))
@@ -1029,6 +1044,13 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
break;
}
+ if (async_flip) {
+ ret = drm_atomic_connector_get_property(connector, connector_state,
+ prop, &old_val);
+ ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+ break;
+ }
+
ret = drm_atomic_connector_set_property(connector,
connector_state, file_priv,
prop, prop_value);
@@ -1044,6 +1066,13 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
break;
}
+ if (async_flip) {
+ ret = drm_atomic_crtc_get_property(crtc, crtc_state,
+ prop, &old_val);
+ ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+ break;
+ }
+
ret = drm_atomic_crtc_set_property(crtc,
crtc_state, prop, prop_value);
break;
@@ -1051,6 +1080,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
case DRM_MODE_OBJECT_PLANE: {
struct drm_plane *plane = obj_to_plane(obj);
struct drm_plane_state *plane_state;
+ struct drm_mode_config *config = &plane->dev->mode_config;
plane_state = drm_atomic_get_plane_state(state, plane);
if (IS_ERR(plane_state)) {
@@ -1058,6 +1088,21 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
break;
}
+ if (async_flip && prop != config->prop_fb_id) {
+ ret = drm_atomic_plane_get_property(plane, plane_state,
+ prop, &old_val);
+ ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+ break;
+ }
+
+ if (async_flip && plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
+ drm_dbg_atomic(prop->dev,
+ "[OBJECT:%d] Only primary planes can be changed during async flip\n",
+ obj->id);
+ ret = -EINVAL;
+ break;
+ }
+
ret = drm_atomic_plane_set_property(plane,
plane_state, file_priv,
prop, prop_value);
@@ -1349,6 +1394,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_out_fence_state *fence_state;
int ret = 0;
unsigned int i, j, num_fences;
+ bool async_flip = false;
/* disallow for drivers not supporting atomic: */
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1385,6 +1431,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
"commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported with atomic\n");
return -EINVAL;
}
+
+ async_flip = true;
}
/* can't test and expect an event at the same time. */
@@ -1469,8 +1517,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
goto out;
}
- ret = drm_atomic_set_property(state, file_priv,
- obj, prop, prop_value);
+ ret = drm_atomic_set_property(state, file_priv, obj,
+ prop, prop_value, async_flip);
if (ret) {
drm_mode_object_put(obj);
goto out;
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 8556c3b3ff88..a4c2ea33b1ef 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -251,7 +251,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
struct drm_file *file_priv,
struct drm_mode_object *obj,
struct drm_property *prop,
- uint64_t prop_value);
+ uint64_t prop_value, bool async_flip);
int drm_atomic_get_property(struct drm_mode_object *obj,
struct drm_property *property, uint64_t *val);
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index ac0d2ce3f870..0e8355063eee 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -538,7 +538,7 @@ static int set_property_atomic(struct drm_mode_object *obj,
obj_to_connector(obj),
prop_value);
} else {
- ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value);
+ ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value, false);
if (ret)
goto out;
ret = drm_atomic_commit(state);
--
2.42.0
From: Pekka Paalanen <[email protected]>
Specify how the atomic state is maintained between userspace and
kernel, plus the special case for async flips.
Signed-off-by: Pekka Paalanen <[email protected]>
Signed-off-by: André Almeida <[email protected]>
---
v8:
- no changes
v7:
- add a note that drivers can make exceptions for ad-hoc prop changes
- add a note about flipping the same FB_ID as a no-op
---
---
Documentation/gpu/drm-uapi.rst | 47 ++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 632989df3727..34bd02270ee7 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -570,3 +570,50 @@ dma-buf interoperability
Please see Documentation/userspace-api/dma-buf-alloc-exchange.rst for
information on how dma-buf is integrated and exposed within DRM.
+
+KMS atomic state
+================
+
+An atomic commit can change multiple KMS properties in an atomic fashion,
+without ever applying intermediate or partial state changes. Either the whole
+commit succeeds or fails, and it will never be applied partially. This is the
+fundamental improvement of the atomic API over the older non-atomic API which is
+referred to as the "legacy API". Applying intermediate state could unexpectedly
+fail, cause visible glitches, or delay reaching the final state.
+
+An atomic commit can be flagged with DRM_MODE_ATOMIC_TEST_ONLY, which means the
+complete state change is validated but not applied. Userspace should use this
+flag to validate any state change before asking to apply it. If validation fails
+for any reason, userspace should attempt to fall back to another, perhaps
+simpler, final state. This allows userspace to probe for various configurations
+without causing visible glitches on screen and without the need to undo a
+probing change.
+
+The changes recorded in an atomic commit apply on top the current KMS state in
+the kernel. Hence, the complete new KMS state is the complete old KMS state with
+the committed property settings done on top. The kernel will try to avoid
+no-operation changes, so it is safe for userspace to send redundant property
+settings. However, not every situation allows for no-op changes, due to the
+need to acquire locks for some attributes. Userspace needs to be aware that some
+redundant information might result in oversynchronization issues. No-operation
+changes do not count towards actually needed changes, e.g. setting MODE_ID to a
+different blob with identical contents as the current KMS state shall not be a
+modeset on its own. As a special exception for VRR needs, explicitly setting
+FB_ID to its current value is not a no-op.
+
+A "modeset" is a change in KMS state that might enable, disable, or temporarily
+disrupt the emitted video signal, possibly causing visible glitches on screen. A
+modeset may also take considerably more time to complete than other kinds of
+changes, and the video sink might also need time to adapt to the new signal
+properties. Therefore a modeset must be explicitly allowed with the flag
+DRM_MODE_ATOMIC_ALLOW_MODESET. This in combination with
+DRM_MODE_ATOMIC_TEST_ONLY allows userspace to determine if a state change is
+likely to cause visible disruption on screen and avoid such changes when end
+users do not expect them.
+
+An atomic commit with the flag DRM_MODE_PAGE_FLIP_ASYNC is allowed to
+effectively change only the FB_ID property on any planes. No-operation changes
+are ignored as always. Changing any other property will cause the commit to be
+rejected. Each driver may relax this restriction if they have guarantees that
+such property change doesn't cause modesets. Userspace can use TEST_ONLY commits
+to query the driver about this.
--
2.42.0
From: Simon Ser <[email protected]>
This new field indicates whether the driver has the necessary logic
to support async page-flips via the atomic uAPI. This is leveraged by
the next commit to allow user-space to use this functionality.
All atomic drivers setting drm_mode_config.async_page_flip are updated
to also set drm_mode_config.atomic_async_page_flip_not_supported. We
will gradually check and update these drivers to properly handle
drm_crtc_state.async_flip in their atomic logic.
The goal of this negative flag is the same as
fb_modifiers_not_supported: we want to eventually get rid of all
drivers missing atomic support for async flips. New drivers should not
set this flag, instead they should support atomic async flips (if
they support async flips at all). IOW, we don't want more drivers
with async flip support for legacy but not atomic.
Signed-off-by: Simon Ser <[email protected]>
Reviewed-by: André Almeida <[email protected]>
Reviewed-by: Alex Deucher <[email protected]>
Signed-off-by: André Almeida <[email protected]>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 +
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 1 +
drivers/gpu/drm/i915/display/intel_display_driver.c | 1 +
drivers/gpu/drm/nouveau/nouveau_display.c | 1 +
include/drm/drm_mode_config.h | 11 +++++++++++
5 files changed, 15 insertions(+)
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 45b8fd61a044..dec6e43e7198 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4003,6 +4003,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
adev_to_drm(adev)->mode_config.prefer_shadow = 1;
/* indicates support for immediate flip */
adev_to_drm(adev)->mode_config.async_page_flip = true;
+ adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = true;
state = kzalloc(sizeof(*state), GFP_KERNEL);
if (!state)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 84c54e8622d1..f1d9bb1d7c34 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -639,6 +639,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
dev->mode_config.max_height = dc->desc->max_height;
dev->mode_config.funcs = &mode_config_funcs;
dev->mode_config.async_page_flip = true;
+ dev->mode_config.atomic_async_page_flip_not_supported = true;
return 0;
}
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
index 44b59ac301e6..6142c83fba06 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
@@ -126,6 +126,7 @@ static void intel_mode_config_init(struct drm_i915_private *i915)
mode_config->helper_private = &intel_mode_config_funcs;
mode_config->async_page_flip = HAS_ASYNC_FLIPS(i915);
+ mode_config->atomic_async_page_flip_not_supported = true;
/*
* Maximum framebuffer dimensions, chosen to match
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index d8c92521226d..586aa51794e8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -720,6 +720,7 @@ nouveau_display_create(struct drm_device *dev)
dev->mode_config.async_page_flip = false;
else
dev->mode_config.async_page_flip = true;
+ dev->mode_config.atomic_async_page_flip_not_supported = true;
drm_kms_helper_poll_init(dev);
drm_kms_helper_poll_disable(dev);
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 973119a9176b..47b005671e6a 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -918,6 +918,17 @@ struct drm_mode_config {
*/
bool async_page_flip;
+ /**
+ * @atomic_async_page_flip_not_supported:
+ *
+ * If true, the driver does not support async page-flips with the
+ * atomic uAPI. This is only used by old drivers which haven't yet
+ * accomodated for &drm_crtc_state.async_flip in their atomic logic,
+ * even if they have &drm_mode_config.async_page_flip set to true.
+ * New drivers shall not set this flag.
+ */
+ bool atomic_async_page_flip_not_supported;
+
/**
* @fb_modifiers_not_supported:
*
--
2.42.0
From: Simon Ser <[email protected]>
amdgpu_dm_commit_planes() already sets the flip_immediate flag for
async page-flips. This flag is used to set the UNP_FLIP_CONTROL
register. Thus, no additional change is required to handle async
page-flips with the atomic uAPI.
Signed-off-by: Simon Ser <[email protected]>
Reviewed-by: André Almeida <[email protected]>
Reviewed-by: Alex Deucher <[email protected]>
Signed-off-by: André Almeida <[email protected]>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 -
1 file changed, 1 deletion(-)
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 dec6e43e7198..45b8fd61a044 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4003,7 +4003,6 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
adev_to_drm(adev)->mode_config.prefer_shadow = 1;
/* indicates support for immediate flip */
adev_to_drm(adev)->mode_config.async_page_flip = true;
- adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = true;
state = kzalloc(sizeof(*state), GFP_KERNEL);
if (!state)
--
2.42.0
It seems like commits were re-ordered at some point. I think we need "drm:
introduce drm_mode_config.atomic_async_page_flip_not_supported" to come before
"drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits" because the latter
uses atomic_async_page_flip_not_supported. Similarly, "drm: Refuse to async flip
with atomic prop changes" should probably come before that same patch because
we don't want to have an intermediary state where we allow user-space to change
arbitrary properties. In general, commits should be constructed so that there
is no "broken state" in-between: each commit needs to build without errors and
not have transient bugs.
While reading this again, I think that now that we only allow primary FB_ID to
change, we don't need atomic_async_page_flip_not_supported anymore? In other
words, allowing async atomic commits on all drivers doesn't require anything
more than they support already, because the atomic codepath can't do anything
more than the legacy codepath.
With that in mind, I also wonder if the new cap is helpful. Since async atomic
commits can fail for pretty much any reason, user-space can just try and
fallback to something else. The cap could be useful to indicate whether async
atomic commits would always fail, but not sure how useful that is? I don't have
strong opinions either way.