On Fri, Nov 27, 2015 at 04:14:01PM +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@xxxxxx> > --- > v2: > - Address Daniels review comments [1] > - Do atomic_begin() and atomic_flush() always if they are defined and > atomic knob is set > - update kerneldoc > - Put drm_atomic_helper_disable_planes_on_crtc() after > drm_atomic_helper_commit_planes_on_crtc() in drm_atomic_helper.c > to have functions in the same order as in drm_atomic_helper.h > > Here the patch is again with latest review comment addressed, however > I am not sure if we are going use the drm_atomic_helper_disable_planes_on_crtc() > in omapdrm. > > BTW, the latest change to this patch makes me wonder a how the > atomic_begin() and atomic_flush() are supposed to work. > > 1. Should it be allowed to call atomic_begin() and atomic_flush() > multiple times in a recursive manner? No. Since the idea is to call this new disable_planes_on_crtc from crtc_funcs->disable, which is called from commit_disables() there's not problem. atomic_begin/end is just for plane updates (not for enabling/disabling the entire display pipeline), and that's done with the various commit_planes()/on_crtc() functions. > If yes, how should the driver cope with that? Keep refcount of > atomic_begins and do the magic only after the final matching > atomic_flush comes and the refcount reaches zero? > > 2. What about calling them consecutively (but not recursively) > multiple times without vblank happening in the middle? > > I am afraid that the current omapdrm implementation does not cope too > well in either situation. Currently atomic updates should only happen once per vblank (there's planes for benchmark mode where we want to flip faster). Maybe just add a drm_crtc_wait_one_vblank() call after calling this in your crtc_disable functions? Anyway, patch looks great, applied to drm-misc. Thanks, Daniel > > Cheers, > Jyri > > [1] http://www.spinics.net/lists/dri-devel/msg94942.html > > drivers/gpu/drm/drm_atomic_helper.c | 43 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_atomic_helper.h | 2 ++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 0c6f621..1ab6821 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1334,6 +1334,49 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state) > EXPORT_SYMBOL(drm_atomic_helper_commit_planes_on_crtc); > > /** > + * 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 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; > + > + if (atomic && crtc_funcs && crtc_funcs->atomic_begin) > + crtc_funcs->atomic_begin(crtc, NULL); > + > + 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; > + > + WARN_ON(!plane_funcs->atomic_disable); > + if (plane_funcs->atomic_disable) > + plane_funcs->atomic_disable(plane, NULL); > + } > + > + if (atomic && crtc_funcs && crtc_funcs->atomic_flush) > + crtc_funcs->atomic_flush(crtc, NULL); > +} > +EXPORT_SYMBOL(drm_atomic_helper_disable_planes_on_crtc); > + > +/** > * drm_atomic_helper_cleanup_planes - cleanup plane resources after commit > * @dev: DRM device > * @old_state: atomic state object with old state structures > 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
