On 10/04/2019 11:13, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-04-10 11:05:13)
On 08/04/2019 10:17, Chris Wilson wrote:
+void __intel_context_enter(struct intel_context *ce)
+{
+ struct drm_i915_private *i915 = ce->gem_context->i915;
+
+ if (!i915->gt.active_requests++)
+ i915_gem_unpark(i915);
+}
+
+void __intel_context_exit(struct intel_context *ce)
+{
+ struct drm_i915_private *i915 = ce->gem_context->i915;
+
+ GEM_BUG_ON(!i915->gt.active_requests);
+ if (!--i915->gt.active_requests)
+ i915_gem_park(i915);
+}
In our normal nomenclature __intel_context_enter would be expected to be
directly called by intel_context_enter on the 0 -> 1 refcnt transition.
Here they are in fact common helpers, so one level of indirection, via a
different layer. I found this a bit confusing at first. My usual remedy
here is to prescribe a better name. __intel_gt_context_enter/exit? To
designate the glue between backend and GT.
The use of gt/gem here is a placeholder. Here we will be iterating over
the engines (so just the one for the usual contexts, and num_siblings for
virtual).
intel_context_enter_engine() ?
And virtual_context_enter remains distinct.
I did not find it at first since from the commit message I expected you
would be assigning backend specific enter/exit callbacks to ce->ops. I
see now that in "drm/i915: Invert the GEM wakeref hierarchy" you in fact
just change the bodies of __intel_context_enter/exit. The only vfunc
override comes with the virtual engine as you wrote.
intel_context_enter_engine sounds okay consider all this.
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
index ebc861b1a49e..95b1fdc5826a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -73,6 +73,26 @@ static inline void __intel_context_pin(struct intel_context *ce)
void intel_context_unpin(struct intel_context *ce);
+void __intel_context_enter(struct intel_context *ce);
+void __intel_context_exit(struct intel_context *ce);
+
+static inline void intel_context_enter(struct intel_context *ce)
+{
+ if (!ce->active_count++)
+ ce->ops->enter(ce);
+}
+
+static inline void intel_context_mark_active(struct intel_context *ce)
+{
GEM_BUG_ON(ce->active_count == 0) ?
We're no longer in Kansas. I was wondering if I could start GT_BUG_ON
and include "intel_gt.h" with a goal of doing a search and replace within
gt/. That gives us some separation from GEM, and maybe we could even
start filtering based on need.
I fear it would be too much for time being.
And some lockdep_assert_held in all three?
Read on :(
The plan is for intel_context_enter/_exit to be under the
timeline->mutex, but that isn't realised for about another 30 patches.
mark_active has special protection because it gets used from the
serialised portion of intel_engine_park.
Ok, but bug on for zero active_count makes sense right? Maybe even
intel_context_pin_active to signify that it can only act on an already
active context?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
