| Message ID | 20230405092904.1129395-3-martin.kepplinger@puri.sm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: hi846: support system suspend while streaming | expand |
Hi Martin, Thank you for the patch. 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; > } > }
Am Donnerstag, dem 06.04.2023 um 04:35 +0300 schrieb Laurent Pinchart: > Hi Martin, > > Thank you for the patch. > > 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? thanks, martin
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 ?
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; } }
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. 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(-)