On 05/05, Pekka Paalanen wrote:
On Fri, 5 May 2023 12:16:55 +0200
Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Fri, May 05, 2023 at 11:43:20AM +0300, Pekka Paalanen wrote:
> > On Thu, 4 May 2023 17:25:57 -0400
> > Harry Wentland <harry.wentland@xxxxxxx> wrote:
> >
> > > We have steered away for a long time now from driver-specific KMS APIs
> > > for good reasons but never codified our stance. With the proposal of
> > > new, driver-specific color management uAPIs [1] it is important to
> > > outline the requirements for the rare times when driver-specific KMS
> > > uAPIs are desired in order to move complex topics along.
> > >
> > > [1] https://patchwork.freedesktop.org/series/116862/
> > >
> > > Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx>
> > > Cc: Simon Ser <contact@xxxxxxxxxxx>
> > > Cc: Joshua Ashton <joshua@xxxxxxxxx>
> > > Cc: Michel Dänzer <mdaenzer@xxxxxxxxxx>
> > > Cc: Sebastian Wick <sebastian.wick@xxxxxxxxxx>
> > > Cc: Jonas Ådahl <jadahl@xxxxxxxxxx>
> > > Cc: Alex Goins <agoins@xxxxxxxxxx>
> > > Cc: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> > > Cc: Melissa Wen <mwen@xxxxxxxxxx>
> > > Cc: Aleix Pol <aleixpol@xxxxxxx>
> > > Cc: Xaver Hugl <xaver.hugl@xxxxxxxxx>
> > > Cc: Victoria Brekenfeld <victoria@xxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > > Cc: Dave Airlie <airlied@xxxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
> > > To: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > ---
> > > Documentation/gpu/drm-uapi.rst | 32 ++++++++++++++++++++++++++++++++
> > > 1 file changed, 32 insertions(+)
> > >
> > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > index ce47b4292481..eaefc3ed980c 100644
> > > --- a/Documentation/gpu/drm-uapi.rst
> > > +++ b/Documentation/gpu/drm-uapi.rst
> > > @@ -118,6 +118,38 @@ is already rather painful for the DRM subsystem, with multiple different uAPIs
> > > for the same thing co-existing. If we add a few more complete mistakes into the
> > > mix every year it would be entirely unmanageable.
> > >
> > > +.. _driver_specific:
> > > +
> > > +Driver-Specific DRM/KMS uAPI
> > > +============================
> > > +
> > > +Driver-specific interfaces are strongly discouraged for DRM/KMS interfaces.
> > > +Kernel-modesetting (KMS) functionality does in principle apply to all drivers.
> > > +Driver-specific uAPIs tends to lead to unique implementations in userspace and
> > > +are often hard to maintain, especially when different drivers implement similar
> > > +but subtly different uAPIs.
> > > +
> > > +At times arriving at a consensus uAPI can be a difficult and lengthy process and
> > > +might benefit from experimentation. This experimentation might warrant
> > > +introducing driver specific APIs in order to move the eosystem forward. If a
> > > +driver decides to go down this path we ask for the following:
>
> I don't like this for two fairly fundamental reasons, neither of which are
> that sometimes merging stuff that's not great is the right thing to do to
> move the community and ecosystem forward.
>
> > Hi,
> >
> > should it be "require" instead of "ask"?
> >
> > > +
> > > +- An agreement within the community that introducing driver-specific uAPIs is
> > > + warranted in this case;
> > > +
> > > +- The new uAPI is behind a CONFIG option that is clearly marked EXPERIMENTAL and
> > > + is disabled by default;
> > > +
> > > +- The new uAPI is enabled when a module parameter for the driver is set, and
> > > + defaults to 'off' otherwise;
> > > +
> > > +- The new uAPI follows all open-source userspace requirements outlined above;
> > > +
> > > +- The focus is maintained on development of a vendor-neutral uAPI and progress
> > > + toward such an uAPI needs to be apparent on public forums. If no such progress
> > > + is visible within a reasonable timeframe (1-2 years) anybody is within their
> > > + right to send, review, and merge patches that remove the driver-specific uAPI.
> > > +
> > > .. _drm_render_node:
> > >
> > > Render nodes
> >
> > Seems fine to me. I have another addition to suggest:
> >
> > When such UAPI is introduced, require that it comes with an expiration
> > date. This date should be unmissable, not just documented. The kernel
> > module parameter name to enable the UAPI could contain the expiry date,
> > for example.
> >
> > After all, the most important thing to get through to users is that
> > this *will* go away and not just theoretically.
>
> There's no taking-backsies with uapi. If there is a regression report,
> we'll have to keep it around, for the usual approximation of "forever"
>
> And this is the first reason I don't like this, from other write-ups and
> talking with people it seems like there's the assumption that if we just
> hide this behind enough knobs, we can remove the uapi again.
>
> We can't.
>
> The times we've hidden uapi behind knobs was _not_ for uapi we
> fundamentally didn't want, at least for the long term. But for the cases
> where the overall scope was simply too big, and we needed some time
> in-tree to shake out all the bugs (across both kernel and userspace), and
> fill out any of the details. Some examples:
>
> - intel hiding new hw enabling behind the alpha support is not about
> hiding that uapi so we can change it. It's about the fact that not yet
> all enabling has landed in upstream, and not yet all full stack
> validation on production silicon has completed. It's about not shipping
> buggy code to users that we can't support.
>
> - atomic kms was simply too big, there was a lot of work in compositors
> needed, testing corner cases, and details like adding the blob support
> for the display mode so that modesets would work too with atomic. We
> never landed a preliminary uabi version of atomic (there were plenty
> floating around) that wasn't deemed ready as the long term solution, we
> were simply not sure we got it right until all the pieces where in
> place.
Hi Daniel,
I would be bold enough to claim that the KMS color processing UAPI has
all the same problems as atomic, except it is even bigger on the UAPI
surface, while the kernel internal driver code independent of the UAPI
is probably trivial(*) in comparison or even non-existing. This is all
about what hardware does and how to generalize a description of that
over all hardware of all vendors. I do not think there would be any
kind of complex state tracking needed inside the kernel, all the
complexity is at the UAPI interface and its definition.
Therefore I doubt the plan you proposed at the end. Do you have any
other suggestions?
Thanks,
pq
(*) I do not want to imply that the driver code is somehow not real
work to write. What I mean is that once the UAPI is defined, and you
know what your hardware does, you shouldn't have any trouble writing
that code. But without UAPI defined, I'd assume there is almost nothing
to write.
I haven't looked at the AMD patches to see what would be left if the
UAPI was dropped. Melissa?
We are exposing these elements in a driver-specific plane color API:
│ ├───"AMD_PLANE_DEGAMMA_LUT": blob = 0
│ ├───"AMD_PLANE_DEGAMMA_LUT_SIZE" (immutable): range [0,
UINT32_MAX] = 4096
│ ├───"AMD_PLANE_DEGAMMA_TF": enum {Default, sRGB, BT.709, PQ
(Perceptual Quantizer), Linear, Unity, HLG (Hybrid Log Gamma), Gamma
2.2, Gamma 2.4, Gamma 2.6} = Default
│ ├───"AMD_PLANE_HDR_MULT": range [0, UINT32_MAX] = 0
│ ├───"AMD_PLANE_SHAPER_LUT": blob = 0
│ ├───"AMD_PLANE_SHAPER_LUT_SIZE" (immutable): range [0,
UINT32_MAX] = 4096
│ ├───"AMD_PLANE_SHAPER_TF": enum {Default, sRGB, BT.709, PQ
(Perceptual Quantizer), Linear, Unity, HLG (Hybrid Log Gamma), Gamma
2.2, Gamma 2.4, Gamma 2.6} = Default
│ ├───"AMD_PLANE_LUT3D": blob = 0
│ ├───"AMD_PLANE_LUT3D_SIZE" (immutable): range [0,
UINT32_MAX] = 4913
│ ├───"AMD_PLANE_BLEND_LUT": blob = 0
│ ├───"AMD_PLANE_BLEND_LUT_SIZE" (immutable): range [0,
UINT32_MAX] = 4096
│ └───"AMD_PLANE_BLEND_TF": enum {Default, sRGB, BT.709, PQ
(Perceptual Quantizer), Linear, Unity, HLG (Hybrid Log Gamma), Gamma
2.2, Gamma 2.4, Gamma 2.6} = Default
In addition, we have extended CRTC color mgmt properties with:
│ │ ├───"AMD_SHAPER_LUT": blob = 0
│ │ ├───"AMD_SHAPER_LUT_SIZE" (immutable): range [0, UINT32_MAX]
= 4096
│ │ ├───"AMD_LUT3D": blob = 0
│ │ ├───"AMD_LUT3D_SIZE" (immutable): range [0, UINT32_MAX] = 4913
│ │ └───"AMD_GAMMA_TF": enum {Default, sRGB, BT.709, PQ
(Perceptual Quantizer), Linear, Unity, HLG (Hybrid Log Gamma), Gamma
2.2, Gamma 2.4, Gamma 2.6} = Default
I agree that defining a generic API is the uncertain part, and the most
problematic one. IMHO, this work gets worse because we don't have a
clear view of color capabilities of other vendors like we have from AMD
that already provided a good documentation of HW color caps and also the
shared-code.
> And viz Xorg-modesetting, in at least one case we still got it wrong and
> had to disable atomic for that userspace.
>
> - nouveau pony years back tried this entire "oh the uapi is just
> experimental" thing, and it resulted in the by far worst flameware
> between Dave and Linus on dri-devel
>
> So _if_ we do this we need to be clear that uapi is forever, and not have
> docs that suggest otherwise.
>
> > If that date needs to be moved forward, it should be possible to do so
> > with a simple patch gathering enough acks. The main thing is to set the
> > date from the start, so there can be no confusion about when its going
> > to the chopping block.
> >
> > I do not suggest that the kernel would automatically runtime disable
> > the UAPI after that date.
> >
> > Does any of the big idea fly with upper maintainers and Linus?
>
> The other reason, and maybe even more fundamental one, is that I think the
> uncertainty of not documenting how pragmatic we are is beneficial.
>
> We should definitely document the gold standard aspirations, to make sure
> everyone knows where to aim for. And I'm definitely all for pragmatic
> merging where it makes sense, we've had tons of that, and happily carry
> the costs to this day for some of them:
>
> - a lot of the early soc drivers are kinda meh, and will stay that way
> forever since they're not maintained anymore
>
> - we've had very much free-for-all vendor kms properties, and I expect the
> hall of shame witht he big table of vendor props with barely any docs
> will never go away
>
> - we're taking all the compute runtimes despite that mesa on the 3d/gl/vk
> side shows how much better collaboration would be (and I think soon will
> show the same for media) because having a compute ecosystem that's
> substantially weaker than the sum of all its parts is still better than
> nothing. And the situation is still that collaboration even with a
> company is often impossible, aiming for better is not very realistic :-/
>
> But the goal is still to have solid code, cross-vendor infrastructure and
> collaboration and all that stuff, because that's why upstream is strong.
> And the uncertainty is helping us for a lot of reasons:
>
> - it makes vendors vary of going with vendor solutions. Minimally they ask
> in private, which gives Dave, me and all the others doing vendor
> outreach or working as some ambassador rule at a vendor an opportunity
> to steer things in a better direction. And often do the steering
> _before_ code gets written.
>
> - it allows Dave&me to more freely decide when to be pragmatic, without
> being bound by rules. The point of pragmatic merging is to bend the
> short term principles for a better long term outcome, splattering that
> entire space full with rules makes rule-bending a lot harder when
> needed.
>
> - most of all I really don't want to be in a discussion with vendors where
> they try to laywer-argue that we must merge their patches because they
> strictly followed the wording of some pragmatic merge rules while
> entirely tossing the spirit of what we aim for. I already have more than
> enough of that, this will result in more.
>
> In all the past examples of pragmatic merging we never documented the
> pragmatic approach, but instead if we documented something, we wrote down
> the ideal standards to aim for. That makes it easier for everyone to do
> the right thing, and harder (and more expensive due to the inherit
> uncertainty) to try to bend them towards the least amount of collaboration
> a vendor can get away with.
>
> That's why I really want to keep the undocumented and hence uncertain
> rules in this space.
>
> For the actual case at hand of plane color handling, I think the pragmatic
> aproach is roughly:
>
> 1. land the amdgpu code, but without uapi
>
> 2. use that (and any other driver code that's been floating around in this
> space) to build up the kernel-internal infrastructure - the proposed graph
> of color transformation blocks will need quite a few things
>
> 3. land the uapi on top in it's hopeful final form, maybe hidden if it's
> not yet complete or ready for prime time as we sometimes do with bigger
> projects
>
> Obviously compositor work, igts, docs and all that too, and most of all
> this can happen in parallel too once we have a rough consensus on where to
> aim for.
>
> Cheers, Daniel
