Re: [PATCH] drm: drivers may provide multiple primary planes per CRTC

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Dec 07, 2020 at 09:10:00AM +0000, Simon Ser wrote:
> On Monday, December 7th, 2020 at 9:45 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > > > > > > - * Cursor and overlay planes are optional. All drivers should provide one
> > > > > > > - * primary plane per CRTC to avoid surprising userspace too much. See enum
> > > > > > > + * Cursor and overlay planes are optional. All drivers should provide at least
> > > > > > > + * one primary plane per CRTC to avoid surprising userspace too much. See enum
> > > > > >
> > > > > > I think that's even more confusing, since this reads like there could be
> > > > > > multiple primary planes for a specific CRTC. That's not the case, there'
> > > > > > only one pointer going from drm_crtc->primary to a drm_plane in the
> > > > > > kernel.
> > > > >
> > > > > There could be multiple primary planes *usable* for a specific CRTC but
> > > > > just one used at a time, right?
> > > >
> > > > I'm not sure what you mean here, the crtc->primary link is invariant over
> > > > the lifetime of a driver load. You can't pick a different one, that's set
> > > > at driver init before drm_dev_register (and hence before userspace ever
> > > > sees anything).
> > >
> > > OK. I'm personally not very interested in documenting legacy bits, so I'll skip
> > > that. I'm mainly interested here in making it clear possible_crtcs for a
> > > primary plane can have more than one bit set. Because of the paragraph in the
> > > current docs, some user-space developers have understood "more than one bit set
> > > in possible_crtcs for a primary plane is a kernel bug".
> > >
> > > I'll send a v2 that makes it clear these pointers are for legacy uAPI.
> >
> > Right, so this and what danvet said seem to be in direct conflict in
> > atomic uAPI, repeating above:
> >
> > > > I'm not sure what you mean here, the crtc->primary link is invariant over
> > > > the lifetime of a driver load. You can't pick a different one, that's set
> > > > at driver init before drm_dev_register (and hence before userspace ever
> > > > sees anything).
> >
> > But still, it is considered not a kernel bug that a primary plane has
> > more than one bit set in its possible_crtcs.
> >
> > If a primary plane has more than one bit set in possible_crtcs, and it
> > is not a kernel bug, then userspace expects to be able to choose any
> > of the multiple indicated possible CRTCs for this primary plane.
> >
> > Which way is it?
> >
> > Or, is there a different limitation that for each CRTC, there must be
> > exactly one primary plane with that CRTCs bit set in its possible_crtcs?
> >
> > IOW, you can have more CRTCs than primary planes in total, and you can
> > activate each CRTC alone, but you cannot activate all CRTCs
> > simultaneously because there are not enough primary planes for them?
> >
> > Representing it mathematically, the possible assignments according to
> > possible_crtcs while ignoring all other limitations are:
> > N CRTCs <-> M primary planes
> >
> > - Is N one or greater than one?
> > - Is M one or greater than one?
> 
> I think the current situation is that:
> 
> - It's perfectly fine for a driver to expose multiple bits in possible_crtcs.
>   User-space can attach the primary plane to any of these CRTCs (of course, a
>   primary plane still can only be attached to a single CRTC at a time). Drivers
>   should provide as many primary planes as there are CRTCs.
> - The legacy API doesn't expose primary planes. Some legacy IOCTLs like
>   drmModeSetCrtc allow user-space to attach a FB directly to a CRTC. The driver
>   needs to implicitly select a primary plane for this operation. That's the
>   only case where the internal CRTC → primary plane link is used in the kernel.
> 
> Is this correct, Daniel?

Yup. atomic doesn't use crtc->primary link at all.

Pekka, where did you see an indication that this crtc->primary link is
used for atomic? My statement was only about legacy ioctl impact of
->primary. Atomic userspace can pick any plane it wants and consider that
the "primary" one (the hw/driver might reject that, but that's a different
issue).

> So I believe M > 1 and N > 1 is possible and isn't a kernel bug. For instance
> some drivers hardcode possible_crtcs to 0xFF (although it might be nicer to
> user-space to set the bitmask depending on the number of CRTCs, to avoid
> setting bits for non-existing CRTCs).

possible_crtcs for a primary plane has exactly the same constraints as
possible_crtcs for any other plane. The only additional constraint there
is that:
- first primary plane you iterate must have the first bit set in
  possible_crtcs, and it is the primary plane for that crtc
- 2nd primary plane has the 2nd bit set in possible_crtcs, and it is the
  primary plane for that crtc

and so on. That's all. I'm not sure all drivers get this right, so I think
it'd be good to check that at drm_dev_register time (we check a few other
things about these possible_crtcs masks already, so it's a good fit).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel