From: Tomas Winkler <[email protected]>
Implement runtime handlers for mei-gsc, to track
idle state of the device properly.
CC: Rodrigo Vivi <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Alexander Usyskin <[email protected]>
---
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);
+
+ 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;
+ }
+
+ 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
On Tue, Mar 08, 2022 at 06:36:53PM +0200, Alexander Usyskin wrote:
> From: Tomas Winkler <[email protected]>
>
> Implement runtime handlers for mei-gsc, to track
> idle state of the device properly.
>
> CC: Rodrigo Vivi <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> Signed-off-by: Alexander Usyskin <[email protected]>
> ---
> 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.
But also I'm missing the call to enable the autosuspend and set the delay.
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)
> +
> + 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...
> +
> + 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
>
> -----Original Message-----
> From: Vivi, Rodrigo <[email protected]>
> Sent: Thursday, March 10, 2022 21:04
> To: Usyskin, Alexander <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Jani Nikula
> <[email protected]>; Joonas Lahtinen
> <[email protected]>; David Airlie <[email protected]>; Daniel
> Vetter <[email protected]>; Tvrtko Ursulin <[email protected]>;
> [email protected]; Winkler, Tomas <[email protected]>;
> Lubart, Vitaly <[email protected]>; [email protected]
> 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 <[email protected]>
> >
> > Implement runtime handlers for mei-gsc, to track
> > idle state of the device properly.
> >
> > CC: Rodrigo Vivi <[email protected]>
> > Signed-off-by: Tomas Winkler <[email protected]>
> > Signed-off-by: Alexander Usyskin <[email protected]>
> > ---
> > 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