Re: [PATCH v6 5/6] drm: Refuse to async flip with atomic prop changes
On Tuesday, August 15th, 2023 at 20:57, André Almeida
wrote:
> 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 exceptions are the framebuffer ID to flip
> to and the mode ID, that could be referring to an identical mode.
>
> Signed-off-by: André Almeida
> ---
> v4: new patch
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 5 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 52 +++--
> drivers/gpu/drm/drm_crtc_internal.h | 2 +-
> drivers/gpu/drm/drm_mode_object.c | 2 +-
> 4 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 2c2c9caf0be5..1e2973f0e1f6 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -629,6 +629,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
>
> if (!drm_mode_equal(&old_crtc_state->mode,
> &new_crtc_state->mode)) {
> + if (new_crtc_state->async_flip) {
> + drm_dbg_atomic(dev, "[CRTC:%d:%s] no mode
> changes allowed during async flip\n",
> +crtc->base.id, crtc->name);
> + return -EINVAL;
> + }
Hmm, so, I've been going back and forth about this for a loong time. Each
time I convince myself that one of the options we have is a good solution, I
think of the drawbacks and change my mind. To sum up:
- Forbid non-FB_ID props from being included in the atomic commit: makes it
painful for compositors, they need to have a special tearing codepath for no
real reason and the tearing API doesn't work the same as the non-tearing API
as Pekka said.
- Check any non-FB_ID props in the atomic commit, forbid changes here: we need
to use get_property(), which is a bit weird back-and-forth between u64s used
in the uAPI and actual underlying values stored in DRM structs. And the
MODE_ID check is a bit split between the set_property() function and the
check_modeset() one. Ideally we'd have a something_changed bool like we have
for modesets (mode_changed) but that would be a massive pain to plumb through
all of the props. Also get_property() is lightweight, it just casts whatever
struct field is being used to u64 and that's it.
All in all, I think I'd settle on the approach in this patch, but I'd prefer to
leave the MODE_ID stuff out. It would result in a less convoluted check, and I
can't think of any current compositor which re-creates the mode blob each
page-flip. That's not 100% consistent with the sync page-flip API, but I'm
worried about accumulating more special cases like this in the future.
Does that make sense?
> drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
> crtc->base.id, crtc->name);
> new_crtc_state->mode_changed = true;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index dfd4cf7169df..536c21f53b5f 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -972,13 +972,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))
> @@ -995,6 +1010,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);
Note to self: I was worried here to pass the "next" state to get_property
Re: [PATCH v6 5/6] drm: Refuse to async flip with atomic prop changes
On Tue, Aug 15, 2023 at 03:57:09PM -0300, André Almeida wrote:
> 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 exceptions are the framebuffer ID to flip
> to and the mode ID, that could be referring to an identical mode.
FYI, the solid fill series adds an enum drm_plane_pixel_source and and a
new solid fill pixel source. Changing the solid fill color would be
effectively the same as changing the FB_ID. On the other hand, changing
the FB_ID no longer necessarily results in an update when the pixel
source is set to solid fill.
> Signed-off-by: André Almeida
> ---
> v5: no changes
> v4: new patch
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 5 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 52 +++--
> drivers/gpu/drm/drm_crtc_internal.h | 2 +-
> drivers/gpu/drm/drm_mode_object.c | 2 +-
> 4 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 292e38eb6218..b34e3104afd1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -629,6 +629,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
>
> if (!drm_mode_equal(&old_crtc_state->mode,
> &new_crtc_state->mode)) {
> + if (new_crtc_state->async_flip) {
> + drm_dbg_atomic(dev, "[CRTC:%d:%s] no mode
> changes allowed during async flip\n",
> +crtc->base.id, crtc->name);
> + return -EINVAL;
> + }
> drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
> crtc->base.id, crtc->name);
> new_crtc_state->mode_changed = true;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index a15121e75a0a..6c423a7e8c7b 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);
> @@ -1037,6 +1059,7 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> case DRM_MODE_OBJECT_CRTC: {
> struct drm_crtc *crtc = obj_to_crtc(obj);
> struct drm_crtc_state *crtc_state;
> + struct drm_mode_config *config = &crtc->dev->mode_config;
>
> crtc_state = drm_atomic_get_crtc_state(state, crtc);
> if (IS_ERR(crtc_state)) {
> @@ -1044,6 +1067,18 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> break;
> }
>
> + /*
> + * We allow mode_id changes here for async flips, because we
> + * check later on drm_atomic_helper_check_modeset() callers if
> + * there are modeset changes or they are equal
> + */
> + if (async_flip && prop != config->prop_mode_id) {
> + ret = drm_atomic_crtc_get_property(crtc, crtc_state,
> +
[PATCH v6 5/6] drm: Refuse to async flip with atomic prop changes
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 exceptions are the framebuffer ID to flip
to and the mode ID, that could be referring to an identical mode.
Signed-off-by: André Almeida
---
v5: no changes
v4: new patch
---
drivers/gpu/drm/drm_atomic_helper.c | 5 +++
drivers/gpu/drm/drm_atomic_uapi.c | 52 +++--
drivers/gpu/drm/drm_crtc_internal.h | 2 +-
drivers/gpu/drm/drm_mode_object.c | 2 +-
4 files changed, 56 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 292e38eb6218..b34e3104afd1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -629,6 +629,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
if (!drm_mode_equal(&old_crtc_state->mode,
&new_crtc_state->mode)) {
+ if (new_crtc_state->async_flip) {
+ drm_dbg_atomic(dev, "[CRTC:%d:%s] no mode
changes allowed during async flip\n",
+ crtc->base.id, crtc->name);
+ return -EINVAL;
+ }
drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
crtc->base.id, crtc->name);
new_crtc_state->mode_changed = true;
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index a15121e75a0a..6c423a7e8c7b 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);
@@ -1037,6 +1059,7 @@ int drm_atomic_set_property(struct drm_atomic_state
*state,
case DRM_MODE_OBJECT_CRTC: {
struct drm_crtc *crtc = obj_to_crtc(obj);
struct drm_crtc_state *crtc_state;
+ struct drm_mode_config *config = &crtc->dev->mode_config;
crtc_state = drm_atomic_get_crtc_state(state, crtc);
if (IS_ERR(crtc_state)) {
@@ -1044,6 +1067,18 @@ int drm_atomic_set_property(struct drm_atomic_state
*state,
break;
}
+ /*
+* We allow mode_id changes here for async flips, because we
+* check later on drm_atomic_helper_check_modeset() callers if
+* there are modeset changes or they are equal
+*/
+ if (async_flip && prop != config->prop_mode_id) {
+ 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 +1086,7 @@ int drm_atomic_set_property(struct drm_atomic_state
*state,
case DRM_MODE_OBJECT_PLANE: {
