Re: [PATCH v1 2/2] media: hi846: preserve the streaming state during system suspend

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Martin,

On Tue, Apr 11, 2023 at 11:39:32AM +0200, Martin Kepplinger wrote:
> Am Donnerstag, dem 06.04.2023 um 04:35 +0300 schrieb Laurent Pinchart:
> > On Wed, Apr 05, 2023 at 11:29:04AM +0200, Martin Kepplinger wrote:
> > > The hi846 driver changed the "streaming" state inside of "start/stop_streaming".
> > > The problem is that inside of the (system) suspend callback, it calls
> > > "stop_streaming" unconditionally. So streaming would always be stopped
> > > when suspending.
> > > 
> > > That makes sense with runtime pm for example, after s_stream(..., 0) but
> > > does not preserve the "streaming" state during system suspend when
> > > currently streaming.
> > 
> > The driver shouldn't need to stop streaming at system suspend time. It
> > should have had its .s_stream(0) operation called and shouldn't be
> > streaming anymore. If that's not the case, there's an issue somewhere
> > else, which should be fixed. The code that stops streaming at system
> > suspend and restarts it at system resume should then be dropped from
> > this driver.
> > 
> > > Fix this by simply setting the streaming state outside of "start/stop_streaming"
> > > which is s_stream().
> > > 
> > > While at it, improve things a bit by not assigning "1", but the "enable"
> > > value we later compare against, and fix one error handling path in
> > > resume().
> > > 
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > > ---
> > >  drivers/media/i2c/hi846.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> > > index 0b0eda2e223cd..1ca6e9407d618 100644
> > > --- a/drivers/media/i2c/hi846.c
> > > +++ b/drivers/media/i2c/hi846.c
> > > @@ -1780,8 +1780,6 @@ static int hi846_start_streaming(struct hi846 *hi846)
> > >                 return ret;
> > >         }
> > >  
> > > -       hi846->streaming = 1;
> > > -
> > >         dev_dbg(&client->dev, "%s: started streaming successfully\n", __func__);
> > >  
> > >         return ret;
> > > @@ -1793,8 +1791,6 @@ static void hi846_stop_streaming(struct hi846 *hi846)
> > >  
> > >         if (hi846_write_reg(hi846, HI846_REG_MODE_SELECT, HI846_MODE_STANDBY))
> > >                 dev_err(&client->dev, "failed to stop stream");
> > > -
> > > -       hi846->streaming = 0;
> > >  }
> > >  
> > >  static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
> > > @@ -1816,10 +1812,12 @@ static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
> > >                 }
> > >  
> > >                 ret = hi846_start_streaming(hi846);
> > > +               hi846->streaming = enable;
> > >         }
> > >  
> > >         if (!enable || ret) {
> > >                 hi846_stop_streaming(hi846);
> > > +               hi846->streaming = 0;
> > >                 pm_runtime_put(&client->dev);
> > >         }
> > >  
> > > @@ -1898,6 +1896,8 @@ static int __maybe_unused hi846_resume(struct device *dev)
> > >                 if (ret) {
> > >                         dev_err(dev, "%s: start streaming failed: %d\n",
> > >                                 __func__, ret);
> > > +                       hi846_stop_streaming(hi846);
> > > +                       hi846->streaming = 0;
> > >                         goto error;
> > >                 }
> > >         }
> 
> hi Laurent,
> 
> ok I see. My first test without any streaming-state handling in
> suspend/resume doesn't succeed. But now I know that's the goal and I'll
> put this on my list to do.
> 
> Since this driver *already* tracks "streaming", would you be ok with
> this fix in the meantime?

I'd rather get a patch that drops streaming handling :-) It's fine
carrying a local patch in the Librem5 kernel to work around the problem
until a proper fix is available, but do we have a need to get the
workaround in mainline too ?

When it comes to a proper fix, it may be as simple as manually calling
device_link_add() in consumer (e.g. the CSI-2 receiver) drivers to
create links to suppliers(e.g. the camera sensor). The PM core should
then guarantee that the consumer gets suspended before the producer.
Would you be able to test that ?

-- 
Regards,

Laurent Pinchart