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@xxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
