drm/i915: Move global activity tracking from GEM to GT
diff mbox series

Message ID 20190718124220.26623-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915: Move global activity tracking from GEM to GT
Related show

Commit Message

Chris Wilson July 18, 2019, 12:42 p.m. UTC
As our global unpark/park keep track of the number of active users, we
can simply move the accounting from the GEM layer to the base GT layer.
It was placed originally inside GEM to benefit from the 100ms extra
delay on idleness, but that has been eliminated and now there is no
substantive difference between the layers. In moving it, we move another
piece of the puzzle out from underneath struct_mutex.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_pm.c | 4 ----
 drivers/gpu/drm/i915/gt/intel_gt_pm.c  | 5 +++++
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Tvrtko Ursulin July 22, 2019, 12:16 p.m. UTC | #1
On 18/07/2019 13:42, Chris Wilson wrote:
> As our global unpark/park keep track of the number of active users, we
> can simply move the accounting from the GEM layer to the base GT layer.
> It was placed originally inside GEM to benefit from the 100ms extra
> delay on idleness, but that has been eliminated and now there is no
> substantive difference between the layers. In moving it, we move another
> piece of the puzzle out from underneath struct_mutex.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_pm.c | 4 ----
>   drivers/gpu/drm/i915/gt/intel_gt_pm.c  | 5 +++++
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> index 8faf262278ae..1afb04660725 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> @@ -9,7 +9,6 @@
>   #include "gt/intel_gt_pm.h"
>   
>   #include "i915_drv.h"
> -#include "i915_globals.h"
>   
>   static void call_idle_barriers(struct intel_engine_cs *engine)
>   {
> @@ -41,8 +40,6 @@ static void i915_gem_park(struct drm_i915_private *i915)
>   
>   	intel_timelines_park(i915);
>   	i915_vma_parked(i915);
> -
> -	i915_globals_park();

Benefit of this placement was also that what will be freed on parking 
was freed by this point so shrinking of slabs is effective. If you move 
it to gt parking then that runs to early. So I think it is better that 
it stays where it is.

Regards,

Tvrtko

>   }
>   
>   static void idle_work_handler(struct work_struct *work)
> @@ -93,7 +90,6 @@ static int pm_notifier(struct notifier_block *nb,
>   
>   	switch (action) {
>   	case INTEL_GT_UNPARK:
> -		i915_globals_unpark();
>   		queue_delayed_work(i915->wq,
>   				   &i915->gem.retire_work,
>   				   round_jiffies_up_relative(HZ));
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> index 61ed912341f1..ce7733ee76cf 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> @@ -5,6 +5,7 @@
>    */
>   
>   #include "i915_drv.h"
> +#include "i915_globals.h"
>   #include "i915_params.h"
>   #include "intel_engine_pm.h"
>   #include "intel_gt.h"
> @@ -24,6 +25,8 @@ static int intel_gt_unpark(struct intel_wakeref *wf)
>   
>   	GEM_TRACE("\n");
>   
> +	i915_globals_unpark();
> +
>   	/*
>   	 * It seems that the DMC likes to transition between the DC states a lot
>   	 * when there are no connected displays (no active power domains) during
> @@ -77,6 +80,8 @@ static int intel_gt_park(struct intel_wakeref *wf)
>   	GEM_BUG_ON(!wakeref);
>   	intel_display_power_put(i915, POWER_DOMAIN_GT_IRQ, wakeref);
>   
> +	i915_globals_park();
> +
>   	return 0;
>   }
>   
>
Chris Wilson July 22, 2019, 9:46 p.m. UTC | #2
Quoting Tvrtko Ursulin (2019-07-22 13:16:38)
> 
> On 18/07/2019 13:42, Chris Wilson wrote:
> > As our global unpark/park keep track of the number of active users, we
> > can simply move the accounting from the GEM layer to the base GT layer.
> > It was placed originally inside GEM to benefit from the 100ms extra
> > delay on idleness, but that has been eliminated and now there is no
> > substantive difference between the layers. In moving it, we move another
> > piece of the puzzle out from underneath struct_mutex.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_pm.c | 4 ----
> >   drivers/gpu/drm/i915/gt/intel_gt_pm.c  | 5 +++++
> >   2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> > index 8faf262278ae..1afb04660725 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> > @@ -9,7 +9,6 @@
> >   #include "gt/intel_gt_pm.h"
> >   
> >   #include "i915_drv.h"
> > -#include "i915_globals.h"
> >   
> >   static void call_idle_barriers(struct intel_engine_cs *engine)
> >   {
> > @@ -41,8 +40,6 @@ static void i915_gem_park(struct drm_i915_private *i915)
> >   
> >       intel_timelines_park(i915);
> >       i915_vma_parked(i915);
> > -
> > -     i915_globals_park();
> 
> Benefit of this placement was also that what will be freed on parking 
> was freed by this point so shrinking of slabs is effective. If you move 
> it to gt parking then that runs to early. So I think it is better that 
> it stays where it is.

But there's nothing here. You are imaging things. call_idle_barriers and
batch_pool are in engine_park, timelines_park is gone, and vma_parked is
now a timer.
-Chris
Tvrtko Ursulin July 23, 2019, 1:34 p.m. UTC | #3
On 22/07/2019 22:46, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-22 13:16:38)
>>
>> On 18/07/2019 13:42, Chris Wilson wrote:
>>> As our global unpark/park keep track of the number of active users, we
>>> can simply move the accounting from the GEM layer to the base GT layer.
>>> It was placed originally inside GEM to benefit from the 100ms extra
>>> delay on idleness, but that has been eliminated and now there is no
>>> substantive difference between the layers. In moving it, we move another
>>> piece of the puzzle out from underneath struct_mutex.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gem/i915_gem_pm.c | 4 ----
>>>    drivers/gpu/drm/i915/gt/intel_gt_pm.c  | 5 +++++
>>>    2 files changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
>>> index 8faf262278ae..1afb04660725 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
>>> @@ -9,7 +9,6 @@
>>>    #include "gt/intel_gt_pm.h"
>>>    
>>>    #include "i915_drv.h"
>>> -#include "i915_globals.h"
>>>    
>>>    static void call_idle_barriers(struct intel_engine_cs *engine)
>>>    {
>>> @@ -41,8 +40,6 @@ static void i915_gem_park(struct drm_i915_private *i915)
>>>    
>>>        intel_timelines_park(i915);
>>>        i915_vma_parked(i915);
>>> -
>>> -     i915_globals_park();
>>
>> Benefit of this placement was also that what will be freed on parking
>> was freed by this point so shrinking of slabs is effective. If you move
>> it to gt parking then that runs to early. So I think it is better that
>> it stays where it is.
> 
> But there's nothing here. You are imaging things. call_idle_barriers and
> batch_pool are in engine_park, timelines_park is gone, and vma_parked is
> now a timer.

I was thinking about slab shrinkers. They do seem to be called from 
i915_globals_park or I am still imagining?

Regards,

Tvrtko
Chris Wilson July 23, 2019, 1:41 p.m. UTC | #4
Quoting Tvrtko Ursulin (2019-07-23 14:34:27)
> 
> On 22/07/2019 22:46, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-22 13:16:38)
> >>
> >> On 18/07/2019 13:42, Chris Wilson wrote:
> >>> As our global unpark/park keep track of the number of active users, we
> >>> can simply move the accounting from the GEM layer to the base GT layer.
> >>> It was placed originally inside GEM to benefit from the 100ms extra
> >>> delay on idleness, but that has been eliminated and now there is no
> >>> substantive difference between the layers. In moving it, we move another
> >>> piece of the puzzle out from underneath struct_mutex.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/gem/i915_gem_pm.c | 4 ----
> >>>    drivers/gpu/drm/i915/gt/intel_gt_pm.c  | 5 +++++
> >>>    2 files changed, 5 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> >>> index 8faf262278ae..1afb04660725 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> >>> @@ -9,7 +9,6 @@
> >>>    #include "gt/intel_gt_pm.h"
> >>>    
> >>>    #include "i915_drv.h"
> >>> -#include "i915_globals.h"
> >>>    
> >>>    static void call_idle_barriers(struct intel_engine_cs *engine)
> >>>    {
> >>> @@ -41,8 +40,6 @@ static void i915_gem_park(struct drm_i915_private *i915)
> >>>    
> >>>        intel_timelines_park(i915);
> >>>        i915_vma_parked(i915);
> >>> -
> >>> -     i915_globals_park();
> >>
> >> Benefit of this placement was also that what will be freed on parking
> >> was freed by this point so shrinking of slabs is effective. If you move
> >> it to gt parking then that runs to early. So I think it is better that
> >> it stays where it is.
> > 
> > But there's nothing here. You are imaging things. call_idle_barriers and
> > batch_pool are in engine_park, timelines_park is gone, and vma_parked is
> > now a timer.
> 
> I was thinking about slab shrinkers. They do seem to be called from 
> i915_globals_park or I am still imagining?

No, they are. I'm trying to tie together the future strands where
i915_gem_park() is empty, and all the work we have is in engine_park /
gt_park, with the outlier being the user vma cache which I plan to put
inside a timer. From that pov, I think the vma cache itself will take a
globals reference.
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
index 8faf262278ae..1afb04660725 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
@@ -9,7 +9,6 @@ 
 #include "gt/intel_gt_pm.h"
 
 #include "i915_drv.h"
-#include "i915_globals.h"
 
 static void call_idle_barriers(struct intel_engine_cs *engine)
 {
@@ -41,8 +40,6 @@  static void i915_gem_park(struct drm_i915_private *i915)
 
 	intel_timelines_park(i915);
 	i915_vma_parked(i915);
-
-	i915_globals_park();
 }
 
 static void idle_work_handler(struct work_struct *work)
@@ -93,7 +90,6 @@  static int pm_notifier(struct notifier_block *nb,
 
 	switch (action) {
 	case INTEL_GT_UNPARK:
-		i915_globals_unpark();
 		queue_delayed_work(i915->wq,
 				   &i915->gem.retire_work,
 				   round_jiffies_up_relative(HZ));
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index 61ed912341f1..ce7733ee76cf 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -5,6 +5,7 @@ 
  */
 
 #include "i915_drv.h"
+#include "i915_globals.h"
 #include "i915_params.h"
 #include "intel_engine_pm.h"
 #include "intel_gt.h"
@@ -24,6 +25,8 @@  static int intel_gt_unpark(struct intel_wakeref *wf)
 
 	GEM_TRACE("\n");
 
+	i915_globals_unpark();
+
 	/*
 	 * It seems that the DMC likes to transition between the DC states a lot
 	 * when there are no connected displays (no active power domains) during
@@ -77,6 +80,8 @@  static int intel_gt_park(struct intel_wakeref *wf)
 	GEM_BUG_ON(!wakeref);
 	intel_display_power_put(i915, POWER_DOMAIN_GT_IRQ, wakeref);
 
+	i915_globals_park();
+
 	return 0;
 }