Message-ID: <1vjWvRxgo-TI_cWpN8HouVtqP0eftQcTA58yrbbelGK02RChNF3owgcgZJVjjREIBnz3RXwtPuRHh2pGI4qMkSmG4UCiY_4T6D0V1TjBjE0=@emersion.fr>
Date:   Sun, 15 Oct 2023 15:37:25 +0000
From:   Simon Ser <contact@...rsion.fr>
To:     André Almeida <andrealmeid@...lia.com>
Cc:     dri-devel@...ts.freedesktop.org, amd-gfx@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, wayland-devel@...ts.freedesktop.org,
        kernel-dev@...lia.com, alexander.deucher@....com,
        christian.koenig@....com, pierre-eric.pelloux-prayer@....com,
        Rob Clark <robdclark@...il.com>,
        Pekka Paalanen <ppaalanen@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Daniel Stone <daniel@...ishbar.org>,
        'Marek Olšák' <maraeo@...il.com>,
        Dave Airlie <airlied@...il.com>,
        Michel Dänzer <michel.daenzer@...lbox.org>,
        Randy Dunlap <rdunlap@...radead.org>, hwentlan@....com,
        joshua@...ggi.es, ville.syrjala@...ux.intel.com
Subject: Re: [PATCH v6 5/6] drm: Refuse to async flip with atomic prop changes

On Tuesday, August 15th, 2023 at 20:57, André Almeida <andrealmeid@...lia.com> 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@...lia.com>
> ---
> 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

Powered by blists - more mailing lists