[PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Daniel Vetter]

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

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>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 include/drm/drm_atomic.h | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index cf8e1220a4ac..09bef1b6c170 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -372,8 +372,22 @@ 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 never change this flag, it is only under the control
+	 *   of userspace.
 	 */
 	bool allow_modeset : 1;
 	/**
-- 
2.40.1

[PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Daniel Vetter]

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().
+	 *
+	 * - 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;
 	/**
-- 
2.40.1

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4430 bytes --]

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;
>   	/**

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Simona Vetter]

On Wed, Oct 11, 2023 at 12:53:14PM +0200, Thomas Zimmermann wrote:
> 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?

See Dmitry's enforcement patch set he just sent out. Also finally got
around to picking this up again.

> > 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.

Maybe I'm wrong, but my understanding is that English generally doesn't do
compound words connected with dashes, you just line them up with spaces.
Which can be confusing sometimes, especially since German very much makes
a distinction here.

> 
> > +	 *   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

Nope, because English doesn't have unconditional comma before relative
subclauses. Instead the lack of comma signifies that the subclause
specifies essential information which cannot be ommitted without changing
the meaning of the sentence. Which is the case here for all the commas you
want to add.

See https://prowritingaid.com/grammar/1008112/comma-before-which-where-who

> 
> > +	 *   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'.

See above, I think this is just English being English.

> > +	 *   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.

I've done your other changes as suggested.

Thanks, Sima


> 
> Best regards
> Thomas
> 
> >   	 */
> >   	bool allow_modeset : 1;
> >   	/**
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)




-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Michel Dänzer]

On 2025-01-08 18:22, Simona Vetter wrote:
> On Wed, Oct 11, 2023 at 12:53:14PM +0200, Thomas Zimmermann wrote:
> 
>> Comments on the text from a non-native speaker:
>>
>>> +	 *
>>> +	 * - Drivers must not consult @allow_modeset in the atomic commit path,
>>
>> 'atomic-commit' because it's the code path for atomic commits.
> 
> Maybe I'm wrong, but my understanding is that English generally doesn't do
> compound words connected with dashes, you just line them up with spaces.

I hope you don't mind me jumping in, three native German speakers discussing English grammar is merrier than just two. :)

FWIW, LWN writes compound words with spaces when used as nouns ("code in user space"), with dashes when used as adjectives ("user-space code"). I don't know if this is an official / general rule, I'm using it as a guideline though.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Daniel Stone]

Hi,

On Thu, 9 Jan 2025 at 15:30, Michel Dänzer <michel.daenzer@mailbox.org> wrote:
> On 2025-01-08 18:22, Simona Vetter wrote:
> > Maybe I'm wrong, but my understanding is that English generally doesn't do
> > compound words connected with dashes, you just line them up with spaces.
>
> I hope you don't mind me jumping in, three native German speakers discussing English grammar is merrier than just two. :)
>
> FWIW, LWN writes compound words with spaces when used as nouns ("code in user space"), with dashes when used as adjectives ("user-space code"). I don't know if this is an official / general rule, I'm using it as a guideline though.

To the extent that Australian counts as native English: yes, that's
exactly the rule.

Cheers,
Daniel

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Thomas Zimmermann]

Hi


Am 09.01.25 um 16:45 schrieb Daniel Stone:
> Hi,
>
> On Thu, 9 Jan 2025 at 15:30, Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>> On 2025-01-08 18:22, Simona Vetter wrote:
>>> Maybe I'm wrong, but my understanding is that English generally doesn't do
>>> compound words connected with dashes, you just line them up with spaces.
>> I hope you don't mind me jumping in, three native German speakers discussing English grammar is merrier than just two. :)
>>
>> FWIW, LWN writes compound words with spaces when used as nouns ("code in user space"), with dashes when used as adjectives ("user-space code"). I don't know if this is an official / general rule, I'm using it as a guideline though.
> To the extent that Australian counts as native English: yes, that's
> exactly the rule.

Thanks! My understanding is that words are right associative by default. 
So "user space code" means 'user (space code)'. The dash makes it left 
associative, hence "user-space code".

Best regards
Thomas

>
> Cheers,
> Daniel

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Simona Vetter]

On Thu, Jan 09, 2025 at 03:45:53PM +0000, Daniel Stone wrote:
> Hi,
> 
> On Thu, 9 Jan 2025 at 15:30, Michel Dänzer <michel.daenzer@mailbox.org> wrote:
> > On 2025-01-08 18:22, Simona Vetter wrote:
> > > Maybe I'm wrong, but my understanding is that English generally doesn't do
> > > compound words connected with dashes, you just line them up with spaces.
> >
> > I hope you don't mind me jumping in, three native German speakers
> > discussing English grammar is merrier than just two. :)
> >
> > FWIW, LWN writes compound words with spaces when used as nouns ("code
> > in user space"), with dashes when used as adjectives ("user-space
> > code"). I don't know if this is an official / general rule, I'm using
> > it as a guideline though.
> 
> To the extent that Australian counts as native English: yes, that's
> exactly the rule.

TIL, thanks a lot.

Pretty sure we never got around to this rule, and I had close to a decade
of English lessons. The one about commas I do remember from school though.
-Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3729 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Dmitry Baryshkov]

Hello,

On Wed, 11 Oct 2023 at 12:20, 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.

I have been looking at the drm_atomic_helper_check_modeset() issue, I
couldn't help but notice that this hasn't been committed.
Maybe it's time for v3?

> 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().
> +        *
> +        * - 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;
>         /**
> --
> 2.40.1
>


-- 
With best wishes
Dmitry

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Simona Vetter]

On Sun, Dec 22, 2024 at 06:47:18AM +0200, Dmitry Baryshkov wrote:
> Hello,
> 
> On Wed, 11 Oct 2023 at 12:20, 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.
> 
> I have been looking at the drm_atomic_helper_check_modeset() issue, I
> couldn't help but notice that this hasn't been committed.
> Maybe it's time for v3?

Oops. Working on it now.
-Sima

> 
> > 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().
> > +        *
> > +        * - 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;
> >         /**
> > --
> > 2.40.1
> >
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Simona Vetter]

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.

v3: Wording polish from Pekka and Thomas

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.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: Simona Vetter <simona.vetter@intel.com>
Signed-off-by: Simona Vetter <simona.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 31ca88deb10d..1ded9a8d4e84 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -376,8 +376,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().
+	 * generally not consult this flag, but instead look at the output of
+	 * drm_atomic_crtc_needs_modeset(). The detailed rules are:
+	 *
+	 * - Drivers must not consult @allow_modeset in the atomic commit path.
+	 *   Use drm_atomic_crtc_needs_modeset() instead.
+	 *
+	 * - 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 under the exclusive
+	 *   control of userspace.
+	 *
+	 * - 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.
 	 */
 	bool allow_modeset : 1;
 	/**
-- 
2.45.2

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Dmitry Baryshkov]

On Wed, Jan 08, 2025 at 06:24:16PM +0100, Simona Vetter 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.
> 
> v3: Wording polish from Pekka and Thomas
> 
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.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: Simona Vetter <simona.vetter@intel.com>
> Signed-off-by: Simona Vetter <simona.vetter@ffwll.ch>
> ---
>  include/drm/drm_atomic.h | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 

Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH] drm/atomic: clarify the rules around drm_atomic_state->allow_modeset

[Simona Vetter]

On Wed, Jan 08, 2025 at 08:34:28PM +0200, Dmitry Baryshkov wrote:
> On Wed, Jan 08, 2025 at 06:24:16PM +0100, Simona Vetter 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.
> > 
> > v3: Wording polish from Pekka and Thomas
> > 
> > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.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: Simona Vetter <simona.vetter@intel.com>
> > Signed-off-by: Simona Vetter <simona.vetter@ffwll.ch>
> > ---
> >  include/drm/drm_atomic.h | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> 
> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Applied to drm-misc-next, thanks for pinging me about this one.
-Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch