[PATCH 4/5] drm/atomic: Make the drm_atomic_state documentation less ambiguous

Fri Dec 8 13:59:46 UTC 2023

On Fri, 8 Dec 2023 13:25:22 +0100
Maxime Ripard <mripard at kernel.org> wrote:

> On Fri, Dec 08, 2023 at 10:08:28AM +0200, Pekka Paalanen wrote:
> > On Thu, 7 Dec 2023 15:27:33 +0100
> > Maxime Ripard <mripard at kernel.org> wrote:
> >   
> > > On Tue, Dec 05, 2023 at 10:51:06AM +0200, Pekka Paalanen wrote:  
> > > > On Mon,  4 Dec 2023 13:17:06 +0100
> > > > Maxime Ripard <mripard at kernel.org> wrote:
> > > >     
> > > > > The current documentation of drm_atomic_state says that it's the "global
> > > > > state object". This is confusing since, while it does contain all the
> > > > > objects affected by an update and their respective states, if an object
> > > > > isn't affected by this update it won't be part of it.
> > > > > 
> > > > > Thus, it's not truly a "global state", unlike object state structures
> > > > > that do contain the entire state of a given object.
> > > > > 
> > > > > Signed-off-by: Maxime Ripard <mripard at kernel.org>
> > > > > ---
> > > > >  include/drm/drm_atomic.h | 10 +++++++++-
> > > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > > > > index 914574b58ae7..81ad7369b90d 100644
> > > > > --- a/include/drm/drm_atomic.h
> > > > > +++ b/include/drm/drm_atomic.h
> > > > > @@ -346,11 +346,19 @@ struct __drm_private_objs_state {
> > > > >  };
> > > > >  
> > > > >  /**
> > > > > - * struct drm_atomic_state - the global state object for atomic updates
> > > > > + * struct drm_atomic_state - Atomic Update structure
> > > > > + *
> > > > > + * This structure is the kernel counterpart of @drm_mode_atomic and contains
> > > > > + * all the objects affected by an atomic modeset update and their states.    
> > > > 
> > > > Drop "modeset"? An update can be without a modeset.    
> > > 
> > > Yeah, good point
> > >   
> > > > >   *
> > > > >   * States are added to an atomic update by calling drm_atomic_get_crtc_state(),
> > > > >   * drm_atomic_get_plane_state(), drm_atomic_get_connector_state(), or for
> > > > >   * private state structures, drm_atomic_get_private_obj_state().
> > > > > + *
> > > > > + * NOTE: While this structure looks to be global and affecting the whole DRM
> > > > > + * device, it only contains the objects affected by the atomic commit.    
> > > > 
> > > > This new phrasing is much more clear to me than the old one.    
> > > 
> > > Great
> > >   
> > > > > + * Unaffected objects will not be part of that update, unless they have been
> > > > > + * explicitly added by either the framework or the driver.    
> > > > 
> > > > If the framework or a driver adds an object, then it's no longer
> > > > unaffected, is it?    
> > > 
> > > I guess what I meant to say is that it's affected as a side effect that
> > > the userspace cannot anticipate.
> > > 
> > > The typical example being that changing a property on a connector would
> > > need to pull the CRTC into the update to disable / enable the CRTC,
> > > encoder and connector.  
> > 
> > Right, that's the easy case. This can even be documented and therefore
> > become expected by userspace. The associations between CRTC and
> > connector are published, e.g. the current routing.
> > 
> > What I was thinking of was much more hidden:
> > 
> > Userspace explicitly programs CRTC A and the connector connected to it.
> > None of the mentioned KMS objects in that atomic commit refer to CRTC B
> > in any way, not in old nor in new device state. Still, the driver
> > decides to pull CRTC B in, because it needs to adjust something there
> > (ISTR hearing "watermarks" in some conversation) in order to
> > accommodate changes in CRTC A.
> > 
> > So there are two categories of pulling in extra KMS objects: knowable
> > and unknowable. Or expected and unexpected.
> > 
> > If userspace changes things on a connector connected to CRTC A, you can
> > expect to pull in CRTC A. That's knowable. If the driver unexpectedly
> > also pulls in CRTC B while userspace made absolutely no explicit nor
> > implicit reference to it, that's unknowable.  
> 
> > The unknowable/unexpected additions are very hardware and driver
> > specific and not reliably determinable from an atomic commit UAPI
> > structure.  
> 
> So I had a quick look at all the drivers we have in-tree, and it looks
> to be either a plane or a connector pulling its CRTC in. I guess they
> would all qualify as knowable.

Yes.

> For unknowable, yeah, it's kind of bad, but at the same time you have to
> deal with the hardware you have. Like, for vc4 for example, there's a
> single controller before the CRTCs that deals with the blending, scaling
> and all the other stuff. That controller has a limited number of FIFOs
> and muxes to output the result of the blending to the right CRTC.
> 
> So if you commit something on one CRTC, you might very well wait for
> another one to complete because some hidden state (to userspace) is
> shared between the two and you just can't run them in parallel.
> 
> So yeah... We should strive to make it as transparent to userspace as
> possible, but I also don't think we can express all the variations we
> support.

I do not expect this could be prevented. It's important to acknowledge
that this can happen, even if it cannot be specified when it happens.
It's also important to document the requirement, like maybe it's not ok
to pull in unexpected CRTCs without ALLOW_MODESET? Maybe there is
already a check in DRM core for this?

If userspace is using per-CRTC flip completion events, then userspace
must always know before-hand which flip events it's going to get
eventually.

If userspace is using non-blocking commits (async is yet another
dimension), then the kernel pulling in an unexpected CRTC will risk
userspace failing its next commit on the unexpected CRTC with EBUSY.

When userspace uses non-blocking commits, it practically always also
expects flip events.

I don't know how ALLOW_MODESET + non-blocking should work. If it pulls
in unexpected CRTCs, will userspace also get unexpected flip events, or
is it only prone to EBUSY?

I would guess that unexpected flip events are avoided, and the expected
flip event is just delayed until unexpected CRTCs have completed as
well?

> > > As far as userspace is concerned, only the connector will be affected,
> > > and only the connector will initially be part of the drm_atomic_state.
> > > 
> > > But then some part of the atomic_check logic will pull the CRTC into the
> > > update.  
> > 
> > Is this a rule that happens always? If so, it can be documented to make
> > it expected.
> >   
> > > It's still invisible to userspace, so I guess
> > > "unaffected-but-turns-out-to-be-affected" would be the right term :)
> > > 
> > > Would something like:
> > > 
> > > Unaffected objects will not be part of the initial update, but might be
> > > added explicitly by either the framework or the driver during
> > > atomic_check.
> > > 
> > > be better?  
> > 
> > I'm not comfortable with the use of "unaffected" here. I'd be more in
> > the direction of: More objects can be affected than explicitly
> > mentioned.  
> 
> I think Sima's suggestion uses a different phrasing that should be much
> better. Can you check if it works for you?

Sima's phrasing is an addition, not a replacement, to this. There are
two things:

a) An atomic commit does not need to contain all the objects of a
   drm_device.

b) An atomic commit may affect more objects than it originally
   contained. (from userspace perspective)

Here b) is important for userspace to know, because it can be
surprising, but I understand that this patch is not userspace doc.

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231208/093ca3fe/attachment.sig>