| Message ID | 20231011092051.640422-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atomic: clarify the rules around drm_atomic_state->allow_modeset | expand |
Hi Am 11.10.23 um 11:20 schrieb Daniel Vetter: > msm is automagically upgrading normal commits to full modesets, and Can you give context or pointers here? > that's a big no-no: > > - for one this results in full on->off->on transitions on all these > crtc, at least if you're using the usual helpers. Which seems to be > the case, and is breaking uapi > > - further even if the ctm change itself would not result in flicker, > this can hide modesets for other reasons. Which again breaks the > uapi > > v2: I forgot the case of adding unrelated crtc state. Add that case > and link to the existing kerneldoc explainers. This has come up in an > irc discussion with Manasi and Ville about intel's bigjoiner mode. > Also cc everyone involved in the msm irc discussion, more people > joined after I sent out v1. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Pekka Paalanen <pekka.paalanen@collabora.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Simon Ser <contact@emersion.fr> > Cc: Manasi Navare <navaremanasi@google.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > include/drm/drm_atomic.h | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index cf8e1220a4ac..545c48545402 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -372,8 +372,27 @@ struct drm_atomic_state { > * > * Allow full modeset. This is used by the ATOMIC IOCTL handler to > * implement the DRM_MODE_ATOMIC_ALLOW_MODESET flag. Drivers should > - * never consult this flag, instead looking at the output of > - * drm_atomic_crtc_needs_modeset(). > + * not consult this flag, instead looking at the output of > + * drm_atomic_crtc_needs_modeset(). The detailed rules are: Comments on the text from a non-native speaker: I assume that you follow RFC 2119. I'd leave out the new sentence ('should not'), as it is weaker than the actual rules. Maybe list the rules directly. Something along the lines of "Drivers consulting this flag must follow the following rules". > + * > + * - Drivers must not consult @allow_modeset in the atomic commit path, 'atomic-commit' because it's the code path for atomic commits. > + * and instead use drm_atomic_crtc_needs_modeset(). > + * > + * - Drivers may consult @allow_modeset in the atomic check path, if 'atomic-check' > + * they have the choice between an optimal hardware configuration comma after configuration > + * which requires a modeset, and a less optimal configuration which 'less-optimal' > + * can be committed without a modeset. An example would be suboptimal > + * scanout FIFO allocation resulting in increased idle power This sentence is hard to understand for me. Is it 'scanout FIFO allocation' or 'scanout-FIFO allocation'? Maybe also try putting a comma after 'allocation'. > + * consumption. This allows userspace to avoid flickering and delays > + * for the normal composition loop at reasonable cost. > + * > + * - Drivers must consult @allow_modeset before adding unrelated struct > + * drm_crtc_state to this commit by calling > + * drm_atomic_get_crtc_state(). See also the warning in the > + * documentation for that function. > + * > + * - Drivers must never change this flag, it is only under the control Maybe try 'it is under exclusive control of user space' ? > + * of userspace. I'd also order these points like that - must not change - must not use it in atomic_commit - must - may so that the 'don't dos' are first. Best regards Thomas > */ > bool allow_modeset : 1; > /**
On Wed, 11 Oct 2023 11:20:51 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > msm is automagically upgrading normal commits to full modesets, and > that's a big no-no: > > - for one this results in full on->off->on transitions on all these > crtc, at least if you're using the usual helpers. Which seems to be > the case, and is breaking uapi > > - further even if the ctm change itself would not result in flicker, > this can hide modesets for other reasons. Which again breaks the > uapi > > v2: I forgot the case of adding unrelated crtc state. Add that case > and link to the existing kerneldoc explainers. This has come up in an > irc discussion with Manasi and Ville about intel's bigjoiner mode. > Also cc everyone involved in the msm irc discussion, more people > joined after I sent out v1. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Pekka Paalanen <pekka.paalanen@collabora.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Simon Ser <contact@emersion.fr> > Cc: Manasi Navare <navaremanasi@google.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > include/drm/drm_atomic.h | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index cf8e1220a4ac..545c48545402 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -372,8 +372,27 @@ struct drm_atomic_state { > * > * Allow full modeset. This is used by the ATOMIC IOCTL handler to > * implement the DRM_MODE_ATOMIC_ALLOW_MODESET flag. Drivers should > - * never consult this flag, instead looking at the output of > - * drm_atomic_crtc_needs_modeset(). > + * not consult this flag, instead looking at the output of > + * drm_atomic_crtc_needs_modeset(). The detailed rules are: > + * > + * - Drivers must not consult @allow_modeset in the atomic commit path, > + * and instead use drm_atomic_crtc_needs_modeset(). Change to Drivers must not consult @allow_modeset in the atomic commit path. Use drm_atomic_crtc_needs_modeset() instead. maybe? It's hard for me to see the difference between "instead use X" and "instead of X". "Use Y instead (of X)." helps me at least. > + * > + * - Drivers may consult @allow_modeset in the atomic check path, if > + * they have the choice between an optimal hardware configuration > + * which requires a modeset, and a less optimal configuration which > + * can be committed without a modeset. An example would be suboptimal > + * scanout FIFO allocation resulting in increased idle power > + * consumption. This allows userspace to avoid flickering and delays > + * for the normal composition loop at reasonable cost. > + * > + * - Drivers must consult @allow_modeset before adding unrelated struct > + * drm_crtc_state to this commit by calling > + * drm_atomic_get_crtc_state(). See also the warning in the > + * documentation for that function. > + * > + * - Drivers must never change this flag, it is only under the control > + * of userspace. *only userspace may set it. ? > */ > bool allow_modeset : 1; > /** Wording bikeshed aside, Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> Thanks, pq
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index cf8e1220a4ac..545c48545402 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -372,8 +372,27 @@ struct drm_atomic_state { * * Allow full modeset. This is used by the ATOMIC IOCTL handler to * implement the DRM_MODE_ATOMIC_ALLOW_MODESET flag. Drivers should - * never consult this flag, instead looking at the output of - * drm_atomic_crtc_needs_modeset(). + * not consult this flag, instead looking at the output of + * drm_atomic_crtc_needs_modeset(). The detailed rules are: + * + * - Drivers must not consult @allow_modeset in the atomic commit path, + * and instead use drm_atomic_crtc_needs_modeset(). + * + * - Drivers may consult @allow_modeset in the atomic check path, if + * they have the choice between an optimal hardware configuration + * which requires a modeset, and a less optimal configuration which + * can be committed without a modeset. An example would be suboptimal + * scanout FIFO allocation resulting in increased idle power + * consumption. This allows userspace to avoid flickering and delays + * for the normal composition loop at reasonable cost. + * + * - Drivers must consult @allow_modeset before adding unrelated struct + * drm_crtc_state to this commit by calling + * drm_atomic_get_crtc_state(). See also the warning in the + * documentation for that function. + * + * - Drivers must never change this flag, it is only under the control + * of userspace. */ bool allow_modeset : 1; /**