| Message ID | aeeb8f778dae8f170294b6da92819973b742feb2.1499956639.git-series.maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Thu, Jul 13, 2017 at 04:41:13PM +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> > --- > drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++-------- > drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +---------- > include/drm/drm_atomic_helper.h | 1 +- > 5 files changed, 36 insertions(+), 78 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 86d3093c6c9b..a288805078f9 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1245,23 +1245,11 @@ 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. > * > * 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. Please sprinkle links into both functions (and everywhere the old one was referenced) to make this more discoverable, and explain when to use the other variant. > */ > void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) > { > @@ -1281,6 +1269,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_runtime_pm - commit atomic update to hardware > + * @old_state: new modeset state to be committed > + * > + * This is a variant of drm_atomic_helper_commit_tail suited for > + * drivers that implement runtime_pm. > + * > + * Note that the default ordering of how the various stages are called is to > + * match the legacy modeset helper library closest. > + */ > +void drm_atomic_helper_commit_tail_runtime_pm(struct drm_atomic_state *old_state) Bikeshed: I'd go with _rpm since this thing is already super long. But fee free to ignore that. With the docs polished I think this looks good. -Daniel > +{ > + 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_runtime_pm); > + > 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..71f6873255f1 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_runtime_pm, > }; > > static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > index f4125c8ca902..cb0f6266fbae 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > @@ -249,28 +249,12 @@ static int rcar_du_atomic_check(struct drm_device *dev, > return rcar_du_atomic_check_planes(dev, state); > } > > -static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state) > -{ > - struct drm_device *dev = old_state->dev; > - > - /* Apply the atomic update. */ > - 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); > -} > - > /* ----------------------------------------------------------------------------- > * Initialization > */ > > static const struct drm_mode_config_helper_funcs rcar_du_mode_config_helper = { > - .atomic_commit_tail = rcar_du_atomic_commit_tail, > + .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm, > }; > > static const struct drm_mode_config_funcs rcar_du_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..647745479c39 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_runtime_pm, > }; > > 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..9ff64c6d3043 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_runtime_pm(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 Maxime, Thank you for the patch. On Thursday 13 Jul 2017 16:41:13 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> > --- > drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++-------- > drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker" that changes the rcar-du implementation to the standard disable/update planes/enable order, so I'd appreciate if you could drop the rcar-du part of this patch to avoid conflicts. This being said, the reason why I switched back from the "runtime PM" to the "standard" order is probably of interest to you. Quoting the commit message, > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC > start to CRTC resume") changed the order of the plane commit and CRTC > enable operations to accommodate the runtime PM requirements. However, > this introduced corruption in the first displayed frame, as the CRTC is > now enabled without any plane configured. On Gen2 hardware the first > frame will be black and likely unnoticed, but on Gen3 hardware we end up > starting the display before the VSP compositor, which is more > noticeable. > > To fix this, revert the order of the commit operations back, and handle > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable() > helper operation handlers. I believe that the "runtime PM" order is problematic in most drivers. The problem usually goes unnoticed as most monitors will not even display the first frame, and I assume many devices will just output it black, but it's an issue nonetheless. Note that my driver hasn't lost the "runtime PM" requirements, so I had to support them with the "standard" order. The best way I've found was to runtime resume in the one of .atomic_begin() and .enable() that is run first. Not very neat, as similar code would be needed in most drivers. I wonder whether it wouldn't be useful to add resume/suspend helper callbacks for the CRTC. > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +---------- > include/drm/drm_atomic_helper.h | 1 +- > 5 files changed, 36 insertions(+), 78 deletions(-)
On Fri, Jul 14, 2017 at 1:43 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC >> start to CRTC resume") changed the order of the plane commit and CRTC >> enable operations to accommodate the runtime PM requirements. However, >> this introduced corruption in the first displayed frame, as the CRTC is >> now enabled without any plane configured. On Gen2 hardware the first >> frame will be black and likely unnoticed, but on Gen3 hardware we end up >> starting the display before the VSP compositor, which is more >> noticeable. >> >> To fix this, revert the order of the commit operations back, and handle >> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable() >> helper operation handlers. > > I believe that the "runtime PM" order is problematic in most drivers. The > problem usually goes unnoticed as most monitors will not even display the > first frame, and I assume many devices will just output it black, but it's an > issue nonetheless. > > Note that my driver hasn't lost the "runtime PM" requirements, so I had to > support them with the "standard" order. The best way I've found was to runtime > resume in the one of .atomic_begin() and .enable() that is run first. Not very > neat, as similar code would be needed in most drivers. I wonder whether it > wouldn't be useful to add resume/suspend helper callbacks for the CRTC. I discussed this with Laurent and explained that "first black frame" is exactly what i915 does. I'd say given special customer requests we don't yet have to bother with this in general ... Wrt adding more hooks for rpm: I honestly don't like that, because then someone else wants to add a hook for clocks, or for the sideband and then we have a horror show of hooks where every driver uses just a very small subset. The point of atomic helpers is to make them modular, if one part doesn't fit, just redo in your driver. Goal should be that shared parts are good for about 90% of drivers/use-cases (maybe even less, there's sooooo many special cases). tldr; I still think the _runtime_pm variant is the recommended way to do this. -Daniel
Hi Laurent, On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote: > Hi Maxime, > > Thank you for the patch. > > On Thursday 13 Jul 2017 16:41:13 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> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++-------- > > drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- > > I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to > avoid flicker" that changes the rcar-du implementation to the standard > disable/update planes/enable order, so I'd appreciate if you could drop the > rcar-du part of this patch to avoid conflicts. I will. > This being said, the reason why I switched back from the "runtime PM" to the > "standard" order is probably of interest to you. Quoting the commit message, > > > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC > > start to CRTC resume") changed the order of the plane commit and CRTC > > enable operations to accommodate the runtime PM requirements. However, > > this introduced corruption in the first displayed frame, as the CRTC is > > now enabled without any plane configured. On Gen2 hardware the first > > frame will be black and likely unnoticed, but on Gen3 hardware we end up > > starting the display before the VSP compositor, which is more > > noticeable. > > > > To fix this, revert the order of the commit operations back, and handle > > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable() > > helper operation handlers. > > I believe that the "runtime PM" order is problematic in most drivers. The > problem usually goes unnoticed as most monitors will not even display the > first frame, and I assume many devices will just output it black, but it's an > issue nonetheless. > > Note that my driver hasn't lost the "runtime PM" requirements, so I had to > support them with the "standard" order. The best way I've found was to runtime > resume in the one of .atomic_begin() and .enable() that is run first. Not very > neat, as similar code would be needed in most drivers. I wonder whether it > wouldn't be useful to add resume/suspend helper callbacks for the CRTC. I'm not sure it would apply. Our driver doesn't use runtime_pm at all, but in order for the commits to happen, we need to have the CRTC active, but it will remain powered up the whole time. I'm not sure if we'll ever see such a frame. But since this seems to be a pretty generic, maybe we should address it in the helper itself? Maxime
Hi Maxime, On Tuesday 18 Jul 2017 09:05:22 Maxime Ripard wrote: > On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote: > > On Thursday 13 Jul 2017 16:41:13 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> > >> --- > >> > >> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++-------- > >> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > >> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- > > > > I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to > > avoid flicker" that changes the rcar-du implementation to the standard > > disable/update planes/enable order, so I'd appreciate if you could drop > > the rcar-du part of this patch to avoid conflicts. > > I will. > > > This being said, the reason why I switched back from the "runtime PM" to > > the "standard" order is probably of interest to you. Quoting the commit > > message, > > > >> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC > >> start to CRTC resume") changed the order of the plane commit and CRTC > >> enable operations to accommodate the runtime PM requirements. However, > >> this introduced corruption in the first displayed frame, as the CRTC is > >> now enabled without any plane configured. On Gen2 hardware the first > >> frame will be black and likely unnoticed, but on Gen3 hardware we end up > >> starting the display before the VSP compositor, which is more > >> noticeable. > >> > >> To fix this, revert the order of the commit operations back, and handle > >> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable() > >> helper operation handlers. > > > > I believe that the "runtime PM" order is problematic in most drivers. The > > problem usually goes unnoticed as most monitors will not even display the > > first frame, and I assume many devices will just output it black, but it's > > an issue nonetheless. > > > > Note that my driver hasn't lost the "runtime PM" requirements, so I had to > > support them with the "standard" order. The best way I've found was to > > runtime resume in the one of .atomic_begin() and .enable() that is run > > first. Not very neat, as similar code would be needed in most drivers. I > > wonder whether it wouldn't be useful to add resume/suspend helper > > callbacks for the CRTC. > > I'm not sure it would apply. Our driver doesn't use runtime_pm at all, > but in order for the commits to happen, we need to have the CRTC > active, but it will remain powered up the whole time. I'm not sure if > we'll ever see such a frame. > > But since this seems to be a pretty generic, maybe we should address > it in the helper itself? I think that would make sense. There are a few options that result in too many combinations for separate commit tail helpers to be provided in my opinion: - disable/enable/planes vs. disable/planes/enable - DRM_PLANE_COMMIT_ACTIVE_ONLY vs. all CRTCs - drm_atomic_helper_wait_for_vblanks vs drm_atomic_helper_wait_for_flip_done Maybe we could add a few CRTC commit helper flags along the line of DRM_PLANE_COMMIT_ACTIVE_ONLY, add a field to the drm_crtc structure to store them, and have drm_atomic_helper_commit_tail() use those flags to control the sequence of operations.
On Tue, Jul 18, 2017 at 01:14:12PM +0300, Laurent Pinchart wrote: > Hi Maxime, > > On Tuesday 18 Jul 2017 09:05:22 Maxime Ripard wrote: > > On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote: > > > On Thursday 13 Jul 2017 16:41:13 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> > > >> --- > > >> > > >> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++-------- > > >> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > > >> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- > > > > > > I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to > > > avoid flicker" that changes the rcar-du implementation to the standard > > > disable/update planes/enable order, so I'd appreciate if you could drop > > > the rcar-du part of this patch to avoid conflicts. > > > > I will. > > > > > This being said, the reason why I switched back from the "runtime PM" to > > > the "standard" order is probably of interest to you. Quoting the commit > > > message, > > > > > >> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC > > >> start to CRTC resume") changed the order of the plane commit and CRTC > > >> enable operations to accommodate the runtime PM requirements. However, > > >> this introduced corruption in the first displayed frame, as the CRTC is > > >> now enabled without any plane configured. On Gen2 hardware the first > > >> frame will be black and likely unnoticed, but on Gen3 hardware we end up > > >> starting the display before the VSP compositor, which is more > > >> noticeable. > > >> > > >> To fix this, revert the order of the commit operations back, and handle > > >> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable() > > >> helper operation handlers. > > > > > > I believe that the "runtime PM" order is problematic in most drivers. The > > > problem usually goes unnoticed as most monitors will not even display the > > > first frame, and I assume many devices will just output it black, but it's > > > an issue nonetheless. > > > > > > Note that my driver hasn't lost the "runtime PM" requirements, so I had to > > > support them with the "standard" order. The best way I've found was to > > > runtime resume in the one of .atomic_begin() and .enable() that is run > > > first. Not very neat, as similar code would be needed in most drivers. I > > > wonder whether it wouldn't be useful to add resume/suspend helper > > > callbacks for the CRTC. > > > > I'm not sure it would apply. Our driver doesn't use runtime_pm at all, > > but in order for the commits to happen, we need to have the CRTC > > active, but it will remain powered up the whole time. I'm not sure if > > we'll ever see such a frame. > > > > But since this seems to be a pretty generic, maybe we should address > > it in the helper itself? > > I think that would make sense. > > There are a few options that result in too many combinations for separate > commit tail helpers to be provided in my opinion: > > - disable/enable/planes vs. disable/planes/enable > - DRM_PLANE_COMMIT_ACTIVE_ONLY vs. all CRTCs > - drm_atomic_helper_wait_for_vblanks vs drm_atomic_helper_wait_for_flip_done > > Maybe we could add a few CRTC commit helper flags along the line of > DRM_PLANE_COMMIT_ACTIVE_ONLY, add a field to the drm_crtc structure to store > them, and have drm_atomic_helper_commit_tail() use those flags to control the > sequence of operations. Why not write your own? Yes it's a bit of copypaste, but imo that's really not horrible. I'm already not happy with the flags for commit_planes because the docs for them are not great and it's hard to know when to use them and when not to. ->commit_tail was specifically done to allow drivers to overwrite the hw commit stage without having to reinvent all the other commit tracking. I expect most non-simple drivers to have their own commit_tail function. -Daniel
Hi Daniel, On Tuesday 18 Jul 2017 14:08:39 Daniel Vetter wrote: > On Tue, Jul 18, 2017 at 01:14:12PM +0300, Laurent Pinchart wrote: > > On Tuesday 18 Jul 2017 09:05:22 Maxime Ripard wrote: > >> On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote: > >>> On Thursday 13 Jul 2017 16:41:13 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> > >>>> --- > >>>> > >>>> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++-------- > >>>> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > >>>> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- > >>> > >>> I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling > >>> CRTC to avoid flicker" that changes the rcar-du implementation to the > >>> standard disable/update planes/enable order, so I'd appreciate if you > >>> could drop the rcar-du part of this patch to avoid conflicts. > >> > >> I will. > >> > >>> This being said, the reason why I switched back from the "runtime PM" > >>> to the "standard" order is probably of interest to you. Quoting the > >>> commit message, > >>> > >>>> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC > >>>> start to CRTC resume") changed the order of the plane commit and CRTC > >>>> enable operations to accommodate the runtime PM requirements. > >>>> However, this introduced corruption in the first displayed frame, as > >>>> the CRTC is now enabled without any plane configured. On Gen2 > >>>> hardware the first frame will be black and likely unnoticed, but on > >>>> Gen3 hardware we end up starting the display before the VSP > >>>> compositor, which is more noticeable. > >>>> > >>>> To fix this, revert the order of the commit operations back, and > >>>> handle runtime PM requirements in the CRTC .atomic_begin() and > >>>> .atomic_enable() helper operation handlers. > >>> > >>> I believe that the "runtime PM" order is problematic in most drivers. > >>> The problem usually goes unnoticed as most monitors will not even > >>> display the first frame, and I assume many devices will just output it > >>> black, but it's an issue nonetheless. > >>> > >>> Note that my driver hasn't lost the "runtime PM" requirements, so I > >>> had to support them with the "standard" order. The best way I've found > >>> was to runtime resume in the one of .atomic_begin() and .enable() that > >>> is run first. Not very neat, as similar code would be needed in most > >>> drivers. I wonder whether it wouldn't be useful to add resume/suspend > >>> helper callbacks for the CRTC. > >> > >> I'm not sure it would apply. Our driver doesn't use runtime_pm at all, > >> but in order for the commits to happen, we need to have the CRTC > >> active, but it will remain powered up the whole time. I'm not sure if > >> we'll ever see such a frame. > >> > >> But since this seems to be a pretty generic, maybe we should address > >> it in the helper itself? > > > > I think that would make sense. > > > > There are a few options that result in too many combinations for separate > > commit tail helpers to be provided in my opinion: > > > > - disable/enable/planes vs. disable/planes/enable > > - DRM_PLANE_COMMIT_ACTIVE_ONLY vs. all CRTCs > > - drm_atomic_helper_wait_for_vblanks vs > > drm_atomic_helper_wait_for_flip_done > > > > Maybe we could add a few CRTC commit helper flags along the line of > > DRM_PLANE_COMMIT_ACTIVE_ONLY, add a field to the drm_crtc structure to > > store them, and have drm_atomic_helper_commit_tail() use those flags to > > control the sequence of operations. > > Why not write your own? Yes it's a bit of copypaste, but imo that's really > not horrible. I don't find it horrible either, it's not too much code. The question was more about which version(s) to consider standard enough to provide a core helper for. What bothers me a bit more with the copy&paste implementations isn't so much the commit tail handling itself, but the consequences it has on the rest of the driver. Drivers pick the order they want based on their requirements, and that order then leads to different race conditions between the drivers. We don't document the potential issues there, so new drivers (and for that matter, possibly existing ones) will likely have bugs if the author is not aware of the subtle issues related to the particular operation order they happen to use. > I'm already not happy with the flags for commit_planes because the docs for > them are not great and it's hard to know when to use them and when not to. > > ->commit_tail was specifically done to allow drivers to overwrite the hw > commit stage without having to reinvent all the other commit tracking. I > expect most non-simple drivers to have their own commit_tail function. Maybe that should be all of them instead of most of them ?
On Tue, Jul 18, 2017 at 2:47 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Tuesday 18 Jul 2017 14:08:39 Daniel Vetter wrote: >> On Tue, Jul 18, 2017 at 01:14:12PM +0300, Laurent Pinchart wrote: >> > On Tuesday 18 Jul 2017 09:05:22 Maxime Ripard wrote: >> >> On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote: >> >>> On Thursday 13 Jul 2017 16:41:13 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> >> >>>> --- >> >>>> >> >>>> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++-------- >> >>>> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- >> >>>> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- >> >>> >> >>> I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling >> >>> CRTC to avoid flicker" that changes the rcar-du implementation to the >> >>> standard disable/update planes/enable order, so I'd appreciate if you >> >>> could drop the rcar-du part of this patch to avoid conflicts. >> >> >> >> I will. >> >> >> >>> This being said, the reason why I switched back from the "runtime PM" >> >>> to the "standard" order is probably of interest to you. Quoting the >> >>> commit message, >> >>> >> >>>> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC >> >>>> start to CRTC resume") changed the order of the plane commit and CRTC >> >>>> enable operations to accommodate the runtime PM requirements. >> >>>> However, this introduced corruption in the first displayed frame, as >> >>>> the CRTC is now enabled without any plane configured. On Gen2 >> >>>> hardware the first frame will be black and likely unnoticed, but on >> >>>> Gen3 hardware we end up starting the display before the VSP >> >>>> compositor, which is more noticeable. >> >>>> >> >>>> To fix this, revert the order of the commit operations back, and >> >>>> handle runtime PM requirements in the CRTC .atomic_begin() and >> >>>> .atomic_enable() helper operation handlers. >> >>> >> >>> I believe that the "runtime PM" order is problematic in most drivers. >> >>> The problem usually goes unnoticed as most monitors will not even >> >>> display the first frame, and I assume many devices will just output it >> >>> black, but it's an issue nonetheless. >> >>> >> >>> Note that my driver hasn't lost the "runtime PM" requirements, so I >> >>> had to support them with the "standard" order. The best way I've found >> >>> was to runtime resume in the one of .atomic_begin() and .enable() that >> >>> is run first. Not very neat, as similar code would be needed in most >> >>> drivers. I wonder whether it wouldn't be useful to add resume/suspend >> >>> helper callbacks for the CRTC. >> >> >> >> I'm not sure it would apply. Our driver doesn't use runtime_pm at all, >> >> but in order for the commits to happen, we need to have the CRTC >> >> active, but it will remain powered up the whole time. I'm not sure if >> >> we'll ever see such a frame. >> >> >> >> But since this seems to be a pretty generic, maybe we should address >> >> it in the helper itself? >> > >> > I think that would make sense. >> > >> > There are a few options that result in too many combinations for separate >> > commit tail helpers to be provided in my opinion: >> > >> > - disable/enable/planes vs. disable/planes/enable >> > - DRM_PLANE_COMMIT_ACTIVE_ONLY vs. all CRTCs >> > - drm_atomic_helper_wait_for_vblanks vs >> > drm_atomic_helper_wait_for_flip_done >> > >> > Maybe we could add a few CRTC commit helper flags along the line of >> > DRM_PLANE_COMMIT_ACTIVE_ONLY, add a field to the drm_crtc structure to >> > store them, and have drm_atomic_helper_commit_tail() use those flags to >> > control the sequence of operations. >> >> Why not write your own? Yes it's a bit of copypaste, but imo that's really >> not horrible. > > I don't find it horrible either, it's not too much code. The question was more > about which version(s) to consider standard enough to provide a core helper > for. > > What bothers me a bit more with the copy&paste implementations isn't so much > the commit tail handling itself, but the consequences it has on the rest of > the driver. Drivers pick the order they want based on their requirements, and > that order then leads to different race conditions between the drivers. We > don't document the potential issues there, so new drivers (and for that > matter, possibly existing ones) will likely have bugs if the author is not > aware of the subtle issues related to the particular operation order they > happen to use. Imo the answer to that is implementing CRC support in your driver and running igt. That checks whether you get those race conditions right, at least everywhere where it's well-defined across drivers. >> I'm already not happy with the flags for commit_planes because the docs for >> them are not great and it's hard to know when to use them and when not to. >> >> ->commit_tail was specifically done to allow drivers to overwrite the hw >> commit stage without having to reinvent all the other commit tracking. I >> expect most non-simple drivers to have their own commit_tail function. > > Maybe that should be all of them instead of most of them ? Valid suggestion - the default reflects the legacy crtc helpers, for easier transition, and I think we've run out of drivers to transition. Most of the existing drivers are probably better if you rewrite them in e.g. the simple display pipe helpers. Imo we could nuke the default commit_tail and replace it purely with kernel-code, together with the transitional plane/crtc helpers. Otoh it's still a useful template, to know what all should be in there ... -Daniel
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 86d3093c6c9b..a288805078f9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1245,23 +1245,11 @@ 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. * * 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 +1269,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_runtime_pm - commit atomic update to hardware + * @old_state: new modeset state to be committed + * + * This is a variant of drm_atomic_helper_commit_tail suited for + * drivers that implement runtime_pm. + * + * Note that the default ordering of how the various stages are called is to + * match the legacy modeset helper library closest. + */ +void drm_atomic_helper_commit_tail_runtime_pm(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_runtime_pm); + 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..71f6873255f1 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_runtime_pm, }; static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index f4125c8ca902..cb0f6266fbae 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -249,28 +249,12 @@ static int rcar_du_atomic_check(struct drm_device *dev, return rcar_du_atomic_check_planes(dev, state); } -static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state) -{ - struct drm_device *dev = old_state->dev; - - /* Apply the atomic update. */ - 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); -} - /* ----------------------------------------------------------------------------- * Initialization */ static const struct drm_mode_config_helper_funcs rcar_du_mode_config_helper = { - .atomic_commit_tail = rcar_du_atomic_commit_tail, + .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm, }; static const struct drm_mode_config_funcs rcar_du_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..647745479c39 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_runtime_pm, }; 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..9ff64c6d3043 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_runtime_pm(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 | 47 +++++++++++++++-------- drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +---------- include/drm/drm_atomic_helper.h | 1 +- 5 files changed, 36 insertions(+), 78 deletions(-)