The dynamically created mei client device (mei csi) is used as one V4L2
sub device of the whole video pipeline, and the V4L2 connection graph is
built by software node. The mei_stop() and mei_restart() will delete the
old mei csi client device and create a new mei client device, which will
cause the software node information saved in old mei csi device lost and
the whole video pipeline will be broken.
Removing mei_stop()/mei_restart() during system suspend/resume can fix
the issue above and won't impact hardware actual power saving logic.
Fixes: 386a766c4169 ("mei: Add MEI hardware support for IVSC device")
Cc: [email protected] # for 6.8+
Reported-by: Hao Yao <[email protected]>
Signed-off-by: Wentong Wu <[email protected]>
Tested-by: Jason Chen <[email protected]>
---
drivers/misc/mei/platform-vsc.c | 39 +++++++++++++--------------------
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/drivers/misc/mei/platform-vsc.c b/drivers/misc/mei/platform-vsc.c
index b543e6b9f3cf..1ec65d87488a 100644
--- a/drivers/misc/mei/platform-vsc.c
+++ b/drivers/misc/mei/platform-vsc.c
@@ -399,41 +399,32 @@ static void mei_vsc_remove(struct platform_device *pdev)
static int mei_vsc_suspend(struct device *dev)
{
- struct mei_device *mei_dev = dev_get_drvdata(dev);
- struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
+ struct mei_device *mei_dev;
+ int ret = 0;
- mei_stop(mei_dev);
+ mei_dev = dev_get_drvdata(dev);
+ if (!mei_dev)
+ return -ENODEV;
- mei_disable_interrupts(mei_dev);
+ mutex_lock(&mei_dev->device_lock);
- vsc_tp_free_irq(hw->tp);
+ if (!mei_write_is_idle(mei_dev))
+ ret = -EAGAIN;
- return 0;
+ mutex_unlock(&mei_dev->device_lock);
+
+ return ret;
}
static int mei_vsc_resume(struct device *dev)
{
- struct mei_device *mei_dev = dev_get_drvdata(dev);
- struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
- int ret;
-
- ret = vsc_tp_request_irq(hw->tp);
- if (ret)
- return ret;
-
- ret = mei_restart(mei_dev);
- if (ret)
- goto err_free;
+ struct mei_device *mei_dev;
- /* start timer if stopped in suspend */
- schedule_delayed_work(&mei_dev->timer_work, HZ);
+ mei_dev = dev_get_drvdata(dev);
+ if (!mei_dev)
+ return -ENODEV;
return 0;
-
-err_free:
- vsc_tp_free_irq(hw->tp);
-
- return ret;
}
static DEFINE_SIMPLE_DEV_PM_OPS(mei_vsc_pm_ops, mei_vsc_suspend, mei_vsc_resume);
--
2.34.1
Hi Wentong,
Thanks for the patch. I thought something like this would indeed have been
possible.
On Thu, May 16, 2024 at 09:54:00AM +0800, Wentong Wu wrote:
> The dynamically created mei client device (mei csi) is used as one V4L2
> sub device of the whole video pipeline, and the V4L2 connection graph is
> built by software node. The mei_stop() and mei_restart() will delete the
> old mei csi client device and create a new mei client device, which will
> cause the software node information saved in old mei csi device lost and
> the whole video pipeline will be broken.
>
> Removing mei_stop()/mei_restart() during system suspend/resume can fix
> the issue above and won't impact hardware actual power saving logic.
>
> Fixes: 386a766c4169 ("mei: Add MEI hardware support for IVSC device")
I think this should be instead:
Fixes: f6085a96c973 ("mei: vsc: Unregister interrupt handler for system suspend")
As this fix depends on the previous not-quite-as-good fix.
Reviewed-by: Sakari Ailus <[email protected]>
Tested-by: Sakari Ailus <[email protected]>
> Cc: [email protected] # for 6.8+
> Reported-by: Hao Yao <[email protected]>
> Signed-off-by: Wentong Wu <[email protected]>
> Tested-by: Jason Chen <[email protected]>
> ---
> drivers/misc/mei/platform-vsc.c | 39 +++++++++++++--------------------
> 1 file changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/misc/mei/platform-vsc.c b/drivers/misc/mei/platform-vsc.c
> index b543e6b9f3cf..1ec65d87488a 100644
> --- a/drivers/misc/mei/platform-vsc.c
> +++ b/drivers/misc/mei/platform-vsc.c
> @@ -399,41 +399,32 @@ static void mei_vsc_remove(struct platform_device *pdev)
>
> static int mei_vsc_suspend(struct device *dev)
> {
> - struct mei_device *mei_dev = dev_get_drvdata(dev);
> - struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
> + struct mei_device *mei_dev;
> + int ret = 0;
>
> - mei_stop(mei_dev);
> + mei_dev = dev_get_drvdata(dev);
> + if (!mei_dev)
> + return -ENODEV;
>
> - mei_disable_interrupts(mei_dev);
> + mutex_lock(&mei_dev->device_lock);
>
> - vsc_tp_free_irq(hw->tp);
> + if (!mei_write_is_idle(mei_dev))
> + ret = -EAGAIN;
>
> - return 0;
> + mutex_unlock(&mei_dev->device_lock);
> +
> + return ret;
> }
>
> static int mei_vsc_resume(struct device *dev)
> {
> - struct mei_device *mei_dev = dev_get_drvdata(dev);
> - struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
> - int ret;
> -
> - ret = vsc_tp_request_irq(hw->tp);
> - if (ret)
> - return ret;
> -
> - ret = mei_restart(mei_dev);
> - if (ret)
> - goto err_free;
> + struct mei_device *mei_dev;
>
> - /* start timer if stopped in suspend */
> - schedule_delayed_work(&mei_dev->timer_work, HZ);
> + mei_dev = dev_get_drvdata(dev);
> + if (!mei_dev)
> + return -ENODEV;
>
> return 0;
> -
> -err_free:
> - vsc_tp_free_irq(hw->tp);
> -
> - return ret;
> }
>
> static DEFINE_SIMPLE_DEV_PM_OPS(mei_vsc_pm_ops, mei_vsc_suspend, mei_vsc_resume);
--
Kind regards,
Sakari Ailus
> From: Sakari Ailus <[email protected]>
>
> Hi Wentong,
>
> Thanks for the patch. I thought something like this would indeed have been
> possible.
Hi Sakari,
Thanks for your review.
>
> On Thu, May 16, 2024 at 09:54:00AM +0800, Wentong Wu wrote:
> > The dynamically created mei client device (mei csi) is used as one
> > V4L2 sub device of the whole video pipeline, and the V4L2 connection
> > graph is built by software node. The mei_stop() and mei_restart() will
> > delete the old mei csi client device and create a new mei client
> > device, which will cause the software node information saved in old
> > mei csi device lost and the whole video pipeline will be broken.
> >
> > Removing mei_stop()/mei_restart() during system suspend/resume can fix
> > the issue above and won't impact hardware actual power saving logic.
> >
> > Fixes: 386a766c4169 ("mei: Add MEI hardware support for IVSC device")
>
> I think this should be instead:
>
> Fixes: f6085a96c973 ("mei: vsc: Unregister interrupt handler for system
> suspend")
>
> As this fix depends on the previous not-quite-as-good fix.
Agree, v2 patch will address this. Thanks
BR,
Wentong
>
> Reviewed-by: Sakari Ailus <[email protected]>
> Tested-by: Sakari Ailus <[email protected]>
>
> > Cc: [email protected] # for 6.8+
> > Reported-by: Hao Yao <[email protected]>
> > Signed-off-by: Wentong Wu <[email protected]>
> > Tested-by: Jason Chen <[email protected]>
> > ---
> > drivers/misc/mei/platform-vsc.c | 39
> > +++++++++++++--------------------
> > 1 file changed, 15 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/misc/mei/platform-vsc.c
> > b/drivers/misc/mei/platform-vsc.c index b543e6b9f3cf..1ec65d87488a
> > 100644
> > --- a/drivers/misc/mei/platform-vsc.c
> > +++ b/drivers/misc/mei/platform-vsc.c
> > @@ -399,41 +399,32 @@ static void mei_vsc_remove(struct
> > platform_device *pdev)
> >
> > static int mei_vsc_suspend(struct device *dev) {
> > - struct mei_device *mei_dev = dev_get_drvdata(dev);
> > - struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
> > + struct mei_device *mei_dev;
> > + int ret = 0;
> >
> > - mei_stop(mei_dev);
> > + mei_dev = dev_get_drvdata(dev);
> > + if (!mei_dev)
> > + return -ENODEV;
> >
> > - mei_disable_interrupts(mei_dev);
> > + mutex_lock(&mei_dev->device_lock);
> >
> > - vsc_tp_free_irq(hw->tp);
> > + if (!mei_write_is_idle(mei_dev))
> > + ret = -EAGAIN;
> >
> > - return 0;
> > + mutex_unlock(&mei_dev->device_lock);
> > +
> > + return ret;
> > }
> >
> > static int mei_vsc_resume(struct device *dev) {
> > - struct mei_device *mei_dev = dev_get_drvdata(dev);
> > - struct mei_vsc_hw *hw = mei_dev_to_vsc_hw(mei_dev);
> > - int ret;
> > -
> > - ret = vsc_tp_request_irq(hw->tp);
> > - if (ret)
> > - return ret;
> > -
> > - ret = mei_restart(mei_dev);
> > - if (ret)
> > - goto err_free;
> > + struct mei_device *mei_dev;
> >
> > - /* start timer if stopped in suspend */
> > - schedule_delayed_work(&mei_dev->timer_work, HZ);
> > + mei_dev = dev_get_drvdata(dev);
> > + if (!mei_dev)
> > + return -ENODEV;
> >
> > return 0;
> > -
> > -err_free:
> > - vsc_tp_free_irq(hw->tp);
> > -
> > - return ret;
> > }
> >
> > static DEFINE_SIMPLE_DEV_PM_OPS(mei_vsc_pm_ops, mei_vsc_suspend,
> > mei_vsc_resume);
>
> --
> Kind regards,
>
> Sakari Ailus