[Intel-gfx] [PATCH v3 3/5] drm/i915: Add checks specific to async flips

[Ville Syrjälä]

On Thu, May 28, 2020 at 11:09:29AM +0530, Karthik B S wrote:
> Support added only for async flips on primary plane.
> If flip is requested on any other plane, reject it.
> 
> Make sure there is no change in fbc, offset and framebuffer modifiers
> when async flip is requested.
> 
> If any of these are modified, reject async flip.
> 
> v2: -Replace DRM_ERROR (Paulo)
>     -Add check for changes in OFFSET, FBC, RC(Paulo)
> 
> v3: -Removed TODO as benchmarking tests have been run now.
> 
> Signed-off-by: Karthik B S <karthik.b.s at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 55 ++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index eb1c360431ae..2307f924732c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14798,6 +14798,53 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
>  	return false;
>  }
>  
> +static int intel_atomic_check_async(struct intel_atomic_state *state)
> +{
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct intel_plane_state *new_plane_state, *old_plane_state;
> +	struct intel_crtc *crtc;
> +	struct intel_plane *intel_plane;
> +	int i, j;
> +
> +	/*FIXME: Async flip is only supported for primary plane currently
> +	 * Support for overlays to be added.
> +	 */
> +	for_each_new_plane_in_state(&state->base, plane, plane_state, j) {
> +		if (plane->type != DRM_PLANE_TYPE_PRIMARY) {

I think skl+ can do async flips on any universal planes. Earlier
platforms were limited to primary only I think. Can't remember if g4x
already had usable async flip via mmio. Pretty sure at least ilk+ had
it.

Also intel_ types are preferred, so this should use those, and I
think since we're talking about hw planes we should rather check for
PLANE_PRIMARY here.

> +			DRM_DEBUG_KMS("Async flips is NOT supported for non-primary plane\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (old_crtc_state->enable_fbc != new_crtc_state->enable_fbc) {

enable_fbc is bork, so this probably doesn't do anything particularly
sensible.

> +			DRM_DEBUG_KMS("FBC status cannot be changed in async flip\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	for_each_oldnew_intel_plane_in_state(state, intel_plane, old_plane_state,
> +					     new_plane_state, i) {
> +		if ((old_plane_state->color_plane[0].x !=
> +		     new_plane_state->color_plane[0].x) ||
> +		    (old_plane_state->color_plane[0].y !=
> +		     new_plane_state->color_plane[0].y)) {

Don't think we've even calculated those by the time you call this. So
this stuff has to be called much later I think.

> +			DRM_DEBUG_KMS("Offsets cannot be changed in async\n");
> +			return -EINVAL;
> +		}
> +
> +		if (old_plane_state->uapi.fb->modifier !=
> +		    new_plane_state->uapi.fb->modifier) {

We seem to be missing a lot of state here. Basically I think async flip
can *only* change the plane surface address, so anything else changing
we should reject. I guess if this comes in via the legacy page flip path
the code/helpers do prevent most other things changing, but not sure.
I don't really like relying on such core checks since someone could
blindly expose this via the atomic ioctl without having those same
restrictions in place.

We might also want a dedicated plane hook for async flips since writing
all the plane registers for these is rather pointless. I'm not even sure
what happens with all the other double buffered registers if you write
them and then do an async surface address update.

Also if we want more accurate timestmaps based on the flip timestamp
register then we're going to have to limit async flips to single plane
per pipe at a time becasue the timestamp can only be sampled from
a single plane.

> +			DRM_DEBUG_KMS("Framebuffer modifiers cannot be changed in async flip\n");
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}
> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -14825,6 +14872,14 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		goto fail;
>  
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		if (new_crtc_state->uapi.async_flip) {
> +			ret = intel_atomic_check_async(state);

Kinda redundant to call this multiple times. I think the async_flip flag
is actually misplaced. It should probably be in the drm_atomic_state
instead of the crtc state.

Also still not a huge fan of using the "async flip" termonology in
the drm core. IMO we should just adopt the vulkan terminology for
this stuff so it's obviuos what people mean when they talk about these
things.

> +			if  (ret)
> +				goto fail;
> +		}
> +	}
> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (!needs_modeset(new_crtc_state)) {
> -- 
> 2.17.1

-- 
Ville Syrjälä
Intel