Re: [PATCH] drm/i915: Move intel_engine_context_in/out into intel_lrc.c

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

On 25/10/2019 10:13, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-10-25 10:09:52)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Intel_lrc.c is the only caller and so to avoid some header file ordering
>> issues in future patches move these two over there.
> 
> How much pain would you feel if we did intel_lrc.c +
> intel_execlists_submission.c earlier rather than later?

Par for course as you like to say. :)

>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine.h | 55 --------------------------
>>   drivers/gpu/drm/i915/gt/intel_lrc.c    | 55 ++++++++++++++++++++++++++
>>   2 files changed, 55 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
>> index 97bbdd9773c9..c6895938b626 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
>> @@ -290,61 +290,6 @@ void intel_engine_dump(struct intel_engine_cs *engine,
>>                         struct drm_printer *m,
>>                         const char *header, ...);
>>   
>> -static inline void intel_engine_context_in(struct intel_engine_cs *engine)
>> -{
>> -       unsigned long flags;
>> -
>> -       if (READ_ONCE(engine->stats.enabled) == 0)
>> -               return;
>> -
>> -       write_seqlock_irqsave(&engine->stats.lock, flags);
>> -
>> -       if (engine->stats.enabled > 0) {
>> -               if (engine->stats.active++ == 0)
>> -                       engine->stats.start = ktime_get();
>> -               GEM_BUG_ON(engine->stats.active == 0);
>> -       }
>> -
>> -       write_sequnlock_irqrestore(&engine->stats.lock, flags);
>> -}
>> -
>> -static inline void intel_engine_context_out(struct intel_engine_cs *engine)
>> -{
>> -       unsigned long flags;
>> -
>> -       if (READ_ONCE(engine->stats.enabled) == 0)
>> -               return;
>> -
>> -       write_seqlock_irqsave(&engine->stats.lock, flags);
>> -
>> -       if (engine->stats.enabled > 0) {
>> -               ktime_t last;
>> -
>> -               if (engine->stats.active && --engine->stats.active == 0) {
>> -                       /*
>> -                        * Decrement the active context count and in case GPU
>> -                        * is now idle add up to the running total.
>> -                        */
>> -                       last = ktime_sub(ktime_get(), engine->stats.start);
>> -
>> -                       engine->stats.total = ktime_add(engine->stats.total,
>> -                                                       last);
>> -               } else if (engine->stats.active == 0) {
>> -                       /*
>> -                        * After turning on engine stats, context out might be
>> -                        * the first event in which case we account from the
>> -                        * time stats gathering was turned on.
>> -                        */
>> -                       last = ktime_sub(ktime_get(), engine->stats.enabled_at);
>> -
>> -                       engine->stats.total = ktime_add(engine->stats.total,
>> -                                                       last);
>> -               }
>> -       }
>> -
>> -       write_sequnlock_irqrestore(&engine->stats.lock, flags);
>> -}
>> -
>>   int intel_enable_engine_stats(struct intel_engine_cs *engine);
>>   void intel_disable_engine_stats(struct intel_engine_cs *engine);
>>   
>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> index 73eae85a2cc9..523de1fd4452 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> @@ -944,6 +944,61 @@ execlists_context_status_change(struct i915_request *rq, unsigned long status)
>>                                     status, rq);
>>   }
>>   
>> +static void intel_engine_context_in(struct intel_engine_cs *engine)
> 
> stats_in() / stats_out() ?
> 
> Now that's it entirely local and we may end up doing other per-context
> in/out ops?

Yeah, could make sense. I did rename it to intel_context_in/out in a 
local patch which adds per ce stats. Lets see when I un-bit-rot that 
work how it will look.

> Purely mechanical, so
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

In the meantime I have pushed this so at least header file is smaller. 
Thanks.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx