Re: [PATCH 10/29] drm/i915: Introduce context->enter() and context->exit()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx