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

[Daniel Vetter]

On Fri, Aug 07, 2020 at 12:38:02PM +0300, Pekka Paalanen wrote:
> On Fri, 7 Aug 2020 11:07:06 +0200
> Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> > On Thu, Aug 06, 2020 at 10:33:31AM +0000, Simon Ser wrote:
> > > Some drivers may expose primary planes compatible with multiple CRTCs.
> > > Make this clear in the docs: the current wording may be misunderstood as
> > > "exactly one primary plane per CRTC".
> > > 
> > > Signed-off-by: Simon Ser <contact at emersion.fr>
> > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index b7b90b3a2e38..108a922e8c23 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -49,8 +49,8 @@
> > >   * &struct drm_plane (possibly as part of a larger structure) and registers it
> > >   * with a call to drm_universal_plane_init().
> > >   *
> > > - * 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).

> > The problem is that userspace doesn't have a drm_property to read this
> > pointer, and needs to guess.
> > 
> > I thought the rule is:
> > 
> > Nth primary plane (or cursor) is the primary plane for the Nth crtc.
> > Enumaration with increasing drm kms object ids.
> 
> Why is that needed? With universal planes, I thought
> drmModePlane::possible_crtcs bitmask is trustworthy?

Yes it should be.

> In the legacy KMS UAPI you can't even pick your primary plane, because
> it's implied in drmModeSetCrtc(), right?

Yup, I thought this all was so userspace knows which plane is the implied
one for legacy ioctls. Which does somewhat matter, since page_flip ioctl
has more features than atomic (target frame and async mode).

> > And I guess we should explain that on some hw any plane (including primary
> > ones, since that's only a sw construct) can be freely assinged to crtc.
> > 
> > Yes it's probably the most gloriously bonkers uapi we've come up with.
> > Might be so bad that a libdrm helper to look up the primary plane for a
> > crtc (or it's cursor plane if it exists) would be in order :-)
> 
> I'm not sure I see the bonkers there.

Userspace has to guess which primary plane is for which crtc, at least if
the possible_crtc mask has more than one bit set. Which can happen.
-Daniel
> 
> 
> Thanks,
> pq
> 
> > 
> > Cheers, Daniel
> > 
> > 
> > >   * drm_plane_type for a more in-depth discussion of these special uapi-relevant
> > >   * plane types. Special planes are associated with their CRTC by calling
> > >   * drm_crtc_init_with_planes().
> > > -- 
> > > 2.28.0
> > > 
> > >   
> > 
> 



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch