[PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Souptick Joarder]

convert drm_atomic_helper_suspend/resume() to use
drm_mode_config_helper_suspend/resume().

remove suspend_state field from the rcar_du_device
structure as it is no more required.

With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
will left with no consumer. So this function can be removed.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
 drivers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
 drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
 include/drm/drm_fb_cma_helper.h       |  2 --
 4 files changed, 2 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 47e0e2f..96efc88 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma)
 		drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
 }
 EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
-
-/**
- * drm_fbdev_cma_set_suspend_unlocked - wrapper around
- *                                      drm_fb_helper_set_suspend_unlocked
- * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
- * @state: desired state, zero to resume, non-zero to suspend
- *
- * Calls drm_fb_helper_set_suspend, which is a wrapper around
- * fb_set_suspend implemented by fbdev core.
- */
-void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma,
-					bool state)
-{
-	if (fbdev_cma)
-		drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
-						   state);
-}
-EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 02aee6c..288220f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device *dev)
 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);
-	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;
+	return drm_mode_config_helper_suspend(rcdu->ddev);
 }
 
 static int rcar_du_pm_resume(struct device *dev)
 {
 	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
 
-	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;
+	return drm_mode_config_helper_resume(rcdu->ddev);
 }
 #endif
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index b3a25e8..ff25c8d 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -78,7 +78,6 @@ 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;
diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
index 4a65f0d..8dbbe1e 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
 
 void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
 void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
-void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma,
-					bool state);
 
 struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
 	unsigned int plane);
-- 
1.9.1

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Souptick Joarder]

On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> convert drm_atomic_helper_suspend/resume() to use
> drm_mode_config_helper_suspend/resume().
>
> remove suspend_state field from the rcar_du_device
> structure as it is no more required.
>
> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> will left with no consumer. So this function can be removed.
>
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>

Laurent, any comment on this patch ??
> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
>  include/drm/drm_fb_cma_helper.h       |  2 --
>  4 files changed, 2 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 47e0e2f..96efc88 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma)
>                 drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
>  }
>  EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
> -
> -/**
> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
> - *                                      drm_fb_helper_set_suspend_unlocked
> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> - * @state: desired state, zero to resume, non-zero to suspend
> - *
> - * Calls drm_fb_helper_set_suspend, which is a wrapper around
> - * fb_set_suspend implemented by fbdev core.
> - */
> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma,
> -                                       bool state)
> -{
> -       if (fbdev_cma)
> -               drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
> -                                                  state);
> -}
> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index 02aee6c..288220f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device *dev)
>  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);
> -       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;
> +       return drm_mode_config_helper_suspend(rcdu->ddev);
>  }
>
>  static int rcar_du_pm_resume(struct device *dev)
>  {
>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
>
> -       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;
> +       return drm_mode_config_helper_resume(rcdu->ddev);
>  }
>  #endif
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index b3a25e8..ff25c8d 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -78,7 +78,6 @@ 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;
> diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
> index 4a65f0d..8dbbe1e 100644
> --- a/include/drm/drm_fb_cma_helper.h
> +++ b/include/drm/drm_fb_cma_helper.h
> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
>
>  void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
>  void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma,
> -                                       bool state);
>
>  struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
>         unsigned int plane);
> --
> 1.9.1
>

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Laurent Pinchart]

Hi Souptick,

Thank you for the patch.

On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
> On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> > convert drm_atomic_helper_suspend/resume() to use
> > drm_mode_config_helper_suspend/resume().
> > 
> > remove suspend_state field from the rcar_du_device
> > structure as it is no more required.
> > 
> > With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> > will left with no consumer. So this function can be removed.
> > 
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> 
> Laurent, any comment on this patch ??

Sorry for the delay, and thanks for pinging me.

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

Should I apply this to my tree or do you plan to merge it through drm-misc as 
it touches drm_fb_cma_helper.c ?

> > ---
> > 
> >  drivers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
> >  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> >  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
> >  include/drm/drm_fb_cma_helper.h       |  2 --
> >  4 files changed, 2 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma
> > *fbdev_cma)> 
> >                 drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
> >  
> >  }
> >  EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
> > 
> > -
> > -/**
> > - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
> > - *                                     
> > drm_fb_helper_set_suspend_unlocked
> > - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> > - * @state: desired state, zero to resume, non-zero to suspend
> > - *
> > - * Calls drm_fb_helper_set_suspend, which is a wrapper around
> > - * fb_set_suspend implemented by fbdev core.
> > - */
> > -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma,
> > -                                       bool state)
> > -{
> > -       if (fbdev_cma)
> > -               drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
> > -                                                  state);
> > -}
> > -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> > *dev)
> > 
> >  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);
> > -       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;
> > +       return drm_mode_config_helper_suspend(rcdu->ddev);
> > 
> >  }
> >  
> >  static int rcar_du_pm_resume(struct device *dev)
> >  {
> >  
> >         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> > 
> > -       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;
> > +       return drm_mode_config_helper_resume(rcdu->ddev);
> > 
> >  }
> >  #endif
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > @@ -78,7 +78,6 @@ 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;
> > 
> > diff --git a/include/drm/drm_fb_cma_helper.h
> > b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644
> > --- a/include/drm/drm_fb_cma_helper.h
> > +++ b/include/drm/drm_fb_cma_helper.h
> > @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> > drm_device *dev,> 
> >  void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
> >  void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> > 
> > -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma,
> > -                                       bool state);
> > 
> >  struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer
> >  *fb,
> >         unsigned int plane);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Laurent Pinchart]

Hi Souptick,

On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
> On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
> > On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> >> convert drm_atomic_helper_suspend/resume() to use
> >> drm_mode_config_helper_suspend/resume().
> >> 
> >> remove suspend_state field from the rcar_du_device
> >> structure as it is no more required.
> >> 
> >> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> >> will left with no consumer. So this function can be removed.
> >> 
> >> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > 
> > Laurent, any comment on this patch ??
> 
> Sorry for the delay, and thanks for pinging me.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Should I apply this to my tree or do you plan to merge it through drm-misc
> as it touches drm_fb_cma_helper.c ?

I just realized that the same patch got supplied by Noralf Trønnes nearly a 
year ago, and was later superseded by https://patchwork.freedesktop.org/patch/
247861/. I think we should thus apply Noralf's patches instead (once he sends 
v4 out).

> >> ---
> >> 
> >>  drivers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
> >>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> >>  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
> >>  include/drm/drm_fb_cma_helper.h       |  2 --
> >>  4 files changed, 2 insertions(+), 40 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> >> b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644
> >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> >> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct
> >> drm_fbdev_cma *fbdev_cma)
> >>                 drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
> >>  }
> >>  EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
> >> 
> >> -
> >> -/**
> >> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
> >> - *
> >> drm_fb_helper_set_suspend_unlocked
> >> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> >> - * @state: desired state, zero to resume, non-zero to suspend
> >> - *
> >> - * Calls drm_fb_helper_set_suspend, which is a wrapper around
> >> - * fb_set_suspend implemented by fbdev core.
> >> - */
> >> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> >> *fbdev_cma,
> >> -                                       bool state)
> >> -{
> >> -       if (fbdev_cma)
> >> -              
> >> drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
> >> -                                                  state);
> >> -}
> >> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> >> *dev)
> >> 
> >>  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);
> >> -       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;
> >> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> >>  }
> >>  
> >>  static int rcar_du_pm_resume(struct device *dev)
> >>  {
> >>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> >> 
> >> -       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;
> >> +       return drm_mode_config_helper_resume(rcdu->ddev);
> >>  }
> >>  #endif
> >> 
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >> @@ -78,7 +78,6 @@ 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;
> >> diff --git a/include/drm/drm_fb_cma_helper.h
> >> b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644
> >> --- a/include/drm/drm_fb_cma_helper.h
> >> +++ b/include/drm/drm_fb_cma_helper.h
> >> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> >> drm_device *dev,
> >> 
> >>  void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
> >>  void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> >> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> >> *fbdev_cma,
> >> -                                       bool state);
> >> 
> >>  struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct
> >>  drm_framebuffer *fb,
> >>         unsigned int plane);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Laurent Pinchart]

Hi Souptick,

On Friday, 28 September 2018 23:02:32 EEST Souptick Joarder wrote:
> On 28-Sep-2018 9:00 PM, "Laurent Pinchart" wrote:
> > On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
> >> On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
> >>> On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> >>>> convert drm_atomic_helper_suspend/resume() to use
> >>>> drm_mode_config_helper_suspend/resume().
> >>>> 
> >>>> remove suspend_state field from the rcar_du_device
> >>>> structure as it is no more required.
> >>>> 
> >>>> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> >>>> will left with no consumer. So this function can be removed.
> >>>> 
> >>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> >>> 
> >>> Laurent, any comment on this patch ??
> >> 
> >> Sorry for the delay, and thanks for pinging me.
> >> 
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> 
> >> Should I apply this to my tree or do you plan to merge it through
> >> drm-misc as it touches drm_fb_cma_helper.c ?
> > 
> > I just realized that the same patch got supplied by Noralf Trønnes nearly
> > a year ago, and was later superseded by https://patchwork.freedesktop.org/
> > patch/247861/. I think we should thus apply Noralf's patches instead (once
> > he sends v4 out).
> 
> Sure, you can go ahead with his patch.
> 
> Shall I remove drm_fbdev_cma_set_suspend_unlocked in a separate patch once
> all the consumer remove this function ?

Sure, if the function isn't removed as part of Noralf's patch series, please 
send a separate patch to remove it.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Daniel Vetter]

On Fri, Sep 28, 2018 at 06:30:35PM +0300, Laurent Pinchart wrote:
> Hi Souptick,
> 
> On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
> > On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
> > > On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> > >> convert drm_atomic_helper_suspend/resume() to use
> > >> drm_mode_config_helper_suspend/resume().
> > >> 
> > >> remove suspend_state field from the rcar_du_device
> > >> structure as it is no more required.
> > >> 
> > >> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> > >> will left with no consumer. So this function can be removed.
> > >> 
> > >> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > > 
> > > Laurent, any comment on this patch ??
> > 
> > Sorry for the delay, and thanks for pinging me.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Should I apply this to my tree or do you plan to merge it through drm-misc
> > as it touches drm_fb_cma_helper.c ?
> 
> I just realized that the same patch got supplied by Noralf Trønnes nearly a 
> year ago, and was later superseded by https://patchwork.freedesktop.org/patch/
> 247861/. I think we should thus apply Noralf's patches instead (once he sends 
> v4 out).

I don't think Noralf is still actively working on this, he's busy with the
fbdev emulation stuff. Probably better if you pick up one of the existing
ones, than waiting another year or so :-)

Cheers, Daniel

> 
> > >> ---
> > >> 
> > >>  drivers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
> > >>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> > >>  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
> > >>  include/drm/drm_fb_cma_helper.h       |  2 --
> > >>  4 files changed, 2 insertions(+), 40 deletions(-)
> > >> 
> > >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > >> b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644
> > >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > >> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct
> > >> drm_fbdev_cma *fbdev_cma)
> > >>                 drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
> > >>  }
> > >>  EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
> > >> 
> > >> -
> > >> -/**
> > >> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
> > >> - *
> > >> drm_fb_helper_set_suspend_unlocked
> > >> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> > >> - * @state: desired state, zero to resume, non-zero to suspend
> > >> - *
> > >> - * Calls drm_fb_helper_set_suspend, which is a wrapper around
> > >> - * fb_set_suspend implemented by fbdev core.
> > >> - */
> > >> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > >> *fbdev_cma,
> > >> -                                       bool state)
> > >> -{
> > >> -       if (fbdev_cma)
> > >> -              
> > >> drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
> > >> -                                                  state);
> > >> -}
> > >> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > >> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> > >> *dev)
> > >> 
> > >>  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);
> > >> -       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;
> > >> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> > >>  }
> > >>  
> > >>  static int rcar_du_pm_resume(struct device *dev)
> > >>  {
> > >>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> > >> 
> > >> -       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;
> > >> +       return drm_mode_config_helper_resume(rcdu->ddev);
> > >>  }
> > >>  #endif
> > >> 
> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644
> > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > >> @@ -78,7 +78,6 @@ 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;
> > >> diff --git a/include/drm/drm_fb_cma_helper.h
> > >> b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644
> > >> --- a/include/drm/drm_fb_cma_helper.h
> > >> +++ b/include/drm/drm_fb_cma_helper.h
> > >> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> > >> drm_device *dev,
> > >> 
> > >>  void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
> > >>  void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> > >> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > >> *fbdev_cma,
> > >> -                                       bool state);
> > >> 
> > >>  struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct
> > >>  drm_framebuffer *fb,
> > >>         unsigned int plane);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Laurent Pinchart]

Hi Daniel,

On Monday, 1 October 2018 09:52:20 EEST Daniel Vetter wrote:
> On Fri, Sep 28, 2018 at 06:30:35PM +0300, Laurent Pinchart wrote:
> > On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
> > > On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
> > > > On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> > > >> convert drm_atomic_helper_suspend/resume() to use
> > > >> drm_mode_config_helper_suspend/resume().
> > > >> 
> > > >> remove suspend_state field from the rcar_du_device
> > > >> structure as it is no more required.
> > > >> 
> > > >> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> > > >> will left with no consumer. So this function can be removed.
> > > >> 
> > > >> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > > > 
> > > > Laurent, any comment on this patch ??
> > > 
> > > Sorry for the delay, and thanks for pinging me.
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > Should I apply this to my tree or do you plan to merge it through
> > > drm-misc
> > > as it touches drm_fb_cma_helper.c ?
> > 
> > I just realized that the same patch got supplied by Noralf Trønnes nearly
> > a
> > year ago, and was later superseded by
> > https://patchwork.freedesktop.org/patch/247861/. I think we should thus
> > apply Noralf's patches instead (once he sends v4 out).
> 
> I don't think Noralf is still actively working on this, he's busy with the
> fbdev emulation stuff. Probably better if you pick up one of the existing
> ones, than waiting another year or so :-)

Isn't https://patchwork.freedesktop.org/patch/247861/ part of the fbdev 
emulation stuff ? That patch series is from less than a month ago, so I 
thought it was actively developed.

Let's ask him. Noralf, any comment ? :-) 

> > > >> ---
> > > >> 
> > > >>  drivers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
> > > >>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> > > >>  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
> > > >>  include/drm/drm_fb_cma_helper.h       |  2 --
> > > >>  4 files changed, 2 insertions(+), 40 deletions(-)
> > > >> 
> > > >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > >> b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644
> > > >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > >> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct
> > > >> drm_fbdev_cma *fbdev_cma)
> > > >> 
> > > >>                 drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
> > > >>  
> > > >>  }
> > > >>  EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
> > > >> 
> > > >> -
> > > >> -/**
> > > >> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
> > > >> - *
> > > >> drm_fb_helper_set_suspend_unlocked
> > > >> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> > > >> - * @state: desired state, zero to resume, non-zero to suspend
> > > >> - *
> > > >> - * Calls drm_fb_helper_set_suspend, which is a wrapper around
> > > >> - * fb_set_suspend implemented by fbdev core.
> > > >> - */
> > > >> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > > >> *fbdev_cma,
> > > >> -                                       bool state)
> > > >> -{
> > > >> -       if (fbdev_cma)
> > > >> -
> > > >> drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
> > > >> -                                                  state);
> > > >> -}
> > > >> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> > > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > >> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> > > >> *dev)
> > > >> 
> > > >>  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);
> > > >> -       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;
> > > >> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> > > >> 
> > > >>  }
> > > >>  
> > > >>  static int rcar_du_pm_resume(struct device *dev)
> > > >>  {
> > > >>  
> > > >>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> > > >> 
> > > >> -       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;
> > > >> +       return drm_mode_config_helper_resume(rcdu->ddev);
> > > >> 
> > > >>  }
> > > >>  #endif
> > > >> 
> > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > > >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644
> > > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > > >> @@ -78,7 +78,6 @@ 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;
> > > >> 
> > > >> diff --git a/include/drm/drm_fb_cma_helper.h
> > > >> b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644
> > > >> --- a/include/drm/drm_fb_cma_helper.h
> > > >> +++ b/include/drm/drm_fb_cma_helper.h
> > > >> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> > > >> drm_device *dev,
> > > >> 
> > > >>  void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
> > > >>  void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> > > >> 
> > > >> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > > >> *fbdev_cma,
> > > >> -                                       bool state);
> > > >> 
> > > >>  struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct
> > > >>  drm_framebuffer *fb,
> > > >>  
> > > >>         unsigned int plane);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Noralf Trønnes]

Den 01.10.2018 13.56, skrev Laurent Pinchart:
> Hi Daniel,
>
> On Monday, 1 October 2018 09:52:20 EEST Daniel Vetter wrote:
>> On Fri, Sep 28, 2018 at 06:30:35PM +0300, Laurent Pinchart wrote:
>>> On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
>>>> On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
>>>>> On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
>>>>>> convert drm_atomic_helper_suspend/resume() to use
>>>>>> drm_mode_config_helper_suspend/resume().
>>>>>>
>>>>>> remove suspend_state field from the rcar_du_device
>>>>>> structure as it is no more required.
>>>>>>
>>>>>> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
>>>>>> will left with no consumer. So this function can be removed.
>>>>>>
>>>>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>>>>> Laurent, any comment on this patch ??
>>>> Sorry for the delay, and thanks for pinging me.
>>>>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>> Should I apply this to my tree or do you plan to merge it through
>>>> drm-misc
>>>> as it touches drm_fb_cma_helper.c ?
>>> I just realized that the same patch got supplied by Noralf Trønnes nearly
>>> a
>>> year ago, and was later superseded by
>>> https://patchwork.freedesktop.org/patch/247861/. I think we should thus
>>> apply Noralf's patches instead (once he sends v4 out).
>> I don't think Noralf is still actively working on this, he's busy with the
>> fbdev emulation stuff. Probably better if you pick up one of the existing
>> ones, than waiting another year or so :-)
> Isn't https://patchwork.freedesktop.org/patch/247861/ part of the fbdev
> emulation stuff ? That patch series is from less than a month ago, so I
> thought it was actively developed.
>
> Let's ask him. Noralf, any comment ? :-)

I see now that I've caused some confusion here.

I did some refactoring of the fbdev stuff in the CMA helper last year.
In the last series of that work there is this patch:

[v3,07/11] drm/rcar-du: Use drm_mode_config_helper_suspend/resume()
https://patchwork.freedesktop.org/patch/192414/

During Christmas I got the idea that maybe it was possible to use a dumb
buffer to do generic fbdev emulation. That work took me 6 months before a
solution using a general internal DRM client was hammered out. By the
time I started to convert drivers I had forgotten about that patch.

When I wrote the patch (#247861) to convert rcar to the generic fbdev, I
just bundled up the suspend/resume change with the fbdev conversion.
Laurent and Sam commented on that bundling, so I said that I would split
it up in the next version.

All that being said, I think that this patch should be applied and I'll
just rebase the rcar fbdev conversion patch on top of that.
It's the easiest for me at least :-)

Noralf.

>>>>>> ---
>>>>>>
>>>>>>   drivers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
>>>>>>   include/drm/drm_fb_cma_helper.h       |  2 --
>>>>>>   4 files changed, 2 insertions(+), 40 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644
>>>>>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct
>>>>>> drm_fbdev_cma *fbdev_cma)
>>>>>>
>>>>>>                  drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
>>>>>>   
>>>>>>   }
>>>>>>   EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
>>>>>>
>>>>>> -
>>>>>> -/**
>>>>>> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
>>>>>> - *
>>>>>> drm_fb_helper_set_suspend_unlocked
>>>>>> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
>>>>>> - * @state: desired state, zero to resume, non-zero to suspend
>>>>>> - *
>>>>>> - * Calls drm_fb_helper_set_suspend, which is a wrapper around
>>>>>> - * fb_set_suspend implemented by fbdev core.
>>>>>> - */
>>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
>>>>>> *fbdev_cma,
>>>>>> -                                       bool state)
>>>>>> -{
>>>>>> -       if (fbdev_cma)
>>>>>> -
>>>>>> drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
>>>>>> -                                                  state);
>>>>>> -}
>>>>>> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>>>>>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
>>>>>> *dev)
>>>>>>
>>>>>>   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);
>>>>>> -       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;
>>>>>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
>>>>>>
>>>>>>   }
>>>>>>   
>>>>>>   static int rcar_du_pm_resume(struct device *dev)
>>>>>>   {
>>>>>>   
>>>>>>          struct rcar_du_device *rcdu = dev_get_drvdata(dev);
>>>>>>
>>>>>> -       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;
>>>>>> +       return drm_mode_config_helper_resume(rcdu->ddev);
>>>>>>
>>>>>>   }
>>>>>>   #endif
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644
>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>>>>>> @@ -78,7 +78,6 @@ 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;
>>>>>>
>>>>>> diff --git a/include/drm/drm_fb_cma_helper.h
>>>>>> b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644
>>>>>> --- a/include/drm/drm_fb_cma_helper.h
>>>>>> +++ b/include/drm/drm_fb_cma_helper.h
>>>>>> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
>>>>>> drm_device *dev,
>>>>>>
>>>>>>   void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
>>>>>>   void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
>>>>>>
>>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
>>>>>> *fbdev_cma,
>>>>>> -                                       bool state);
>>>>>>
>>>>>>   struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct
>>>>>>   drm_framebuffer *fb,
>>>>>>   
>>>>>>          unsigned int plane);

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Souptick Joarder]

Hi Laurent,
On Mon, Oct 1, 2018 at 6:12 PM Noralf Trønnes <noralf@tronnes.org> wrote:
>
>
> Den 01.10.2018 13.56, skrev Laurent Pinchart:
> > Hi Daniel,
> >
> > On Monday, 1 October 2018 09:52:20 EEST Daniel Vetter wrote:
> >> On Fri, Sep 28, 2018 at 06:30:35PM +0300, Laurent Pinchart wrote:
> >>> On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
> >>>> On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
> >>>>> On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> >>>>>> convert drm_atomic_helper_suspend/resume() to use
> >>>>>> drm_mode_config_helper_suspend/resume().
> >>>>>>
> >>>>>> remove suspend_state field from the rcar_du_device
> >>>>>> structure as it is no more required.
> >>>>>>
> >>>>>> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> >>>>>> will left with no consumer. So this function can be removed.
> >>>>>>
> >>>>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> >>>>> Laurent, any comment on this patch ??
> >>>> Sorry for the delay, and thanks for pinging me.
> >>>>
> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>
> >>>> Should I apply this to my tree or do you plan to merge it through
> >>>> drm-misc
> >>>> as it touches drm_fb_cma_helper.c ?
> >>> I just realized that the same patch got supplied by Noralf Trønnes nearly
> >>> a
> >>> year ago, and was later superseded by
> >>> https://patchwork.freedesktop.org/patch/247861/. I think we should thus
> >>> apply Noralf's patches instead (once he sends v4 out).
> >> I don't think Noralf is still actively working on this, he's busy with the
> >> fbdev emulation stuff. Probably better if you pick up one of the existing
> >> ones, than waiting another year or so :-)
> > Isn't https://patchwork.freedesktop.org/patch/247861/ part of the fbdev
> > emulation stuff ? That patch series is from less than a month ago, so I
> > thought it was actively developed.
> >
> > Let's ask him. Noralf, any comment ? :-)
>
> I see now that I've caused some confusion here.
>
> I did some refactoring of the fbdev stuff in the CMA helper last year.
> In the last series of that work there is this patch:
>
> [v3,07/11] drm/rcar-du: Use drm_mode_config_helper_suspend/resume()
> https://patchwork.freedesktop.org/patch/192414/
>
> During Christmas I got the idea that maybe it was possible to use a dumb
> buffer to do generic fbdev emulation. That work took me 6 months before a
> solution using a general internal DRM client was hammered out. By the
> time I started to convert drivers I had forgotten about that patch.
>
> When I wrote the patch (#247861) to convert rcar to the generic fbdev, I
> just bundled up the suspend/resume change with the fbdev conversion.
> Laurent and Sam commented on that bundling, so I said that I would split
> it up in the next version.
>
> All that being said, I think that this patch should be applied and I'll
> just rebase the rcar fbdev conversion patch on top of that.
> It's the easiest for me at least :-)
>
Any conclusion on this patch ? :-)

ers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
> >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
> >>>>>>   include/drm/drm_fb_cma_helper.h       |  2 --
> >>>>>>   4 files changed, 2 insertions(+), 40 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> >>>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644
> >>>>>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> >>>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> >>>>>> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct
> >>>>>> drm_fbdev_cma *fbdev_cma)
> >>>>>>
> >>>>>>                  drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
> >>>>>>
> >>>>>>   }
> >>>>>>   EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
> >>>>>>
> >>>>>> -
> >>>>>> -/**
> >>>>>> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
> >>>>>> - *
> >>>>>> drm_fb_helper_set_suspend_unlocked
> >>>>>> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> >>>>>> - * @state: desired state, zero to resume, non-zero to suspend
> >>>>>> - *
> >>>>>> - * Calls drm_fb_helper_set_suspend, which is a wrapper around
> >>>>>> - * fb_set_suspend implemented by fbdev core.
> >>>>>> - */
> >>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> >>>>>> *fbdev_cma,
> >>>>>> -                                       bool state)
> >>>>>> -{
> >>>>>> -       if (fbdev_cma)
> >>>>>> -
> >>>>>> drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
> >>>>>> -                                                  state);
> >>>>>> -}
> >>>>>> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>>>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> >>>>>> *dev)
> >>>>>>
> >>>>>>   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);
> >>>>>> -       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;
> >>>>>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> >>>>>>
> >>>>>>   }
> >>>>>>
> >>>>>>   static int rcar_du_pm_resume(struct device *dev)
> >>>>>>   {
> >>>>>>
> >>>>>>          struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> >>>>>>
> >>>>>> -       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;
> >>>>>> +       return drm_mode_config_helper_resume(rcdu->ddev);
> >>>>>>
> >>>>>>   }
> >>>>>>   #endif
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644
> >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >>>>>> @@ -78,7 +78,6 @@ 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;
> >>>>>>
> >>>>>> diff --git a/include/drm/drm_fb_cma_helper.h
> >>>>>> b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644
> >>>>>> --- a/include/drm/drm_fb_cma_helper.h
> >>>>>> +++ b/include/drm/drm_fb_cma_helper.h
> >>>>>> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> >>>>>> drm_device *dev,
> >>>>>>
> >>>>>>   void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
> >>>>>>   void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> >>>>>>
> >>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> >>>>>> *fbdev_cma,
> >>>>>> -                                       bool state);
> >>>>>>
> >>>>>>   struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct
> >>>>>>   drm_framebuffer *fb,
> >>>>>>
> >>>>>>          unsigned int plane);
>

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Daniel Vetter]

On Mon, Oct 08, 2018 at 09:57:52PM +0530, Souptick Joarder wrote:
> Hi Laurent,
> On Mon, Oct 1, 2018 at 6:12 PM Noralf Trønnes <noralf@tronnes.org> wrote:
> >
> >
> > Den 01.10.2018 13.56, skrev Laurent Pinchart:
> > > Hi Daniel,
> > >
> > > On Monday, 1 October 2018 09:52:20 EEST Daniel Vetter wrote:
> > >> On Fri, Sep 28, 2018 at 06:30:35PM +0300, Laurent Pinchart wrote:
> > >>> On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
> > >>>> On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
> > >>>>> On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> > >>>>>> convert drm_atomic_helper_suspend/resume() to use
> > >>>>>> drm_mode_config_helper_suspend/resume().
> > >>>>>>
> > >>>>>> remove suspend_state field from the rcar_du_device
> > >>>>>> structure as it is no more required.
> > >>>>>>
> > >>>>>> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> > >>>>>> will left with no consumer. So this function can be removed.
> > >>>>>>
> > >>>>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > >>>>> Laurent, any comment on this patch ??
> > >>>> Sorry for the delay, and thanks for pinging me.
> > >>>>
> > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>>
> > >>>> Should I apply this to my tree or do you plan to merge it through
> > >>>> drm-misc
> > >>>> as it touches drm_fb_cma_helper.c ?
> > >>> I just realized that the same patch got supplied by Noralf Trønnes nearly
> > >>> a
> > >>> year ago, and was later superseded by
> > >>> https://patchwork.freedesktop.org/patch/247861/. I think we should thus
> > >>> apply Noralf's patches instead (once he sends v4 out).
> > >> I don't think Noralf is still actively working on this, he's busy with the
> > >> fbdev emulation stuff. Probably better if you pick up one of the existing
> > >> ones, than waiting another year or so :-)
> > > Isn't https://patchwork.freedesktop.org/patch/247861/ part of the fbdev
> > > emulation stuff ? That patch series is from less than a month ago, so I
> > > thought it was actively developed.
> > >
> > > Let's ask him. Noralf, any comment ? :-)
> >
> > I see now that I've caused some confusion here.
> >
> > I did some refactoring of the fbdev stuff in the CMA helper last year.
> > In the last series of that work there is this patch:
> >
> > [v3,07/11] drm/rcar-du: Use drm_mode_config_helper_suspend/resume()
> > https://patchwork.freedesktop.org/patch/192414/
> >
> > During Christmas I got the idea that maybe it was possible to use a dumb
> > buffer to do generic fbdev emulation. That work took me 6 months before a
> > solution using a general internal DRM client was hammered out. By the
> > time I started to convert drivers I had forgotten about that patch.
> >
> > When I wrote the patch (#247861) to convert rcar to the generic fbdev, I
> > just bundled up the suspend/resume change with the fbdev conversion.
> > Laurent and Sam commented on that bundling, so I said that I would split
> > it up in the next version.
> >
> > All that being said, I think that this patch should be applied and I'll
> > just rebase the rcar fbdev conversion patch on top of that.
> > It's the easiest for me at least :-)
> >
> Any conclusion on this patch ? :-)

Laurent should pick it up and apply to his rcar-du tree I think. We can
also stuff it into drm-misc as a fallback.
-Daniel

> 
> ers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
> > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
> > >>>>>>   include/drm/drm_fb_cma_helper.h       |  2 --
> > >>>>>>   4 files changed, 2 insertions(+), 40 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > >>>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644
> > >>>>>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > >>>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > >>>>>> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct
> > >>>>>> drm_fbdev_cma *fbdev_cma)
> > >>>>>>
> > >>>>>>                  drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
> > >>>>>>
> > >>>>>>   }
> > >>>>>>   EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
> > >>>>>>
> > >>>>>> -
> > >>>>>> -/**
> > >>>>>> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
> > >>>>>> - *
> > >>>>>> drm_fb_helper_set_suspend_unlocked
> > >>>>>> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> > >>>>>> - * @state: desired state, zero to resume, non-zero to suspend
> > >>>>>> - *
> > >>>>>> - * Calls drm_fb_helper_set_suspend, which is a wrapper around
> > >>>>>> - * fb_set_suspend implemented by fbdev core.
> > >>>>>> - */
> > >>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > >>>>>> *fbdev_cma,
> > >>>>>> -                                       bool state)
> > >>>>>> -{
> > >>>>>> -       if (fbdev_cma)
> > >>>>>> -
> > >>>>>> drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
> > >>>>>> -                                                  state);
> > >>>>>> -}
> > >>>>>> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > >>>>>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> > >>>>>> *dev)
> > >>>>>>
> > >>>>>>   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);
> > >>>>>> -       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;
> > >>>>>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> > >>>>>>
> > >>>>>>   }
> > >>>>>>
> > >>>>>>   static int rcar_du_pm_resume(struct device *dev)
> > >>>>>>   {
> > >>>>>>
> > >>>>>>          struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> > >>>>>>
> > >>>>>> -       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;
> > >>>>>> +       return drm_mode_config_helper_resume(rcdu->ddev);
> > >>>>>>
> > >>>>>>   }
> > >>>>>>   #endif
> > >>>>>>
> > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644
> > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > >>>>>> @@ -78,7 +78,6 @@ 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;
> > >>>>>>
> > >>>>>> diff --git a/include/drm/drm_fb_cma_helper.h
> > >>>>>> b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644
> > >>>>>> --- a/include/drm/drm_fb_cma_helper.h
> > >>>>>> +++ b/include/drm/drm_fb_cma_helper.h
> > >>>>>> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> > >>>>>> drm_device *dev,
> > >>>>>>
> > >>>>>>   void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
> > >>>>>>   void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> > >>>>>>
> > >>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > >>>>>> *fbdev_cma,
> > >>>>>> -                                       bool state);
> > >>>>>>
> > >>>>>>   struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct
> > >>>>>>   drm_framebuffer *fb,
> > >>>>>>
> > >>>>>>          unsigned int plane);
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Souptick Joarder]

Hi Laurent,

On Thu, Oct 11, 2018 at 1:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Oct 08, 2018 at 09:57:52PM +0530, Souptick Joarder wrote:
> > Hi Laurent,
> > On Mon, Oct 1, 2018 at 6:12 PM Noralf Trønnes <noralf@tronnes.org> wrote:
> > >
> > >
> > > Den 01.10.2018 13.56, skrev Laurent Pinchart:
> > > > Hi Daniel,
> > > >
> > > > On Monday, 1 October 2018 09:52:20 EEST Daniel Vetter wrote:
> > > >> On Fri, Sep 28, 2018 at 06:30:35PM +0300, Laurent Pinchart wrote:
> > > >>> On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
> > > >>>> On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
> > > >>>>> On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> > > >>>>>> convert drm_atomic_helper_suspend/resume() to use
> > > >>>>>> drm_mode_config_helper_suspend/resume().
> > > >>>>>>
> > > >>>>>> remove suspend_state field from the rcar_du_device
> > > >>>>>> structure as it is no more required.
> > > >>>>>>
> > > >>>>>> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> > > >>>>>> will left with no consumer. So this function can be removed.
> > > >>>>>>
> > > >>>>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > > >>>>> Laurent, any comment on this patch ??
> > > >>>> Sorry for the delay, and thanks for pinging me.
> > > >>>>
> > > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >>>>
> > > >>>> Should I apply this to my tree or do you plan to merge it through
> > > >>>> drm-misc
> > > >>>> as it touches drm_fb_cma_helper.c ?
> > > >>> I just realized that the same patch got supplied by Noralf Trønnes nearly
> > > >>> a
> > > >>> year ago, and was later superseded by
> > > >>> https://patchwork.freedesktop.org/patch/247861/. I think we should thus
> > > >>> apply Noralf's patches instead (once he sends v4 out).
> > > >> I don't think Noralf is still actively working on this, he's busy with the
> > > >> fbdev emulation stuff. Probably better if you pick up one of the existing
> > > >> ones, than waiting another year or so :-)
> > > > Isn't https://patchwork.freedesktop.org/patch/247861/ part of the fbdev
> > > > emulation stuff ? That patch series is from less than a month ago, so I
> > > > thought it was actively developed.
> > > >
> > > > Let's ask him. Noralf, any comment ? :-)
> > >
> > > I see now that I've caused some confusion here.
> > >
> > > I did some refactoring of the fbdev stuff in the CMA helper last year.
> > > In the last series of that work there is this patch:
> > >
> > > [v3,07/11] drm/rcar-du: Use drm_mode_config_helper_suspend/resume()
> > > https://patchwork.freedesktop.org/patch/192414/
> > >
> > > During Christmas I got the idea that maybe it was possible to use a dumb
> > > buffer to do generic fbdev emulation. That work took me 6 months before a
> > > solution using a general internal DRM client was hammered out. By the
> > > time I started to convert drivers I had forgotten about that patch.
> > >
> > > When I wrote the patch (#247861) to convert rcar to the generic fbdev, I
> > > just bundled up the suspend/resume change with the fbdev conversion.
> > > Laurent and Sam commented on that bundling, so I said that I would split
> > > it up in the next version.
> > >
> > > All that being said, I think that this patch should be applied and I'll
> > > just rebase the rcar fbdev conversion patch on top of that.
> > > It's the easiest for me at least :-)
> > >
> > Any conclusion on this patch ? :-)
>
> Laurent should pick it up and apply to his rcar-du tree I think. We can
> also stuff it into drm-misc as a fallback.

Can we get this patch in queue for 4.20 ?

> -Daniel
>
> >
> > ers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
> > > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> > > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
> > > >>>>>>   include/drm/drm_fb_cma_helper.h       |  2 --
> > > >>>>>>   4 files changed, 2 insertions(+), 40 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > >>>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644
> > > >>>>>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > >>>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > >>>>>> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct
> > > >>>>>> drm_fbdev_cma *fbdev_cma)
> > > >>>>>>
> > > >>>>>>                  drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
> > > >>>>>>
> > > >>>>>>   }
> > > >>>>>>   EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
> > > >>>>>>
> > > >>>>>> -
> > > >>>>>> -/**
> > > >>>>>> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
> > > >>>>>> - *
> > > >>>>>> drm_fb_helper_set_suspend_unlocked
> > > >>>>>> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> > > >>>>>> - * @state: desired state, zero to resume, non-zero to suspend
> > > >>>>>> - *
> > > >>>>>> - * Calls drm_fb_helper_set_suspend, which is a wrapper around
> > > >>>>>> - * fb_set_suspend implemented by fbdev core.
> > > >>>>>> - */
> > > >>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > > >>>>>> *fbdev_cma,
> > > >>>>>> -                                       bool state)
> > > >>>>>> -{
> > > >>>>>> -       if (fbdev_cma)
> > > >>>>>> -
> > > >>>>>> drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
> > > >>>>>> -                                                  state);
> > > >>>>>> -}
> > > >>>>>> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > >>>>>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> > > >>>>>> *dev)
> > > >>>>>>
> > > >>>>>>   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);
> > > >>>>>> -       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;
> > > >>>>>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> > > >>>>>>
> > > >>>>>>   }
> > > >>>>>>
> > > >>>>>>   static int rcar_du_pm_resume(struct device *dev)
> > > >>>>>>   {
> > > >>>>>>
> > > >>>>>>          struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> > > >>>>>>
> > > >>>>>> -       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;
> > > >>>>>> +       return drm_mode_config_helper_resume(rcdu->ddev);
> > > >>>>>>
> > > >>>>>>   }
> > > >>>>>>   #endif
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644
> > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > > >>>>>> @@ -78,7 +78,6 @@ 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;
> > > >>>>>>
> > > >>>>>> diff --git a/include/drm/drm_fb_cma_helper.h
> > > >>>>>> b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644
> > > >>>>>> --- a/include/drm/drm_fb_cma_helper.h
> > > >>>>>> +++ b/include/drm/drm_fb_cma_helper.h
> > > >>>>>> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> > > >>>>>> drm_device *dev,
> > > >>>>>>
> > > >>>>>>   void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
> > > >>>>>>   void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> > > >>>>>>
> > > >>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > > >>>>>> *fbdev_cma,
> > > >>>>>> -                                       bool state);
> > > >>>>>>
> > > >>>>>>   struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct
> > > >>>>>>   drm_framebuffer *fb,
> > > >>>>>>
> > > >>>>>>          unsigned int plane);
> > >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Daniel Vetter]

On Mon, Oct 22, 2018 at 01:51:35PM +0530, Souptick Joarder wrote:
> Hi Laurent,
> 
> On Thu, Oct 11, 2018 at 1:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Oct 08, 2018 at 09:57:52PM +0530, Souptick Joarder wrote:
> > > Hi Laurent,
> > > On Mon, Oct 1, 2018 at 6:12 PM Noralf Trønnes <noralf@tronnes.org> wrote:
> > > >
> > > >
> > > > Den 01.10.2018 13.56, skrev Laurent Pinchart:
> > > > > Hi Daniel,
> > > > >
> > > > > On Monday, 1 October 2018 09:52:20 EEST Daniel Vetter wrote:
> > > > >> On Fri, Sep 28, 2018 at 06:30:35PM +0300, Laurent Pinchart wrote:
> > > > >>> On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
> > > > >>>> On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:
> > > > >>>>> On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> > > > >>>>>> convert drm_atomic_helper_suspend/resume() to use
> > > > >>>>>> drm_mode_config_helper_suspend/resume().
> > > > >>>>>>
> > > > >>>>>> remove suspend_state field from the rcar_du_device
> > > > >>>>>> structure as it is no more required.
> > > > >>>>>>
> > > > >>>>>> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()
> > > > >>>>>> will left with no consumer. So this function can be removed.
> > > > >>>>>>
> > > > >>>>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > > > >>>>> Laurent, any comment on this patch ??
> > > > >>>> Sorry for the delay, and thanks for pinging me.
> > > > >>>>
> > > > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >>>>
> > > > >>>> Should I apply this to my tree or do you plan to merge it through
> > > > >>>> drm-misc
> > > > >>>> as it touches drm_fb_cma_helper.c ?
> > > > >>> I just realized that the same patch got supplied by Noralf Trønnes nearly
> > > > >>> a
> > > > >>> year ago, and was later superseded by
> > > > >>> https://patchwork.freedesktop.org/patch/247861/. I think we should thus
> > > > >>> apply Noralf's patches instead (once he sends v4 out).
> > > > >> I don't think Noralf is still actively working on this, he's busy with the
> > > > >> fbdev emulation stuff. Probably better if you pick up one of the existing
> > > > >> ones, than waiting another year or so :-)
> > > > > Isn't https://patchwork.freedesktop.org/patch/247861/ part of the fbdev
> > > > > emulation stuff ? That patch series is from less than a month ago, so I
> > > > > thought it was actively developed.
> > > > >
> > > > > Let's ask him. Noralf, any comment ? :-)
> > > >
> > > > I see now that I've caused some confusion here.
> > > >
> > > > I did some refactoring of the fbdev stuff in the CMA helper last year.
> > > > In the last series of that work there is this patch:
> > > >
> > > > [v3,07/11] drm/rcar-du: Use drm_mode_config_helper_suspend/resume()
> > > > https://patchwork.freedesktop.org/patch/192414/
> > > >
> > > > During Christmas I got the idea that maybe it was possible to use a dumb
> > > > buffer to do generic fbdev emulation. That work took me 6 months before a
> > > > solution using a general internal DRM client was hammered out. By the
> > > > time I started to convert drivers I had forgotten about that patch.
> > > >
> > > > When I wrote the patch (#247861) to convert rcar to the generic fbdev, I
> > > > just bundled up the suspend/resume change with the fbdev conversion.
> > > > Laurent and Sam commented on that bundling, so I said that I would split
> > > > it up in the next version.
> > > >
> > > > All that being said, I think that this patch should be applied and I'll
> > > > just rebase the rcar fbdev conversion patch on top of that.
> > > > It's the easiest for me at least :-)
> > > >
> > > Any conclusion on this patch ? :-)
> >
> > Laurent should pick it up and apply to his rcar-du tree I think. We can
> > also stuff it into drm-misc as a fallback.
> 
> Can we get this patch in queue for 4.20 ?

5.1, 4.20 is already done.

But yes, I pulled this into drm-misc-next. Thanks for your patch.
-Daniel

> 
> > -Daniel
> >
> > >
> > > ers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
> > > > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> > > > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
> > > > >>>>>>   include/drm/drm_fb_cma_helper.h       |  2 --
> > > > >>>>>>   4 files changed, 2 insertions(+), 40 deletions(-)
> > > > >>>>>>
> > > > >>>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > >>>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644
> > > > >>>>>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > >>>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > >>>>>> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct
> > > > >>>>>> drm_fbdev_cma *fbdev_cma)
> > > > >>>>>>
> > > > >>>>>>                  drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);
> > > > >>>>>>
> > > > >>>>>>   }
> > > > >>>>>>   EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);
> > > > >>>>>>
> > > > >>>>>> -
> > > > >>>>>> -/**
> > > > >>>>>> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around
> > > > >>>>>> - *
> > > > >>>>>> drm_fb_helper_set_suspend_unlocked
> > > > >>>>>> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> > > > >>>>>> - * @state: desired state, zero to resume, non-zero to suspend
> > > > >>>>>> - *
> > > > >>>>>> - * Calls drm_fb_helper_set_suspend, which is a wrapper around
> > > > >>>>>> - * fb_set_suspend implemented by fbdev core.
> > > > >>>>>> - */
> > > > >>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > > > >>>>>> *fbdev_cma,
> > > > >>>>>> -                                       bool state)
> > > > >>>>>> -{
> > > > >>>>>> -       if (fbdev_cma)
> > > > >>>>>> -
> > > > >>>>>> drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,
> > > > >>>>>> -                                                  state);
> > > > >>>>>> -}
> > > > >>>>>> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> > > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> > > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > > >>>>>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> > > > >>>>>> *dev)
> > > > >>>>>>
> > > > >>>>>>   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);
> > > > >>>>>> -       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;
> > > > >>>>>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> > > > >>>>>>
> > > > >>>>>>   }
> > > > >>>>>>
> > > > >>>>>>   static int rcar_du_pm_resume(struct device *dev)
> > > > >>>>>>   {
> > > > >>>>>>
> > > > >>>>>>          struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> > > > >>>>>>
> > > > >>>>>> -       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;
> > > > >>>>>> +       return drm_mode_config_helper_resume(rcdu->ddev);
> > > > >>>>>>
> > > > >>>>>>   }
> > > > >>>>>>   #endif
> > > > >>>>>>
> > > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > > > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644
> > > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > > > >>>>>> @@ -78,7 +78,6 @@ 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;
> > > > >>>>>>
> > > > >>>>>> diff --git a/include/drm/drm_fb_cma_helper.h
> > > > >>>>>> b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644
> > > > >>>>>> --- a/include/drm/drm_fb_cma_helper.h
> > > > >>>>>> +++ b/include/drm/drm_fb_cma_helper.h
> > > > >>>>>> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> > > > >>>>>> drm_device *dev,
> > > > >>>>>>
> > > > >>>>>>   void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
> > > > >>>>>>   void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> > > > >>>>>>
> > > > >>>>>> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma
> > > > >>>>>> *fbdev_cma,
> > > > >>>>>> -                                       bool state);
> > > > >>>>>>
> > > > >>>>>>   struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct
> > > > >>>>>>   drm_framebuffer *fb,
> > > > >>>>>>
> > > > >>>>>>          unsigned int plane);
> > > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Laurent Pinchart]

Hi Daniel,

On Tuesday, 23 October 2018 16:40:41 EEST Daniel Vetter wrote:
> On Mon, Oct 22, 2018 at 01:51:35PM +0530, Souptick Joarder wrote:
> > On Thu, Oct 11, 2018 at 1:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >> On Mon, Oct 08, 2018 at 09:57:52PM +0530, Souptick Joarder wrote:
> >>> On Mon, Oct 1, 2018 at 6:12 PM Noralf Trønnes wrote:
> >>>> Den 01.10.2018 13.56, skrev Laurent Pinchart:
> >>>>> On Monday, 1 October 2018 09:52:20 EEST Daniel Vetter wrote:
> >>>>>> On Fri, Sep 28, 2018 at 06:30:35PM +0300, Laurent Pinchart wrote:
> >>>>>>> On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:
> >>>>>>>> On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder 
wrote:
> >>>>>>>>> On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:
> >>>>>>>>>> convert drm_atomic_helper_suspend/resume() to use
> >>>>>>>>>> drm_mode_config_helper_suspend/resume().
> >>>>>>>>>> 
> >>>>>>>>>> remove suspend_state field from the rcar_du_device
> >>>>>>>>>> structure as it is no more required.
> >>>>>>>>>> 
> >>>>>>>>>> With this conversion, also
> >>>>>>>>>> drm_fbdev_cma_set_suspend_unlocked()
> >>>>>>>>>> will left with no consumer. So this function can be removed.
> >>>>>>>>>> 
> >>>>>>>>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> >>>>>>>>> 
> >>>>>>>>> Laurent, any comment on this patch ??
> >>>>>>>> 
> >>>>>>>> Sorry for the delay, and thanks for pinging me.
> >>>>>>>> 
> >>>>>>>> Reviewed-by: Laurent Pinchart
> >>>>>>>> <laurent.pinchart@ideasonboard.com>
> >>>>>>>> 
> >>>>>>>> Should I apply this to my tree or do you plan to merge it
> >>>>>>>> through drm-misc as it touches drm_fb_cma_helper.c ?
> >>>>>>> 
> >>>>>>> I just realized that the same patch got supplied by Noralf
> >>>>>>> Trønnes nearly a year ago, and was later superseded by
> >>>>>>> https://patchwork.freedesktop.org/patch/247861/. I think we
> >>>>>>> should thus apply Noralf's patches instead (once he sends v4
> >>>>>>> out).
> >>>>>> 
> >>>>>> I don't think Noralf is still actively working on this, he's busy
> >>>>>> with the fbdev emulation stuff. Probably better if you pick up
> >>>>>> one of the existing ones, than waiting another year or so :-)
> >>>>> 
> >>>>> Isn't https://patchwork.freedesktop.org/patch/247861/ part of the
> >>>>> fbdev emulation stuff ? That patch series is from less than a
> >>>>> month ago, so I thought it was actively developed.
> >>>>> 
> >>>>> Let's ask him. Noralf, any comment ? :-)
> >>>> 
> >>>> I see now that I've caused some confusion here.
> >>>> 
> >>>> I did some refactoring of the fbdev stuff in the CMA helper last
> >>>> year. In the last series of that work there is this patch:
> >>>> 
> >>>> [v3,07/11] drm/rcar-du: Use drm_mode_config_helper_suspend/resume()
> >>>> https://patchwork.freedesktop.org/patch/192414/
> >>>> 
> >>>> During Christmas I got the idea that maybe it was possible to use a
> >>>> dumb buffer to do generic fbdev emulation. That work took me 6
> >>>> months before a solution using a general internal DRM client was
> >>>> hammered out. By the time I started to convert drivers I had
> >>>> forgotten about that patch.
> >>>> 
> >>>> When I wrote the patch (#247861) to convert rcar to the generic
> >>>> fbdev, I just bundled up the suspend/resume change with the fbdev
> >>>> conversion. Laurent and Sam commented on that bundling, so I said
> >>>> that I would split it up in the next version.
> >>>> 
> >>>> All that being said, I think that this patch should be applied and
> >>>> I'll just rebase the rcar fbdev conversion patch on top of that.
> >>>> It's the easiest for me at least :-)
> >>> 
> >>> Any conclusion on this patch ? :-)
> >> 
> >> Laurent should pick it up and apply to his rcar-du tree I think. We can
> >> also stuff it into drm-misc as a fallback.
> > 
> > Can we get this patch in queue for 4.20 ?
> 
> 5.1, 4.20 is already done.
> 
> But yes, I pulled this into drm-misc-next. Thanks for your patch.

I would have preferred applying this through my tree. I'm currently at ELCE 
and had to prepare for the conference, hence the delay.

> >>>>>>>>>>   drivers/gpu/drm/drm_fb_cma_helper.c   | 18 ------------------
> >>>>>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-----------------
> >>>>>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 -
> >>>>>>>>>>   include/drm/drm_fb_cma_helper.h       |  2 --
> >>>>>>>>>>   4 files changed, 2 insertions(+), 40 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Souptick Joarder]

On Mon, Jul 30, 2018 at 9:15 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Souptick,
>
> On Monday, 30 July 2018 18:13:13 EEST Souptick Joarder wrote:
>> On Mon, Jul 30, 2018 at 7:54 PM, Laurent Pinchart wrote:
>> > On Monday, 30 July 2018 16:58:09 EEST Souptick Joarder wrote:
>> >> On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart wrote:
>> >>> On Saturday, 28 July 2018 21:50:58 EEST Souptick Joarder wrote:
>> >>>> On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar wrote:
>> >>>>> On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder wrote:
>> >>>>>> convert drm_atomic_helper_suspend/resume() to use
>> >>>>>> drm_mode_config_helper_suspend/resume().
>> >>>>>
>> >>>>> Hi Souptick,
>> >>>>>
>> >>>>> Thanks for your patch.
>> >>>>>
>> >>>>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>> >>>>>> Signed-off-by: Ajit Negi <ajitn.linux@gmail.com>
>> >>>>>> ---
>> >>>>>>
>> >>>>>>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
>> >>>>>>  1 file changed, 2 insertions(+), 19 deletions(-)
>> >>>>>>
>> >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f
>> >>>>>> 100644
>> >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >>>>>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct
>> >>>>>> drm_device *dev)
>> >>>>>>
>> >>>>>>  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);
>> >>>>>> -       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);
>> >>>>>
>> >>>>> I don't think we can use drm_mode_config_helper_(suspend/resume)
>> >>>>> API here as this file uses CMA functions.
>> >>>>
>> >>>> drm_fbdev_cma_set_suspend_unlocked() is wrapper function which
>> >>>> invokes drm_fb_helper_set_suspend_unlocked().
>> >>>>
>> >>>> Where the new API drm_mode_config_helper_suspend/resume() directly
>> >>>> invokes drm_fb_helper_set_suspend_unlocked(). So it is safe to replace
>> >>>> exiting code with API drm_mode_config_helper_suspend/resume().
>> >>>
>> >>> I agree that they're functionally equivalent for now, but what if
>> >>> drm_fbdev_cma_set_suspend_unlocked() gets extended later ? This change
>> >>> risks introducing a breakage that could could unnoticed at that point.
>> >>
>> >> No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have
>> >> any impact on driver because with this patch we will be retaining the
>> >> original suspend/resume logic of the rcar-du driver and further this
>> >> driver is not going to use drm_fbdev_cma_set_suspend_unlocked().
>> >
>> > My point is that if the fb cma helpers gets later extended with a feature
>> > that need special handling and suspend/resume time, with the
>> > drm_fbdev_cma_set_suspend_unlocked() function properly updated to take
>> > that feature into account, driver using those helpers but converted to
>> > drm_atomic_helper_suspend/resume() will break.
>> >
>> >>> At the very
>> >>> least you should add a comment in drm_fbdev_cma_set_suspend_unlocked()
>> >>> to explain that any extension of the function should also address all
>> >>> drivers using drm_mode_config_helper_suspend() and
>> >>> drm_mode_config_helper_resume().
>> >>
>> >> The consumers of drm_fbdev_cma_set_suspend_unlocked() are -
>> >> drivers/gpu/drm/arm/hdlcd_drv.c
>> >> drivers/gpu/drm/drm_fb_cma_helper.c
>> >>
>> >> and both will be converted to use API
>> >> drm_mode_config_helper_suspend/resume(). As there will be no more
>> >> consumer of drm_fbdev_cma_set_suspend_unlocked() , we can remove this
>> >> wrapper API forever :)
>> >
>> > OK, if you remove the function completely then anyone wanting to extend
>> > the fbdev cma helpers in the way described above will notice that
>> > something will need to be done, so it's fine. Please thus make sure that
>> > you go all the way to removing that function.
>>
>> Sure, once both the drivers are converted to use
>> drm_mode_config_helper_suspend/resume()
>> and goes into linus's tree, then we can remove it.
>
> Could we get the two driver changes and the function removal merged all
> together ?

Sure, I will send it.

>
>> But will wait for some more feedback before concluding on this.
>>
>> Dave/ Daniel, Would you like to add any feedback ?
>>
>> >>>>> And from git grep it seems that there are very few drivers using it
>> >>>>> at the moment, so not sure if introducing new API functions similar to
>> >>>>> drm_mode_config will make sense or not.
>> >>>>
>> >>>> https://www.kernel.org/doc/html/latest/gpu/todo.html
>> >>>>
>> >>>> It was picked up from TODO list after discussing with Daniel.
>> >>>>
>> >>>>>> -               drm_kms_helper_poll_enable(rcdu->ddev);
>> >>>>>> -               return PTR_ERR(state);
>> >>>>>> -       }
>> >>>>>> -
>> >>>>>> -       rcdu->suspend_state = state;
>> >>>
>> >>> Additionally, I think you can remove the suspend_state field from the
>> >>> rcdu structure.
>> >>
>> >> Sure, I will remove it in v2.
>> >>
>> >>>>>> -       return 0;
>> >>>>>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
>> >>>>>>  }
>> >>>>>>
>> >>>>>>  static int rcar_du_pm_resume(struct device *dev)
>> >>>>>>  {
>> >>>>>>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
>> >>>>>>
>> >>>>>> -       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;
>> >>>>>> +       return drm_mode_config_helper_resume(rcdu->ddev);
>> >>>>>>  }
>> >>>>>>  #endif
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Laurent Pinchart]

Hi Souptick,

On Monday, 30 July 2018 18:13:13 EEST Souptick Joarder wrote:
> On Mon, Jul 30, 2018 at 7:54 PM, Laurent Pinchart wrote:
> > On Monday, 30 July 2018 16:58:09 EEST Souptick Joarder wrote:
> >> On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart wrote:
> >>> On Saturday, 28 July 2018 21:50:58 EEST Souptick Joarder wrote:
> >>>> On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar wrote:
> >>>>> On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder wrote:
> >>>>>> convert drm_atomic_helper_suspend/resume() to use
> >>>>>> drm_mode_config_helper_suspend/resume().
> >>>>> 
> >>>>> Hi Souptick,
> >>>>> 
> >>>>> Thanks for your patch.
> >>>>> 
> >>>>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> >>>>>> Signed-off-by: Ajit Negi <ajitn.linux@gmail.com>
> >>>>>> ---
> >>>>>> 
> >>>>>>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> >>>>>>  1 file changed, 2 insertions(+), 19 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f
> >>>>>> 100644
> >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>>>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct
> >>>>>> drm_device *dev)
> >>>>>> 
> >>>>>>  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);
> >>>>>> -       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);
> >>>>> 
> >>>>> I don't think we can use drm_mode_config_helper_(suspend/resume)
> >>>>> API here as this file uses CMA functions.
> >>>> 
> >>>> drm_fbdev_cma_set_suspend_unlocked() is wrapper function which
> >>>> invokes drm_fb_helper_set_suspend_unlocked().
> >>>> 
> >>>> Where the new API drm_mode_config_helper_suspend/resume() directly
> >>>> invokes drm_fb_helper_set_suspend_unlocked(). So it is safe to replace
> >>>> exiting code with API drm_mode_config_helper_suspend/resume().
> >>> 
> >>> I agree that they're functionally equivalent for now, but what if
> >>> drm_fbdev_cma_set_suspend_unlocked() gets extended later ? This change
> >>> risks introducing a breakage that could could unnoticed at that point.
> >> 
> >> No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have
> >> any impact on driver because with this patch we will be retaining the
> >> original suspend/resume logic of the rcar-du driver and further this
> >> driver is not going to use drm_fbdev_cma_set_suspend_unlocked().
> > 
> > My point is that if the fb cma helpers gets later extended with a feature
> > that need special handling and suspend/resume time, with the
> > drm_fbdev_cma_set_suspend_unlocked() function properly updated to take
> > that feature into account, driver using those helpers but converted to
> > drm_atomic_helper_suspend/resume() will break.
> > 
> >>> At the very
> >>> least you should add a comment in drm_fbdev_cma_set_suspend_unlocked()
> >>> to explain that any extension of the function should also address all
> >>> drivers using drm_mode_config_helper_suspend() and
> >>> drm_mode_config_helper_resume().
> >> 
> >> The consumers of drm_fbdev_cma_set_suspend_unlocked() are -
> >> drivers/gpu/drm/arm/hdlcd_drv.c
> >> drivers/gpu/drm/drm_fb_cma_helper.c
> >> 
> >> and both will be converted to use API
> >> drm_mode_config_helper_suspend/resume(). As there will be no more
> >> consumer of drm_fbdev_cma_set_suspend_unlocked() , we can remove this
> >> wrapper API forever :)
> > 
> > OK, if you remove the function completely then anyone wanting to extend
> > the fbdev cma helpers in the way described above will notice that
> > something will need to be done, so it's fine. Please thus make sure that
> > you go all the way to removing that function.
> 
> Sure, once both the drivers are converted to use
> drm_mode_config_helper_suspend/resume()
> and goes into linus's tree, then we can remove it.

Could we get the two driver changes and the function removal merged all 
together ?

> But will wait for some more feedback before concluding on this.
> 
> Dave/ Daniel, Would you like to add any feedback ?
> 
> >>>>> And from git grep it seems that there are very few drivers using it
> >>>>> at the moment, so not sure if introducing new API functions similar to
> >>>>> drm_mode_config will make sense or not.
> >>>> 
> >>>> https://www.kernel.org/doc/html/latest/gpu/todo.html
> >>>> 
> >>>> It was picked up from TODO list after discussing with Daniel.
> >>>> 
> >>>>>> -               drm_kms_helper_poll_enable(rcdu->ddev);
> >>>>>> -               return PTR_ERR(state);
> >>>>>> -       }
> >>>>>> -
> >>>>>> -       rcdu->suspend_state = state;
> >>> 
> >>> Additionally, I think you can remove the suspend_state field from the
> >>> rcdu structure.
> >> 
> >> Sure, I will remove it in v2.
> >> 
> >>>>>> -       return 0;
> >>>>>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> >>>>>>  }
> >>>>>>  
> >>>>>>  static int rcar_du_pm_resume(struct device *dev)
> >>>>>>  {
> >>>>>>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> >>>>>> 
> >>>>>> -       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;
> >>>>>> +       return drm_mode_config_helper_resume(rcdu->ddev);
> >>>>>>  }
> >>>>>>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Souptick Joarder]

On Mon, Jul 30, 2018 at 7:54 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Souptick,
>
> On Monday, 30 July 2018 16:58:09 EEST Souptick Joarder wrote:
>> On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart wrote:
>> > On Saturday, 28 July 2018 21:50:58 EEST Souptick Joarder wrote:
>> >> On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar wrote:
>> >>> On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder wrote:
>> >>>> convert drm_atomic_helper_suspend/resume() to use
>> >>>> drm_mode_config_helper_suspend/resume().
>> >>>
>> >>> Hi Souptick,
>> >>>
>> >>> Thanks for your patch.
>> >>>
>> >>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>> >>>> Signed-off-by: Ajit Negi <ajitn.linux@gmail.com>
>> >>>> ---
>> >>>>
>> >>>>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
>> >>>>  1 file changed, 2 insertions(+), 19 deletions(-)
>> >>>>
>> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
>> >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >>>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
>> >>>> *dev)
>> >>>>
>> >>>>  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);
>> >>>> -       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);
>> >>>
>> >>> I don't think we can use drm_mode_config_helper_(suspend/resume)
>> >>> API here as this file uses CMA functions.
>> >>
>> >> drm_fbdev_cma_set_suspend_unlocked() is wrapper function which
>> >> invokes drm_fb_helper_set_suspend_unlocked().
>> >>
>> >> Where the new API drm_mode_config_helper_suspend/resume() directly
>> >> invokes
>> >> drm_fb_helper_set_suspend_unlocked(). So it is safe to replace exiting
>> >> code with API drm_mode_config_helper_suspend/resume().
>> >
>> > I agree that they're functionally equivalent for now, but what if
>> > drm_fbdev_cma_set_suspend_unlocked() gets extended later ? This change
>> > risks introducing a breakage that could could unnoticed at that point.
>>
>> No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have
>> any impact on driver because with this patch we will be retaining the
>> original suspend/resume logic of the rcar-du driver and further this driver
>> is not going to use drm_fbdev_cma_set_suspend_unlocked().
>
> My point is that if the fb cma helpers gets later extended with a feature that
> need special handling and suspend/resume time, with the
> drm_fbdev_cma_set_suspend_unlocked() function properly updated to take that
> feature into account, driver using those helpers but converted to
> drm_atomic_helper_suspend/resume() will break.
>
>> > At the very
>> > least you should add a comment in drm_fbdev_cma_set_suspend_unlocked() to
>> > explain that any extension of the function should also address all drivers
>> > using drm_mode_config_helper_suspend() and
>> > drm_mode_config_helper_resume().
>>
>> The consumers of drm_fbdev_cma_set_suspend_unlocked() are -
>> drivers/gpu/drm/arm/hdlcd_drv.c
>> drivers/gpu/drm/drm_fb_cma_helper.c
>>
>> and both will be converted to use API
>> drm_mode_config_helper_suspend/resume(). As there will be no more consumer
>> of drm_fbdev_cma_set_suspend_unlocked() , we can remove this wrapper API
>> forever :)
>
> OK, if you remove the function completely then anyone wanting to extend the
> fbdev cma helpers in the way described above will notice that something will
> need to be done, so it's fine. Please thus make sure that you go all the way
> to removing that function.
>

Sure, once both the drivers are converted to use
drm_mode_config_helper_suspend/resume()
and goes into linus's tree, then we can remove it.

But will wait for some more feedback before concluding on this.

Dave/ Daniel, Would you like to add any feedback ?

>> >>> And from git grep it seems that there are very few drivers using it at
>> >>> the moment, so not sure if introducing new API functions similar to
>> >>> drm_mode_config will make sense or not.
>> >>
>> >> https://www.kernel.org/doc/html/latest/gpu/todo.html
>> >>
>> >> It was picked up from TODO list after discussing with
>> >> Daniel.
>> >>
>> >> >> -               drm_kms_helper_poll_enable(rcdu->ddev);
>> >> >> -               return PTR_ERR(state);
>> >> >> -       }
>> >> >> -
>> >> >> -       rcdu->suspend_state = state;
>> >
>> > Additionally, I think you can remove the suspend_state field from the rcdu
>> > structure.
>>
>> Sure, I will remove it in v2.
>>
>> >>>> -       return 0;
>> >>>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
>> >>>>  }
>> >>>>
>> >>>>  static int rcar_du_pm_resume(struct device *dev)
>> >>>>  {
>> >>>>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
>> >>>>
>> >>>> -       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;
>> >>>> +       return drm_mode_config_helper_resume(rcdu->ddev);
>> >>>>  }
>> >>>>  #endif
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Laurent Pinchart]

Hi Souptick,

On Monday, 30 July 2018 16:58:09 EEST Souptick Joarder wrote:
> On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart wrote:
> > On Saturday, 28 July 2018 21:50:58 EEST Souptick Joarder wrote:
> >> On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar wrote:
> >>> On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder wrote:
> >>>> convert drm_atomic_helper_suspend/resume() to use
> >>>> drm_mode_config_helper_suspend/resume().
> >>> 
> >>> Hi Souptick,
> >>> 
> >>> Thanks for your patch.
> >>> 
> >>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> >>>> Signed-off-by: Ajit Negi <ajitn.linux@gmail.com>
> >>>> ---
> >>>> 
> >>>>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> >>>>  1 file changed, 2 insertions(+), 19 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >>>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> >>>> *dev)
> >>>> 
> >>>>  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);
> >>>> -       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);
> >>> 
> >>> I don't think we can use drm_mode_config_helper_(suspend/resume)
> >>> API here as this file uses CMA functions.
> >> 
> >> drm_fbdev_cma_set_suspend_unlocked() is wrapper function which
> >> invokes drm_fb_helper_set_suspend_unlocked().
> >> 
> >> Where the new API drm_mode_config_helper_suspend/resume() directly
> >> invokes
> >> drm_fb_helper_set_suspend_unlocked(). So it is safe to replace exiting
> >> code with API drm_mode_config_helper_suspend/resume().
> > 
> > I agree that they're functionally equivalent for now, but what if
> > drm_fbdev_cma_set_suspend_unlocked() gets extended later ? This change
> > risks introducing a breakage that could could unnoticed at that point.
> 
> No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have
> any impact on driver because with this patch we will be retaining the
> original suspend/resume logic of the rcar-du driver and further this driver
> is not going to use drm_fbdev_cma_set_suspend_unlocked().

My point is that if the fb cma helpers gets later extended with a feature that 
need special handling and suspend/resume time, with the 
drm_fbdev_cma_set_suspend_unlocked() function properly updated to take that 
feature into account, driver using those helpers but converted to 
drm_atomic_helper_suspend/resume() will break.

> > At the very
> > least you should add a comment in drm_fbdev_cma_set_suspend_unlocked() to
> > explain that any extension of the function should also address all drivers
> > using drm_mode_config_helper_suspend() and
> > drm_mode_config_helper_resume().
> 
> The consumers of drm_fbdev_cma_set_suspend_unlocked() are -
> drivers/gpu/drm/arm/hdlcd_drv.c
> drivers/gpu/drm/drm_fb_cma_helper.c
> 
> and both will be converted to use API
> drm_mode_config_helper_suspend/resume(). As there will be no more consumer
> of drm_fbdev_cma_set_suspend_unlocked() , we can remove this wrapper API
> forever :)

OK, if you remove the function completely then anyone wanting to extend the 
fbdev cma helpers in the way described above will notice that something will 
need to be done, so it's fine. Please thus make sure that you go all the way 
to removing that function.

> >>> And from git grep it seems that there are very few drivers using it at
> >>> the moment, so not sure if introducing new API functions similar to
> >>> drm_mode_config will make sense or not.
> >> 
> >> https://www.kernel.org/doc/html/latest/gpu/todo.html
> >> 
> >> It was picked up from TODO list after discussing with
> >> Daniel.
> >> 
> >> >> -               drm_kms_helper_poll_enable(rcdu->ddev);
> >> >> -               return PTR_ERR(state);
> >> >> -       }
> >> >> -
> >> >> -       rcdu->suspend_state = state;
> > 
> > Additionally, I think you can remove the suspend_state field from the rcdu
> > structure.
> 
> Sure, I will remove it in v2.
> 
> >>>> -       return 0;
> >>>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> >>>>  }
> >>>>  
> >>>>  static int rcar_du_pm_resume(struct device *dev)
> >>>>  {
> >>>>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> >>>> 
> >>>> -       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;
> >>>> +       return drm_mode_config_helper_resume(rcdu->ddev);
> >>>>  }
> >>>>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Souptick Joarder]

On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Souptick,
>
> Thank you for the patch.
>
> On Saturday, 28 July 2018 21:50:58 EEST Souptick Joarder wrote:
>> On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar wrote:
>> > On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder wrote:
>> >> convert drm_atomic_helper_suspend/resume() to use
>> >> drm_mode_config_helper_suspend/resume().
>> >
>> > Hi Souptick,
>> >
>> > Thanks for your patch.
>> >
>> >> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>> >> Signed-off-by: Ajit Negi <ajitn.linux@gmail.com>
>> >> ---
>> >>
>> >>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
>> >>  1 file changed, 2 insertions(+), 19 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
>> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> >> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
>> >> *dev)
>> >>
>> >>  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);
>> >> -       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);
>> >
>> > I don't think we can use drm_mode_config_helper_(suspend/resume)
>> > API here as this file uses CMA functions.
>>
>> drm_fbdev_cma_set_suspend_unlocked() is wrapper function which
>> invokes drm_fb_helper_set_suspend_unlocked().
>>
>> Where the new API drm_mode_config_helper_suspend/resume() directly invokes
>> drm_fb_helper_set_suspend_unlocked(). So it is safe to replace exiting
>> code with API drm_mode_config_helper_suspend/resume().
>

> I agree that they're functionally equivalent for now, but what if
> drm_fbdev_cma_set_suspend_unlocked() gets extended later ? This change risks
> introducing a breakage that could could unnoticed at that point.

No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have
any impact on driver because with this patch we will be retaining the original
suspend/resume logic of the rcar-du driver and further this driver is not going
to use drm_fbdev_cma_set_suspend_unlocked().


> At the very
> least you should add a comment in drm_fbdev_cma_set_suspend_unlocked() to
> explain that any extension of the function should also address all drivers
> using drm_mode_config_helper_suspend() and drm_mode_config_helper_resume().

The consumers of drm_fbdev_cma_set_suspend_unlocked() are -
drivers/gpu/drm/arm/hdlcd_drv.c
drivers/gpu/drm/drm_fb_cma_helper.c

and both will be converted to use API drm_mode_config_helper_suspend/resume().
As there will be no more consumer of drm_fbdev_cma_set_suspend_unlocked() , we
can remove this wrapper API forever :)

>
>> > And from git grep it seems that there are very few drivers using it at the
>> > moment, so not sure if introducing new API functions similar to
>> > drm_mode_config will make sense or not.
>>
>> https://www.kernel.org/doc/html/latest/gpu/todo.html
>>
>> It was picked up from TODO list after discussing with
>> Daniel.
>>
>> > Thanks.
>> >
>> >> -               drm_kms_helper_poll_enable(rcdu->ddev);
>> >> -               return PTR_ERR(state);
>> >> -       }
>> >> -
>> >> -       rcdu->suspend_state = state;
>
> Additionally, I think you can remove the suspend_state field from the rcdu
> structure.

Sure, I will remove it in v2.

>
>> >> -       return 0;
>> >> +       return drm_mode_config_helper_suspend(rcdu->ddev);
>> >>  }
>> >>
>> >>  static int rcar_du_pm_resume(struct device *dev)
>> >>  {
>> >>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
>> >>
>> >> -       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;
>> >> +       return drm_mode_config_helper_resume(rcdu->ddev);
>> >>  }
>> >>  #endif
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Laurent Pinchart]

Hi Souptick,

Thank you for the patch.

On Saturday, 28 July 2018 21:50:58 EEST Souptick Joarder wrote:
> On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar wrote:
> > On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder wrote:
> >> convert drm_atomic_helper_suspend/resume() to use
> >> drm_mode_config_helper_suspend/resume().
> > 
> > Hi Souptick,
> > 
> > Thanks for your patch.
> > 
> >> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> >> Signed-off-by: Ajit Negi <ajitn.linux@gmail.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
> >>  1 file changed, 2 insertions(+), 19 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device
> >> *dev)
> >> 
> >>  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);
> >> -       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);
> > 
> > I don't think we can use drm_mode_config_helper_(suspend/resume)
> > API here as this file uses CMA functions.
> 
> drm_fbdev_cma_set_suspend_unlocked() is wrapper function which
> invokes drm_fb_helper_set_suspend_unlocked().
> 
> Where the new API drm_mode_config_helper_suspend/resume() directly invokes
> drm_fb_helper_set_suspend_unlocked(). So it is safe to replace exiting
> code with API drm_mode_config_helper_suspend/resume().

I agree that they're functionally equivalent for now, but what if 
drm_fbdev_cma_set_suspend_unlocked() gets extended later ? This change risks 
introducing a breakage that could could unnoticed at that point. At the very 
least you should add a comment in drm_fbdev_cma_set_suspend_unlocked() to 
explain that any extension of the function should also address all drivers 
using drm_mode_config_helper_suspend() and drm_mode_config_helper_resume().

> > And from git grep it seems that there are very few drivers using it at the
> > moment, so not sure if introducing new API functions similar to 
> > drm_mode_config will make sense or not.
> 
> https://www.kernel.org/doc/html/latest/gpu/todo.html
> 
> It was picked up from TODO list after discussing with
> Daniel.
> 
> > Thanks.
> > 
> >> -               drm_kms_helper_poll_enable(rcdu->ddev);
> >> -               return PTR_ERR(state);
> >> -       }
> >> -
> >> -       rcdu->suspend_state = state;

Additionally, I think you can remove the suspend_state field from the rcdu 
structure.

> >> -       return 0;
> >> +       return drm_mode_config_helper_suspend(rcdu->ddev);
> >>  }
> >>  
> >>  static int rcar_du_pm_resume(struct device *dev)
> >>  {
> >>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> >> 
> >> -       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;
> >> +       return drm_mode_config_helper_resume(rcdu->ddev);
> >>  }
> >>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Souptick Joarder]

On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar
<vthakkar@vaishalithakkar.in> wrote:
> On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
>> convert drm_atomic_helper_suspend/resume() to use
>> drm_mode_config_helper_suspend/resume().
>
> Hi Souptick,
>
> Thanks for your patch.
>
>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>> Signed-off-by: Ajit Negi <ajitn.linux@gmail.com>
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
>>  1 file changed, 2 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> index 02aee6c..288220f 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device *dev)
>>  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);
>> -       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);
>

> I don't think we can use drm_mode_config_helper_(suspend/resume)
> API here as this file uses CMA functions.

drm_fbdev_cma_set_suspend_unlocked() is wrapper function which
invokes drm_fb_helper_set_suspend_unlocked().

Where the new API drm_mode_config_helper_suspend/resume() directly invokes
drm_fb_helper_set_suspend_unlocked(). So it is safe to replace exiting
code with API drm_mode_config_helper_suspend/resume().


> And from git grep it seems that
> there are very few drivers using it at the moment, so not sure if introducing
> new API functions similar to  drm_mode_config will make sense or not.
>

https://www.kernel.org/doc/html/latest/gpu/todo.html

It was picked up from TODO list after discussing with
Daniel.


> Thanks.
>
>> -               drm_kms_helper_poll_enable(rcdu->ddev);
>> -               return PTR_ERR(state);
>> -       }
>> -
>> -       rcdu->suspend_state = state;
>> -
>> -       return 0;
>> +       return drm_mode_config_helper_suspend(rcdu->ddev);
>>  }
>>
>>  static int rcar_du_pm_resume(struct device *dev)
>>  {
>>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
>>
>> -       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;
>> +       return drm_mode_config_helper_resume(rcdu->ddev);
>>  }
>>  #endif
>>
>> --
>> 1.9.1
>>

Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Vaishali Thakkar]

On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
> convert drm_atomic_helper_suspend/resume() to use
> drm_mode_config_helper_suspend/resume().

Hi Souptick,

Thanks for your patch.

> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Signed-off-by: Ajit Negi <ajitn.linux@gmail.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index 02aee6c..288220f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device *dev)
>  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);
> -       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);

I don't think we can use drm_mode_config_helper_(suspend/resume)
API here as this file uses CMA functions. And from git grep it seems that
there are very few drivers using it at the moment, so not sure if introducing
new API functions similar to  drm_mode_config will make sense or not.

Thanks.

> -               drm_kms_helper_poll_enable(rcdu->ddev);
> -               return PTR_ERR(state);
> -       }
> -
> -       rcdu->suspend_state = state;
> -
> -       return 0;
> +       return drm_mode_config_helper_suspend(rcdu->ddev);
>  }
>
>  static int rcar_du_pm_resume(struct device *dev)
>  {
>         struct rcar_du_device *rcdu = dev_get_drvdata(dev);
>
> -       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;
> +       return drm_mode_config_helper_resume(rcdu->ddev);
>  }
>  #endif
>
> --
> 1.9.1
>

[PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[Souptick Joarder]

convert drm_atomic_helper_suspend/resume() to use
drm_mode_config_helper_suspend/resume().

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Signed-off-by: Ajit Negi <ajitn.linux@gmail.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 02aee6c..288220f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device *dev)
 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);
-	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;
+	return drm_mode_config_helper_suspend(rcdu->ddev);
 }
 
 static int rcar_du_pm_resume(struct device *dev)
 {
 	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
 
-	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;
+	return drm_mode_config_helper_resume(rcdu->ddev);
 }
 #endif
 
-- 
1.9.1