RE: [Intel-gfx] [PATCH v10 4/5] mei: gsc: add runtime pm handlers

[Usyskin, Alexander]

> -----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: [Intel-gfx] [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