Hi Laurent, On 15/09/17 17:58, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Friday, 15 September 2017 19:42:05 EEST Kieran Bingham wrote: >> DRM pipelines utilising the VSP must stop all frame processing as part >> of the suspend operation to ensure the hardware is idle. Upon resume, >> the pipeline must not be started until the DU performs an atomic flush >> to restore the hardware configuration and state. >> >> Therefore the vsp1_pipeline_resume() call is not needed for DRM >> pipelines, and we can disable it in this instance. > > Being familiar with the issue I certainly understand the commit message, but I > think it can be a bit confusing to a reader not familiar to the VSP/DU. How > about something similar to the following ? > > "When used as part of a display pipeline, the VSP is stopped and restarted > explicitly by the DU from its suspend and resume handlers. There is thus no > need to stop or restart pipelines in the VSP suspend and resume handlers." That's fine with me. >> CC: linux-media@xxxxxxxxxxxxxxx >> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> >> --- >> drivers/media/platform/vsp1/vsp1_drv.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c >> b/drivers/media/platform/vsp1/vsp1_drv.c index 962e4c304076..7604c7994c74 >> 100644 >> --- a/drivers/media/platform/vsp1/vsp1_drv.c >> +++ b/drivers/media/platform/vsp1/vsp1_drv.c >> @@ -582,7 +582,13 @@ static int __maybe_unused vsp1_pm_resume(struct device >> *dev) struct vsp1_device *vsp1 = dev_get_drvdata(dev); >> >> pm_runtime_force_resume(vsp1->dev); >> - vsp1_pipelines_resume(vsp1); >> + >> + /* >> + * DRM pipelines are stopped before suspend, and will be resumed after >> + * the DRM subsystem has reconfigured its pipeline with an atomic flush >> + */ > > I would also adapt this comment similarly to the commit message. > >> + if (!vsp1->drm) >> + vsp1_pipelines_resume(vsp1); > > Should we do the same in vsp1_pm_suspend() ? I know it shouldn't be strictly > needed at the moment as vsp1_pipelines_suspend() should be a no-op when the > pipelines are already stopped, but a symmetrical implementation sounds better > to me. I'm OK with that, it's not needed - but it doesn't hurt to be symmetrical. (Updated locally for a v1.1 repost or such) > I also wonder whether the check shouldn't be moved inside the > vsp1_pipelines_suspend() and vsp1_pipelines_resume() functions as we will > likely need to handle suspend/resume of display pipelines when adding > writeback support, but we could do so later. I'll have to retest the writeback implementation - but I think that has got quite stale now anyway and will need some rework. > >> return 0; >> } >
