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

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@chris-wilson.co.uk Cc: Matthew Auld matthew.auld@intel.com Cc: stable@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@linux.intel.com

Regards, Joonas