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
