* [PATCH] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc() @ 2015-11-23 10:44 Jyri Sarha 2015-11-23 16:09 ` Daniel Vetter 0 siblings, 1 reply; 4+ messages in thread From: Jyri Sarha @ 2015-11-23 10:44 UTC (permalink / raw) To: dri-devel; +Cc: tomi.valkeinen, Jyri Sarha, laurent.pinchart Add drm_atomic_helper_disable_planes_on_crtc() for disabling all planes associated with the given CRTC. This can be used for instance in the CRTC helper disable callback to disable all planes before shutting down the display pipeline. Signed-off-by: Jyri Sarha <jsarha@ti.com> --- This a follow version to "[PATCH RFC] drm/crtc_helper: Add drm_crtc_helper_disable_planes()"-patch [1], with Daniel Vetter's review comments [2] implemented. Hope I got everything right this time. Thanks a lot for the prompt review. Best regards, Jyri [1] http://www.spinics.net/lists/dri-devel/msg94720.html [2] http://www.spinics.net/lists/dri-devel/msg94722.html drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 54 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a53ed7a..229c810 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1281,6 +1281,58 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, EXPORT_SYMBOL(drm_atomic_helper_commit_planes); /** + * drm_atomic_helper_disable_planes_on_crtc - helper to disable CRTC's planes + * @crtc: CRTC + * @atomic: if set, synchronize with CRTC's atomic_begin/flush hooks + * + * Disables all planes associated with the given CRTC. This can be + * used for instance in the CRTC helper disable callback to disable + * all planes before shutting down the display pipeline. + * + * If there are planes to disable and the atomic-parameter is set the + * function calls the CRTC's atomic_begin hook before and atomic_flush + * hook after disabling the planes. + * + * It is a bug to call this function without having implemented the + * ->atomic_disable() plane hook. + */ +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc, + bool atomic) +{ + const struct drm_crtc_helper_funcs *crtc_funcs = + crtc->helper_private; + struct drm_plane *plane; + bool flush = false; + + if (!crtc_funcs || !crtc_funcs->atomic_begin) + atomic = false; + + drm_for_each_plane(plane, crtc->dev) { + const struct drm_plane_helper_funcs *plane_funcs = + plane->helper_private; + + if (plane->state->crtc != crtc || !plane_funcs) + continue; + + if (atomic && !flush) { + crtc_funcs->atomic_begin(crtc, NULL); + flush = true; + } + + WARN_ON(!plane_funcs->atomic_disable); + if (plane_funcs->atomic_disable) + plane_funcs->atomic_disable(plane, NULL); + } + + if (flush) { + WARN_ON(!crtc_funcs->atomic_flush); + if (crtc_funcs->atomic_flush) + crtc_funcs->atomic_flush(crtc, NULL); + } +} +EXPORT_SYMBOL(drm_atomic_helper_disable_planes_on_crtc); + +/** * drm_atomic_helper_commit_planes_on_crtc - commit plane state for a crtc * @old_crtc_state: atomic state object with the old crtc state * diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 8cba54a..b7d4237 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -62,6 +62,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, void drm_atomic_helper_cleanup_planes(struct drm_device *dev, struct drm_atomic_state *old_state); void drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state); +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc, + bool atomic); void drm_atomic_helper_swap_state(struct drm_device *dev, struct drm_atomic_state *state); -- 1.9.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc() 2015-11-23 10:44 [PATCH] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc() Jyri Sarha @ 2015-11-23 16:09 ` Daniel Vetter 2015-11-23 21:20 ` Jyri Sarha 0 siblings, 1 reply; 4+ messages in thread From: Daniel Vetter @ 2015-11-23 16:09 UTC (permalink / raw) To: Jyri Sarha; +Cc: tomi.valkeinen, dri-devel, laurent.pinchart On Mon, Nov 23, 2015 at 12:44:35PM +0200, Jyri Sarha wrote: > Add drm_atomic_helper_disable_planes_on_crtc() for disabling all > planes associated with the given CRTC. This can be used for instance > in the CRTC helper disable callback to disable all planes before > shutting down the display pipeline. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > This a follow version to "[PATCH RFC] drm/crtc_helper: Add > drm_crtc_helper_disable_planes()"-patch [1], with Daniel Vetter's > review comments [2] implemented. Hope I got everything right this > time. Thanks a lot for the prompt review. When resending a patch please add a revision log to remind reviewers of what you changed. Otherwise they have to dig out the old thread again to figure that out. E.g. v2 per Daniel's feedback: - Improve kerneldoc. - WARN_ON when atomic_disable is missing. - Also use crtc_funcs->atomic_begin/atomic_flush optionally. > > Best regards, > Jyri > > [1] http://www.spinics.net/lists/dri-devel/msg94720.html > [2] http://www.spinics.net/lists/dri-devel/msg94722.html > > drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_atomic_helper.h | 2 ++ > 2 files changed, 54 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index a53ed7a..229c810 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1281,6 +1281,58 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, > EXPORT_SYMBOL(drm_atomic_helper_commit_planes); > > /** > + * drm_atomic_helper_disable_planes_on_crtc - helper to disable CRTC's planes > + * @crtc: CRTC > + * @atomic: if set, synchronize with CRTC's atomic_begin/flush hooks > + * > + * Disables all planes associated with the given CRTC. This can be > + * used for instance in the CRTC helper disable callback to disable > + * all planes before shutting down the display pipeline. > + * > + * If there are planes to disable and the atomic-parameter is set the > + * function calls the CRTC's atomic_begin hook before and atomic_flush > + * hook after disabling the planes. > + * > + * It is a bug to call this function without having implemented the > + * ->atomic_disable() plane hook. > + */ > +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc, > + bool atomic) > +{ > + const struct drm_crtc_helper_funcs *crtc_funcs = > + crtc->helper_private; > + struct drm_plane *plane; > + bool flush = false; > + > + if (!crtc_funcs || !crtc_funcs->atomic_begin) > + atomic = false; > + > + drm_for_each_plane(plane, crtc->dev) { > + const struct drm_plane_helper_funcs *plane_funcs = > + plane->helper_private; > + > + if (plane->state->crtc != crtc || !plane_funcs) > + continue; > + > + if (atomic && !flush) { > + crtc_funcs->atomic_begin(crtc, NULL); > + flush = true; I think it's clearer if you do that upfront with a simple if (crtc_funcs->atomic_begin && atomic) crtc_funcs->atomic_begin(); > + } > + > + WARN_ON(!plane_funcs->atomic_disable); > + if (plane_funcs->atomic_disable) > + plane_funcs->atomic_disable(plane, NULL); > + } > + > + if (flush) { > + WARN_ON(!crtc_funcs->atomic_flush); > + if (crtc_funcs->atomic_flush) > + crtc_funcs->atomic_flush(crtc, NULL); > + } Same here below. Imo the tracking you do in flush isn't required, since drivers can opt to not implement either atomic_begin or atomic_flush (on some hw you only need atomic_flush, and your code would break such a driver here). In short the code flow for atomic_begin/flush should look exactly like in drm_atomic_helper_commit_planes_on_crtc, except for the added && atomic check. Otherwise looks good. Cheers, Daniel > +} > +EXPORT_SYMBOL(drm_atomic_helper_disable_planes_on_crtc); > + > +/** > * drm_atomic_helper_commit_planes_on_crtc - commit plane state for a crtc > * @old_crtc_state: atomic state object with the old crtc state > * > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 8cba54a..b7d4237 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -62,6 +62,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, > void drm_atomic_helper_cleanup_planes(struct drm_device *dev, > struct drm_atomic_state *old_state); > void drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state); > +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc, > + bool atomic); > > void drm_atomic_helper_swap_state(struct drm_device *dev, > struct drm_atomic_state *state); > -- > 1.9.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc() 2015-11-23 16:09 ` Daniel Vetter @ 2015-11-23 21:20 ` Jyri Sarha 2015-11-24 8:48 ` Daniel Vetter 0 siblings, 1 reply; 4+ messages in thread From: Jyri Sarha @ 2015-11-23 21:20 UTC (permalink / raw) To: Daniel Vetter; +Cc: tomi.valkeinen, laurent.pinchart, dri-devel On 11/23/15 18:09, Daniel Vetter wrote: > On Mon, Nov 23, 2015 at 12:44:35PM +0200, Jyri Sarha wrote: >> Add drm_atomic_helper_disable_planes_on_crtc() for disabling all >> planes associated with the given CRTC. This can be used for instance >> in the CRTC helper disable callback to disable all planes before >> shutting down the display pipeline. >> >> Signed-off-by: Jyri Sarha <jsarha@ti.com> >> --- >> This a follow version to "[PATCH RFC] drm/crtc_helper: Add >> drm_crtc_helper_disable_planes()"-patch [1], with Daniel Vetter's >> review comments [2] implemented. Hope I got everything right this >> time. Thanks a lot for the prompt review. > > When resending a patch please add a revision log to remind reviewers of > what you changed. Otherwise they have to dig out the old thread again to > figure that out. E.g. > > v2 per Daniel's feedback: > - Improve kerneldoc. > - WARN_ON when atomic_disable is missing. > - Also use crtc_funcs->atomic_begin/atomic_flush optionally. I usually do that, but not when I drop RFC off, but I'll do it next time even then. >> >> Best regards, >> Jyri >> >> [1] http://www.spinics.net/lists/dri-devel/msg94720.html >> [2] http://www.spinics.net/lists/dri-devel/msg94722.html >> >> drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++ >> include/drm/drm_atomic_helper.h | 2 ++ >> 2 files changed, 54 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index a53ed7a..229c810 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -1281,6 +1281,58 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, >> EXPORT_SYMBOL(drm_atomic_helper_commit_planes); >> >> /** >> + * drm_atomic_helper_disable_planes_on_crtc - helper to disable CRTC's planes >> + * @crtc: CRTC >> + * @atomic: if set, synchronize with CRTC's atomic_begin/flush hooks >> + * >> + * Disables all planes associated with the given CRTC. This can be >> + * used for instance in the CRTC helper disable callback to disable >> + * all planes before shutting down the display pipeline. >> + * >> + * If there are planes to disable and the atomic-parameter is set the >> + * function calls the CRTC's atomic_begin hook before and atomic_flush >> + * hook after disabling the planes. >> + * >> + * It is a bug to call this function without having implemented the >> + * ->atomic_disable() plane hook. >> + */ >> +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc, >> + bool atomic) >> +{ >> + const struct drm_crtc_helper_funcs *crtc_funcs = >> + crtc->helper_private; >> + struct drm_plane *plane; >> + bool flush = false; >> + >> + if (!crtc_funcs || !crtc_funcs->atomic_begin) >> + atomic = false; >> + >> + drm_for_each_plane(plane, crtc->dev) { >> + const struct drm_plane_helper_funcs *plane_funcs = >> + plane->helper_private; >> + >> + if (plane->state->crtc != crtc || !plane_funcs) >> + continue; >> + >> + if (atomic && !flush) { >> + crtc_funcs->atomic_begin(crtc, NULL); >> + flush = true; > > I think it's clearer if you do that upfront with a simple > > if (crtc_funcs->atomic_begin && atomic) > crtc_funcs->atomic_begin(); > That would cause ->atomic_begin() and ->atomic_flush() cycle even when there is no planes to disable. At least with omapdrm that causes a write HW and crtc_disable() is called once at probe time when the piece of HW not yet enabled. I am not that familiar with either drm or omapdrm yet to tell if that is wrong, but in any case, calling ->atomic_begin() and ->atomic_flush() for nothing makes no sense to me. Would you like it better if there would be two drm_for_each_plane() loops, one for just checking if there is anything to do and the second doing actual job and ->atomic_begin() there in between? >> + } >> + >> + WARN_ON(!plane_funcs->atomic_disable); >> + if (plane_funcs->atomic_disable) >> + plane_funcs->atomic_disable(plane, NULL); >> + } >> + >> + if (flush) { >> + WARN_ON(!crtc_funcs->atomic_flush); >> + if (crtc_funcs->atomic_flush) >> + crtc_funcs->atomic_flush(crtc, NULL); >> + } > > Same here below. Imo the tracking you do in flush isn't required, since > drivers can opt to not implement either atomic_begin or atomic_flush (on > some hw you only need atomic_flush, and your code would break such a > driver here). > Ok, thanks I did not know that. I'll fix that. > In short the code flow for atomic_begin/flush should look exactly like in > drm_atomic_helper_commit_planes_on_crtc, except for the added && atomic > check. > > Otherwise looks good. > > Cheers, Daniel > Thanks, Jyri >> +} >> +EXPORT_SYMBOL(drm_atomic_helper_disable_planes_on_crtc); >> + >> +/** >> * drm_atomic_helper_commit_planes_on_crtc - commit plane state for a crtc >> * @old_crtc_state: atomic state object with the old crtc state >> * >> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h >> index 8cba54a..b7d4237 100644 >> --- a/include/drm/drm_atomic_helper.h >> +++ b/include/drm/drm_atomic_helper.h >> @@ -62,6 +62,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, >> void drm_atomic_helper_cleanup_planes(struct drm_device *dev, >> struct drm_atomic_state *old_state); >> void drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state); >> +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc, >> + bool atomic); >> >> void drm_atomic_helper_swap_state(struct drm_device *dev, >> struct drm_atomic_state *state); >> -- >> 1.9.1 >> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc() 2015-11-23 21:20 ` Jyri Sarha @ 2015-11-24 8:48 ` Daniel Vetter 0 siblings, 0 replies; 4+ messages in thread From: Daniel Vetter @ 2015-11-24 8:48 UTC (permalink / raw) To: Jyri Sarha; +Cc: tomi.valkeinen, dri-devel, laurent.pinchart On Mon, Nov 23, 2015 at 11:20:15PM +0200, Jyri Sarha wrote: > On 11/23/15 18:09, Daniel Vetter wrote: > >On Mon, Nov 23, 2015 at 12:44:35PM +0200, Jyri Sarha wrote: > >>Add drm_atomic_helper_disable_planes_on_crtc() for disabling all > >>planes associated with the given CRTC. This can be used for instance > >>in the CRTC helper disable callback to disable all planes before > >>shutting down the display pipeline. > >> > >>Signed-off-by: Jyri Sarha <jsarha@ti.com> > >>--- > >>This a follow version to "[PATCH RFC] drm/crtc_helper: Add > >>drm_crtc_helper_disable_planes()"-patch [1], with Daniel Vetter's > >>review comments [2] implemented. Hope I got everything right this > >>time. Thanks a lot for the prompt review. > > > >When resending a patch please add a revision log to remind reviewers of > >what you changed. Otherwise they have to dig out the old thread again to > >figure that out. E.g. > > > >v2 per Daniel's feedback: > >- Improve kerneldoc. > >- WARN_ON when atomic_disable is missing. > >- Also use crtc_funcs->atomic_begin/atomic_flush optionally. > > I usually do that, but not when I drop RFC off, but I'll do it next time > even then. > > >> > >>Best regards, > >>Jyri > >> > >>[1] http://www.spinics.net/lists/dri-devel/msg94720.html > >>[2] http://www.spinics.net/lists/dri-devel/msg94722.html > >> > >> drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++ > >> include/drm/drm_atomic_helper.h | 2 ++ > >> 2 files changed, 54 insertions(+) > >> > >>diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >>index a53ed7a..229c810 100644 > >>--- a/drivers/gpu/drm/drm_atomic_helper.c > >>+++ b/drivers/gpu/drm/drm_atomic_helper.c > >>@@ -1281,6 +1281,58 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, > >> EXPORT_SYMBOL(drm_atomic_helper_commit_planes); > >> > >> /** > >>+ * drm_atomic_helper_disable_planes_on_crtc - helper to disable CRTC's planes > >>+ * @crtc: CRTC > >>+ * @atomic: if set, synchronize with CRTC's atomic_begin/flush hooks > >>+ * > >>+ * Disables all planes associated with the given CRTC. This can be > >>+ * used for instance in the CRTC helper disable callback to disable > >>+ * all planes before shutting down the display pipeline. > >>+ * > >>+ * If there are planes to disable and the atomic-parameter is set the > >>+ * function calls the CRTC's atomic_begin hook before and atomic_flush > >>+ * hook after disabling the planes. > >>+ * > >>+ * It is a bug to call this function without having implemented the > >>+ * ->atomic_disable() plane hook. > >>+ */ > >>+void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc, > >>+ bool atomic) > >>+{ > >>+ const struct drm_crtc_helper_funcs *crtc_funcs = > >>+ crtc->helper_private; > >>+ struct drm_plane *plane; > >>+ bool flush = false; > >>+ > >>+ if (!crtc_funcs || !crtc_funcs->atomic_begin) > >>+ atomic = false; > >>+ > >>+ drm_for_each_plane(plane, crtc->dev) { > >>+ const struct drm_plane_helper_funcs *plane_funcs = > >>+ plane->helper_private; > >>+ > >>+ if (plane->state->crtc != crtc || !plane_funcs) > >>+ continue; > >>+ > >>+ if (atomic && !flush) { > >>+ crtc_funcs->atomic_begin(crtc, NULL); > >>+ flush = true; > > > >I think it's clearer if you do that upfront with a simple > > > > if (crtc_funcs->atomic_begin && atomic) > > crtc_funcs->atomic_begin(); > > > > That would cause ->atomic_begin() and ->atomic_flush() cycle even when there > is no planes to disable. At least with omapdrm that causes a write HW and > crtc_disable() is called once at probe time when the piece of HW not yet > enabled. > > I am not that familiar with either drm or omapdrm yet to tell if that is > wrong, but in any case, calling ->atomic_begin() and ->atomic_flush() for > nothing makes no sense to me. > Would you like it better if there would be two drm_for_each_plane() loops, > one for just checking if there is anything to do and the second doing actual > job and ->atomic_begin() there in between? If we do this optimization, we should do it for all plane commit functions. Just for consistency. The problem with omapdrm is that it still uses drm_helper_disable_unused_functions. That function is a legacy crtc helper one and shouldn't be used by atomic drivers any more: It unconditionally disable all logically unused hw functions, which breaks the guarantee atomic helpers have of never calling ->disable on something already disabled. This is a core design assumption for atomic helpers (since the old crtc helper assumption of ->disabling being harmless on disabled hw was causing way too much trouble and subtle bugs). The proper fix is to drop that call. Ofc atomic still requires that drivers align sw and hw state at boot-up, but that should be done through the ->reset hooks. The default helpers only reset sw state to all of, so if your hw could be on while booting then you need to add code for that. But only run it when the hw really is on ofc, like this: omap_crtc_reset() { drm_atomic_helper_crtc_reset(); if (crtc_is_running()) crtc_funcs->disable(); } Similar for anything else that might still be running. Other option is to not the default reset helpers but instead read out hw state into atomic sw structures (which is what i915 does). Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-11-24 8:48 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-23 10:44 [PATCH] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc() Jyri Sarha 2015-11-23 16:09 ` Daniel Vetter 2015-11-23 21:20 ` Jyri Sarha 2015-11-24 8:48 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.