[Intel-gfx] [PATCH 4/5] drm/i915: Move fb_bits updating later in atomic_commit

[Chris Wilson]

On Mon, Jun 13, 2016 at 04:13:48PM +0200, Daniel Vetter wrote:
> Currently it's part of prepare_fb, still in the first phase of
> atomic_commit which might fail. Which means that we need to have some
> heuristics in cleanup_fb to figure out whether things failed, or
> whether we just clean up the old fbs.
> 
> That's fragile, and worse, once we start pipelining commits gets
> confused: While the last commit is still getting cleanup up we already
> hammer in the new one, and fb_bits aren't refcounted, resulting in
> lost bits and WARN_ON galore. We could instead try to make cleanup_fb
> more clever, but a simpler fix is to postpone the fb_bits tracking
> past the point of no return, where we commit all the software state.
> 
> That also makes conceptually more sense, since fb_bits must be updated
> synchronously from the ioctl (they track usage from userspace pov, not
> from the hw pov), right before we're fully committed to the updated.
> 
> This fixes WARNING splats from track_fb with page_flip implemented
> through atomic_commit.
> 
> Testcase: igt/kms_flip/flip-vs-rmfb
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a16a307dbb76..fd46db4882e5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13863,6 +13863,25 @@ static void intel_atomic_commit_work(struct work_struct *work)
>  	intel_atomic_commit_tail(state);
>  }
>  
> +static void intel_atomic_track_fbs(struct drm_atomic_state *state)
> +{
> +	struct drm_plane_state *old_plane_state;
> +	struct drm_plane *plane;
> +	struct drm_i915_gem_object *obj, *old_obj;
> +	struct intel_plane *intel_plane;
> +	int i;
> +
> +	mutex_lock(&state->dev->struct_mutex);
> +	for_each_plane_in_state(state, plane, old_plane_state, i) {
> +		obj = intel_fb_obj(plane->state->fb);
> +		old_obj = intel_fb_obj(old_plane_state->fb);
> +		intel_plane = to_intel_plane(plane);
> +
> +		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
> +	}
> +	mutex_unlock(&state->dev->struct_mutex);
> +}

Seems like an opportune time to pull in

https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=d16c3d8f3dd395b8d13a3691efb356a23e0b702b
https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=4a5c85282ba3c15a64dc0f600969234f30ebe820

and fwiw i915_gem_track_fb would be better off inside intel_display.c
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre