Message-ID: <20231201181616.4c1f0acc.pekka.paalanen@collabora.com>
Date:   Fri, 1 Dec 2023 18:16:16 +0200
From:   Pekka Paalanen <pekka.paalanen@...labora.com>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:     André Almeida <andrealmeid@...lia.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Maxime Ripard <mripard@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        'Marek Olšák' <maraeo@...il.com>,
        Michel Dänzer 
        <michel.daenzer@...lbox.org>, Randy Dunlap <rdunlap@...radead.org>,
        linux-doc@...r.kernel.org, Thomas Zimmermann <tzimmermann@...e.de>,
        kernel-dev@...lia.com, alexander.deucher@....com,
        christian.koenig@....com
Subject: Re: [PATCH] drm/doc: Define KMS atomic state set

On Fri, 1 Dec 2023 17:00:32 +0200
Ville Syrjälä <ville.syrjala@...ux.intel.com> wrote:

> On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:
> > From: Pekka Paalanen <pekka.paalanen@...labora.com>
> > 
> > Specify how the atomic state is maintained between userspace and
> > kernel, plus the special case for async flips.
> > 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen@...labora.com>
> > Signed-off-by: André Almeida <andrealmeid@...lia.com>
> > ---
> > 
> > This is a standalone patch from the following serie, the other patches are
> > already merged:
> > https://lore.kernel.org/lkml/20231122161941.320564-1-andrealmeid@igalia.com/
> > 
> >  Documentation/gpu/drm-uapi.rst | 47 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index 370d820be248..d0693f902a5c 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -570,3 +570,50 @@ dma-buf interoperability
> >  
> >  Please see Documentation/userspace-api/dma-buf-alloc-exchange.rst for
> >  information on how dma-buf is integrated and exposed within DRM.
> > +
> > +KMS atomic state
> > +================
> > +
> > +An atomic commit can change multiple KMS properties in an atomic fashion,
> > +without ever applying intermediate or partial state changes.  Either the whole
> > +commit succeeds or fails, and it will never be applied partially. This is the
> > +fundamental improvement of the atomic API over the older non-atomic API which is
> > +referred to as the "legacy API".  Applying intermediate state could unexpectedly
> > +fail, cause visible glitches, or delay reaching the final state.
> > +
> > +An atomic commit can be flagged with DRM_MODE_ATOMIC_TEST_ONLY, which means the
> > +complete state change is validated but not applied.  Userspace should use this
> > +flag to validate any state change before asking to apply it. If validation fails
> > +for any reason, userspace should attempt to fall back to another, perhaps
> > +simpler, final state.  This allows userspace to probe for various configurations
> > +without causing visible glitches on screen and without the need to undo a
> > +probing change.
> > +
> > +The changes recorded in an atomic commit apply on top the current KMS state in
> > +the kernel. Hence, the complete new KMS state is the complete old KMS state with
> > +the committed property settings done on top. The kernel will try to avoid
> > +no-operation changes,  
> 
> Not how things work. The driver may try to avoid some really
> expensive operations, but generally it will just blindly blast
> the full state to the hardware.
> 
> IIRC this was discussed long ago when atomic was being designed
> and the general concensus was that the kernel shouldn't generally
> do this kind of stuff, and instead we just leave it to userspace
> to generate optimal commits.

I don't think userspace ever got that memo. If I was cheeky, I could
ask where that is documented, so you could point at it and say "told
you so".

When I was working on Weston atomic KMS support many years ago, I
created a framework that emitted KMS property changes only when they
actually needed changing. By review feedback (*), all that machinery was
dropped in a re-design, and today Weston always emits all KMS
properties it knows to program for a specific CRTC update including all
relevant planes and connectors.

(*) Why do we need to repeat the same state tracking that the kernel
does anyway, and also risk getting out of sync with the kernel due to
bugs which then become more difficult to diagnose. I guess (assumed)
kernel internals leaked to userspace. Oops.

> > so it is safe for userspace to send redundant property
> > +settings.  
> 
> Safe but not optimal. Any object included in the state will cause said
> object to be part of the commit, and side effects will also need to be
> observed.
> 
> So if you add an extra crtc (either directly or indirectly) it will
> have a new commit inserted into the queue and thus and any subsequent
> commit will either block or be rejected with -EBUSY. Also for directly
> added crtcs an event will be emitted once the commit is done.

It is not too hard to keep CRTCs well separated, until the kernel
driver decides under the hood to pull in an unwanted CRTC.

But yes, that caveat could use extending in the doc.

> Any plane added will also need to observe side effects even if the FB
> doesn't change, such as invalidating any internal compressed version
> of the old FB contents, PSR/DSI command mode/etc. will need to upload
> the frame to the display, etc. I suppose we could specify that if no
> FB is specified at all then these kind of side effects could be ignored,
> but that is certainly not how things are implemented right now.

Well, this is all surprise news to me.

> So for optimal behaviour userspace should be minimizing the commits.
> 


Thanks,
pq

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists