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 at 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 >> >
