On Tuesday, August 15th, 2023 at 20:57, André Almeida <andrealmeid@xxxxxxxxxx> 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 <andrealmeid@xxxxxxxxxx>
> ---
> 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 loooooong 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(),
but it's before the set_property() call on that state, so should be fine.
> + break;
> + }
> +
> ret = drm_atomic_connector_set_property(connector,
> connector_state, file_priv,
> prop, prop_value);
> @@ -1003,6 +1025,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)) {
> @@ -1010,6 +1033,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;
> @@ -1017,6 +1052,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)) {
> @@ -1024,6 +1060,13 @@ 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;
> + }
> +
> ret = drm_atomic_plane_set_property(plane,
> plane_state, file_priv,
> prop, prop_value);
> @@ -1312,6 +1355,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))
> @@ -1348,6 +1392,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. */
> @@ -1427,8 +1473,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 501a10edd0e1..381130cebe81 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 ba1608effc0f..64f519254895 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -536,7 +536,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.41.0
