* [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() @ 2018-09-18 16:39 Souptick Joarder 2018-09-27 6:34 ` Souptick Joarder 0 siblings, 1 reply; 22+ messages in thread From: Souptick Joarder @ 2018-09-18 16:39 UTC (permalink / raw) To: gustavo, maarten.lankhorst, sean, airlied, laurent.pinchart, daniel Cc: dri-devel, linux-kernel, linux-renesas-soc 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-09-18 16:39 [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() Souptick Joarder @ 2018-09-27 6:34 ` Souptick Joarder 2018-09-28 15:05 ` Laurent Pinchart 0 siblings, 1 reply; 22+ messages in thread From: Souptick Joarder @ 2018-09-27 6:34 UTC (permalink / raw) To: Gustavo Padovan, Maarten Lankhorst, sean, airlied, Laurent Pinchart, Daniel Vetter Cc: dri-devel, linux-kernel, linux-renesas-soc 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 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-09-27 6:34 ` Souptick Joarder @ 2018-09-28 15:05 ` Laurent Pinchart 2018-09-28 15:30 ` Laurent Pinchart 0 siblings, 1 reply; 22+ messages in thread From: Laurent Pinchart @ 2018-09-28 15:05 UTC (permalink / raw) To: Souptick Joarder Cc: Gustavo Padovan, Maarten Lankhorst, sean, airlied, Daniel Vetter, dri-devel, linux-kernel, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-09-28 15:05 ` Laurent Pinchart @ 2018-09-28 15:30 ` Laurent Pinchart [not found] ` <CAFqt6zbx2tSpnE+reNLN4pC7Br5XbgSuYP4-taxyyWraVpHoMg@mail.gmail.com> 2018-10-01 6:52 ` Daniel Vetter 0 siblings, 2 replies; 22+ messages in thread From: Laurent Pinchart @ 2018-09-28 15:30 UTC (permalink / raw) To: Souptick Joarder Cc: Gustavo Padovan, Maarten Lankhorst, sean, airlied, Daniel Vetter, dri-devel, linux-kernel, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <CAFqt6zbx2tSpnE+reNLN4pC7Br5XbgSuYP4-taxyyWraVpHoMg@mail.gmail.com>]
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() [not found] ` <CAFqt6zbx2tSpnE+reNLN4pC7Br5XbgSuYP4-taxyyWraVpHoMg@mail.gmail.com> @ 2018-09-30 6:57 ` Laurent Pinchart 0 siblings, 0 replies; 22+ messages in thread From: Laurent Pinchart @ 2018-09-30 6:57 UTC (permalink / raw) To: Souptick Joarder Cc: linux-renesas-soc, linux-kernel, Maarten Lankhorst, Gustavo Padovan, Daniel Vetter, airlied, sean, dri-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-09-28 15:30 ` Laurent Pinchart [not found] ` <CAFqt6zbx2tSpnE+reNLN4pC7Br5XbgSuYP4-taxyyWraVpHoMg@mail.gmail.com> @ 2018-10-01 6:52 ` Daniel Vetter 2018-10-01 11:56 ` Laurent Pinchart 1 sibling, 1 reply; 22+ messages in thread From: Daniel Vetter @ 2018-10-01 6:52 UTC (permalink / raw) To: Laurent Pinchart Cc: Souptick Joarder, Gustavo Padovan, Maarten Lankhorst, sean, airlied, Daniel Vetter, dri-devel, linux-kernel, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-10-01 6:52 ` Daniel Vetter @ 2018-10-01 11:56 ` Laurent Pinchart 2018-10-01 12:42 ` Noralf Trønnes 0 siblings, 1 reply; 22+ messages in thread From: Laurent Pinchart @ 2018-10-01 11:56 UTC (permalink / raw) To: Daniel Vetter Cc: Souptick Joarder, Gustavo Padovan, Maarten Lankhorst, sean, airlied, dri-devel, linux-kernel, linux-renesas-soc, Noralf Trønnes 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-10-01 11:56 ` Laurent Pinchart @ 2018-10-01 12:42 ` Noralf Trønnes 2018-10-08 16:27 ` Souptick Joarder 0 siblings, 1 reply; 22+ messages in thread From: Noralf Trønnes @ 2018-10-01 12:42 UTC (permalink / raw) To: Laurent Pinchart, Daniel Vetter Cc: Souptick Joarder, Gustavo Padovan, Maarten Lankhorst, sean, airlied, dri-devel, linux-kernel, linux-renesas-soc 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); ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-10-01 12:42 ` Noralf Trønnes @ 2018-10-08 16:27 ` Souptick Joarder 2018-10-11 8:18 ` Daniel Vetter 0 siblings, 1 reply; 22+ messages in thread From: Souptick Joarder @ 2018-10-08 16:27 UTC (permalink / raw) To: noralf Cc: Laurent Pinchart, Daniel Vetter, Gustavo Padovan, Maarten Lankhorst, sean, airlied, dri-devel, linux-kernel, linux-renesas-soc 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); > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-10-08 16:27 ` Souptick Joarder @ 2018-10-11 8:18 ` Daniel Vetter 2018-10-22 8:21 ` Souptick Joarder 0 siblings, 1 reply; 22+ messages in thread From: Daniel Vetter @ 2018-10-11 8:18 UTC (permalink / raw) To: Souptick Joarder Cc: noralf, Laurent Pinchart, Daniel Vetter, Gustavo Padovan, Maarten Lankhorst, sean, airlied, dri-devel, linux-kernel, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-10-11 8:18 ` Daniel Vetter @ 2018-10-22 8:21 ` Souptick Joarder 2018-10-23 13:40 ` Daniel Vetter 0 siblings, 1 reply; 22+ messages in thread From: Souptick Joarder @ 2018-10-22 8:21 UTC (permalink / raw) To: noralf, Laurent Pinchart, Gustavo Padovan, Maarten Lankhorst, sean, airlied, dri-devel, linux-kernel, linux-renesas-soc Cc: Daniel Vetter 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-10-22 8:21 ` Souptick Joarder @ 2018-10-23 13:40 ` Daniel Vetter 2018-10-23 14:15 ` Laurent Pinchart 0 siblings, 1 reply; 22+ messages in thread From: Daniel Vetter @ 2018-10-23 13:40 UTC (permalink / raw) To: Souptick Joarder Cc: noralf, Laurent Pinchart, Gustavo Padovan, Maarten Lankhorst, sean, airlied, dri-devel, linux-kernel, linux-renesas-soc, 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-10-23 13:40 ` Daniel Vetter @ 2018-10-23 14:15 ` Laurent Pinchart 0 siblings, 0 replies; 22+ messages in thread From: Laurent Pinchart @ 2018-10-23 14:15 UTC (permalink / raw) To: Daniel Vetter Cc: Souptick Joarder, noralf, Gustavo Padovan, Maarten Lankhorst, sean, airlied, dri-devel, linux-kernel, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() @ 2018-07-28 15:40 Souptick Joarder 2018-07-28 17:50 ` Vaishali Thakkar 0 siblings, 1 reply; 22+ messages in thread From: Souptick Joarder @ 2018-07-28 15:40 UTC (permalink / raw) To: laurent.pinchart, airlied, ajitn.linux Cc: dri-devel, linux-renesas-soc, linux-kernel, daniel 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-07-28 15:40 Souptick Joarder @ 2018-07-28 17:50 ` Vaishali Thakkar 2018-07-28 18:50 ` Souptick Joarder 0 siblings, 1 reply; 22+ messages in thread From: Vaishali Thakkar @ 2018-07-28 17:50 UTC (permalink / raw) To: Souptick Joarder Cc: Laurent Pinchart, airlied, ajitn.linux, dri-devel, linux-renesas-soc, Linux Kernel Mailing List, Daniel Vetter 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 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-07-28 17:50 ` Vaishali Thakkar @ 2018-07-28 18:50 ` Souptick Joarder 2018-07-28 20:20 ` Laurent Pinchart 0 siblings, 1 reply; 22+ messages in thread From: Souptick Joarder @ 2018-07-28 18:50 UTC (permalink / raw) To: Vaishali Thakkar Cc: Laurent Pinchart, airlied, Ajit Linux, dri-devel, linux-renesas-soc, Linux Kernel Mailing List, Daniel Vetter 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 >> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-07-28 18:50 ` Souptick Joarder @ 2018-07-28 20:20 ` Laurent Pinchart 2018-07-30 13:58 ` Souptick Joarder 0 siblings, 1 reply; 22+ messages in thread From: Laurent Pinchart @ 2018-07-28 20:20 UTC (permalink / raw) To: Souptick Joarder Cc: Vaishali Thakkar, airlied, Ajit Linux, dri-devel, linux-renesas-soc, Linux Kernel Mailing List, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-07-28 20:20 ` Laurent Pinchart @ 2018-07-30 13:58 ` Souptick Joarder 2018-07-30 14:24 ` Laurent Pinchart 0 siblings, 1 reply; 22+ messages in thread From: Souptick Joarder @ 2018-07-30 13:58 UTC (permalink / raw) To: Laurent Pinchart Cc: Vaishali Thakkar, airlied, Ajit Linux, dri-devel, linux-renesas-soc, Linux Kernel Mailing List, Daniel Vetter 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 > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-07-30 13:58 ` Souptick Joarder @ 2018-07-30 14:24 ` Laurent Pinchart 2018-07-30 15:13 ` Souptick Joarder 0 siblings, 1 reply; 22+ messages in thread From: Laurent Pinchart @ 2018-07-30 14:24 UTC (permalink / raw) To: Souptick Joarder Cc: Vaishali Thakkar, airlied, Ajit Linux, dri-devel, linux-renesas-soc, Linux Kernel Mailing List, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-07-30 14:24 ` Laurent Pinchart @ 2018-07-30 15:13 ` Souptick Joarder 2018-07-30 15:45 ` Laurent Pinchart 0 siblings, 1 reply; 22+ messages in thread From: Souptick Joarder @ 2018-07-30 15:13 UTC (permalink / raw) To: Laurent Pinchart, airlied, Daniel Vetter Cc: Vaishali Thakkar, Ajit Linux, dri-devel, linux-renesas-soc, Linux Kernel Mailing List 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 > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-07-30 15:13 ` Souptick Joarder @ 2018-07-30 15:45 ` Laurent Pinchart 2018-07-30 16:44 ` Souptick Joarder 0 siblings, 1 reply; 22+ messages in thread From: Laurent Pinchart @ 2018-07-30 15:45 UTC (permalink / raw) To: Souptick Joarder Cc: airlied, Daniel Vetter, Vaishali Thakkar, Ajit Linux, dri-devel, linux-renesas-soc, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() 2018-07-30 15:45 ` Laurent Pinchart @ 2018-07-30 16:44 ` Souptick Joarder 0 siblings, 0 replies; 22+ messages in thread From: Souptick Joarder @ 2018-07-30 16:44 UTC (permalink / raw) To: Laurent Pinchart Cc: airlied, Daniel Vetter, Vaishali Thakkar, Ajit Linux, dri-devel, linux-renesas-soc, Linux Kernel Mailing List 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 > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-10-23 14:15 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-18 16:39 [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() Souptick Joarder
2018-09-27 6:34 ` Souptick Joarder
2018-09-28 15:05 ` Laurent Pinchart
2018-09-28 15:30 ` Laurent Pinchart
[not found] ` <CAFqt6zbx2tSpnE+reNLN4pC7Br5XbgSuYP4-taxyyWraVpHoMg@mail.gmail.com>
2018-09-30 6:57 ` Laurent Pinchart
2018-10-01 6:52 ` Daniel Vetter
2018-10-01 11:56 ` Laurent Pinchart
2018-10-01 12:42 ` Noralf Trønnes
2018-10-08 16:27 ` Souptick Joarder
2018-10-11 8:18 ` Daniel Vetter
2018-10-22 8:21 ` Souptick Joarder
2018-10-23 13:40 ` Daniel Vetter
2018-10-23 14:15 ` Laurent Pinchart
-- strict thread matches above, loose matches on Subject: below --
2018-07-28 15:40 Souptick Joarder
2018-07-28 17:50 ` Vaishali Thakkar
2018-07-28 18:50 ` Souptick Joarder
2018-07-28 20:20 ` Laurent Pinchart
2018-07-30 13:58 ` Souptick Joarder
2018-07-30 14:24 ` Laurent Pinchart
2018-07-30 15:13 ` Souptick Joarder
2018-07-30 15:45 ` Laurent Pinchart
2018-07-30 16:44 ` Souptick Joarder
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).