| Message ID | 20180918163903.GA11172@jordon-HP-15-Notebook-PC (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/rcar-du: Convert drm_atomic_helper_suspend/resume() | expand |
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 >
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);
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);
On 28-Sep-2018 9:00 PM, "Laurent Pinchart" < laurent.pinchart@ideasonboard.com> 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). 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 ? > > > >> --- > > >> > > >> 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 > > > <p dir="ltr"></p> <p dir="ltr">On 28-Sep-2018 9:00 PM, "Laurent Pinchart" <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br> ><br> > Hi Souptick,<br> ><br> > On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote:<br> > > On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote:<br> > > > On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote:<br> > > >> convert drm_atomic_helper_suspend/resume() to use<br> > > >> drm_mode_config_helper_suspend/resume().<br> > > >> <br> > > >> remove suspend_state field from the rcar_du_device<br> > > >> structure as it is no more required.<br> > > >> <br> > > >> With this conversion, also drm_fbdev_cma_set_suspend_unlocked()<br> > > >> will left with no consumer. So this function can be removed.<br> > > >> <br> > > >> Signed-off-by: Souptick Joarder <<a href="mailto:jrdr.linux@gmail.com">jrdr.linux@gmail.com</a>><br> > > > <br> > > > Laurent, any comment on this patch ??<br> > > <br> > > Sorry for the delay, and thanks for pinging me.<br> > > <br> > > Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>><br> > > <br> > > Should I apply this to my tree or do you plan to merge it through drm-misc<br> > > as it touches drm_fb_cma_helper.c ?<br> ><br> > I just realized that the same patch got supplied by Noralf Trønnes nearly a <br> > year ago, and was later superseded by <a href="https://patchwork.freedesktop.org/patch/">https://patchwork.freedesktop.org/patch/</a><br> > 247861/. I think we should thus apply Noralf's patches instead (once he sends <br> > v4 out).</p> <p dir="ltr">Sure, you can go ahead with his patch.</p> <p dir="ltr">Shall I remove drm_fbdev_cma_set_suspend_unlocked in a separate patch once all the consumer remove this function ?<br> ><br> > > >> ---<br> > > >> <br> > > >> drivers/gpu/drm/drm_fb_cma_helper.c | 18 ------------------<br> > > >> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++-------------------<br> > > >> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 -<br> > > >> include/drm/drm_fb_cma_helper.h | 2 --<br> > > >> 4 files changed, 2 insertions(+), 40 deletions(-)<br> > > >> <br> > > >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c<br> > > >> b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f..96efc88 100644<br> > > >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c<br> > > >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c<br> > > >> @@ -224,21 +224,3 @@ void drm_fbdev_cma_hotplug_event(struct<br> > > >> drm_fbdev_cma *fbdev_cma)<br> > > >> drm_fb_helper_hotplug_event(&fbdev_cma->fb_helper);<br> > > >> }<br> > > >> EXPORT_SYMBOL_GPL(drm_fbdev_cma_hotplug_event);<br> > > >> <br> > > >> -<br> > > >> -/**<br> > > >> - * drm_fbdev_cma_set_suspend_unlocked - wrapper around<br> > > >> - *<br> > > >> drm_fb_helper_set_suspend_unlocked<br> > > >> - * @fbdev_cma: The drm_fbdev_cma struct, may be NULL<br> > > >> - * @state: desired state, zero to resume, non-zero to suspend<br> > > >> - *<br> > > >> - * Calls drm_fb_helper_set_suspend, which is a wrapper around<br> > > >> - * fb_set_suspend implemented by fbdev core.<br> > > >> - */<br> > > >> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma<br> > > >> *fbdev_cma,<br> > > >> - bool state)<br> > > >> -{<br> > > >> - if (fbdev_cma)<br> > > >> - <br> > > >> drm_fb_helper_set_suspend_unlocked(&fbdev_cma->fb_helper,<br> > > >> - state);<br> > > >> -}<br> > > >> -EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);<br> > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c<br> > > >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644<br> > > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c<br> > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c<br> > > >> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device<br> > > >> *dev)<br> > > >> <br> > > >> static int rcar_du_pm_suspend(struct device *dev)<br> > > >> {<br> > > >> struct rcar_du_device *rcdu = dev_get_drvdata(dev);<br> > > >> - struct drm_atomic_state *state;<br> > > >> <br> > > >> - drm_kms_helper_poll_disable(rcdu->ddev);<br> > > >> - drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, true);<br> > > >> -<br> > > >> - state = drm_atomic_helper_suspend(rcdu->ddev);<br> > > >> - if (IS_ERR(state)) {<br> > > >> - drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);<br> > > >> - drm_kms_helper_poll_enable(rcdu->ddev);<br> > > >> - return PTR_ERR(state);<br> > > >> - }<br> > > >> -<br> > > >> - rcdu->suspend_state = state;<br> > > >> -<br> > > >> - return 0;<br> > > >> + return drm_mode_config_helper_suspend(rcdu->ddev);<br> > > >> }<br> > > >> <br> > > >> static int rcar_du_pm_resume(struct device *dev)<br> > > >> {<br> > > >> struct rcar_du_device *rcdu = dev_get_drvdata(dev);<br> > > >> <br> > > >> - drm_atomic_helper_resume(rcdu->ddev, rcdu->suspend_state);<br> > > >> - drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);<br> > > >> - drm_kms_helper_poll_enable(rcdu->ddev);<br> > > >> -<br> > > >> - return 0;<br> > > >> + return drm_mode_config_helper_resume(rcdu->ddev);<br> > > >> }<br> > > >> #endif<br> > > >> <br> > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h<br> > > >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..ff25c8d 100644<br> > > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h<br> > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h<br> > > >> @@ -78,7 +78,6 @@ struct rcar_du_device {<br> > > >> struct drm_device *ddev;<br> > > >> struct drm_fbdev_cma *fbdev;<br> > > >> <br> > > >> - struct drm_atomic_state *suspend_state;<br> > > >> <br> > > >> struct rcar_du_crtc crtcs[RCAR_DU_MAX_CRTCS];<br> > > >> unsigned int num_crtcs;<br> > > >> diff --git a/include/drm/drm_fb_cma_helper.h<br> > > >> b/include/drm/drm_fb_cma_helper.h index 4a65f0d..8dbbe1e 100644<br> > > >> --- a/include/drm/drm_fb_cma_helper.h<br> > > >> +++ b/include/drm/drm_fb_cma_helper.h<br> > > >> @@ -26,8 +26,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct<br> > > >> drm_device *dev,<br> > > >> <br> > > >> void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);<br> > > >> void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);<br> > > >> -void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma<br> > > >> *fbdev_cma,<br> > > >> - bool state);<br> > > >> <br> > > >> struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct<br> > > >> drm_framebuffer *fb,<br> > > >> unsigned int plane);<br> ><br> > -- <br> > Regards,<br> ><br> > Laurent Pinchart<br> ><br> ><br> ></p>
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.
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 > > >
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);
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);
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); >
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); > >
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
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
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(-)
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);
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(-)