On Fri, Dec 08, 2023 at 03:59:46PM +0200, Pekka Paalanen wrote:
> On Fri, 8 Dec 2023 13:25:22 +0100
> Maxime Ripard <mripard@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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.
Ack.
> 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?
Those are all good questions, and the answer is... I don't know, sorry :/
> > > > 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.
Right, so my initial goal from this series was to make sure the
ambiguous meaning of what a state was was (hopefully) lifted, and thus
we could progress on your userspace doc patch. So I want to address
point A here.
B is also important, but maybe we should discuss it into a separate
patch?
Maxime
Attachment:
signature.asc
Description: PGP signature
