Re: [PATCH 00/20] drm: Split out the formats API and move it to a common place

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Apr 21, 2019 at 01:40:45AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> On Thu, Apr 18, 2019 at 01:49:54PM +0200, Paul Kocialkowski wrote:
> > On Thu, 2019-04-18 at 11:02 +0200, Maxime Ripard wrote:
> > > On Thu, Apr 18, 2019 at 09:52:10AM +0200, Daniel Vetter wrote:
> > >> And a lot of people pushed for the "fourcc is a standard", when
> > >> really it's totally not.
> > > 
> > > Even if it's not a standard, having consistency would be a good thing.
> > > 
> > > And you said yourself that DRM fourcc is now pretty much an authority
> > > for the fourcc, so it definitely looks like a standard to me.
> > 
> > I think trying to make the V4L2 and DRM fourccs converge is a lost
> > cause, as it has already significantly diverged. Even if we coordinate
> > an effort to introduce new formats with the same fourcc on both sides,
> > I don't see what good that would be since the formats we have now are
> > still plagued by the inconsistency.
> > 
> > I think we always need an explicit translation step from either v4l2 or
> > drm to the internal representation and back, without ever assuming that
> > formats might be compatible because they share the same fourcc.
> 
> I don't agree. APIs evolve, and while we can't switch from one set of
> 4CCs to another in existing APIs, we could in new APIs. Boris is working
> on new ioctls to handle formats in V4L2, and while 4CC unification could
> be impopular from a userspace developers point of view there, I don't
> think we have ruled it out completely. The move to the request API is
> also an area where a common set of 4CCs could be used, as it will depart
> from the existing V4L2 ioctls. To summarize my opinion, we're not there
> yet, but I wouldn't rule it out completely for the future.
> 
> > It looks like so far, V4L2 pixel formats describe a DRM pixel format +
> > modifier.
> 
> DRM modifiers are mostly about tiling and compression, and we hardly
> support these in V4L2. What are the modifiers you think are hardcoded in
> 4CCs in V4L2 ?

Hm maybe it was a drm one that didn't come from v4l or anywhere else
really, but the NV12MT one is nv12 + some tiling. I think we managed to
uapi-bend that one into shape in at least drm.
-Daniel

> > I think Boris (CCed) is working to change that by allowing to
> > pass a DRM modifier through V4L2. With that, we'd be in a situation
> > where some formats are described by the v4l2 pixfmt alone and some
> > formats are also described a modifier (but I looked at it from a
> > distance so might have misunderstod). That feels better since it avoids
> > the combinatory explosion from describing each format + modifier
> > individually.
> > 
> > What do you think?
> > 
> > >> v4l tends to conflate pixel format with stuff that we tend to encode
> > >> in modifiers a lot more.
> > > 
> > > Boris is working on adding the modifiers concept to v4l2, so we're
> > > converging here, and we can totally have a layer in v4l2 to convert
> > > between old v4l2 "format+modifiers" formats, and DRM style formats.
> > > 
> > >> There's a bunch of reasons we can't just use v4l, and they're as
> > >> valid as ever:
> > >> 
> > >> - We overlap badly in some areas, so even if fourcc codes match, we
> > >>   can't use them and need a duplicated DRM_FOURCC code.
> > > 
> > > Do yo have an example of one of those areas?
> > > 
> > >> - v4l encodes some metadata into the fourcc that we encode elsewhere,
> > >>   e.g. offset for planar yuv formats, or tiling mode
> > > 
> > > As I was saying, this changes on the v4l2 side, and converging to
> > > what DRM is doing.
> > > 
> > >> - drm fourcc code doesn't actually define the drm_format_info
> > >>   uniquely, drivers can override that (that's an explicit design
> > >>   intent of modifiers, to allow drivers to add another plane for
> > >>   e.g. compression information). You'd need to pull that driver
> > >>   knowledge into your format library.
> > > 
> > > I'm not sure how my patches are changing anything here. This is
> > > litterally the same code, with the functions renamed.
> > > 
> > > If drivers want to override that, then yeah, fine, we can let them do
> > > that. Just like any helper this just provides a default that covers
> > > most of the cases.
> > > 
> > >> Iow there's no way we can easily adopt v4l fourcc, except if we do
> > >> something like a new addfb flag.
> > > 
> > > For the formats that would be described as a modifier, sure. For all
> > > the others (that are not yet supported by DRM), then I don't really
> > > see why not.
> > > 
> > >>> And given how the current state is a mess in this regard, I'm not too
> > >>> optimistic about keeping the formats in their relevant frameworks.
> > >>> 
> > >>> Having a shared library, governed by both, will make this far easier,
> > >>> since it will be easy to discover the formats that are already
> > >>> supported by the other subsystem.
> > >> 
> > >> I think a compat library that (tries to, best effort) convert between
> > >> v4l and drm fourcc would make sense. Somewhere in drivers/video, next
> > >> to the conversion functions for videomode <-> drm_display_mode
> > >> perhaps. That should be useful for drivers.
> > > 
> > > That's not really what this series is about though. That series is
> > > about sharing the (image|pixels) formats database across everyone so
> > > that everyone can benefit from it.
> > > 
> > >> Unifying the formats themselves, and all the associated metadata is
> > >> imo a no-go, and was a pretty conscious decision when we implemented
> > >> drm_fourcc a few years back.
> > >> 
> > >>> If we want to keep the current library in DRM, we have two options
> > >>> then:
> > >>> 
> > >>>   - Support all the v4l2 formats in the DRM library, which is
> > >>>     essentially what I'm doing in the last patches. However, that
> > >>>     would require to have the v4l2 developpers also reviewing stuff
> > >>>     there. And given how busy they are, I cannot really see how that
> > >>>     would work.
> > >> 
> > >> Well, if we end up with a common library then yes we need cross
> > >> review. There's no way around that. Doesn't matter where exactly that
> > >> library is in the filesystem tree, and adding a special MAINTAINERS
> > >> entry for anything related to fourcc (both drm and v4l) to make sure
> > >> they get cross-posted is easy. No file renaming needed.
> > > 
> > > That would create some governing issues as well. For example, if you
> > > ever have a patch from one fourcc addition (that might or might not be
> > > covered by v4l2), will you wait for any v4l2 developper to review it?
> > > 
> > > If it's shared code, then it should be shared, and every client
> > > framework put on an equal footing.
> > > 
> > >>>   - Develop the same library from within v4l2. That is really a poor
> > >>>     solution, since the information would be greatly duplicated
> > >>>     between the two, and in terms of maintainance, code, and binary
> > >>>     size that would be duplicated too.
> > >> 
> > >> It's essentially what we decided to do for drm years back.
> > > 
> > > And it was probably the right solution back then, but I'm really not
> > > convinced it's still the right thing to do today.
> > > 
> > >>> Having it shared allows to easily share, and discover formats from the
> > >>> other subsystem, and to have a single, unique place where this is
> > >>> centralized.
> > >> 
> > >> What I think could work as middle ground:
> > >> - Put drm_format stuff into a separate .ko
> > >> - Add a MAINTAINERS entry to make sure all things fourcc are cross
> > >> posted to both drm and v4l lists. Easy on the drm side, since it's all
> > >> separate files. Not sure it's so convenient for v4l uapi.
> > >> - Add a conversion library that tries to best-effort map between drm
> > >> and v4l formats. On the drm side that most likely means you need
> > >> offsets for planes, and modifiers too (since those are implied in some
> > >> v4l fourcc). Emphasis on "best effort" i.e. only support as much as
> > >> the drivers that use this library need.
> > >> - Add drm_fourcc as-needed by these drivers that want to use a unified
> > >> format space.
> > >> 
> > >> Forcing this unification on everyone otoh is imo way too much.
> > > 
> > > v4l2 is starting to converge with DRM, and we're using the DRM API
> > > pretty much untouched for that library, so I'm not really sure how
> > > anyone is hurt by that unification.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

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