| Message ID | a8f92dc70048bab746e94dadd1c23200626aff60.1500555652.git-series.maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > The current drm_atomic_helper_commit_tail helper works only if the CRTC is > accessible, and documents an alternative implementation that is supposed to > be used if that happens. > > That implementation is then duplicated by some drivers. Instead of > documenting it, let's implement an helper that all the relevant users can > use directly. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Should I throw this into drm-misc, or do you want to merge this through some driver tree? -Daniel > --- > drivers/gpu/drm/drm_atomic_helper.c | 49 +++++++++++++++-------- > drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +---------- > include/drm/drm_atomic_helper.h | 1 +- > 4 files changed, 37 insertions(+), 61 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 86d3093c6c9b..d06a65ed3a57 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1245,23 +1245,13 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks); > * @old_state: atomic state object with old state structures > * > * This is the default implementation for the > - * &drm_mode_config_helper_funcs.atomic_commit_tail hook. > + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers > + * that do not support runtime_pm or do not need the CRTC to be > + * enabled to perform a commit. Otherwise, see > + * drm_atomic_helper_commit_tail_rpm(). > * > * Note that the default ordering of how the various stages are called is to > - * match the legacy modeset helper library closest. One peculiarity of that is > - * that it doesn't mesh well with runtime PM at all. > - * > - * For drivers supporting runtime PM the recommended sequence is instead :: > - * > - * drm_atomic_helper_commit_modeset_disables(dev, old_state); > - * > - * drm_atomic_helper_commit_modeset_enables(dev, old_state); > - * > - * drm_atomic_helper_commit_planes(dev, old_state, > - * DRM_PLANE_COMMIT_ACTIVE_ONLY); > - * > - * for committing the atomic update to hardware. See the kerneldoc entries for > - * these three functions for more details. > + * match the legacy modeset helper library closest. > */ > void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) > { > @@ -1281,6 +1271,35 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) > } > EXPORT_SYMBOL(drm_atomic_helper_commit_tail); > > +/** > + * drm_atomic_helper_commit_tail_rpm - commit atomic update to hardware > + * @old_state: new modeset state to be committed > + * > + * This is an alternative implementation for the > + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers > + * that support runtime_pm or need the CRTC to be enabled to perform a > + * commit. Otherwise, one should use the default implementation > + * drm_atomic_helper_commit_tail(). > + */ > +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) > +{ > + struct drm_device *dev = old_state->dev; > + > + drm_atomic_helper_commit_modeset_disables(dev, old_state); > + > + drm_atomic_helper_commit_modeset_enables(dev, old_state); > + > + drm_atomic_helper_commit_planes(dev, old_state, > + DRM_PLANE_COMMIT_ACTIVE_ONLY); > + > + drm_atomic_helper_commit_hw_done(old_state); > + > + drm_atomic_helper_wait_for_vblanks(dev, old_state); > + > + drm_atomic_helper_cleanup_planes(dev, old_state); > +} > +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); > + > static void commit_tail(struct drm_atomic_state *old_state) > { > struct drm_device *dev = old_state->dev; > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c > index d48fd7c918f8..ed1a648d518c 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c > @@ -187,33 +187,8 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) > return exynos_fb->dma_addr[index]; > } > > -static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) > -{ > - struct drm_device *dev = state->dev; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - > - drm_atomic_helper_commit_modeset_enables(dev, state); > - > - /* > - * Exynos can't update planes with CRTCs and encoders disabled, > - * its updates routines, specially for FIMD, requires the clocks > - * to be enabled. So it is necessary to handle the modeset operations > - * *before* the commit_planes() step, this way it will always > - * have the relevant clocks enabled to perform the update. > - */ > - drm_atomic_helper_commit_planes(dev, state, > - DRM_PLANE_COMMIT_ACTIVE_ONLY); > - > - drm_atomic_helper_commit_hw_done(state); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - > - drm_atomic_helper_cleanup_planes(dev, state); > -} > - > static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { > - .atomic_commit_tail = exynos_drm_atomic_commit_tail, > + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, > }; > > static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > index 81f9548672b0..35ad386c401e 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > @@ -174,27 +174,8 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev) > drm_fb_helper_hotplug_event(fb_helper); > } > > -static void > -rockchip_atomic_commit_tail(struct drm_atomic_state *state) > -{ > - struct drm_device *dev = state->dev; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - > - drm_atomic_helper_commit_modeset_enables(dev, state); > - > - drm_atomic_helper_commit_planes(dev, state, > - DRM_PLANE_COMMIT_ACTIVE_ONLY); > - > - drm_atomic_helper_commit_hw_done(state); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - > - drm_atomic_helper_cleanup_planes(dev, state); > -} > - > static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = { > - .atomic_commit_tail = rockchip_atomic_commit_tail, > + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, > }; > > static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = { > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index f0a8678ae98e..af4aaff9ec0b 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -41,6 +41,7 @@ int drm_atomic_helper_check_planes(struct drm_device *dev, > int drm_atomic_helper_check(struct drm_device *dev, > struct drm_atomic_state *state); > void drm_atomic_helper_commit_tail(struct drm_atomic_state *state); > +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state); > int drm_atomic_helper_commit(struct drm_device *dev, > struct drm_atomic_state *state, > bool nonblock); > -- > git-series 0.9.1 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, On Thu, Jul 20, 2017 at 08:46:28PM +0200, Daniel Vetter wrote: > On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > > The current drm_atomic_helper_commit_tail helper works only if the CRTC is > > accessible, and documents an alternative implementation that is supposed to > > be used if that happens. > > > > That implementation is then duplicated by some drivers. Instead of > > documenting it, let's implement an helper that all the relevant users can > > use directly. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Should I throw this into drm-misc, or do you want to merge this through > some driver tree? Yes, you can put them in drm-misc, it will the be the easiest option. Thanks! Maxime
On Tue, Jul 25, 2017 at 08:57:34AM +0200, Maxime Ripard wrote: > Hi Daniel, > > On Thu, Jul 20, 2017 at 08:46:28PM +0200, Daniel Vetter wrote: > > On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > > > The current drm_atomic_helper_commit_tail helper works only if the CRTC is > > > accessible, and documents an alternative implementation that is supposed to > > > be used if that happens. > > > > > > That implementation is then duplicated by some drivers. Instead of > > > documenting it, let's implement an helper that all the relevant users can > > > use directly. > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Should I throw this into drm-misc, or do you want to merge this through > > some driver tree? > > Yes, you can put them in drm-misc, it will the be the easiest option. Merged, thanks. -Daniel
Hi Maxime, On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > The current drm_atomic_helper_commit_tail helper works only if the CRTC is > accessible, and documents an alternative implementation that is supposed to > be used if that happens. > > That implementation is then duplicated by some drivers. Instead of > documenting it, let's implement an helper that all the relevant users can > use directly. Sorry for not noticing the series earlier, I was fooled by the series top commit summary into thinking it was a driver specific change and only paid proper attention when it ended up in drm-next. I have a comment to make though. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 49 +++++++++++++++-------- > drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +---------- > include/drm/drm_atomic_helper.h | 1 +- > 4 files changed, 37 insertions(+), 61 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 86d3093c6c9b..d06a65ed3a57 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1245,23 +1245,13 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks); > * @old_state: atomic state object with old state structures > * > * This is the default implementation for the > - * &drm_mode_config_helper_funcs.atomic_commit_tail hook. > + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers > + * that do not support runtime_pm or do not need the CRTC to be > + * enabled to perform a commit. Otherwise, see > + * drm_atomic_helper_commit_tail_rpm(). > * > * Note that the default ordering of how the various stages are called is to > - * match the legacy modeset helper library closest. One peculiarity of that is > - * that it doesn't mesh well with runtime PM at all. > - * > - * For drivers supporting runtime PM the recommended sequence is instead :: > - * > - * drm_atomic_helper_commit_modeset_disables(dev, old_state); > - * > - * drm_atomic_helper_commit_modeset_enables(dev, old_state); > - * > - * drm_atomic_helper_commit_planes(dev, old_state, > - * DRM_PLANE_COMMIT_ACTIVE_ONLY); > - * > - * for committing the atomic update to hardware. See the kerneldoc entries for > - * these three functions for more details. > + * match the legacy modeset helper library closest. > */ > void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) > { > @@ -1281,6 +1271,35 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) > } > EXPORT_SYMBOL(drm_atomic_helper_commit_tail); > > +/** > + * drm_atomic_helper_commit_tail_rpm - commit atomic update to hardware > + * @old_state: new modeset state to be committed > + * > + * This is an alternative implementation for the > + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers > + * that support runtime_pm or need the CRTC to be enabled to perform a > + * commit. Otherwise, one should use the default implementation > + * drm_atomic_helper_commit_tail(). > + */ > +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) > +{ > + struct drm_device *dev = old_state->dev; > + > + drm_atomic_helper_commit_modeset_disables(dev, old_state); > + > + drm_atomic_helper_commit_modeset_enables(dev, old_state); > + > + drm_atomic_helper_commit_planes(dev, old_state, > + DRM_PLANE_COMMIT_ACTIVE_ONLY); > + > + drm_atomic_helper_commit_hw_done(old_state); > + > + drm_atomic_helper_wait_for_vblanks(dev, old_state); > + > + drm_atomic_helper_cleanup_planes(dev, old_state); > +} > +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); > + Given that this function is supposed to be used by runtime PM aware drivers and that the hook is called from the DRM core, should there not be some pm_runtime_{get,put} calls wrapping the body of this function? Best regards, Liviu > static void commit_tail(struct drm_atomic_state *old_state) > { > struct drm_device *dev = old_state->dev; > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c > index d48fd7c918f8..ed1a648d518c 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c > @@ -187,33 +187,8 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) > return exynos_fb->dma_addr[index]; > } > > -static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) > -{ > - struct drm_device *dev = state->dev; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - > - drm_atomic_helper_commit_modeset_enables(dev, state); > - > - /* > - * Exynos can't update planes with CRTCs and encoders disabled, > - * its updates routines, specially for FIMD, requires the clocks > - * to be enabled. So it is necessary to handle the modeset operations > - * *before* the commit_planes() step, this way it will always > - * have the relevant clocks enabled to perform the update. > - */ > - drm_atomic_helper_commit_planes(dev, state, > - DRM_PLANE_COMMIT_ACTIVE_ONLY); > - > - drm_atomic_helper_commit_hw_done(state); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - > - drm_atomic_helper_cleanup_planes(dev, state); > -} > - > static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { > - .atomic_commit_tail = exynos_drm_atomic_commit_tail, > + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, > }; > > static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > index 81f9548672b0..35ad386c401e 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > @@ -174,27 +174,8 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev) > drm_fb_helper_hotplug_event(fb_helper); > } > > -static void > -rockchip_atomic_commit_tail(struct drm_atomic_state *state) > -{ > - struct drm_device *dev = state->dev; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - > - drm_atomic_helper_commit_modeset_enables(dev, state); > - > - drm_atomic_helper_commit_planes(dev, state, > - DRM_PLANE_COMMIT_ACTIVE_ONLY); > - > - drm_atomic_helper_commit_hw_done(state); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - > - drm_atomic_helper_cleanup_planes(dev, state); > -} > - > static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = { > - .atomic_commit_tail = rockchip_atomic_commit_tail, > + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, > }; > > static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = { > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index f0a8678ae98e..af4aaff9ec0b 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -41,6 +41,7 @@ int drm_atomic_helper_check_planes(struct drm_device *dev, > int drm_atomic_helper_check(struct drm_device *dev, > struct drm_atomic_state *state); > void drm_atomic_helper_commit_tail(struct drm_atomic_state *state); > +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state); > int drm_atomic_helper_commit(struct drm_device *dev, > struct drm_atomic_state *state, > bool nonblock); > -- > git-series 0.9.1 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: >> +/** >> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to hardware >> + * @old_state: new modeset state to be committed >> + * >> + * This is an alternative implementation for the >> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers >> + * that support runtime_pm or need the CRTC to be enabled to perform a >> + * commit. Otherwise, one should use the default implementation >> + * drm_atomic_helper_commit_tail(). >> + */ >> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) >> +{ >> + struct drm_device *dev = old_state->dev; >> + >> + drm_atomic_helper_commit_modeset_disables(dev, old_state); >> + >> + drm_atomic_helper_commit_modeset_enables(dev, old_state); >> + >> + drm_atomic_helper_commit_planes(dev, old_state, >> + DRM_PLANE_COMMIT_ACTIVE_ONLY); >> + >> + drm_atomic_helper_commit_hw_done(old_state); >> + >> + drm_atomic_helper_wait_for_vblanks(dev, old_state); >> + >> + drm_atomic_helper_cleanup_planes(dev, old_state); >> +} >> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); >> + > > Given that this function is supposed to be used by runtime PM aware > drivers and that the hook is called from the DRM core, should there not > be some pm_runtime_{get,put} calls wrapping the body of this function? No, because the drm atomic helpers have no idea which device is backing which part of the drm device. Some drivers might on have one device for the entire driver, some one device for just the display part (which might or might not match drm_device->dev). And many arm drivers have a device for each block separately (and you need to call rpm_get/put on each). And some something in-between (e.g. core device + external encoders). I don't think there's anything the helpers can to to help support this. Also, just wrapping functions with rpm_get/put only papers over bugs in your driver - either you enable something, then the rpm_get needs to be done anyway (since the hw will be shut down otherwise), or you disable something, same reasons. The only thing a rpm_get/put does is paper over the bugs where you try to access the hw when it's off. As soon as this function finishes, the hw is shut down again, drops all register values on the floor, so either that access wasn't needed, or your driver has a bug (because it assumes the register values survive when they don't). So imo all around a bad idea, at least from my experience of doing rpm enabling on i915 hw. -Daniel > > Best regards, > Liviu > > >> static void commit_tail(struct drm_atomic_state *old_state) >> { >> struct drm_device *dev = old_state->dev; >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c >> index d48fd7c918f8..ed1a648d518c 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c >> @@ -187,33 +187,8 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) >> return exynos_fb->dma_addr[index]; >> } >> >> -static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) >> -{ >> - struct drm_device *dev = state->dev; >> - >> - drm_atomic_helper_commit_modeset_disables(dev, state); >> - >> - drm_atomic_helper_commit_modeset_enables(dev, state); >> - >> - /* >> - * Exynos can't update planes with CRTCs and encoders disabled, >> - * its updates routines, specially for FIMD, requires the clocks >> - * to be enabled. So it is necessary to handle the modeset operations >> - * *before* the commit_planes() step, this way it will always >> - * have the relevant clocks enabled to perform the update. >> - */ >> - drm_atomic_helper_commit_planes(dev, state, >> - DRM_PLANE_COMMIT_ACTIVE_ONLY); >> - >> - drm_atomic_helper_commit_hw_done(state); >> - >> - drm_atomic_helper_wait_for_vblanks(dev, state); >> - >> - drm_atomic_helper_cleanup_planes(dev, state); >> -} >> - >> static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { >> - .atomic_commit_tail = exynos_drm_atomic_commit_tail, >> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, >> }; >> >> static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c >> index 81f9548672b0..35ad386c401e 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c >> @@ -174,27 +174,8 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev) >> drm_fb_helper_hotplug_event(fb_helper); >> } >> >> -static void >> -rockchip_atomic_commit_tail(struct drm_atomic_state *state) >> -{ >> - struct drm_device *dev = state->dev; >> - >> - drm_atomic_helper_commit_modeset_disables(dev, state); >> - >> - drm_atomic_helper_commit_modeset_enables(dev, state); >> - >> - drm_atomic_helper_commit_planes(dev, state, >> - DRM_PLANE_COMMIT_ACTIVE_ONLY); >> - >> - drm_atomic_helper_commit_hw_done(state); >> - >> - drm_atomic_helper_wait_for_vblanks(dev, state); >> - >> - drm_atomic_helper_cleanup_planes(dev, state); >> -} >> - >> static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = { >> - .atomic_commit_tail = rockchip_atomic_commit_tail, >> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, >> }; >> >> static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = { >> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h >> index f0a8678ae98e..af4aaff9ec0b 100644 >> --- a/include/drm/drm_atomic_helper.h >> +++ b/include/drm/drm_atomic_helper.h >> @@ -41,6 +41,7 @@ int drm_atomic_helper_check_planes(struct drm_device *dev, >> int drm_atomic_helper_check(struct drm_device *dev, >> struct drm_atomic_state *state); >> void drm_atomic_helper_commit_tail(struct drm_atomic_state *state); >> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state); >> int drm_atomic_helper_commit(struct drm_device *dev, >> struct drm_atomic_state *state, >> bool nonblock); >> -- >> git-series 0.9.1 >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯ > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Aug 02, 2017 at 01:27:23PM +0200, Daniel Vetter wrote: > On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > >> +/** > >> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to hardware > >> + * @old_state: new modeset state to be committed > >> + * > >> + * This is an alternative implementation for the > >> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers > >> + * that support runtime_pm or need the CRTC to be enabled to perform a > >> + * commit. Otherwise, one should use the default implementation > >> + * drm_atomic_helper_commit_tail(). > >> + */ > >> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) > >> +{ > >> + struct drm_device *dev = old_state->dev; > >> + > >> + drm_atomic_helper_commit_modeset_disables(dev, old_state); > >> + > >> + drm_atomic_helper_commit_modeset_enables(dev, old_state); > >> + > >> + drm_atomic_helper_commit_planes(dev, old_state, > >> + DRM_PLANE_COMMIT_ACTIVE_ONLY); > >> + > >> + drm_atomic_helper_commit_hw_done(old_state); > >> + > >> + drm_atomic_helper_wait_for_vblanks(dev, old_state); > >> + > >> + drm_atomic_helper_cleanup_planes(dev, old_state); > >> +} > >> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); > >> + > > > > Given that this function is supposed to be used by runtime PM aware > > drivers and that the hook is called from the DRM core, should there not > > be some pm_runtime_{get,put} calls wrapping the body of this function? Hi Daniel, > > No, because the drm atomic helpers have no idea which device is > backing which part of the drm device. Some drivers might on have one > device for the entire driver, some one device for just the display > part (which might or might not match drm_device->dev). And many arm > drivers have a device for each block separately (and you need to call > rpm_get/put on each). And some something in-between (e.g. core device > + external encoders). Hmm, I understand your point about this function not having to care about pm_runtime_{get,put}, but I do not agree with your view that if it wanted to care about it, it wouldn't be able to do the right thing because it doesn't have the right device. After all, this function is about handling the updates that this atomic commit is concerned about, so having the old_state->dev drm_device pointer and its associated device should be enough. Am I missing something? > > I don't think there's anything the helpers can to to help support this. > > Also, just wrapping functions with rpm_get/put only papers over bugs > in your driver - either you enable something, then the rpm_get needs > to be done anyway (since the hw will be shut down otherwise), or you > disable something, same reasons. The only thing a rpm_get/put does is > paper over the bugs where you try to access the hw when it's off. As > soon as this function finishes, the hw is shut down again, drops all > register values on the floor, so either that access wasn't needed, or > your driver has a bug (because it assumes the register values survive > when they don't). > > So imo all around a bad idea, at least from my experience of doing rpm > enabling on i915 hw. Understood. Thanks! Best regards, Liviu > -Daniel > > > > > Best regards, > > Liviu > > > > > >> static void commit_tail(struct drm_atomic_state *old_state) > >> { > >> struct drm_device *dev = old_state->dev; > >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c > >> index d48fd7c918f8..ed1a648d518c 100644 > >> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c > >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c > >> @@ -187,33 +187,8 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) > >> return exynos_fb->dma_addr[index]; > >> } > >> > >> -static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) > >> -{ > >> - struct drm_device *dev = state->dev; > >> - > >> - drm_atomic_helper_commit_modeset_disables(dev, state); > >> - > >> - drm_atomic_helper_commit_modeset_enables(dev, state); > >> - > >> - /* > >> - * Exynos can't update planes with CRTCs and encoders disabled, > >> - * its updates routines, specially for FIMD, requires the clocks > >> - * to be enabled. So it is necessary to handle the modeset operations > >> - * *before* the commit_planes() step, this way it will always > >> - * have the relevant clocks enabled to perform the update. > >> - */ > >> - drm_atomic_helper_commit_planes(dev, state, > >> - DRM_PLANE_COMMIT_ACTIVE_ONLY); > >> - > >> - drm_atomic_helper_commit_hw_done(state); > >> - > >> - drm_atomic_helper_wait_for_vblanks(dev, state); > >> - > >> - drm_atomic_helper_cleanup_planes(dev, state); > >> -} > >> - > >> static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { > >> - .atomic_commit_tail = exynos_drm_atomic_commit_tail, > >> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, > >> }; > >> > >> static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > >> index 81f9548672b0..35ad386c401e 100644 > >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > >> @@ -174,27 +174,8 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev) > >> drm_fb_helper_hotplug_event(fb_helper); > >> } > >> > >> -static void > >> -rockchip_atomic_commit_tail(struct drm_atomic_state *state) > >> -{ > >> - struct drm_device *dev = state->dev; > >> - > >> - drm_atomic_helper_commit_modeset_disables(dev, state); > >> - > >> - drm_atomic_helper_commit_modeset_enables(dev, state); > >> - > >> - drm_atomic_helper_commit_planes(dev, state, > >> - DRM_PLANE_COMMIT_ACTIVE_ONLY); > >> - > >> - drm_atomic_helper_commit_hw_done(state); > >> - > >> - drm_atomic_helper_wait_for_vblanks(dev, state); > >> - > >> - drm_atomic_helper_cleanup_planes(dev, state); > >> -} > >> - > >> static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = { > >> - .atomic_commit_tail = rockchip_atomic_commit_tail, > >> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, > >> }; > >> > >> static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = { > >> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > >> index f0a8678ae98e..af4aaff9ec0b 100644 > >> --- a/include/drm/drm_atomic_helper.h > >> +++ b/include/drm/drm_atomic_helper.h > >> @@ -41,6 +41,7 @@ int drm_atomic_helper_check_planes(struct drm_device *dev, > >> int drm_atomic_helper_check(struct drm_device *dev, > >> struct drm_atomic_state *state); > >> void drm_atomic_helper_commit_tail(struct drm_atomic_state *state); > >> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state); > >> int drm_atomic_helper_commit(struct drm_device *dev, > >> struct drm_atomic_state *state, > >> bool nonblock); > >> -- > >> git-series 0.9.1 > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > ==================== > > | I would like to | > > | fix the world, | > > | but they're not | > > | giving me the | > > \ source code! / > > --------------- > > ¯\_(ツ)_/¯ > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Livu, On Wednesday 02 Aug 2017 13:46:48 Liviu Dudau wrote: > On Wed, Aug 02, 2017 at 01:27:23PM +0200, Daniel Vetter wrote: > > On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > >> On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > >>> +/** > >>> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to > >>> hardware > >>> + * @old_state: new modeset state to be committed > >>> + * > >>> + * This is an alternative implementation for the > >>> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers > >>> + * that support runtime_pm or need the CRTC to be enabled to perform a > >>> + * commit. Otherwise, one should use the default implementation > >>> + * drm_atomic_helper_commit_tail(). > >>> + */ > >>> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state > >>> *old_state) > >>> +{ > >>> + struct drm_device *dev = old_state->dev; > >>> + > >>> + drm_atomic_helper_commit_modeset_disables(dev, old_state); > >>> + > >>> + drm_atomic_helper_commit_modeset_enables(dev, old_state); > >>> + > >>> + drm_atomic_helper_commit_planes(dev, old_state, > >>> + DRM_PLANE_COMMIT_ACTIVE_ONLY); > >>> + > >>> + drm_atomic_helper_commit_hw_done(old_state); > >>> + > >>> + drm_atomic_helper_wait_for_vblanks(dev, old_state); > >>> + > >>> + drm_atomic_helper_cleanup_planes(dev, old_state); > >>> +} > >>> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); > >>> + > >> > >> Given that this function is supposed to be used by runtime PM aware > >> drivers and that the hook is called from the DRM core, should there not > >> be some pm_runtime_{get,put} calls wrapping the body of this function? > > Hi Daniel, > > > No, because the drm atomic helpers have no idea which device is > > backing which part of the drm device. Some drivers might on have one > > device for the entire driver, some one device for just the display > > part (which might or might not match drm_device->dev). And many arm > > drivers have a device for each block separately (and you need to call > > rpm_get/put on each). And some something in-between (e.g. core device > > + external encoders). > > Hmm, I understand your point about this function not having to care about > pm_runtime_{get,put}, but I do not agree with your view that if it wanted to > care about it, it wouldn't be able to do the right thing because it doesn't > have the right device. After all, this function is about handling the > updates that this atomic commit is concerned about, so having the > old_state->dev drm_device pointer and its associated device should be > enough. Am I missing something? In embedded system it's quite common for display hardware to be made of multiple IP cores. Depending on the SoC you could have to manage runtime PM at the CRTC level for instance. The DRM core doesn't know about that, and sees a single device only. I've expressed my doubts previously about the need for a RPM version of drm_atomic_helper_commit_tail(), as the resulting order of CRTC enable/disable and plane update operations can lead to corrupt frames when enabling the CRTC. I had a commit tail implementation in the rcar-du driver that was very similar to drm_atomic_helper_commit_tail_rpm(), and had to move back to the standard order to fix the corrupt frame issue. The result isn't as clean as I would like, as power handling is split between the CRTC enable/disable and the atomic begin/flush in a way that is not straightforward. It now occurred to me that a simpler implementation could be possible. I'll have to experiment with it first, but the idea is as follows. 1 Call pm_runtime_get_sync() at the beginning of .commit_tail() and pm_runtime_put() at the end. 2. Use the standard CRTC disable, plane update, CRTC enable order in .commit_tail(). 3. Call pm_runtime_get() in the CRTC .enable() handler and pm_runtime_put() in the CRTC .disable() handler; This should guarantee that the device won't be suspended between CRTC disable and CRTC enable during a mode set, without requiring any special collaboration between CRTC enable/disable and atomic begin/flush to handle runtime PM. If drivers implement this, there should be no need for drm_atomic_helper_commit_tail_rpm(). Maxime, Daniel, what do you think about this ? > > I don't think there's anything the helpers can to to help support this. > > > > Also, just wrapping functions with rpm_get/put only papers over bugs > > in your driver - either you enable something, then the rpm_get needs > > to be done anyway (since the hw will be shut down otherwise), or you > > disable something, same reasons. The only thing a rpm_get/put does is > > paper over the bugs where you try to access the hw when it's off. As > > soon as this function finishes, the hw is shut down again, drops all > > register values on the floor, so either that access wasn't needed, or > > your driver has a bug (because it assumes the register values survive > > when they don't). > > > > So imo all around a bad idea, at least from my experience of doing rpm > > enabling on i915 hw. > > Understood. Thanks!
On Wed, Aug 02, 2017 at 04:27:27PM +0300, Laurent Pinchart wrote: > Hi Livu, > > On Wednesday 02 Aug 2017 13:46:48 Liviu Dudau wrote: > > On Wed, Aug 02, 2017 at 01:27:23PM +0200, Daniel Vetter wrote: > > > On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > >> On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > > >>> +/** > > >>> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to > > >>> hardware > > >>> + * @old_state: new modeset state to be committed > > >>> + * > > >>> + * This is an alternative implementation for the > > >>> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers > > >>> + * that support runtime_pm or need the CRTC to be enabled to perform a > > >>> + * commit. Otherwise, one should use the default implementation > > >>> + * drm_atomic_helper_commit_tail(). > > >>> + */ > > >>> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state > > >>> *old_state) > > >>> +{ > > >>> + struct drm_device *dev = old_state->dev; > > >>> + > > >>> + drm_atomic_helper_commit_modeset_disables(dev, old_state); > > >>> + > > >>> + drm_atomic_helper_commit_modeset_enables(dev, old_state); > > >>> + > > >>> + drm_atomic_helper_commit_planes(dev, old_state, > > >>> + DRM_PLANE_COMMIT_ACTIVE_ONLY); > > >>> + > > >>> + drm_atomic_helper_commit_hw_done(old_state); > > >>> + > > >>> + drm_atomic_helper_wait_for_vblanks(dev, old_state); > > >>> + > > >>> + drm_atomic_helper_cleanup_planes(dev, old_state); > > >>> +} > > >>> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); > > >>> + > > >> > > >> Given that this function is supposed to be used by runtime PM aware > > >> drivers and that the hook is called from the DRM core, should there not > > >> be some pm_runtime_{get,put} calls wrapping the body of this function? > > > > Hi Daniel, > > > > > No, because the drm atomic helpers have no idea which device is > > > backing which part of the drm device. Some drivers might on have one > > > device for the entire driver, some one device for just the display > > > part (which might or might not match drm_device->dev). And many arm > > > drivers have a device for each block separately (and you need to call > > > rpm_get/put on each). And some something in-between (e.g. core device > > > + external encoders). > > > > Hmm, I understand your point about this function not having to care about > > pm_runtime_{get,put}, but I do not agree with your view that if it wanted to > > care about it, it wouldn't be able to do the right thing because it doesn't > > have the right device. After all, this function is about handling the > > updates that this atomic commit is concerned about, so having the > > old_state->dev drm_device pointer and its associated device should be > > enough. Am I missing something? > > In embedded system it's quite common for display hardware to be made of > multiple IP cores. Depending on the SoC you could have to manage runtime PM at > the CRTC level for instance. The DRM core doesn't know about that, and sees a > single device only. > > I've expressed my doubts previously about the need for a RPM version of > drm_atomic_helper_commit_tail(), as the resulting order of CRTC enable/disable > and plane update operations can lead to corrupt frames when enabling the CRTC. > I had a commit tail implementation in the rcar-du driver that was very similar > to drm_atomic_helper_commit_tail_rpm(), and had to move back to the standard > order to fix the corrupt frame issue. The result isn't as clean as I would > like, as power handling is split between the CRTC enable/disable and the > atomic begin/flush in a way that is not straightforward. > > It now occurred to me that a simpler implementation could be possible. I'll > have to experiment with it first, but the idea is as follows. > > 1 Call pm_runtime_get_sync() at the beginning of .commit_tail() and > pm_runtime_put() at the end. > > 2. Use the standard CRTC disable, plane update, CRTC enable order in > .commit_tail(). > > 3. Call pm_runtime_get() in the CRTC .enable() handler and pm_runtime_put() in > the CRTC .disable() handler; Well, that is what mali-dp driver currently does, but according to Daniel (and I can see his POV) that is wrong. I'm playing with removing all of that to see if there are any side effects in Mali DP like the ones you mentioned for RCAR. Best regards, Liviu > > This should guarantee that the device won't be suspended between CRTC disable > and CRTC enable during a mode set, without requiring any special collaboration > between CRTC enable/disable and atomic begin/flush to handle runtime PM. If > drivers implement this, there should be no need for > drm_atomic_helper_commit_tail_rpm(). > > Maxime, Daniel, what do you think about this ? > > > > I don't think there's anything the helpers can to to help support this. > > > > > > Also, just wrapping functions with rpm_get/put only papers over bugs > > > in your driver - either you enable something, then the rpm_get needs > > > to be done anyway (since the hw will be shut down otherwise), or you > > > disable something, same reasons. The only thing a rpm_get/put does is > > > paper over the bugs where you try to access the hw when it's off. As > > > soon as this function finishes, the hw is shut down again, drops all > > > register values on the floor, so either that access wasn't needed, or > > > your driver has a bug (because it assumes the register values survive > > > when they don't). > > > > > > So imo all around a bad idea, at least from my experience of doing rpm > > > enabling on i915 hw. > > > > Understood. Thanks! > > -- > Regards, > > Laurent Pinchart >
Hi Liviu, On Wednesday 02 Aug 2017 14:32:06 Liviu Dudau wrote: > On Wed, Aug 02, 2017 at 04:27:27PM +0300, Laurent Pinchart wrote: > > On Wednesday 02 Aug 2017 13:46:48 Liviu Dudau wrote: > >> On Wed, Aug 02, 2017 at 01:27:23PM +0200, Daniel Vetter wrote: > >>> On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau wrote: > >>>> On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > >>>>> +/** > >>>>> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to > >>>>> hardware > >>>>> + * @old_state: new modeset state to be committed > >>>>> + * > >>>>> + * This is an alternative implementation for the > >>>>> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for > >>>>> drivers > >>>>> + * that support runtime_pm or need the CRTC to be enabled to > >>>>> perform a > >>>>> + * commit. Otherwise, one should use the default implementation > >>>>> + * drm_atomic_helper_commit_tail(). > >>>>> + */ > >>>>> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state > >>>>> *old_state) > >>>>> +{ > >>>>> + struct drm_device *dev = old_state->dev; > >>>>> + > >>>>> + drm_atomic_helper_commit_modeset_disables(dev, old_state); > >>>>> + > >>>>> + drm_atomic_helper_commit_modeset_enables(dev, old_state); > >>>>> + > >>>>> + drm_atomic_helper_commit_planes(dev, old_state, > >>>>> + DRM_PLANE_COMMIT_ACTIVE_ONLY); > >>>>> + > >>>>> + drm_atomic_helper_commit_hw_done(old_state); > >>>>> + > >>>>> + drm_atomic_helper_wait_for_vblanks(dev, old_state); > >>>>> + > >>>>> + drm_atomic_helper_cleanup_planes(dev, old_state); > >>>>> +} > >>>>> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); > >>>>> + > >>>> > >>>> Given that this function is supposed to be used by runtime PM aware > >>>> drivers and that the hook is called from the DRM core, should there > >>>> not be some pm_runtime_{get,put} calls wrapping the body of this > >>>> function? > >> > >> Hi Daniel, > >> > >>> No, because the drm atomic helpers have no idea which device is > >>> backing which part of the drm device. Some drivers might on have one > >>> device for the entire driver, some one device for just the display > >>> part (which might or might not match drm_device->dev). And many arm > >>> drivers have a device for each block separately (and you need to call > >>> rpm_get/put on each). And some something in-between (e.g. core device > >>> + external encoders). > >> > >> Hmm, I understand your point about this function not having to care > >> about pm_runtime_{get,put}, but I do not agree with your view that if it > >> wanted to care about it, it wouldn't be able to do the right thing > >> because it doesn't have the right device. After all, this function is > >> about handling the updates that this atomic commit is concerned about, > >> so having the old_state->dev drm_device pointer and its associated device > >> should be enough. Am I missing something? > > > > In embedded system it's quite common for display hardware to be made of > > multiple IP cores. Depending on the SoC you could have to manage runtime > > PM at the CRTC level for instance. The DRM core doesn't know about that, > > and sees a single device only. > > > > I've expressed my doubts previously about the need for a RPM version of > > drm_atomic_helper_commit_tail(), as the resulting order of CRTC > > enable/disable and plane update operations can lead to corrupt frames > > when enabling the CRTC. I had a commit tail implementation in the rcar-du > > driver that was very similar to drm_atomic_helper_commit_tail_rpm(), and > > had to move back to the standard order to fix the corrupt frame issue. > > The result isn't as clean as I would like, as power handling is split > > between the CRTC enable/disable and the atomic begin/flush in a way that > > is not straightforward. > > > > It now occurred to me that a simpler implementation could be possible. > > I'll have to experiment with it first, but the idea is as follows. > > > > 1 Call pm_runtime_get_sync() at the beginning of .commit_tail() and > > pm_runtime_put() at the end. > > > > 2. Use the standard CRTC disable, plane update, CRTC enable order in > > .commit_tail(). > > > > 3. Call pm_runtime_get() in the CRTC .enable() handler and > > pm_runtime_put() in the CRTC .disable() handler; > > Well, that is what mali-dp driver currently does, but according to Daniel > (and I can see his POV) that is wrong. Is it ? I might not have understood his arguments the same way (or possibly failed to even see them). Are you referring to this comments in this mail thread, or to something else ? > I'm playing with removing all of that to see if there are any side effects > in Mali DP like the ones you mentioned for RCAR. Note that the first frame will usually not be noticed as it often takes a few frames for the display to turn on. > > This should guarantee that the device won't be suspended between CRTC > > disable and CRTC enable during a mode set, without requiring any special > > collaboration between CRTC enable/disable and atomic begin/flush to > > handle runtime PM. If drivers implement this, there should be no need for > > drm_atomic_helper_commit_tail_rpm(). > > > > Maxime, Daniel, what do you think about this ? > > > >>> I don't think there's anything the helpers can to to help support > >>> this. > >>> > >>> Also, just wrapping functions with rpm_get/put only papers over bugs > >>> in your driver - either you enable something, then the rpm_get needs > >>> to be done anyway (since the hw will be shut down otherwise), or you > >>> disable something, same reasons. The only thing a rpm_get/put does is > >>> paper over the bugs where you try to access the hw when it's off. As > >>> soon as this function finishes, the hw is shut down again, drops all > >> register values on the floor, so either that access wasn't needed, or > >>> your driver has a bug (because it assumes the register values survive > >>> when they don't). > >>> > >>> So imo all around a bad idea, at least from my experience of doing rpm > >>> enabling on i915 hw. > >> > >> Understood. Thanks!
On Wed, Aug 02, 2017 at 04:49:16PM +0300, Laurent Pinchart wrote: > Hi Liviu, > > On Wednesday 02 Aug 2017 14:32:06 Liviu Dudau wrote: > > On Wed, Aug 02, 2017 at 04:27:27PM +0300, Laurent Pinchart wrote: > > > On Wednesday 02 Aug 2017 13:46:48 Liviu Dudau wrote: > > >> On Wed, Aug 02, 2017 at 01:27:23PM +0200, Daniel Vetter wrote: > > >>> On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau wrote: > > >>>> On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > > >>>>> +/** > > >>>>> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to > > >>>>> hardware > > >>>>> + * @old_state: new modeset state to be committed > > >>>>> + * > > >>>>> + * This is an alternative implementation for the > > >>>>> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for > > >>>>> drivers > > >>>>> + * that support runtime_pm or need the CRTC to be enabled to > > >>>>> perform a > > >>>>> + * commit. Otherwise, one should use the default implementation > > >>>>> + * drm_atomic_helper_commit_tail(). > > >>>>> + */ > > >>>>> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state > > >>>>> *old_state) > > >>>>> +{ > > >>>>> + struct drm_device *dev = old_state->dev; > > >>>>> + > > >>>>> + drm_atomic_helper_commit_modeset_disables(dev, old_state); > > >>>>> + > > >>>>> + drm_atomic_helper_commit_modeset_enables(dev, old_state); > > >>>>> + > > >>>>> + drm_atomic_helper_commit_planes(dev, old_state, > > >>>>> + DRM_PLANE_COMMIT_ACTIVE_ONLY); > > >>>>> + > > >>>>> + drm_atomic_helper_commit_hw_done(old_state); > > >>>>> + > > >>>>> + drm_atomic_helper_wait_for_vblanks(dev, old_state); > > >>>>> + > > >>>>> + drm_atomic_helper_cleanup_planes(dev, old_state); > > >>>>> +} > > >>>>> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); > > >>>>> + > > >>>> > > >>>> Given that this function is supposed to be used by runtime PM aware > > >>>> drivers and that the hook is called from the DRM core, should there > > >>>> not be some pm_runtime_{get,put} calls wrapping the body of this > > >>>> function? > > >> > > >> Hi Daniel, > > >> > > >>> No, because the drm atomic helpers have no idea which device is > > >>> backing which part of the drm device. Some drivers might on have one > > >>> device for the entire driver, some one device for just the display > > >>> part (which might or might not match drm_device->dev). And many arm > > >>> drivers have a device for each block separately (and you need to call > > >>> rpm_get/put on each). And some something in-between (e.g. core device > > >>> + external encoders). > > >> > > >> Hmm, I understand your point about this function not having to care > > >> about pm_runtime_{get,put}, but I do not agree with your view that if it > > >> wanted to care about it, it wouldn't be able to do the right thing > > >> because it doesn't have the right device. After all, this function is > > >> about handling the updates that this atomic commit is concerned about, > > >> so having the old_state->dev drm_device pointer and its associated device > > >> should be enough. Am I missing something? > > > > > > In embedded system it's quite common for display hardware to be made of > > > multiple IP cores. Depending on the SoC you could have to manage runtime > > > PM at the CRTC level for instance. The DRM core doesn't know about that, > > > and sees a single device only. > > > > > > I've expressed my doubts previously about the need for a RPM version of > > > drm_atomic_helper_commit_tail(), as the resulting order of CRTC > > > enable/disable and plane update operations can lead to corrupt frames > > > when enabling the CRTC. I had a commit tail implementation in the rcar-du > > > driver that was very similar to drm_atomic_helper_commit_tail_rpm(), and > > > had to move back to the standard order to fix the corrupt frame issue. > > > The result isn't as clean as I would like, as power handling is split > > > between the CRTC enable/disable and the atomic begin/flush in a way that > > > is not straightforward. > > > > > > It now occurred to me that a simpler implementation could be possible. > > > I'll have to experiment with it first, but the idea is as follows. > > > > > > 1 Call pm_runtime_get_sync() at the beginning of .commit_tail() and > > > pm_runtime_put() at the end. > > > > > > 2. Use the standard CRTC disable, plane update, CRTC enable order in > > > .commit_tail(). > > > > > > 3. Call pm_runtime_get() in the CRTC .enable() handler and > > > pm_runtime_put() in the CRTC .disable() handler; > > > > Well, that is what mali-dp driver currently does, but according to Daniel > > (and I can see his POV) that is wrong. > > Is it ? I might not have understood his arguments the same way (or possibly > failed to even see them). Are you referring to this comments in this mail > thread, or to something else ? I'm talking about his reply above. My understanding: you can't do pm_runtime_{get,set} in the atomic_commit_tail hook because: 1. you don't know if you have the correct drm_device->dev (or all the relevant devices) to call pm_runtime_get() on. 2. If pm_runtime_get() affects in any way the atomic commit behaviour by being at the top of the commit_tail_rpm() function then you are: a) papering over bugs in one's driver b) going to turn off things at the end of commit_tail_rpm() function when you call pm_runtime_put() so your changes are going to be lost. > > > I'm playing with removing all of that to see if there are any side effects > > in Mali DP like the ones you mentioned for RCAR. > > Note that the first frame will usually not be noticed as it often takes a few > frames for the display to turn on. Yes, and I have a TV connected to the output that takes ages to sync. But I can usually run some quick rmmmod+insmod tests and the TV would be too slow to turn off the output, so I can see if there are any artifacts. Best regards, Liviu > > > > This should guarantee that the device won't be suspended between CRTC > > > disable and CRTC enable during a mode set, without requiring any special > > > collaboration between CRTC enable/disable and atomic begin/flush to > > > handle runtime PM. If drivers implement this, there should be no need for > > > drm_atomic_helper_commit_tail_rpm(). > > > > > > Maxime, Daniel, what do you think about this ? > > > > > >>> I don't think there's anything the helpers can to to help support > > >>> this. > > >>> > > >>> Also, just wrapping functions with rpm_get/put only papers over bugs > > >>> in your driver - either you enable something, then the rpm_get needs > > >>> to be done anyway (since the hw will be shut down otherwise), or you > > >>> disable something, same reasons. The only thing a rpm_get/put does is > > >>> paper over the bugs where you try to access the hw when it's off. As > > >>> soon as this function finishes, the hw is shut down again, drops all > > >> register values on the floor, so either that access wasn't needed, or > > >>> your driver has a bug (because it assumes the register values survive > > >>> when they don't). > > >>> > > >>> So imo all around a bad idea, at least from my experience of doing rpm > > >>> enabling on i915 hw. > > >> > > >> Understood. Thanks! > > -- > Regards, > > Laurent Pinchart >
Hi Liviu, On Wednesday 02 Aug 2017 14:57:30 Liviu Dudau wrote: > On Wed, Aug 02, 2017 at 04:49:16PM +0300, Laurent Pinchart wrote: > > On Wednesday 02 Aug 2017 14:32:06 Liviu Dudau wrote: > >> On Wed, Aug 02, 2017 at 04:27:27PM +0300, Laurent Pinchart wrote: > >>> On Wednesday 02 Aug 2017 13:46:48 Liviu Dudau wrote: > >>>> On Wed, Aug 02, 2017 at 01:27:23PM +0200, Daniel Vetter wrote: > >>>>> On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau wrote: > >>>>>> On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > >>>>>>> +/** > >>>>>>> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to > >>>>>>> hardware > >>>>>>> + * @old_state: new modeset state to be committed > >>>>>>> + * > >>>>>>> + * This is an alternative implementation for the > >>>>>>> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for > >>>>>>> drivers > >>>>>>> + * that support runtime_pm or need the CRTC to be enabled to > >>>>>>> perform a > >>>>>>> + * commit. Otherwise, one should use the default implementation > >>>>>>> + * drm_atomic_helper_commit_tail(). > >>>>>>> + */ > >>>>>>> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state > >>>>>>> *old_state) > >>>>>>> +{ > >>>>>>> + struct drm_device *dev = old_state->dev; > >>>>>>> + > >>>>>>> + drm_atomic_helper_commit_modeset_disables(dev, old_state); > >>>>>>> + > >>>>>>> + drm_atomic_helper_commit_modeset_enables(dev, old_state); > >>>>>>> + > >>>>>>> + drm_atomic_helper_commit_planes(dev, old_state, > >>>>>>> + > >>>>>>> DRM_PLANE_COMMIT_ACTIVE_ONLY); > >>>>>>> + > >>>>>>> + drm_atomic_helper_commit_hw_done(old_state); > >>>>>>> + > >>>>>>> + drm_atomic_helper_wait_for_vblanks(dev, old_state); > >>>>>>> + > >>>>>>> + drm_atomic_helper_cleanup_planes(dev, old_state); > >>>>>>> +} > >>>>>>> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); > >>>>>>> + > >>>>>> > >>>>>> Given that this function is supposed to be used by runtime PM aware > >>>>>> drivers and that the hook is called from the DRM core, should there > >>>>>> not be some pm_runtime_{get,put} calls wrapping the body of this > >>>>>> function? > >>>> > >>>> Hi Daniel, > >>>> > >>>>> No, because the drm atomic helpers have no idea which device is > >>>>> backing which part of the drm device. Some drivers might on have one > >>>>> device for the entire driver, some one device for just the display > >>>>> part (which might or might not match drm_device->dev). And many arm > >>>>> drivers have a device for each block separately (and you need to > >>>>> call rpm_get/put on each). And some something in-between (e.g. core > >>>>> device + external encoders). > >>>> > >>>> Hmm, I understand your point about this function not having to care > >>>> about pm_runtime_{get,put}, but I do not agree with your view that if > >>>> it wanted to care about it, it wouldn't be able to do the right thing > >>>> because it doesn't have the right device. After all, this function is > >>>> about handling the updates that this atomic commit is concerned > >>>> about, so having the old_state->dev drm_device pointer and its > >>>> associated device should be enough. Am I missing something? > >>> > >>> In embedded system it's quite common for display hardware to be made > >>> of multiple IP cores. Depending on the SoC you could have to manage > >>> runtime PM at the CRTC level for instance. The DRM core doesn't know > >>> about that, and sees a single device only. > >>> > >>> I've expressed my doubts previously about the need for a RPM version > >>> of drm_atomic_helper_commit_tail(), as the resulting order of CRTC > >>> enable/disable and plane update operations can lead to corrupt frames > >>> when enabling the CRTC. I had a commit tail implementation in the > >>> rcar-du driver that was very similar to > >>> drm_atomic_helper_commit_tail_rpm(), and had to move back to the > >>> standard order to fix the corrupt frame issue. The result isn't as clean > >>> as I would like, as power handling is split between the CRTC > >>> enable/disable and the atomic begin/flush in a way that is not > >>> straightforward. > >>> > >>> It now occurred to me that a simpler implementation could be possible. > >>> I'll have to experiment with it first, but the idea is as follows. > >>> > >>> 1 Call pm_runtime_get_sync() at the beginning of .commit_tail() and > >>> pm_runtime_put() at the end. > >>> > >>> 2. Use the standard CRTC disable, plane update, CRTC enable order in > >>> .commit_tail(). > >>> > >>> 3. Call pm_runtime_get() in the CRTC .enable() handler and > >>> pm_runtime_put() in the CRTC .disable() handler; > >> > >> Well, that is what mali-dp driver currently does, but according to > >> Daniel (and I can see his POV) that is wrong. > > > > Is it ? I might not have understood his arguments the same way (or > > possibly failed to even see them). Are you referring to this comments in > > this mail thread, or to something else ? > > I'm talking about his reply above. My understanding: you can't do > pm_runtime_{get,set} in the atomic_commit_tail hook because: > > 1. you don't know if you have the correct drm_device->dev (or all the > relevant devices) to call pm_runtime_get() on. You can't call pm_runtime_get() in the DRM core for that exact reason, but you can call it in a driver's implementation of .atomic_commit_tail(), which was my proposal. The .atomic_commit_tail() handler would then become something like { for_each_ip_affected(ip, ...) pm_runtime_get_sync(ip); drm_atomic_helper_commit_tail(...); for_each_ip_affected(ip, ...) pm_runtime_put(ip); } > 2. If pm_runtime_get() affects in any way the atomic commit behaviour by > being at the top of the commit_tail_rpm() function then you are: > a) papering over bugs in one's driver > b) going to turn off things at the end of commit_tail_rpm() function > when you call pm_runtime_put() so your changes are going to be lost. You won't, because you also call pm_runtime_get() in the CRTC .enable() handler. > >> I'm playing with removing all of that to see if there are any side > >> effects in Mali DP like the ones you mentioned for RCAR. > > > > Note that the first frame will usually not be noticed as it often takes a > > few frames for the display to turn on. > > Yes, and I have a TV connected to the output that takes ages to sync. But I > can usually run some quick rmmmod+insmod tests and the TV would be too slow > to turn off the output, so I can see if there are any artifacts. One good way to test this is to implement support for CRC calculation if your hardware supports it. > >>> This should guarantee that the device won't be suspended between CRTC > >>> disable and CRTC enable during a mode set, without requiring any > >>> special collaboration between CRTC enable/disable and atomic > >>> begin/flush to handle runtime PM. If drivers implement this, there > >>> should be no need for drm_atomic_helper_commit_tail_rpm(). > >>> > >>> Maxime, Daniel, what do you think about this ? [snip]
On Wed, Aug 2, 2017 at 3:27 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Livu, > > On Wednesday 02 Aug 2017 13:46:48 Liviu Dudau wrote: >> On Wed, Aug 02, 2017 at 01:27:23PM +0200, Daniel Vetter wrote: >> > On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: >> >> On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: >> >>> +/** >> >>> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to >> >>> hardware >> >>> + * @old_state: new modeset state to be committed >> >>> + * >> >>> + * This is an alternative implementation for the >> >>> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers >> >>> + * that support runtime_pm or need the CRTC to be enabled to perform a >> >>> + * commit. Otherwise, one should use the default implementation >> >>> + * drm_atomic_helper_commit_tail(). >> >>> + */ >> >>> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state >> >>> *old_state) >> >>> +{ >> >>> + struct drm_device *dev = old_state->dev; >> >>> + >> >>> + drm_atomic_helper_commit_modeset_disables(dev, old_state); >> >>> + >> >>> + drm_atomic_helper_commit_modeset_enables(dev, old_state); >> >>> + >> >>> + drm_atomic_helper_commit_planes(dev, old_state, >> >>> + DRM_PLANE_COMMIT_ACTIVE_ONLY); >> >>> + >> >>> + drm_atomic_helper_commit_hw_done(old_state); >> >>> + >> >>> + drm_atomic_helper_wait_for_vblanks(dev, old_state); >> >>> + >> >>> + drm_atomic_helper_cleanup_planes(dev, old_state); >> >>> +} >> >>> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); >> >>> + >> >> >> >> Given that this function is supposed to be used by runtime PM aware >> >> drivers and that the hook is called from the DRM core, should there not >> >> be some pm_runtime_{get,put} calls wrapping the body of this function? >> >> Hi Daniel, >> >> > No, because the drm atomic helpers have no idea which device is >> > backing which part of the drm device. Some drivers might on have one >> > device for the entire driver, some one device for just the display >> > part (which might or might not match drm_device->dev). And many arm >> > drivers have a device for each block separately (and you need to call >> > rpm_get/put on each). And some something in-between (e.g. core device >> > + external encoders). >> >> Hmm, I understand your point about this function not having to care about >> pm_runtime_{get,put}, but I do not agree with your view that if it wanted to >> care about it, it wouldn't be able to do the right thing because it doesn't >> have the right device. After all, this function is about handling the >> updates that this atomic commit is concerned about, so having the >> old_state->dev drm_device pointer and its associated device should be >> enough. Am I missing something? > > In embedded system it's quite common for display hardware to be made of > multiple IP cores. Depending on the SoC you could have to manage runtime PM at > the CRTC level for instance. The DRM core doesn't know about that, and sees a > single device only. > > I've expressed my doubts previously about the need for a RPM version of > drm_atomic_helper_commit_tail(), as the resulting order of CRTC enable/disable > and plane update operations can lead to corrupt frames when enabling the CRTC. > I had a commit tail implementation in the rcar-du driver that was very similar > to drm_atomic_helper_commit_tail_rpm(), and had to move back to the standard > order to fix the corrupt frame issue. The result isn't as clean as I would > like, as power handling is split between the CRTC enable/disable and the > atomic begin/flush in a way that is not straightforward. > > It now occurred to me that a simpler implementation could be possible. I'll > have to experiment with it first, but the idea is as follows. > > 1 Call pm_runtime_get_sync() at the beginning of .commit_tail() and > pm_runtime_put() at the end. > > 2. Use the standard CRTC disable, plane update, CRTC enable order in > .commit_tail(). > > 3. Call pm_runtime_get() in the CRTC .enable() handler and pm_runtime_put() in > the CRTC .disable() handler; > > This should guarantee that the device won't be suspended between CRTC disable > and CRTC enable during a mode set, without requiring any special collaboration > between CRTC enable/disable and atomic begin/flush to handle runtime PM. If > drivers implement this, there should be no need for > drm_atomic_helper_commit_tail_rpm(). > > Maxime, Daniel, what do you think about this ? Ok, since you just said on irc that the corrupted frame is some random color, that's indeed not cool. What all other drivers (well at least i915) do is: 1. Enable the screen, but scan out nothing as a pure black. This might mean you need to set up the blend unit with a background color of black. 2. Do an atomic flip to the new screen contents. This way you get the sequence implement in the new _rpm implementation. And for most normal hw this gives you the simplest implementation, and has the benefit that writing to disabled hw blocks indicates a bug. Wrapping an entire sequence with rpm_get/put like Laurent describes is a really good way to hide bugs (e.g. write new stuff to hw you're about to disable and then drop the hw settings on the floor). Yes it makes implementation a bit easier, but easier != more correct in many cases. Nasty self-checks are good for getting kms implementation rights. Another benefit of this sequence is that the initial plane enable after a crtc enable is like any other "background black -> planes" atomic switch, and atomic does allow you to disable all planes. So your driver better supports that (or is one of the few which has to reject a config without any planes), since userspace might want to enable the CRTC with no planes, and then you have the exact same bug again. It sounds like Laurent's 3rd gen rcar-du is special, and so needs special code (or well just a bugfix to enable the composer with all black in the CRTC enable code. Cheers, Daniel >> > I don't think there's anything the helpers can to to help support this. >> > >> > Also, just wrapping functions with rpm_get/put only papers over bugs >> > in your driver - either you enable something, then the rpm_get needs >> > to be done anyway (since the hw will be shut down otherwise), or you >> > disable something, same reasons. The only thing a rpm_get/put does is >> > paper over the bugs where you try to access the hw when it's off. As >> > soon as this function finishes, the hw is shut down again, drops all >> > register values on the floor, so either that access wasn't needed, or >> > your driver has a bug (because it assumes the register values survive >> > when they don't). >> > >> > So imo all around a bad idea, at least from my experience of doing rpm >> > enabling on i915 hw. >> >> Understood. Thanks! > > -- > Regards, > > Laurent Pinchart >
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 86d3093c6c9b..d06a65ed3a57 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1245,23 +1245,13 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks); * @old_state: atomic state object with old state structures * * This is the default implementation for the - * &drm_mode_config_helper_funcs.atomic_commit_tail hook. + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers + * that do not support runtime_pm or do not need the CRTC to be + * enabled to perform a commit. Otherwise, see + * drm_atomic_helper_commit_tail_rpm(). * * Note that the default ordering of how the various stages are called is to - * match the legacy modeset helper library closest. One peculiarity of that is - * that it doesn't mesh well with runtime PM at all. - * - * For drivers supporting runtime PM the recommended sequence is instead :: - * - * drm_atomic_helper_commit_modeset_disables(dev, old_state); - * - * drm_atomic_helper_commit_modeset_enables(dev, old_state); - * - * drm_atomic_helper_commit_planes(dev, old_state, - * DRM_PLANE_COMMIT_ACTIVE_ONLY); - * - * for committing the atomic update to hardware. See the kerneldoc entries for - * these three functions for more details. + * match the legacy modeset helper library closest. */ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) { @@ -1281,6 +1271,35 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) } EXPORT_SYMBOL(drm_atomic_helper_commit_tail); +/** + * drm_atomic_helper_commit_tail_rpm - commit atomic update to hardware + * @old_state: new modeset state to be committed + * + * This is an alternative implementation for the + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers + * that support runtime_pm or need the CRTC to be enabled to perform a + * commit. Otherwise, one should use the default implementation + * drm_atomic_helper_commit_tail(). + */ +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) +{ + struct drm_device *dev = old_state->dev; + + drm_atomic_helper_commit_modeset_disables(dev, old_state); + + drm_atomic_helper_commit_modeset_enables(dev, old_state); + + drm_atomic_helper_commit_planes(dev, old_state, + DRM_PLANE_COMMIT_ACTIVE_ONLY); + + drm_atomic_helper_commit_hw_done(old_state); + + drm_atomic_helper_wait_for_vblanks(dev, old_state); + + drm_atomic_helper_cleanup_planes(dev, old_state); +} +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); + static void commit_tail(struct drm_atomic_state *old_state) { struct drm_device *dev = old_state->dev; diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index d48fd7c918f8..ed1a648d518c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -187,33 +187,8 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) return exynos_fb->dma_addr[index]; } -static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) -{ - struct drm_device *dev = state->dev; - - drm_atomic_helper_commit_modeset_disables(dev, state); - - drm_atomic_helper_commit_modeset_enables(dev, state); - - /* - * Exynos can't update planes with CRTCs and encoders disabled, - * its updates routines, specially for FIMD, requires the clocks - * to be enabled. So it is necessary to handle the modeset operations - * *before* the commit_planes() step, this way it will always - * have the relevant clocks enabled to perform the update. - */ - drm_atomic_helper_commit_planes(dev, state, - DRM_PLANE_COMMIT_ACTIVE_ONLY); - - drm_atomic_helper_commit_hw_done(state); - - drm_atomic_helper_wait_for_vblanks(dev, state); - - drm_atomic_helper_cleanup_planes(dev, state); -} - static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { - .atomic_commit_tail = exynos_drm_atomic_commit_tail, + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, }; static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index 81f9548672b0..35ad386c401e 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c @@ -174,27 +174,8 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev) drm_fb_helper_hotplug_event(fb_helper); } -static void -rockchip_atomic_commit_tail(struct drm_atomic_state *state) -{ - struct drm_device *dev = state->dev; - - drm_atomic_helper_commit_modeset_disables(dev, state); - - drm_atomic_helper_commit_modeset_enables(dev, state); - - drm_atomic_helper_commit_planes(dev, state, - DRM_PLANE_COMMIT_ACTIVE_ONLY); - - drm_atomic_helper_commit_hw_done(state); - - drm_atomic_helper_wait_for_vblanks(dev, state); - - drm_atomic_helper_cleanup_planes(dev, state); -} - static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = { - .atomic_commit_tail = rockchip_atomic_commit_tail, + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, }; static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = { diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index f0a8678ae98e..af4aaff9ec0b 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -41,6 +41,7 @@ int drm_atomic_helper_check_planes(struct drm_device *dev, int drm_atomic_helper_check(struct drm_device *dev, struct drm_atomic_state *state); void drm_atomic_helper_commit_tail(struct drm_atomic_state *state); +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state); int drm_atomic_helper_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock);
The current drm_atomic_helper_commit_tail helper works only if the CRTC is accessible, and documents an alternative implementation that is supposed to be used if that happens. That implementation is then duplicated by some drivers. Instead of documenting it, let's implement an helper that all the relevant users can use directly. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- drivers/gpu/drm/drm_atomic_helper.c | 49 +++++++++++++++-------- drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +---------- include/drm/drm_atomic_helper.h | 1 +- 4 files changed, 37 insertions(+), 61 deletions(-)