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

[Paul Kocialkowski]

Hi,

Le mardi 23 avril 2019 Ã 18:45 +0300, Laurent Pinchart a Ãcrit :
> Hi Daniel,
> 
> On Tue, Apr 23, 2019 at 09:25:54AM +0200, Daniel Vetter wrote:
> > On Sun, Apr 21, 2019 at 01:59:04AM +0300, Laurent Pinchart wrote:
> > > On Thu, Apr 18, 2019 at 12:07:44PM +0200, Daniel Vetter wrote:
> > > > On Thu, Apr 18, 2019 at 11:02 AM Maxime Ripard wrote:
> > > > > On Thu, Apr 18, 2019 at 09:52:10AM +0200, Daniel Vetter wrote:
> > > > > > On Thu, Apr 18, 2019 at 8:22 AM Maxime Ripard wrote:
> > > > > > > On Wed, Apr 17, 2019 at 05:41:21PM +0200, Daniel Vetter wrote:
> > > > > > > > On Wed, Apr 17, 2019 at 09:54:26AM +0200, Maxime Ripard wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > DRM comes with an extensive format support to retrieve the various
> > > > > > > > > parameters associated with a given format (such as the subsampling, or the
> > > > > > > > > bits per pixel), as well as some helpers and utilities to ease the driver
> > > > > > > > > development.
> > > > > > > > > 
> > > > > > > > > v4l2, on the other side, doesn't provide such facilities, leaving each
> > > > > > > > > driver reimplement a subset of the formats parameters for the one supported
> > > > > > > > > by that particular driver. This leads to a lot of duplication and
> > > > > > > > > boilerplate code in the v4l2 drivers.
> > > > > > > > > 
> > > > > > > > > This series tries to address this by moving the DRM format API into lib and
> > > > > > > > > turning it into a more generic API. In order to do this, we've needed to do
> > > > > > > > > some preliminary changes on the DRM drivers, then moved the API and finally
> > > > > > > > > converted a v4l2 driver to give an example of how such library could be
> > > > > > > > > used.
> > > > > > > > > 
> > > > > > > > > Let me know what you think,
> > > > > > > > > Maxime
> > > > > > > > > 
> > > > > > > > > Changes from RFC:
> > > > > > > > >   - Rebased on next
> > > > > > > > >   - Fixed the various formats mapping
> > > > > > > > >   - Added tags
> > > > > > > > >   - Did most of the formats functions as inline functions
> > > > > > > > >   - Used a consistent prefix for all the utilities functions
> > > > > > > > >   - Fixed the compilation breakages, and did a run of allmodconfig for arm,
> > > > > > > > >     arm64 and x86_64
> > > > > > > > >   - Fixed out of array bounds warnings in the image_format_info_block_*
> > > > > > > > >     functions
> > > > > > > > >   - Added License and copyright headers on missing files
> > > > > > > > > 
> > > > > > > > > Maxime Ripard (20):
> > > > > > > > >   drm: Remove users of drm_format_num_planes
> > > > > > > > >   drm: Remove users of drm_format_(horz|vert)_chroma_subsampling
> > > > > > > > >   drm/fourcc: Pass the format_info pointer to drm_format_plane_cpp
> > > > > > > > >   drm/fourcc: Pass the format_info pointer to drm_format_plane_width/height
> > > > > > > > >   drm: Replace instances of drm_format_info by drm_get_format_info
> > > > > > > > >   lib: Add video format information library
> > > > > > > > >   drm/fb: Move from drm_format_info to image_format_info
> > > > > > > > >   drm/malidp: Convert to generic image format library
> > > > > > > > >   drm/client: Convert to generic image format library
> > > > > > > > >   drm/exynos: Convert to generic image format library
> > > > > > > > >   drm/i915: Convert to generic image format library
> > > > > > > > >   drm/ipuv3: Convert to generic image format library
> > > > > > > > >   drm/msm: Convert to generic image format library
> > > > > > > > >   drm/omap: Convert to generic image format library
> > > > > > > > >   drm/rockchip: Convert to generic image format library
> > > > > > > > >   drm/tegra: Convert to generic image format library
> > > > > > > > >   drm/fourcc: Remove old DRM format API
> > > > > > > > >   lib: image-formats: Add v4l2 formats support
> > > > > > > > >   lib: image-formats: Add more functions
> > > > > > > > >   media: sun6i: Convert to the image format API
> > > > > > > > 
> > > > > > > > In the interest of making myself unpopular: Why move this out of drm?
> > > > > > > > 
> > > > > > > > We do have a bunch of other such shared helpers already (mostly in
> > > > > > > > drivers/video) for dt videomode and hdmi infoframes, and I'm not super
> > > > > > > > sure that's going better than keeping it maintained in drm.
> > > 
> > > That's a bit of a different situation as both DRM and FBDEV address the
> > > same features (display output), and FBDEV is deprecared and replaced by
> > > DRM.
> > > 
> > > I'm not against maintaining a 4CC library in DRM (adding a third-party
> > > maintainer would likely create more problems than it would solve), but
> > > that doesn't mean the library has to live in drivers/gpu/, nor that it
> > > needs to have the drm_ prefix. I would actually advocate to make it live
> > > in a neutral directory, with a neutral prefix (kudos to anyone who can
> > > propose a nice naming convention that would establish a new shared
> > > ground for image/video-related Linux APIs), and maintained through the
> > > DRM tree (possibly with extra entries in MAINTAINERS to ensure it
> > > reaches all the related folks).
> > > 
> > > > > > > > Plus the uapi is already that you include drm_fourcc.h to get at these,
> > > > > > > > dropping the drm prefix isn't going to help I think.
> > > > > > > > 
> > > > > > > > Of course we'd need to make it a separate drm_formats.ko (so that v4l can
> > > > > > > > use it without dragging in all of drm), and we need to add some fields for
> > > > > > > > converting to v4l fourcc codes (which are different). But that should be
> > > > > > > > all possible. And I don't think the drm_ prefix in v4l code is a problem,
> > > > > > > > it's actually a feature: It makes it really clear that these are the drm
> > > > > > > > fourcc codes, as allocated in drm_fourcc.h, plus their modifiers, and all
> > > > > > > > that. That allocation authority is also already baked into various khr/ext
> > > > > > > > standards, too.
> > > 
> > > There's one thing that V4L2 has and DRM hasn't for 4CCs: good
> > > documentation. Look at
> > > https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/pixfmt-packed-rgb.html
> > > or
> > > https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/yuv-formats.html
> > > for instance. It's painful to write, painful to read, but defines the
> > > 4CCs very clearly without ambiguity. I wouldn't be surprised if
> > > different drivers used the same DRM 4CC for different formats due to the
> > > lack of detailed documentation. Moving to a shared library for 4CCs
> > > should also address the documentation side, and any new format added to
> > > the kernel, whether from the V4L2 side or the DRM side, would be
> > > required to provide detailed documentation. That would be a great
> > > improvement for DRM 4CC handling.
> > > 
> > > > > > > The way I see it, there's a fundamental difference between the UAPI
> > > > > > > and the kernel. I don't suggest we change anything about the UAPI: the
> > > > > > > drm formats should keep their prefix, drm_fourcc.h can remain that
> > > > > > > authority, it's all fine.
> > > > > > > 
> > > > > > > Internally however, the long term goal is to share the fourcc's
> > > > > > > between DRM and V4L2 for the same formats. It basically means that of
> > > > > > > course v4l2 should be using the DRM fourcc when a format exists in DRM
> > > > > > > and not v4l2, but also that DRM should use v4l2 fourcc when the format
> > > > > > > exists in v4l2 but not DRM, and that is far more likely, given the
> > > > > > > already extensive v4l2 formats support.
> > > > > > 
> > > > > > Uh no. We did look at v4l fourcc extensively when deciding upon a drm
> > > > > > format identifier space.
> > > > > 
> > > > > No to what exactly?
> > > > > 
> > > > > > 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.
> > > > 
> > > > drm fourcc is the authority for drm fourcc codes. Not for any of the
> > > > others (and there's lots of them). Now it's used in a bunch of other
> > > > places (khr standards, dri protocols in wayland/X11), but they're
> > > > still only drm fourcc. There is no overall fourcc standard.
> > > > 
> > > > > > 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?
> > > > 
> > > > I think the rgb stuff was one of the big reasons to not reuse any
> > > > existing fourcc standard (whether v4l, or another one from e.g. video
> > > > container formats). We had initial patches to reuse the fourcc that
> > > > existed, but the end result was really inconsistent, so we ditch that
> > > > and rolled out a new set of entirely drm specific fourcc codes for
> > > > rgba.
> > > 
> > > Could you give one or a couple of examples of V4L2 4CCs that are not
> > > OCD-compatible ? :-)
> > > 
> > > > > > - 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.
> > > 
> > > That's a mistake in my opinion. We tried that in V4L2 to store metadata
> > > in a separate plane, and had to go another route eventually as it
> > > created a very bad mess.
> > 
> > Just quick clarification in the middle here: This is how the hw works.
> 
> The hardware takes parameters from a buffer, but it doesn't mandate how
> that buffer is exposed to userspace :-) Using an extra plane is one
> option, but other APIs are possible.
> 
> > It's not metadata that sw ever touches (in general, testcases to make sure
> > we display these correctly excepted).
> > 
> > There has been some talking to add maybe a bit more mixed metadata, for
> > fast-clear colors (which isn't used by any display engine afaik yet). That
> 
> What are fast-clear colors ?
> 
> > would generally be written by the cpu (in the gl stack), but again read by
> > the hw (loaded as indirect state packet most likely, or something like
> > that). So again hw specific layout, because the hw needs to read it.
> > 
> > Pure metadata only of interest for the cpu/sw stack has been shot down
> > completely on the drm side too.
> > 
> > > There's a tendency in both subsystems to look at the other as a bit of a
> > > retarded relative, and that's a shame, we have lots to learn from each
> > > other's mistakes. That wouldn't be too difficult if people started
> > > talking to each other.
> > > 
> > > A semi-related comment: DRM also carries other mistakes of its own, I'm
> > > thinking about DRM_FORMAT_BIG_ENDIAN in particular
> > 
> > Yeah that one is hilarios, but in practice big endian is dead, except for
> > a very few server chips, and there I think Gerd's work mostly fixed up
> > that mess.
> > 
> > > > > 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.
> > > > 
> > > > See above, we tried that initially, didn't pass the ocd tests when
> > > > reviewing. Maybe the situation is better outside of rbgx/a formats,
> > > > and I think we do at least try to use the same fourcc codes there when
> > > > there already is one. But then the details occasionally look
> > > > different.
> > > > 
> > > > > > > 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.
> > > > 
> > > > Yeah I know. I still think leaving the drm fourcc with the drm prefix
> > > > would be good, since there's really no standard here.
> > > > 
> > > > > > 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?
> > > > 
> > > > None of this is fixed by code renaming or locations. Either way we
> > > > need to figure that out.
> > > > 
> > > > And yes part of the reasons for not moving this out of drm is that I'm
> > > > not a fan of boutique trees for small stuff. If sharing means we need
> > > > to split the drm_fourcc code and library out of drm trees, I'm not
> > > > sure that's a good idea. We're already super liberal with merging
> > > > anything through driver trees with acks, and integrating them quickly
> > > > into drm-next. This would still be workable if v4l sends regular pull
> > > > requests to drm-next (every 1-2 weeks, like the other big gpu trees
> > > > do). If we can only sync up once per merge window with a shared
> > > > boutique tree for formats only, life is going to be painful.
> > > 
> > > That should be solved by the proposal above, maintaining the shared
> > > library in the DRM tree. We would need to be careful there, and ideally
> > > maintain that in a separate branch that could be merged in both DRM and
> > > V4L2 without having to merge most of the other subsystem's pending
> > > changes at the same time, but I think it's doable without any big issue.
> > > 
> > > > > 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.
> > > > 
> > > > It might make sense to regularly pull v4l updates into drm-next then
> > > > anyway. That would also remove the need to have the format library
> > > > somewhere else.
> > > 
> > > Are you saying it should the live in V4L2 ? ;-)
> > 
> > Maybe a few clarifications on how the drm shared core thing usually works,
> > and why I'm a sticker here. Bottom reply since I'm not sure where to put
> > it:
> > 
> > - Refactorings usually go in through drm-misc (at least since a few
> >   years).
> > 
> > - Small patches go in through the relevant driver tree (which is often
> >   drm-misc, but not always), with an Ack from drm maintainers.
> > 
> > - No topic branches, everyone just pushes patches where it's most
> >   convenient.
> > 
> > We get away with this mess because everyone sends regular pull requests to
> > drm, where the entire history is baked in and others can backmerge/fast
> > forward/rebase. Worst case you wait one month (around the merge window,
> > when drm-next is closed for features), but usually it's just 1-2 weeks.
> > Plus we tend to have fairly big trees, with good chances that the next
> > patch series lands in the same tree again and no work at all is needed.
> > 
> > If we start regularly sharing lots of code with v4l (which seems to be the
> > long term goal here), then I think we need something equally convenient
> > for all that.
> > 
> > We're not going to be able to teach some complicated topic branch scheme
> > to the 50+ submaintainers/committers we have in drm - a lot much more
> > basic stuff already takes lots of work to get it to stick. If the proposal
> > is "to be careful" and "maintain it in a separate branch", I'm not in
> > favour because I think that just wouldn't work.
> 
> Why not ? It can be a fast-moving branch that gets merged in drm-misc
> as often as you want (even at every commit if that's desired). We're
> dealing with a limited amount of code here, and there's no more reason
> that V4L2 should pull in drm-misc to get 4CC updates than DRM should
> pull V4L2 for the same. I have no objection against a 4CC branch being
> officially maintained under the DRM umbrella, but I think the code
> should live elsewhere than drivers/gpu/drm/, have a neutral prefix, and
> not require pulling an entire subsystem in.

I think that having a subsystem-neutral tree is what Maxime had in mind
initially. The way I see it, the idea would indeed be to keep it low-
traffic and avoid having any serious core code in there. Basically just
stick to format helpers and ideally later buffer description structures
and helpers.

I also like the idea of having a few shared ioctls to retrieve a buffer
memory layout information from a dma-buf fd and mmap-ing on a per-plane 
basis, but that's rather unlikely to evolve much once it's there.

Cheers,

Paul