On Wed, 11 Oct 2023 11:20:51 +0200
Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> Cc: Rob Clark <robdclark@xxxxxxxxx>
> Cc: Simon Ser <contact@xxxxxxxxxxx>
> Cc: Manasi Navare <navaremanasi@xxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx>
Thanks,
pq
Attachment:
pgpSHyFTLWHNV.pgp
Description: OpenPGP digital signature
