Re: [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
> Sent: Thursday, June 23, 2022 2:11 AM
> To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>; Gupta, Anshuman
> <anshuman.gupta@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ewins, Jon
> <jon.ewins@xxxxxxxxx>; Belgaumkar, Vinay <vinay.belgaumkar@xxxxxxxxx>;
> Wilson, Chris P <chris.p.wilson@xxxxxxxxx>; Dixit, Ashutosh
> <ashutosh.dixit@xxxxxxxxx>; Nilawar, Badal <badal.nilawar@xxxxxxxxx>;
> Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; Gupta, saurabhg
> <saurabhg.gupta@xxxxxxxxx>; Iddamsetty, Aravind
> <aravind.iddamsetty@xxxxxxxxx>; Sundaresan, Sujaritha
> <sujaritha.sundaresan@xxxxxxxxx>; Deak, Imre <imre.deak@xxxxxxxxx>
> Subject: Re:  [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed
> helper
> 
> On Wed, Jun 22, 2022 at 03:55:03PM +0300, Jani Nikula wrote:
> > On Tue, 21 Jun 2022, "Tangudu, Tilak" <tilak.tangudu@xxxxxxxxx> wrote:
> > >> -----Original Message-----
> > >> From: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>
> > >> Sent: Tuesday, June 21, 2022 7:47 PM
> > >> To: Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>;
> > >> intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ewins, Jon <jon.ewins@xxxxxxxxx>;
> > >> Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Belgaumkar, Vinay
> > >> <vinay.belgaumkar@xxxxxxxxx>; Wilson, Chris P
> > >> <chris.p.wilson@xxxxxxxxx>; Dixit, Ashutosh
> > >> <ashutosh.dixit@xxxxxxxxx>; Nilawar, Badal
> > >> <badal.nilawar@xxxxxxxxx>; Roper, Matthew D
> > >> <matthew.d.roper@xxxxxxxxx>; Gupta, saurabhg
> > >> <saurabhg.gupta@xxxxxxxxx>; Iddamsetty, Aravind
> > >> <aravind.iddamsetty@xxxxxxxxx>; Sundaresan, Sujaritha
> > >> <sujaritha.sundaresan@xxxxxxxxx>
> > >> Subject: RE: [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed
> > >> helper
> > >>
> > >>
> > >>
> > >> > -----Original Message-----
> > >> > From: Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>
> > >> > Sent: Tuesday, June 21, 2022 6:05 PM
> > >> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ewins, Jon
> > >> > <jon.ewins@xxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>;
> > >> > Belgaumkar, Vinay <vinay.belgaumkar@xxxxxxxxx>; Wilson, Chris P
> > >> > <chris.p.wilson@xxxxxxxxx>; Dixit, Ashutosh
> > >> > <ashutosh.dixit@xxxxxxxxx>; Nilawar, Badal
> > >> > <badal.nilawar@xxxxxxxxx>; Gupta, Anshuman
> > >> > <anshuman.gupta@xxxxxxxxx>; Tangudu, Tilak
> > >> > <tilak.tangudu@xxxxxxxxx>; Roper, Matthew D
> > >> > <matthew.d.roper@xxxxxxxxx>; Gupta, saurabhg
> > >> > <saurabhg.gupta@xxxxxxxxx>; Iddamsetty, Aravind
> > >> > <aravind.iddamsetty@xxxxxxxxx>; Sundaresan, Sujaritha
> > >> > <sujaritha.sundaresan@xxxxxxxxx>
> > >> > Subject: [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed
> > >> > helper
> > >> >
> > >> > Added is_intel_rpm_allowed function to query the runtime_pm
> > >> > status and disllow during suspending and resuming.
> > >> This seems a hack,
> > >> Not sure if we have better way to handle it.
> > >> May be check this in intel_pm_runtime_{get,put} to keep entire code
> simple ?
> > > Yes, that would be simple without code refactoring.
> > > Checked the same with Chris, he suggested unbalancing of wakeref
> > > might popup If used at intel_pm_runtime_{get,put}  . So used like
> > > this,  @Wilson, Chris P , Please comment .
> > > @Vivi, Rodrigo , Any suggestion ?
> >
> > One option would be to track this in intel_wakeref_t, i.e. _get flags
> > the case in the returned wakeref and _put skips in that case.

@Jani Nikula 

I did not understand the suggestion, Can you please elaborate ?
Did you mean below or something more ? please help clarify.

8< ------------------------------
linux-desk:~/Code/drm-tip$ git diff
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 3759a8596084..ce272c569a89 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -369,12 +369,16 @@ static intel_wakeref_t __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
                                                     runtime_pm);
        int ret;

+       if (!is_intel_rpm_allowed(rpm))
+               goto out:
+
        ret = pm_runtime_get_sync(rpm->kdev);
        drm_WARN_ONCE(&i915->drm, ret < 0,
                      "pm_runtime_get_sync() failed: %d\n", ret);

        intel_runtime_pm_acquire(rpm, wakelock);

+out:
        return track_intel_runtime_pm_wakeref(rpm);
 }

@@ -505,6 +509,9 @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,

        untrack_intel_runtime_pm_wakeref(rpm, wref);

+       if (!is_intel_rpm_allowed(rpm))
+               return;
+
        intel_runtime_pm_release(rpm, wakelock);

        pm_runtime_mark_last_busy(kdev);
----------------------------------------------------------  >8

Thanks
Tilak
> 
> yeap, this seems to be the quick path at this moment...
> 
> Imre, do you see any other quick option?
> 
> In general I don't like much the big wakeref infra we end up creating here
> because all of the historical unbalanced cases we got.
> We should be able to get something cleaner and use the rpm infra as other
> drivers are using, or improve in the rpm side itself whatever we feel that we
> are missing to deal with these cases.
> 
> But back to this specific case/usage here we might need to duplicate some
> functions to be called just from the inside the resuming/suspending path...
> and/or moving the gets & puts upper on the stack...
> 
> The quick hacks will solve our short term problems and continue bloating our
> rpm infra.
> 
> >
> > BR,
> > Jani.
> >
> >
> > >
> > >> >
> > >> > Signed-off-by: Tilak Tangudu <tilak.tangudu@xxxxxxxxx>
> > >> > ---
> > >> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++++++++++
> > >> > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> > >> >  2 files changed, 16 insertions(+)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >> > index 6ed5786bcd29..3759a8596084 100644
> > >> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >> > @@ -320,6 +320,21 @@ untrack_all_intel_runtime_pm_wakerefs(struct
> > >> > intel_runtime_pm *rpm)  }
> > >> >
> > >> >  #endif
> > >> > +static int intel_runtime_pm_status(struct intel_runtime_pm *rpm)
> > >> > +{ return rpm->kdev->power.runtime_status; }
> > >> This is racy in principal, we need a kdev->power lock here.
> > >> Regards,
> > >> Anshuman Gupta.
> > >> > +
> > >> > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm) { int
> > >> > +rpm_status;
> > >> > +
> > >> > +rpm_status = intel_runtime_pm_status(rpm); if (rpm_status ==
> > >> > +RPM_RESUMING || rpm_status ==
> > >> > RPM_SUSPENDING)
> > >> > +return false;
> > >> > +else
> > >> > +return true;
> > >> > +}
> > >> >
> > >> >  static void
> > >> >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool
> > >> > wakelock) diff -- git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > >> > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > >> > index d9160e3ff4af..99418c3a934a 100644
> > >> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > >> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > >> > @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> > >> > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > >> > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > >> > intel_runtime_pm *rpm);  void
> > >> > intel_runtime_pm_driver_release(struct
> > >> > intel_runtime_pm *rpm);
> > >> > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> > >> >
> > >> >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm
> > >> > *rpm); intel_wakeref_t intel_runtime_pm_get_if_in_use(struct
> > >> > intel_runtime_pm *rpm);
> > >> > --
> > >> > 2.25.1
> > >>
> > >
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux