[PATCH v1 0/3] drm/media: Implement DU Suspend and Resume on VSP pipelines

[PATCH v1 0/3] drm/media: Implement DU Suspend and Resume on VSP pipelines

[Kieran Bingham]

This short series covers two subsystems and implements support for suspend and
resume operations on the DU pipelines on Gen3 Rcar platforms.

Patch 1: Prevent resuming DRM pipelines,
  - Ensures that the VSP does not incorrectly start DU pipelines.

Patch 2: Add suspend resume helpers
  - Makes use of the atomic helper functions to control the CRTCs
    and fbdev emulation.

Patch 3: Remove unused CRTC suspend/resume functions
  - Cleans up some old, related but unused functions that are not
    necessary to keep in the code base.

Whilst this is posted as a single series, there are no hard dependencies
between any of the three patches. They can be picked up independently as
and when they are successfully reviewed.

This series can be fetched from the following:

 git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git tags/vsp-du/du-suspend-resume/v1
  
It is based upon a merge of both the current linux-media master branch and the DRM drm-next tree.

Kieran Bingham (3):
  media: vsp1: Prevent resuming DRM pipelines
  drm: rcar-du: Add suspend resume helpers
  drm: rcar-du: Remove unused CRTC suspend/resume functions

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 35 +---------------------------
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 18 +++++++++++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  1 +-
 drivers/media/platform/vsp1/vsp1_drv.c |  8 +++++-
 4 files changed, 23 insertions(+), 39 deletions(-)

base-commit: 8d2ec9ae96657bbd539d88dbc9d01088f2c9ee63
-- 
git-series 0.9.1

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

[Kieran Bingham]

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.

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
+	 */
+	if (!vsp1->drm)
+		vsp1_pipelines_resume(vsp1);
 
 	return 0;
 }
-- 
git-series 0.9.1

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

[Laurent Pinchart]

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

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

>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

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

[Kieran Bingham]

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@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 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;
>>  }
> 

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

[Laurent Pinchart]

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

[PATCH v1 2/3] drm: rcar-du: Add suspend resume helpers

[Kieran Bingham]

The pipeline needs to ensure that the hardware is idle for suspend and
resume operations.

Implement suspend and resume functions using the DRM atomic helper
functions.

CC: dri-devel@lists.freedesktop.org

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 18 +++++++++++++++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 09fbceade6b1..01b91d0c169c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -22,6 +22,7 @@
 #include <linux/wait.h>
 
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
@@ -267,9 +268,19 @@ static struct drm_driver rcar_du_driver = {
 static int rcar_du_pm_suspend(struct device *dev)
 {
 	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
+	struct drm_atomic_state *state;
 
 	drm_kms_helper_poll_disable(rcdu->ddev);
-	/* TODO Suspend the CRTC */
+	drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, true);
+
+	state = drm_atomic_helper_suspend(rcdu->ddev);
+	if (IS_ERR(state)) {
+		drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);
+		drm_kms_helper_poll_enable(rcdu->ddev);
+		return PTR_ERR(state);
+	}
+
+	rcdu->suspend_state = state;
 
 	return 0;
 }
@@ -278,9 +289,10 @@ static int rcar_du_pm_resume(struct device *dev)
 {
 	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
 
-	/* TODO Resume the CRTC */
-
+	drm_atomic_helper_resume(rcdu->ddev, rcdu->suspend_state);
+	drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);
 	drm_kms_helper_poll_enable(rcdu->ddev);
+
 	return 0;
 }
 #endif
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index f8cd79488ece..f400fde65a0c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -81,6 +81,7 @@ struct rcar_du_device {
 
 	struct drm_device *ddev;
 	struct drm_fbdev_cma *fbdev;
+	struct drm_atomic_state *suspend_state;
 
 	struct rcar_du_crtc crtcs[RCAR_DU_MAX_CRTCS];
 	unsigned int num_crtcs;
-- 
git-series 0.9.1

Re: [PATCH v1 2/3] drm: rcar-du: Add suspend resume helpers

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday, 15 September 2017 19:42:06 EEST Kieran Bingham wrote:
> The pipeline needs to ensure that the hardware is idle for suspend and
> resume operations.

I'm not sure to really understand this sentence.

> Implement suspend and resume functions using the DRM atomic helper
> functions.
> 
> CC: dri-devel@lists.freedesktop.org
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The rest of the patch looks good to me. With the commit message clarified,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 18 +++++++++++++++---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 +
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 09fbceade6b1..01b91d0c169c
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -22,6 +22,7 @@
>  #include <linux/wait.h>
> 
>  #include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
> @@ -267,9 +268,19 @@ static struct drm_driver rcar_du_driver = {
>  static int rcar_du_pm_suspend(struct device *dev)
>  {
>  	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> +	struct drm_atomic_state *state;
> 
>  	drm_kms_helper_poll_disable(rcdu->ddev);
> -	/* TODO Suspend the CRTC */
> +	drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, true);
> +
> +	state = drm_atomic_helper_suspend(rcdu->ddev);
> +	if (IS_ERR(state)) {
> +		drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);
> +		drm_kms_helper_poll_enable(rcdu->ddev);
> +		return PTR_ERR(state);
> +	}
> +
> +	rcdu->suspend_state = state;
> 
>  	return 0;
>  }
> @@ -278,9 +289,10 @@ static int rcar_du_pm_resume(struct device *dev)
>  {
>  	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> 
> -	/* TODO Resume the CRTC */
> -
> +	drm_atomic_helper_resume(rcdu->ddev, rcdu->suspend_state);
> +	drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);
>  	drm_kms_helper_poll_enable(rcdu->ddev);
> +
>  	return 0;
>  }
>  #endif
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index f8cd79488ece..f400fde65a0c
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -81,6 +81,7 @@ struct rcar_du_device {
> 
>  	struct drm_device *ddev;
>  	struct drm_fbdev_cma *fbdev;
> +	struct drm_atomic_state *suspend_state;
> 
>  	struct rcar_du_crtc crtcs[RCAR_DU_MAX_CRTCS];
>  	unsigned int num_crtcs;


-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/3] drm: rcar-du: Add suspend resume helpers

[Kieran Bingham]

Hi Laurent,

Thanks for your speedy review!

On 15/09/17 18:02, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 19:42:06 EEST Kieran Bingham wrote:
>> The pipeline needs to ensure that the hardware is idle for suspend and
>> resume operations.
> 
> I'm not sure to really understand this sentence.

It makes sense to me ... :) - But I'm not the (only) target audience.

How about re-wording it in a similar way to your suggestion in [1/3]

"""
To support system suspend operations we must ensure the hardware is stopped, and
resumed explicitly from the suspend and resume handlers.

Implement suspend and resume functions using the DRM atomic helper
functions.
"""


> 
>> Implement suspend and resume functions using the DRM atomic helper
>> functions.
>>
>> CC: dri-devel@lists.freedesktop.org
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The rest of the patch looks good to me. With the commit message clarified,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 18 +++++++++++++++---
>>  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 +
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 09fbceade6b1..01b91d0c169c
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/wait.h>
>>
>>  #include <drm/drmP.h>
>> +#include <drm/drm_atomic_helper.h>
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_fb_cma_helper.h>
>>  #include <drm/drm_gem_cma_helper.h>
>> @@ -267,9 +268,19 @@ static struct drm_driver rcar_du_driver = {
>>  static int rcar_du_pm_suspend(struct device *dev)
>>  {
>>  	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
>> +	struct drm_atomic_state *state;
>>
>>  	drm_kms_helper_poll_disable(rcdu->ddev);
>> -	/* TODO Suspend the CRTC */
>> +	drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, true);
>> +
>> +	state = drm_atomic_helper_suspend(rcdu->ddev);
>> +	if (IS_ERR(state)) {
>> +		drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);
>> +		drm_kms_helper_poll_enable(rcdu->ddev);
>> +		return PTR_ERR(state);
>> +	}
>> +
>> +	rcdu->suspend_state = state;
>>
>>  	return 0;
>>  }
>> @@ -278,9 +289,10 @@ static int rcar_du_pm_resume(struct device *dev)
>>  {
>>  	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
>>
>> -	/* TODO Resume the CRTC */
>> -
>> +	drm_atomic_helper_resume(rcdu->ddev, rcdu->suspend_state);
>> +	drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);
>>  	drm_kms_helper_poll_enable(rcdu->ddev);
>> +
>>  	return 0;
>>  }
>>  #endif
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index f8cd79488ece..f400fde65a0c
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> @@ -81,6 +81,7 @@ struct rcar_du_device {
>>
>>  	struct drm_device *ddev;
>>  	struct drm_fbdev_cma *fbdev;
>> +	struct drm_atomic_state *suspend_state;
>>
>>  	struct rcar_du_crtc crtcs[RCAR_DU_MAX_CRTCS];
>>  	unsigned int num_crtcs;
> 
> 

Re: [PATCH v1 2/3] drm: rcar-du: Add suspend resume helpers

[Laurent Pinchart]

Hi Kieran,

On Friday, 15 September 2017 20:49:15 EEST Kieran Bingham wrote:
> On 15/09/17 18:02, Laurent Pinchart wrote:
> > On Friday, 15 September 2017 19:42:06 EEST Kieran Bingham wrote:
> >> The pipeline needs to ensure that the hardware is idle for suspend and
> >> resume operations.
> > 
> > I'm not sure to really understand this sentence.
> 
> It makes sense to me ... :) - But I'm not the (only) target audience.
> 
> How about re-wording it in a similar way to your suggestion in [1/3]
> 
> """
> To support system suspend operations we must ensure the hardware is stopped,
> and resumed explicitly from the suspend and resume handlers.
> 
> Implement suspend and resume functions using the DRM atomic helper
> functions.
> """

Sounds good to me. I'll update the commit message in my tree, and update the 
subject line to "drm: rcar-du: Implement system suspend/resume support".

> >> Implement suspend and resume functions using the DRM atomic helper
> >> functions.
> >> 
> >> CC: dri-devel@lists.freedesktop.org
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > The rest of the patch looks good to me. With the commit message clarified,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> >> ---
> >> 
> >>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 18 +++++++++++++++---
> >>  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 +
> >>  2 files changed, 16 insertions(+), 3 deletions(-)

[snip]

-- 
Regards,

Laurent Pinchart

[PATCH v1 3/3] drm: rcar-du: Remove unused CRTC suspend/resume functions

[Kieran Bingham]

An early implementation of suspend-resume helpers are available in the
CRTC module, however they are unused and no longer needed.

With suspend and resume handled by the core DRM atomic helpers, we can
remove the unused functions.

CC: dri-devel@lists.freedesktop.org

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 35 +---------------------------
 1 file changed, 35 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 301ea1a8018e..b492063a6e1f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -557,41 +557,6 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 	rcar_du_group_start_stop(rcrtc->group, false);
 }
 
-void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc)
-{
-	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
-		rcar_du_vsp_disable(rcrtc);
-
-	rcar_du_crtc_stop(rcrtc);
-	rcar_du_crtc_put(rcrtc);
-}
-
-void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
-{
-	unsigned int i;
-
-	if (!rcrtc->crtc.state->active)
-		return;
-
-	rcar_du_crtc_get(rcrtc);
-	rcar_du_crtc_setup(rcrtc);
-
-	/* Commit the planes state. */
-	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
-		for (i = 0; i < rcrtc->group->num_planes; ++i) {
-			struct rcar_du_plane *plane = &rcrtc->group->planes[i];
-
-			if (plane->plane.state->crtc != &rcrtc->crtc)
-				continue;
-
-			rcar_du_plane_setup(plane);
-		}
-	}
-
-	rcar_du_crtc_update_planes(rcrtc);
-	rcar_du_crtc_start(rcrtc);
-}
-
 /* -----------------------------------------------------------------------------
  * CRTC Functions
  */
-- 
git-series 0.9.1

Re: [PATCH v1 3/3] drm: rcar-du: Remove unused CRTC suspend/resume functions

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday, 15 September 2017 19:42:07 EEST Kieran Bingham wrote:
> An early implementation of suspend-resume helpers are available in the
> CRTC module, however they are unused and no longer needed.
> 
> With suspend and resume handled by the core DRM atomic helpers, we can
> remove the unused functions.
> 
> CC: dri-devel@lists.freedesktop.org
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'll take this in my tree with patch 2/3.

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 35 +---------------------------
>  1 file changed, 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 301ea1a8018e..b492063a6e1f
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -557,41 +557,6 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc
> *rcrtc) rcar_du_group_start_stop(rcrtc->group, false);
>  }
> 
> -void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc)
> -{
> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> -		rcar_du_vsp_disable(rcrtc);
> -
> -	rcar_du_crtc_stop(rcrtc);
> -	rcar_du_crtc_put(rcrtc);
> -}
> -
> -void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
> -{
> -	unsigned int i;
> -
> -	if (!rcrtc->crtc.state->active)
> -		return;
> -
> -	rcar_du_crtc_get(rcrtc);
> -	rcar_du_crtc_setup(rcrtc);
> -
> -	/* Commit the planes state. */
> -	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> -		for (i = 0; i < rcrtc->group->num_planes; ++i) {
> -			struct rcar_du_plane *plane = &rcrtc->group->planes[i];
> -
> -			if (plane->plane.state->crtc != &rcrtc->crtc)
> -				continue;
> -
> -			rcar_du_plane_setup(plane);
> -		}
> -	}
> -
> -	rcar_du_crtc_update_planes(rcrtc);
> -	rcar_du_crtc_start(rcrtc);
> -}
> -
>  /* ------------------------------------------------------------------------
>   * CRTC Functions
>   */


-- 
Regards,

Laurent Pinchart

[PATCH v2] media: vsp1: Prevent suspending and resuming DRM pipelines

[Kieran Bingham]

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, and doing so would cause the hardware to be
left in a misconfigured state.

Ensure that the VSP suspend and resume handlers do not affect DRM
based pipelines.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_drv.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index 962e4c304076..ed25ba9d551b 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -571,7 +571,13 @@ static int __maybe_unused vsp1_pm_suspend(struct device *dev)
 {
 	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
 
-	vsp1_pipelines_suspend(vsp1);
+	/*
+	 * When used as part of a display pipeline, the VSP is stopped and
+	 * restarted explicitly by the DU
+	 */
+	if (!vsp1->drm)
+		vsp1_pipelines_suspend(vsp1);
+
 	pm_runtime_force_suspend(vsp1->dev);
 
 	return 0;
@@ -582,7 +588,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);
+
+	/*
+	 * When used as part of a display pipeline, the VSP is stopped and
+	 * restarted explicitly by the DU
+	 */
+	if (!vsp1->drm)
+		vsp1_pipelines_resume(vsp1);
 
 	return 0;
 }
-- 
git-series 0.9.1

Re: [PATCH v2] media: vsp1: Prevent suspending and resuming DRM pipelines

[Kieran Bingham]

Ping ...

This patch appears to have got lost in the post.

Could someone pick it up please?

--
Regards

Kieran

On 20/09/17 10:16, Kieran Bingham wrote:
> 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, and doing so would cause the hardware to be
> left in a misconfigured state.
> 
> Ensure that the VSP suspend and resume handlers do not affect DRM
> based pipelines.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_drv.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
> index 962e4c304076..ed25ba9d551b 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -571,7 +571,13 @@ static int __maybe_unused vsp1_pm_suspend(struct device *dev)
>  {
>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>  
> -	vsp1_pipelines_suspend(vsp1);
> +	/*
> +	 * When used as part of a display pipeline, the VSP is stopped and
> +	 * restarted explicitly by the DU
> +	 */
> +	if (!vsp1->drm)
> +		vsp1_pipelines_suspend(vsp1);
> +
>  	pm_runtime_force_suspend(vsp1->dev);
>  
>  	return 0;
> @@ -582,7 +588,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);
> +
> +	/*
> +	 * When used as part of a display pipeline, the VSP is stopped and
> +	 * restarted explicitly by the DU
> +	 */
> +	if (!vsp1->drm)
> +		vsp1_pipelines_resume(vsp1);
>  
>  	return 0;
>  }
> 

Re: [PATCH v2] media: vsp1: Prevent suspending and resuming DRM pipelines

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday, 20 September 2017 12:16:54 EET Kieran Bingham wrote:
> 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, and doing so would cause the hardware to be
> left in a misconfigured state.
> 
> Ensure that the VSP suspend and resume handlers do not affect DRM
> based pipelines.

s/DRM-base/DRM-based/

> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_drv.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> b/drivers/media/platform/vsp1/vsp1_drv.c index 962e4c304076..ed25ba9d551b
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -571,7 +571,13 @@ static int __maybe_unused vsp1_pm_suspend(struct device
> *dev) {
>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> 
> -	vsp1_pipelines_suspend(vsp1);
> +	/*
> +	 * When used as part of a display pipeline, the VSP is stopped and
> +	 * restarted explicitly by the DU

s/DU/DU./

> +	 */
> +	if (!vsp1->drm)
> +		vsp1_pipelines_suspend(vsp1);
> +
>  	pm_runtime_force_suspend(vsp1->dev);
> 
>  	return 0;
> @@ -582,7 +588,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);
> +
> +	/*
> +	 * When used as part of a display pipeline, the VSP is stopped and
> +	 * restarted explicitly by the DU

s/DU/DU./

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 */
> +	if (!vsp1->drm)
> +		vsp1_pipelines_resume(vsp1);
> 
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: vsp1: Prevent suspending and resuming DRM pipelines

[Kieran Bingham]

Hi Laurent,

On 12/11/17 04:28, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday, 20 September 2017 12:16:54 EET Kieran Bingham wrote:
>> 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, and doing so would cause the hardware to be
>> left in a misconfigured state.
>>
>> Ensure that the VSP suspend and resume handlers do not affect DRM
>> based pipelines.
> 
> s/DRM-base/DRM-based/

-ENOMATCH


> 
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/vsp1/vsp1_drv.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
>> b/drivers/media/platform/vsp1/vsp1_drv.c index 962e4c304076..ed25ba9d551b
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drv.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
>> @@ -571,7 +571,13 @@ static int __maybe_unused vsp1_pm_suspend(struct device
>> *dev) {
>>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>>
>> -	vsp1_pipelines_suspend(vsp1);
>> +	/*
>> +	 * When used as part of a display pipeline, the VSP is stopped and
>> +	 * restarted explicitly by the DU
> 
> s/DU/DU./
> 
>> +	 */
>> +	if (!vsp1->drm)
>> +		vsp1_pipelines_suspend(vsp1);
>> +
>>  	pm_runtime_force_suspend(vsp1->dev);
>>
>>  	return 0;
>> @@ -582,7 +588,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);
>> +
>> +	/*
>> +	 * When used as part of a display pipeline, the VSP is stopped and
>> +	 * restarted explicitly by the DU
> 
> s/DU/DU./
> 
> Apart from that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks,

I'll add the full-stops and repost a v2.1 with your RB tag.

> 
>> +	 */
>> +	if (!vsp1->drm)
>> +		vsp1_pipelines_resume(vsp1);
>>
>>  	return 0;
>>  }
> 

Re: [PATCH v2] media: vsp1: Prevent suspending and resuming DRM pipelines

[Laurent Pinchart]

Hi Kieran,

On Sunday, 12 November 2017 18:38:31 EET Kieran Bingham wrote:
> On 12/11/17 04:28, Laurent Pinchart wrote:
> > On Wednesday, 20 September 2017 12:16:54 EET Kieran Bingham wrote:
> >> 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, and doing so would cause the hardware to be
> >> left in a misconfigured state.
> >> 
> >> Ensure that the VSP suspend and resume handlers do not affect DRM
> >> based pipelines.
> > 
> > s/DRM-base/DRM-based/
> 
> -ENOMATCH

This was of course meant to be s/DRM based/DRM-based/ :-)

> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> ---
> >> 
> >>  drivers/media/platform/vsp1/vsp1_drv.c | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> >> b/drivers/media/platform/vsp1/vsp1_drv.c index 962e4c304076..ed25ba9d551b
> >> 100644
> >> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> >> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> >> @@ -571,7 +571,13 @@ static int __maybe_unused vsp1_pm_suspend(struct
> >> device *dev) {
> >> 
> >>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> >> 
> >> -	vsp1_pipelines_suspend(vsp1);
> >> +	/*
> >> +	 * When used as part of a display pipeline, the VSP is stopped and
> >> +	 * restarted explicitly by the DU
> > 
> > s/DU/DU./
> > 
> >> +	 */
> >> +	if (!vsp1->drm)
> >> +		vsp1_pipelines_suspend(vsp1);
> >> +
> >> 
> >>  	pm_runtime_force_suspend(vsp1->dev);
> >>  	
> >>  	return 0;
> >> 
> >> @@ -582,7 +588,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);
> >> +
> >> +	/*
> >> +	 * When used as part of a display pipeline, the VSP is stopped and
> >> +	 * restarted explicitly by the DU
> > 
> > s/DU/DU./
> > 
> > Apart from that,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks,
> 
> I'll add the full-stops and repost a v2.1 with your RB tag.
> 
> >> +	 */
> >> +	if (!vsp1->drm)
> >> +		vsp1_pipelines_resume(vsp1);
> >> 
> >>  	return 0;
> >>  
> >>  }

-- 
Regards,

Laurent Pinchart