On Wed, Jan 08, 2025 at 06:53:51PM +0100, Simona Vetter wrote:
> On Sun, Dec 22, 2024 at 07:00:42AM +0200, Dmitry Baryshkov wrote:
> > Some drivers might fail to follow the restrictions documented for
> > drm_atomic_helper_check_modesets(). In order to catch such an issues,
> > add the drm_atomic_state->dirty_needs_modeset field and check it in
> > drm_atomic_check_only(). Make sure that neither of atomic_check()
> > callbacks can set that field without calling
> > drm_atomic_helper_check_modesets() again.
> >
> > Suggested-by: Simona Vetter <simona.vet...@ffwll.ch>
> > Signed-off-by: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
>
> Thanks a lot of creating this patch. But looking at it I'm not so sure I
> actually had a good idea, since there's still lots of ways for drivers to
> change drm_atomic_crtc_needs_modeset() that we miss. And trying to use an
> inverted bitfield of all crtc that we've run through in check_modeset, and
> then in atomic_check_only compare it against all crtc that need a modeset
> also has corner cases it gets wrong I think, like just not using the
> helpers in specific case, I think something like i915's fastset would trip
> that.
I think we should try to get something merged. I mean, the documentation
was there, but it didn't prevent driver authors from being creative, as
you wrote. So, having false negatives should be fine. We just have not
to trigger false warning reports. I will try giving it another tought.
>
> Plus there's lots more corners that drivers have gotten creatively wrong,
> so I feel like really clear docs is the best we can do.
>
> So unless you think it was really useful to fix msm I feel like best to
> skip this. Apologies for making you put work in here :-/
I think it's useful to prevent us (authors) from doing nasty things :-/
> -Sima
>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 3 ++
> > drivers/gpu/drm/drm_atomic_helper.c | 77
> > +++++++++++++++++++++++++++++++++----
> > include/drm/drm_atomic.h | 10 +++++
> > 3 files changed, 82 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index
> > 9ea2611770f43ce7ccba410406d5f2c528aab022..202e4e64bd31921d0a4d4b86605b501311e14c33
> > 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1449,6 +1449,9 @@ int drm_atomic_check_only(struct drm_atomic_state
> > *state)
> > }
> > }
> >
> > + WARN_RATELIMIT(state->dirty_needs_modeset,
> > + "Driver changed needs_modeset under
> > drm_atomic_helper_check_modeset()");
> > +
Maybe it should be drm_warn or drm_info for now, instead of full WARN().
> > if (!state->allow_modeset) {
> > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index
> > f26887c3fe8b194137200f9f2426653274c50fda..2c62840416f4b807d6a880b5c30ae024a16af528
> > 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -433,6 +433,7 @@ mode_fixup(struct drm_atomic_state *state)
> >
> > for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> > const struct drm_encoder_helper_funcs *funcs;
> > + bool old_needs_modeset = false;
> > struct drm_encoder *encoder;
> > struct drm_bridge *bridge;
> >
> > @@ -451,6 +452,9 @@ mode_fixup(struct drm_atomic_state *state)
> > encoder = new_conn_state->best_encoder;
> > funcs = encoder->helper_private;
> >
> > + if (new_crtc_state)
> > + old_needs_modeset =
> > drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > bridge = drm_bridge_chain_get_first_bridge(encoder);
> > ret = drm_atomic_bridge_chain_check(bridge,
> > new_crtc_state,
> > @@ -479,6 +483,12 @@ mode_fixup(struct drm_atomic_state *state)
> > return -EINVAL;
> > }
> > }
> > +
> > + if (new_crtc_state) {
> > + bool new_needs_modeset =
> > drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > + state->dirty_needs_modeset |= (new_needs_modeset !=
> > old_needs_modeset);
> > + }
> > }
> >
> > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > @@ -574,6 +584,36 @@ mode_valid(struct drm_atomic_state *state)
> > return 0;
> > }
> >
> > +static int
> > +connector_atomic_check(struct drm_atomic_state *state,
> > + struct drm_connector *connector,
> > + struct drm_connector_state *old_connector_state,
> > + struct drm_connector_state *new_connector_state)
> > +{
> > + const struct drm_connector_helper_funcs *funcs =
> > connector->helper_private;
> > + struct drm_crtc_state *new_crtc_state;
> > + bool old_needs_modeset = false;
> > + int ret;
> > +
> > + if (new_connector_state->crtc)
> > + new_crtc_state = drm_atomic_get_new_crtc_state(state,
> > new_connector_state->crtc);
> > + if (new_crtc_state)
> > + old_needs_modeset =
> > drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > + if (funcs->atomic_check)
> > + ret = funcs->atomic_check(connector, state);
> > + else
> > + ret = 0;
> > +
> > + if (new_crtc_state) {
> > + bool new_needs_modeset =
> > drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > + state->dirty_needs_modeset |= (new_needs_modeset !=
> > old_needs_modeset);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * drm_atomic_helper_check_modeset - validate state object for modeset
> > changes
> > * @dev: DRM device
> > @@ -628,6 +668,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > int i, ret;
> > unsigned int connectors_mask = 0, user_connectors_mask = 0;
> >
> > + state->dirty_needs_modeset = false;
> > +
> > for_each_oldnew_connector_in_state(state, connector,
> > old_connector_state, new_connector_state, i)
> > user_connectors_mask |= BIT(i);
> >
> > @@ -683,8 +725,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > return ret;
> >
> > for_each_oldnew_connector_in_state(state, connector,
> > old_connector_state, new_connector_state, i) {
> > - const struct drm_connector_helper_funcs *funcs =
> > connector->helper_private;
> > -
> >
> > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >
> > /*
> > @@ -710,8 +750,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > new_crtc_state->connectors_changed = true;
> > }
> >
> > - if (funcs->atomic_check)
> > - ret = funcs->atomic_check(connector, state);
> > + ret = connector_atomic_check(state, connector,
> > + old_connector_state,
> > new_connector_state);
> > if (ret) {
> > drm_dbg_atomic(dev,
> > "[CONNECTOR:%d:%s] driver check
> > failed\n",
> > @@ -752,13 +792,11 @@ drm_atomic_helper_check_modeset(struct drm_device
> > *dev,
> > * has been called on them when a modeset is forced.
> > */
> > for_each_oldnew_connector_in_state(state, connector,
> > old_connector_state, new_connector_state, i) {
> > - const struct drm_connector_helper_funcs *funcs =
> > connector->helper_private;
> > -
> > if (connectors_mask & BIT(i))
> > continue;
> >
> > - if (funcs->atomic_check)
> > - ret = funcs->atomic_check(connector, state);
> > + ret = connector_atomic_check(state, connector,
> > + old_connector_state,
> > new_connector_state);
> > if (ret) {
> > drm_dbg_atomic(dev,
> > "[CONNECTOR:%d:%s] driver check
> > failed\n",
> > @@ -994,6 +1032,7 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> >
> > for_each_oldnew_plane_in_state(state, plane, old_plane_state,
> > new_plane_state, i) {
> > const struct drm_plane_helper_funcs *funcs;
> > + bool old_needs_modeset = false;
> >
> > WARN_ON(!drm_modeset_is_locked(&plane->mutex));
> >
> > @@ -1006,6 +1045,12 @@ drm_atomic_helper_check_planes(struct drm_device
> > *dev,
> > if (!funcs || !funcs->atomic_check)
> > continue;
> >
> > + if (new_plane_state->crtc)
> > + new_crtc_state = drm_atomic_get_new_crtc_state(state,
> > +
> > new_plane_state->crtc);
> > + if (new_crtc_state)
> > + old_needs_modeset =
> > drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > ret = funcs->atomic_check(plane, state);
> > if (ret) {
> > drm_dbg_atomic(plane->dev,
> > @@ -1013,16 +1058,26 @@ drm_atomic_helper_check_planes(struct drm_device
> > *dev,
> > plane->base.id, plane->name);
> > return ret;
> > }
> > +
> > + if (new_crtc_state) {
> > + bool new_needs_modeset =
> > drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > + state->dirty_needs_modeset |= (new_needs_modeset !=
> > old_needs_modeset);
> > + }
> > }
> >
> > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > const struct drm_crtc_helper_funcs *funcs;
> > + bool old_needs_modeset = false;
> >
> > funcs = crtc->helper_private;
> >
> > if (!funcs || !funcs->atomic_check)
> > continue;
> >
> > + if (new_crtc_state)
> > + old_needs_modeset =
> > drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > ret = funcs->atomic_check(crtc, state);
> > if (ret) {
> > drm_dbg_atomic(crtc->dev,
> > @@ -1030,6 +1085,12 @@ drm_atomic_helper_check_planes(struct drm_device
> > *dev,
> > crtc->base.id, crtc->name);
> > return ret;
> > }
> > +
> > + if (new_crtc_state) {
> > + bool new_needs_modeset =
> > drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > + state->dirty_needs_modeset |= (new_needs_modeset !=
> > old_needs_modeset);
> > + }
> > }
> >
> > return ret;
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index
> > 31ca88deb10d262fb3a3f8e14d2afe24f8410cb1..7b0dbd3c8a3df340399a458aaf79263f0fdc24e5
> > 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -408,6 +408,16 @@ struct drm_atomic_state {
> > */
> > bool duplicated : 1;
> >
> > + /**
> > + * @dirty_needs_modeset:
> > + *
> > + * Indicates whether the drm_atomic_crtc_needs_modeset() changed in an
> > + * unexpected way. Usually this means that driver implements atomic
> > + * helpers using drm_atomic_crtc_needs_modeset(), but mode_changed has
> > + * toggled by one of its atomic_check() callbacks.
> > + */
> > + bool dirty_needs_modeset : 1;
> > +
> > /**
> > * @planes:
> > *
> >
> > --
> > 2.39.5
> >
>
> --
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
With best wishes
Dmitry
