[Intel-gfx] [PATCH 5/8] Drm/i915/rpm: Add intel_runtime_idle

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Sent: Thursday, August 4, 2022 2:22 AM
> To: Tangudu, Tilak <tilak.tangudu at intel.com>
> Cc: Ewins, Jon <jon.ewins at intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar at intel.com>; Roper, Matthew D
> <matthew.d.roper at intel.com>; Wilson, Chris P <chris.p.wilson at intel.com>;
> Nikula, Jani <jani.nikula at intel.com>; Gupta, saurabhg
> <saurabhg.gupta at intel.com>; Gupta, Anshuman
> <anshuman.gupta at intel.com>; Nilawar, Badal <badal.nilawar at intel.com>;
> Deak, Imre <imre.deak at intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 5/8] Drm/i915/rpm: Add intel_runtime_idle
> 
> On Thu, Jul 21, 2022 at 03:29:52PM +0530, tilak.tangudu at intel.com wrote:
> > From: Anshuman Gupta <anshuman.gupta at intel.com>
> >
> > Adding intel_runtime_idle (runtime_idle callback) to prepare the
> > tageted D3 state.
> >
> > Since we have introduced i915 runtime_idle callback.
> > It need to be warranted that Runtime PM Core invokes runtime_idle
> > callback when runtime usages count becomes zero. That requires to use
> > pm_runtime_put instead of pm_runtime_put_autosuspend.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Chris Wilson <chris.p.wilson at intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c      | 12 ++++++++++++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +--
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index deb8a8b76965..4c36554567fd 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1576,6 +1576,17 @@ static int i915_pm_restore(struct device *kdev)
> >  	return i915_pm_resume(kdev);
> >  }
> >
> > +static int intel_runtime_idle(struct device *kdev) {
> > +	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> > +	int ret = 1;
> > +
> > +	pm_runtime_mark_last_busy(kdev);
> > +	pm_runtime_autosuspend(kdev);
> 
> oh, I see the ret = 1 like the other drm drivers..
> do we really know why this flow and not use the runtime_idle like the rest of the
> kernel?
AFAIU it is the mixed type of usages (some drivers like USB use -EBUSY), 
the crux of it to use the pm_runtime_mark_last_busy() and pm_runtime_autosuspend correctly ,
PM Core autosuspend timer start ticking since it was last busy. 

int usb_runtime_idle(struct device *dev)
{
        struct usb_device       *udev = to_usb_device(dev);

        /* An idle USB device can be suspended if it passes the various
         * autosuspend checks.
         */
        if (autosuspend_check(udev) == 0)
                pm_runtime_autosuspend(dev);
        /* Tell the core not to suspend it, though. */
        return -EBUSY;
}

Intel snd driver uses -EBUSY and 0 as a return value.
static int azx_runtime_idle(struct device *dev)
{
        struct snd_card *card = dev_get_drvdata(dev);
        struct azx *chip;
        struct hda_intel *hda;

        if (!card)
                return 0;

        chip = card->private_data;
        hda = container_of(chip, struct hda_intel, chip);
        if (chip->disabled || hda->init_failed)
                return 0;

        if (!power_save_controller || !azx_has_pm_runtime(chip) ||
            azx_bus(chip)->codec_powered || !chip->running)
                return -EBUSY;

        /* ELD notification gets broken when HD-audio bus is off */
        if (needs_eld_notify_link(chip))
                return -EBUSY;

        return 0;
}

When rutime_idle callback returns 0, then only PM core will call rpm_suspend with RPM_AUTO flag.
return retval ? retval : rpm_suspend(dev, rpmflags | RPM_AUTO);

> 
> I believe we could use more the runtime_idle to block the runtime_pm, but
> following the original documented design of it.
Do you mean it is not correct to use runtime_idle callback for d3cold policy ?
Yes it is being used to block the runtime pm, if device is busy for other kernel drivers.

Thanks,
Anshuman Gupta.
> 
> > +
> > +	return ret;
> > +}
> > +
> >  static int intel_runtime_suspend(struct device *kdev)  {
> >  	struct drm_i915_private *dev_priv = kdev_to_i915(kdev); @@ -1752,6
> > +1763,7 @@ const struct dev_pm_ops i915_pm_ops = {
> >  	.restore = i915_pm_restore,
> >
> >  	/* S0ix (via runtime suspend) event handlers */
> > +	.runtime_idle = intel_runtime_idle,
> >  	.runtime_suspend = intel_runtime_suspend,
> >  	.runtime_resume = intel_runtime_resume,  }; diff --git
> > a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 704beeeb560b..1c3ed0c29330 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -513,8 +513,7 @@ static void __intel_runtime_pm_put(struct
> > intel_runtime_pm *rpm,
> >
> >  	intel_runtime_pm_release(rpm, wakelock);
> >
> > -	pm_runtime_mark_last_busy(kdev);
> > -	pm_runtime_put_autosuspend(kdev);
> > +	pm_runtime_put(kdev);
> >  }
> >
> >  /**
> > --
> > 2.25.1
> >