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. -- Regards, Laurent Pinchart
