Re: [PATCH v1 1/3] media: vsp1: Prevent resuming DRM pipelines

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

Hi Kieran,

On Monday, 18 September 2017 03:04:13 EEST Kieran Bingham wrote:
> On 15/09/17 17:58, Laurent Pinchart wrote:
> > 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@vger.kernel.org
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> ---
> >> 
> >>  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've taken patches 2/3 and 3/3 in my tree so feel free to report 1/3 only.

> > 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.

Agreed, I don't expect it to work out of the box. We can move the check later 
when rebasing the writeback patches.

> >>  	return 0;
> >>  
> >>  }

-- 
Regards,

Laurent Pinchart