Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Fri, 1 Dec 2023 20:09:22 +0200
From:   Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
To:     Pekka Paalanen <pekka.paalanen@collabora.com>
Cc:     =?iso-8859-1?Q?Andr=E9?= Almeida <andrealmeid@igalia.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        Maxime Ripard <mripard@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        'Marek =?utf-8?B?T2zFocOhayc=?= <maraeo@gmail.com>,
        Michel =?iso-8859-1?Q?D=E4nzer?= <michel.daenzer@mailbox.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        linux-doc@vger.kernel.org, Thomas Zimmermann <tzimmermann@suse.de>,
        kernel-dev@igalia.com, alexander.deucher@amd.com,
        christian.koenig@amd.com
Subject: Re: [PATCH] drm/doc: Define KMS atomic state set
Message-ID: <ZWohUl3Ma6Q2fccG@intel.com>
References: <20231130200740.53454-1-andrealmeid@igalia.com>
 <ZWn1EC04wBSN9hu2@intel.com>
 <20231201181616.4c1f0acc.pekka.paalanen@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231201181616.4c1f0acc.pekka.paalanen@collabora.com>
Precedence: bulk

On Fri, Dec 01, 2023 at 06:16:16PM +0200, Pekka Paalanen wrote:
> On Fri, 1 Dec 2023 17:00:32 +0200
> Ville Syrj?l? <ville.syrjala@linux.intel.com> wrote:
> 
> > On Thu, Nov 30, 2023 at 05:07:40PM -0300, Andr? Almeida wrote:
> > > From: Pekka Paalanen <pekka.paalanen@collabora.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@collabora.com>
> > > Signed-off-by: Andr? Almeida <andrealmeid@igalia.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".

Probably not docuemented anywhere.

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

The kernel does track the full state sure, but it doesn't generally
go out of its way to figure out what specifically changed in that state.
Doing so would be a lot of extra checks, and kinda less convenient to
do inside the driver since at that point the state is already spread 
all over the various structures. And the fact that those structures
are a mismash of uapi and internal bits of state (and other metadata 
for the single commit that really shouldn't be stored there) doesn't
help matters. I did propose to split the state cleanly into pure uapi
vs. internal stuff but that didn't gain any traction unfortunately.

So I think it might be simpler to do on the uapi property level. It may
result in a somewhat coarser idea of what changed, but it avoids having
to track down all the little bits of state everwhere that could have
changed in response to a single property changing. The kernel could do
that I suppose, but someone would need to come up with a good way to
track that information. Currently there are a handful of foo_changed
booleans ad-hocced here and there, but nothing consistent that covers
everything.

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

Sure. But the way this was worded implied that you can just throw
everything and the kitchen sink into the commit without any
repercussions, which is not the case.

> until the kernel
> driver decides under the hood to pull in an unwanted CRTC.

That is sadly needed too sometimes. Hardware design is often
a bit disappointing.

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

-- 
Ville Syrj?l?
Intel