[Intel-gfx] [PATCH 2/2] drm/i915: Hold reference to intel_frontbuffer as we track activity

[Joonas Lahtinen]

Quoting Chris Wilson (2019-12-16 18:17:17)
> Since obj->frontbuffer is no longer protected by the struct_mutex, as we
> are processing the execbuf, it may be removed. Mark the
> intel_frontbuffer as rcu protected, and so acquire a reference to
> the struct as we track activity upon it.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/827
> Fixes: 8e7cb1799b4f ("drm/i915: Extract intel_frontbuffer active tracking")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld at intel.com>
> Cc: <stable at vger.kernel.org> # v5.4+

<SNIP>

> @@ -54,6 +55,35 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
>  void intel_frontbuffer_flip(struct drm_i915_private *i915,
>                             unsigned frontbuffer_bits);
>  
> +void intel_frontbuffer_put(struct intel_frontbuffer *front);
> +
> +static inline struct intel_frontbuffer *
> +__intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
> +{
> +       struct intel_frontbuffer *front;
> +
> +       if (likely(!rcu_access_pointer(obj->frontbuffer)))
> +               return NULL;
> +
> +       rcu_read_lock();
> +       do {
> +               front = rcu_dereference(obj->frontbuffer);
> +               if (!front)
> +                       break;
> +
> +               if (!kref_get_unless_zero(&front->ref))
> +                       continue;
> +
> +               if (front == rcu_access_pointer(obj->frontbuffer))
> +                       break;
> +
> +               intel_frontbuffer_put(front);
> +       } while (1);
> +       rcu_read_unlock();
> +
> +       return front;
> +}

Understood that there can only be so many writers, so it's not
technically blocking, but the constructs looks like it was designed to
spin.

It would look more appropriate without the loop:

if (unlikely(!kref_get_unless_zero(&front->ref)))
	goto retry;

<SNIP>

> +void __i915_gem_object_flush_frontbuffer(struct drm_i915_gem_object *obj,
> +                                        enum fb_op_origin origin)
> +{
> +       struct intel_frontbuffer *front;
> +
> +       front = __intel_frontbuffer_get(obj);
> +       if (front) {
> +               intel_frontbuffer_flush(front, origin);
> +               intel_frontbuffer_put(front);
> +       }
> +}
> +
> +void __i915_gem_object_invalidate_frontbuffer(struct drm_i915_gem_object *obj,
> +                                             enum fb_op_origin origin)
> +{
> +       struct intel_frontbuffer *front;
> +
> +       front = __intel_frontbuffer_get(obj);
> +       if (front) {
> +               intel_frontbuffer_invalidate(front, origin);
> +               intel_frontbuffer_put(front);
> +       }
> +}

Could be de-duped as by taking the vfunc as parameter, but that's
just by coincidence that parameters match.

The rest checks out, so with the loop removed:

Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

Regards, Joonas