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@xxxxxxxxxxxxxxxxxxxxx
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> The rest of the patch looks good to me. With the commit message clarified,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
>> ---
>> 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;
>
>
