| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <qiqmaotvdfnq66oth6eqcun5n7stmfyw7zyyjxxlh72gst722a@t6zb764t2ttz>
Date: Wed, 8 Jan 2025 20:32:25 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Rob Clark <robdclark@...il.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>, Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>, Chandan Uddaraju <chandanu@...eaurora.org>,
Jeykumar Sankaran <jsanka@...eaurora.org>, Jordan Crouse <jordan@...micpenguin.net>,
Sravanthi Kollukuduru <skolluku@...eaurora.org>, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Archit Taneja <architt@...eaurora.org>, Rajesh Yadav <ryadav@...eaurora.org>,
linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org
Subject: Re: [PATCH 2/6] drm/atomic: prepare to check that drivers follow
restrictions for needs_modeset
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.vetter@...ll.ch>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.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
Powered by blists - more mailing lists