> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
> Sent: Thursday, March 10, 2022 21:04
> To: Usyskin, Alexander <alexander.usyskin@xxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jani Nikula
> <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen
> <joonas.lahtinen@xxxxxxxxxxxxxxx>; David Airlie <airlied@xxxxxxxx>; Daniel
> Vetter <daniel@xxxxxxxx>; Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx; Winkler, Tomas <tomas.winkler@xxxxxxxxx>;
> Lubart, Vitaly <vitaly.lubart@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v10 4/5] mei: gsc: add runtime pm handlers
>
> On Tue, Mar 08, 2022 at 06:36:53PM +0200, Alexander Usyskin wrote:
> > From: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> >
> > Implement runtime handlers for mei-gsc, to track
> > idle state of the device properly.
> >
> > CC: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@xxxxxxxxx>
> > ---
> > drivers/misc/mei/gsc-me.c | 67
> ++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 66 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
> > index cf427f6fdec9..dac482ddab51 100644
> > --- a/drivers/misc/mei/gsc-me.c
> > +++ b/drivers/misc/mei/gsc-me.c
> > @@ -152,7 +152,72 @@ static int __maybe_unused
> mei_gsc_pm_resume(struct device *device)
> > return 0;
> > }
> >
> > -static SIMPLE_DEV_PM_OPS(mei_gsc_pm_ops, mei_gsc_pm_suspend,
> mei_gsc_pm_resume);
> > +static int __maybe_unused mei_gsc_pm_runtime_idle(struct device
> *device)
> > +{
> > + struct mei_device *dev = dev_get_drvdata(device);
> > +
> > + if (!dev)
> > + return -ENODEV;
> > + if (mei_write_is_idle(dev))
> > + pm_runtime_autosuspend(device);
>
> This is not needed. The _idle() callback is called right before the
> autosuspend.
> so you just need to return -EBUSY if not idle.
>
It is taken from blueprint in pci-me.c
IIRC here we ask the autosuspend to kick in after DELAY,
not simply rejecting it unconditionally.
> But also I'm missing the call to enable the autosuspend and set the delay.
>
These calls are in the second patch in the series, at the end of probe.
> Is this flow really working and you are getting device suspended when not in
> use?
> (Maybe it is just my ignorance on other flow types here)
>
GSC low-power is guided by DG card, here we only signaling to parent (i915, I think)
that GSC is idle or that we need resume to perform the operations.
> > +
> > + return -EBUSY;
> > +}
> > +
> > +static int __maybe_unused mei_gsc_pm_runtime_suspend(struct device
> *device)
> > +{
> > + struct mei_device *dev = dev_get_drvdata(device);
> > + struct mei_me_hw *hw;
> > + int ret;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + mutex_lock(&dev->device_lock);
> > +
> > + if (mei_write_is_idle(dev)) {
> > + hw = to_me_hw(dev);
> > + hw->pg_state = MEI_PG_ON;
> > + ret = 0;
> > + } else {
> > + ret = -EAGAIN;
> > + }
>
> probably not needed this here... but it would be good if you use
> the runtime_pm{get,put} to protect your write operations as well...
>
We reuse big portions of mei and mei-me drivers and there
all needed runtime_pm calls are implemented.
The runtime pm callbacks are different as GSC do not have
actual HW registers to handle the low power states as CSME has.
> > +
> > + mutex_unlock(&dev->device_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int __maybe_unused mei_gsc_pm_runtime_resume(struct device
> *device)
> > +{
> > + struct mei_device *dev = dev_get_drvdata(device);
> > + struct mei_me_hw *hw;
> > + irqreturn_t irq_ret;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + mutex_lock(&dev->device_lock);
> > +
> > + hw = to_me_hw(dev);
> > + hw->pg_state = MEI_PG_OFF;
> > +
> > + mutex_unlock(&dev->device_lock);
> > +
> > + irq_ret = mei_me_irq_thread_handler(1, dev);
> > + if (irq_ret != IRQ_HANDLED)
> > + dev_err(dev->dev, "thread handler fail %d\n", irq_ret);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mei_gsc_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mei_gsc_pm_suspend,
> > + mei_gsc_pm_resume)
> > + SET_RUNTIME_PM_OPS(mei_gsc_pm_runtime_suspend,
> > + mei_gsc_pm_runtime_resume,
> > + mei_gsc_pm_runtime_idle)
> > +};
> >
> > static const struct auxiliary_device_id mei_gsc_id_table[] = {
> > {
> > --
> > 2.32.0
> >
--
Thanks,
Sasha
