| Message ID | 20180330145518.29770-1-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Boris Brezillon <boris.brezillon@bootlin.com> writes: > ->atomic_async_update() requires that drivers update the plane->state > object before returning. Make sure at least common properties have been > updated. > > Cc: Gustavo Padovan <gustavo@padovan.org> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > Hello, > > This is a problem I had when debugging the VC4 ->atomic_async_update() > implementation. The function was not updating plane->fb as it's > supposed thus leaving plane->state->fb in an inconsistent state after > each async update. > > Not sure if such WARN_ON_ONCE() are accepted in the core though, so > I'll maintainers decide whether this is relevant or not and whether > they prefer to have WARN_ON() or DRM_ERROR() messages. I'm a huge fan of this kind of safety check. FWIW, Acked-by: Eric Anholt <eric@anholt.net>
On Fri, Mar 30, 2018 at 11:39:05AM -0700, Eric Anholt wrote: > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > ->atomic_async_update() requires that drivers update the plane->state > > object before returning. Make sure at least common properties have been > > updated. > > > > Cc: Gustavo Padovan <gustavo@padovan.org> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > Hello, > > > > This is a problem I had when debugging the VC4 ->atomic_async_update() > > implementation. The function was not updating plane->fb as it's > > supposed thus leaving plane->state->fb in an inconsistent state after > > each async update. > > > > Not sure if such WARN_ON_ONCE() are accepted in the core though, so > > I'll maintainers decide whether this is relevant or not and whether > > they prefer to have WARN_ON() or DRM_ERROR() messages. > > I'm a huge fan of this kind of safety check. FWIW, > > Acked-by: Eric Anholt <eric@anholt.net> Agreed, seems like a useful safety net to me. Acked-by: Sean Paul <seanpaul@chromium.org> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Apr 02, 2018 at 11:45:31AM -0400, Sean Paul wrote: > On Fri, Mar 30, 2018 at 11:39:05AM -0700, Eric Anholt wrote: > > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > > > ->atomic_async_update() requires that drivers update the plane->state > > > object before returning. Make sure at least common properties have been > > > updated. > > > > > > Cc: Gustavo Padovan <gustavo@padovan.org> > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > --- > > > Hello, > > > > > > This is a problem I had when debugging the VC4 ->atomic_async_update() > > > implementation. The function was not updating plane->fb as it's > > > supposed thus leaving plane->state->fb in an inconsistent state after > > > each async update. > > > > > > Not sure if such WARN_ON_ONCE() are accepted in the core though, so > > > I'll maintainers decide whether this is relevant or not and whether > > > they prefer to have WARN_ON() or DRM_ERROR() messages. > > > > I'm a huge fan of this kind of safety check. FWIW, > > > > Acked-by: Eric Anholt <eric@anholt.net> > > Agreed, seems like a useful safety net to me. > > Acked-by: Sean Paul <seanpaul@chromium.org> Yeah, makes sense. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > -- > Sean Paul, Software Engineer, Google / Chromium OS
On Fri, 30 Mar 2018 16:55:18 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > ->atomic_async_update() requires that drivers update the plane->state > object before returning. Make sure at least common properties have been > updated. > > Cc: Gustavo Padovan <gustavo@padovan.org> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> Applied to drm-misc-next. > --- > Hello, > > This is a problem I had when debugging the VC4 ->atomic_async_update() > implementation. The function was not updating plane->fb as it's > supposed thus leaving plane->state->fb in an inconsistent state after > each async update. > > Not sure if such WARN_ON_ONCE() are accepted in the core though, so > I'll maintainers decide whether this is relevant or not and whether > they prefer to have WARN_ON() or DRM_ERROR() messages. > > Regards, > > Boris > --- > drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index c35654591c12..d2b2583487ee 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1561,6 +1561,17 @@ void drm_atomic_helper_async_commit(struct drm_device *dev, > for_each_new_plane_in_state(state, plane, plane_state, i) { > funcs = plane->helper_private; > funcs->atomic_async_update(plane, plane_state); > + > + /* > + * ->atomic_async_update() is supposed to update the > + * plane->state in-place, make sure at least common > + * properties have been properly updated. > + */ > + WARN_ON_ONCE(plane->state->fb != plane_state->fb); > + WARN_ON_ONCE(plane->state->crtc_x != plane_state->crtc_x); > + WARN_ON_ONCE(plane->state->crtc_y != plane_state->crtc_y); > + WARN_ON_ONCE(plane->state->src_x != plane_state->src_x); > + WARN_ON_ONCE(plane->state->src_y != plane_state->src_y); > } > } > EXPORT_SYMBOL(drm_atomic_helper_async_commit);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index c35654591c12..d2b2583487ee 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1561,6 +1561,17 @@ void drm_atomic_helper_async_commit(struct drm_device *dev, for_each_new_plane_in_state(state, plane, plane_state, i) { funcs = plane->helper_private; funcs->atomic_async_update(plane, plane_state); + + /* + * ->atomic_async_update() is supposed to update the + * plane->state in-place, make sure at least common + * properties have been properly updated. + */ + WARN_ON_ONCE(plane->state->fb != plane_state->fb); + WARN_ON_ONCE(plane->state->crtc_x != plane_state->crtc_x); + WARN_ON_ONCE(plane->state->crtc_y != plane_state->crtc_y); + WARN_ON_ONCE(plane->state->src_x != plane_state->src_x); + WARN_ON_ONCE(plane->state->src_y != plane_state->src_y); } } EXPORT_SYMBOL(drm_atomic_helper_async_commit);
->atomic_async_update() requires that drivers update the plane->state object before returning. Make sure at least common properties have been updated. Cc: Gustavo Padovan <gustavo@padovan.org> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- Hello, This is a problem I had when debugging the VC4 ->atomic_async_update() implementation. The function was not updating plane->fb as it's supposed thus leaving plane->state->fb in an inconsistent state after each async update. Not sure if such WARN_ON_ONCE() are accepted in the core though, so I'll maintainers decide whether this is relevant or not and whether they prefer to have WARN_ON() or DRM_ERROR() messages. Regards, Boris --- drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ 1 file changed, 11 insertions(+)