Hi,

On Wed, Apr 17, 2019 at 02:34:54PM +0200, Paul Kocialkowski wrote:
> > +struct image_format_info {
> > + union {
> > + /**
> > + * @drm_fmt:
> > + *
> > + * DRM 4CC format identifier (DRM_FORMAT_*)
> > + */
> > + u32 drm_fmt;
>
> Could we call this one format_drm for consistency with the one below?

The deprecated "format" field will go away at some point, so I'm not
sure the consistency is an argument there.

> > +/**
> > + * image_format_info_min_pitch - computes the minimum required pitch in bytes
> > + * @info: pixel format info
> > + * @plane: plane index
> > + * @buffer_width: buffer width in pixels
> > + *
> > + * Returns:
> > + * The minimum required pitch in bytes for a buffer by taking into consideration
> > + * the pixel format information and the buffer width.
> > + */
> > +static inline
> > +uint64_t image_format_info_min_pitch(const struct image_format_info *info,
> > + int plane, unsigned int buffer_width)
> > +{
> > + if (!info || plane < 0 || plane >= info->num_planes)
> > + return 0;
> > +
> > + return DIV_ROUND_UP_ULL((u64)buffer_width * info->char_per_block[plane],
> > + image_format_info_block_width(info, plane) *
> > + image_format_info_block_height(info, plane));
>
> I'm not sure I understand how this works: char_per_block is 0 for
> almost all formats and this doesn't take in account the cpp. Am I
> missing something here?

I guess it doesn't. That's the DRM function here, without any
modification, but from a quick look at the current users of that
function, there's nobody that uses the value directly.

So you might be right there.

> Also, this might be a good occasion to discuss what meaning we want to
> give "stride" and "pitch": should one be in bytes and the other in bit,
> etc? I keep forgetting what each API expects.

pitch is documented as bytes, and the function computing the stride I
added did too. I'll remove the stride one.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (2.03 kB)
signature.asc (235.00 B)
Download all attachments

2019-04-17 13:14:22

On Thu, Apr 18, 2019 at 09:52:10AM +0200, Daniel Vetter wrote:
> On Thu, Apr 18, 2019 at 8:22 AM Maxime Ripard <[email protected]> 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.
> > >
> > > 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.
> >
> > 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.

> 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.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (10.40 kB)
signature.asc (235.00 B)
Download all attachments

2019-04-18 10:12:35

by Daniel Vetter

[permalink] [raw]

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

On Thu, Apr 18, 2019 at 11:02 AM Maxime Ripard
<[email protected]> 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 <[email protected]> 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.
> > > >
> > > > 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.
> > >
> > > 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.

> > - 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.

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.

> 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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2019-04-18 11:51:51

by Paul Kocialkowski

[permalink] [raw]

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

Hi,

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.

It looks like so far, V4L2 pixel formats describe a DRM pixel format +
modifier. 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?

Cheers,

Paul

> > 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.
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

2019-04-18 12:03:23

Hi

Am 17.04.19 um 09:54 schrieb Maxime Ripard:
> Move the DRM formats API to turn this into a more generic image formats API
> to be able to leverage it into some other places of the kernel, such as
> v4l2 drivers.

Here's some nitpicking: 'image_format_*' appears slightly vague; it
could refer to a disk or ROM image as well. Something like
'pixel_format_*', 'pixbuf_format_*', etc. would clearly indicate graphics.

Best regards
Thomas

>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> include/linux/image-formats.h | 387 +++++++++++++++++++++-
> lib/Kconfig | 7 +-
> lib/Makefile | 3 +-
> lib/image-formats-selftests.c | 325 +++++++++++++++++-
> lib/image-formats.c | 655 +++++++++++++++++++++++++++++++++++-
> 5 files changed, 1377 insertions(+)
> create mode 100644 include/linux/image-formats.h
> create mode 100644 lib/image-formats-selftests.c
> create mode 100644 lib/image-formats.c
>
> diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> new file mode 100644
> index 000000000000..ec43d9f9a527
> --- /dev/null
> +++ b/include/linux/image-formats.h
> @@ -0,0 +1,387 @@
> +/*
> + * Copyright (c) 2016 Laurent Pinchart <[email protected]>
> + * Copyright (c) 2019 Maxime Ripard <[email protected]>
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission. The copyright holders make no representations
> + * about the suitability of this software for any purpose. It is provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#ifndef _IMAGE_FORMATS_H_
> +#define _IMAGE_FORMATS_H_
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct image_format_info - information about a image format
> + */
> +struct image_format_info {
> + union {
> + /**
> + * @drm_fmt:
> + *
> + * DRM 4CC format identifier (DRM_FORMAT_*)
> + */
> + u32 drm_fmt;
> +
> + /**
> + * @format:
> + *
> + * DRM 4CC format identifier (DRM_FORMAT_*). Kept
> + * around for compatibility reasons with the current
> + * DRM drivers.
> + */
> + u32 format;
> + };
> +
> + /**
> + * @depth:
> + *
> + * Color depth (number of bits per pixel excluding padding bits),
> + * valid for a subset of RGB formats only. This is a legacy field, do
> + * not use in new code and set to 0 for new formats.
> + */
> + u8 depth;
> +
> + /** @num_planes: Number of color planes (1 to 3) */
> + u8 num_planes;
> +
> + union {
> + /**
> + * @cpp:
> + *
> + * Number of bytes per pixel (per plane), this is aliased with
> + * @char_per_block. It is deprecated in favour of using the
> + * triplet @char_per_block, @block_w, @block_h for better
> + * describing the pixel format.
> + */
> + u8 cpp[3];
> +
> + /**
> + * @char_per_block:
> + *
> + * Number of bytes per block (per plane), where blocks are
> + * defined as a rectangle of pixels which are stored next to
> + * each other in a byte aligned memory region. Together with
> + * @block_w and @block_h this is used to properly describe tiles
> + * in tiled formats or to describe groups of pixels in packed
> + * formats for which the memory needed for a single pixel is not
> + * byte aligned.
> + *
> + * @cpp has been kept for historical reasons because there are
> + * a lot of places in drivers where it's used. In drm core for
> + * generic code paths the preferred way is to use
> + * @char_per_block, image_format_info_block_width() and
> + * image_format_info_block_height() which allows handling both
> + * block and non-block formats in the same way.
> + *
> + * For formats that are intended to be used only with non-linear
> + * modifiers both @cpp and @char_per_block must be 0 in the
> + * generic format table. Drivers could supply accurate
> + * information from their drm_mode_config.get_format_info hook
> + * if they want the core to be validating the pitch.
> + */
> + u8 char_per_block[3];
> + };
> +
> + /**
> + * @block_w:
> + *
> + * Block width in pixels, this is intended to be accessed through
> + * image_format_info_block_width()
> + */
> + u8 block_w[3];
> +
> + /**
> + * @block_h:
> + *
> + * Block height in pixels, this is intended to be accessed through
> + * image_format_info_block_height()
> + */
> + u8 block_h[3];
> +
> + /** @hsub: Horizontal chroma subsampling factor */
> + u8 hsub;
> + /** @vsub: Vertical chroma subsampling factor */
> + u8 vsub;
> +
> + /** @has_alpha: Does the format embeds an alpha component? */
> + bool has_alpha;
> +
> + /** @is_yuv: Is it a YUV format? */
> + bool is_yuv;
> +};
> +
> +/**
> + * image_format_info_is_yuv_packed - check that the format info matches a YUV
> + * format with data laid in a single plane
> + * @info: format info
> + *
> + * Returns:
> + * A boolean indicating whether the format info matches a packed YUV format.
> + */
> +static inline bool
> +image_format_info_is_yuv_packed(const struct image_format_info *info)
> +{
> + return info->is_yuv && info->num_planes == 1;
> +}
> +
> +/**
> + * image_format_info_is_yuv_semiplanar - check that the format info matches a YUV
> + * format with data laid in two planes (luminance and chrominance)
> + * @info: format info
> + *
> + * Returns:
> + * A boolean indicating whether the format info matches a semiplanar YUV format.
> + */
> +static inline bool
> +image_format_info_is_yuv_semiplanar(const struct image_format_info *info)
> +{
> + return info->is_yuv && info->num_planes == 2;
> +}
> +
> +/**
> + * image_format_info_is_yuv_planar - check that the format info matches a YUV
> + * format with data laid in three planes (one for each YUV component)
> + * @info: format info
> + *
> + * Returns:
> + * A boolean indicating whether the format info matches a planar YUV format.
> + */
> +static inline bool
> +image_format_info_is_yuv_planar(const struct image_format_info *info)
> +{
> + return info->is_yuv && info->num_planes == 3;
> +}
> +
> +/**
> + * image_format_info_is_yuv_sampling_410 - check that the format info matches a
> + * YUV format with 4:1:0 sub-sampling
> + * @info: format info
> + *
> + * Returns:
> + * A boolean indicating whether the format info matches a YUV format with 4:1:0
> + * sub-sampling.
> + */
> +static inline bool
> +image_format_info_is_yuv_sampling_410(const struct image_format_info *info)
> +{
> + return info->is_yuv && info->hsub == 4 && info->vsub == 4;
> +}
> +
> +/**
> + * image_format_info_is_yuv_sampling_411 - check that the format info matches a
> + * YUV format with 4:1:1 sub-sampling
> + * @info: format info
> + *
> + * Returns:
> + * A boolean indicating whether the format info matches a YUV format with 4:1:1
> + * sub-sampling.
> + */
> +static inline bool
> +image_format_info_is_yuv_sampling_411(const struct image_format_info *info)
> +{
> + return info->is_yuv && info->hsub == 4 && info->vsub == 1;
> +}
> +
> +/**
> + * image_format_info_is_yuv_sampling_420 - check that the format info matches a
> + * YUV format with 4:2:0 sub-sampling
> + * @info: format info
> + *
> + * Returns:
> + * A boolean indicating whether the format info matches a YUV format with 4:2:0
> + * sub-sampling.
> + */
> +static inline bool
> +image_format_info_is_yuv_sampling_420(const struct image_format_info *info)
> +{
> + return info->is_yuv && info->hsub == 2 && info->vsub == 2;
> +}
> +
> +/**
> + * image_format_info_is_yuv_sampling_422 - check that the format info matches a
> + * YUV format with 4:2:2 sub-sampling
> + * @info: format info
> + *
> + * Returns:
> + * A boolean indicating whether the format info matches a YUV format with 4:2:2
> + * sub-sampling.
> + */
> +static inline bool
> +image_format_info_is_yuv_sampling_422(const struct image_format_info *info)
> +{
> + return info->is_yuv && info->hsub == 2 && info->vsub == 1;
> +}
> +
> +/**
> + * image_format_info_is_yuv_sampling_444 - check that the format info matches a
> + * YUV format with 4:4:4 sub-sampling
> + * @info: format info
> + *
> + * Returns:
> + * A boolean indicating whether the format info matches a YUV format with 4:4:4
> + * sub-sampling.
> + */
> +static inline bool
> +image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
> +{
> + return info->is_yuv && info->hsub == 1 && info->vsub == 1;
> +}
> +
> +/**
> + * image_format_info_plane_cpp - determine the bytes per pixel value
> + * @format: pixel format info
> + * @plane: plane index
> + *
> + * Returns:
> + * The bytes per pixel value for the specified plane.
> + */
> +static inline
> +int image_format_info_plane_cpp(const struct image_format_info *info, int plane)
> +{
> + if (!info || plane >= info->num_planes)
> + return 0;
> +
> + return info->cpp[plane];
> +}
> +
> +/**
> + * image_format_info_plane_width - width of the plane given the first plane
> + * @format: pixel format info
> + * @width: width of the first plane
> + * @plane: plane index
> + *
> + * Returns:
> + * The width of @plane, given that the width of the first plane is @width.
> + */
> +static inline
> +int image_format_info_plane_width(const struct image_format_info *info, int width,
> + int plane)
> +{
> + if (!info || plane >= info->num_planes)
> + return 0;
> +
> + if (plane == 0)
> + return width;
> +
> + return width / info->hsub;
> +}
> +
> +/**
> + * image_format_info_plane_height - height of the plane given the first plane
> + * @format: pixel format info
> + * @height: height of the first plane
> + * @plane: plane index
> + *
> + * Returns:
> + * The height of @plane, given that the height of the first plane is @height.
> + */
> +static inline
> +int image_format_info_plane_height(const struct image_format_info *info, int height,
> + int plane)
> +{
> + if (!info || plane >= info->num_planes)
> + return 0;
> +
> + if (plane == 0)
> + return height;
> +
> + return height / info->vsub;
> +}
> +
> +/**
> + * image_format_info_block_width - width in pixels of block.
> + * @format: pointer to the image_format_info
> + * @plane: plane index
> + *
> + * Returns:
> + * The width in pixels of a block, depending on the plane index.
> + */
> +static inline
> +unsigned int image_format_info_block_width(const struct image_format_info *format,
> + int plane)
> +{
> + if (!format)
> + return 0;
> +
> + if (plane < 0 || plane >= ARRAY_SIZE(format->block_w))
> + return 0;
> +
> + if (plane >= format->num_planes)
> + return 0;
> +
> + if (!format->block_w[plane])
> + return 1;
> +
> + return format->block_w[plane];
> +}
> +
> +/**
> + * image_format_info_block_height - height in pixels of a block
> + * @info: pointer to the image_format_info
> + * @plane: plane index
> + *
> + * Returns:
> + * The height in pixels of a block, depending on the plane index.
> + */
> +static inline
> +unsigned int image_format_info_block_height(const struct image_format_info *format,
> + int plane)
> +{
> + if (!format)
> + return 0;
> +
> + if (plane < 0 || plane >= ARRAY_SIZE(format->block_w))
> + return 0;
> +
> + if (plane >= format->num_planes)
> + return 0;
> +
> + if (!format->block_h[plane])
> + return 1;
> +
> + return format->block_h[plane];
> +}
> +
> +/**
> + * image_format_info_min_pitch - computes the minimum required pitch in bytes
> + * @info: pixel format info
> + * @plane: plane index
> + * @buffer_width: buffer width in pixels
> + *
> + * Returns:
> + * The minimum required pitch in bytes for a buffer by taking into consideration
> + * the pixel format information and the buffer width.
> + */
> +static inline
> +uint64_t image_format_info_min_pitch(const struct image_format_info *info,
> + int plane, unsigned int buffer_width)
> +{
> + if (!info || plane < 0 || plane >= info->num_planes)
> + return 0;
> +
> + return DIV_ROUND_UP_ULL((u64)buffer_width * info->char_per_block[plane],
> + image_format_info_block_width(info, plane) *
> + image_format_info_block_height(info, plane));
> +}
> +
> +const struct image_format_info *__image_format_drm_lookup(u32 drm);
> +const struct image_format_info *image_format_drm_lookup(u32 drm);
> +
> +#endif /* _IMAGE_FORMATS_H_ */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index fb453afff32e..9a0160d3123b 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -625,3 +625,10 @@ config GENERIC_LIB_UCMPDI2
>
> config OBJAGG
> tristate "objagg" if COMPILE_TEST
> +
> +config IMAGE_FORMATS
> + bool
> +
> +config IMAGE_FORMATS_SELFTESTS
> + tristate "Test image format functions"
> + depends on IMAGE_FORMATS
> diff --git a/lib/Makefile b/lib/Makefile
> index 6996d2b9f401..203336b91248 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -280,3 +280,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
> obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
> obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
> obj-$(CONFIG_OBJAGG) += objagg.o
> +
> +obj-$(CONFIG_IMAGE_FORMATS) += image-formats.o
> +obj-$(CONFIG_IMAGE_FORMATS_SELFTESTS) += image-formats-selftests.o
> diff --git a/lib/image-formats-selftests.c b/lib/image-formats-selftests.c
> new file mode 100644
> index 000000000000..d0f0011b535e
> --- /dev/null
> +++ b/lib/image-formats-selftests.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test cases for the image_format functions
> + */
> +
> +#define pr_fmt(fmt) "image_format: " fmt
> +
> +#include <linux/errno.h>
> +#include <linux/image-formats.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include <drm/drm_fourcc.h>
> +
> +#define FAIL(test, msg, ...) \
> + do { \
> + if (test) { \
> + pr_err("%s/%u: " msg, __FUNCTION__, __LINE__, ##__VA_ARGS__); \
> + return -EINVAL; \
> + } \
> + } while (0)
> +
> +#define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")
> +
> +static int test_image_format_info_block_width(void)
> +{
> + const struct image_format_info *info = NULL;
> +
> + /* Test invalid arguments */
> + FAIL_ON(image_format_info_block_width(info, 0) != 0);
> + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> + FAIL_ON(image_format_info_block_width(info, 1) != 0);
> +
> + /* Test 1 plane format */
> + info = image_format_drm_lookup(DRM_FORMAT_XRGB4444);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_block_width(info, 0) != 1);
> + FAIL_ON(image_format_info_block_width(info, 1) != 0);
> + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> +
> + /* Test 2 planes format */
> + info = image_format_drm_lookup(DRM_FORMAT_NV12);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_block_width(info, 0) != 1);
> + FAIL_ON(image_format_info_block_width(info, 1) != 1);
> + FAIL_ON(image_format_info_block_width(info, 2) != 0);
> + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> +
> + /* Test 3 planes format */
> + info = image_format_drm_lookup(DRM_FORMAT_YUV422);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_block_width(info, 0) != 1);
> + FAIL_ON(image_format_info_block_width(info, 1) != 1);
> + FAIL_ON(image_format_info_block_width(info, 2) != 1);
> + FAIL_ON(image_format_info_block_width(info, 3) != 0);
> + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> +
> + /* Test a tiled format */
> + info = image_format_drm_lookup(DRM_FORMAT_X0L0);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_block_width(info, 0) != 2);
> + FAIL_ON(image_format_info_block_width(info, 1) != 0);
> + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> +
> + return 0;
> +}
> +
> +static int test_image_format_info_block_height(void)
> +{
> + const struct image_format_info *info = NULL;
> +
> + /* Test invalid arguments */
> + FAIL_ON(image_format_info_block_height(info, 0) != 0);
> + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> + FAIL_ON(image_format_info_block_height(info, 1) != 0);
> +
> + /* Test 1 plane format */
> + info = image_format_drm_lookup(DRM_FORMAT_XRGB4444);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_block_height(info, 0) != 1);
> + FAIL_ON(image_format_info_block_height(info, 1) != 0);
> + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> +
> + /* Test 2 planes format */
> + info = image_format_drm_lookup(DRM_FORMAT_NV12);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_block_height(info, 0) != 1);
> + FAIL_ON(image_format_info_block_height(info, 1) != 1);
> + FAIL_ON(image_format_info_block_height(info, 2) != 0);
> + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> +
> + /* Test 3 planes format */
> + info = image_format_drm_lookup(DRM_FORMAT_YUV422);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_block_height(info, 0) != 1);
> + FAIL_ON(image_format_info_block_height(info, 1) != 1);
> + FAIL_ON(image_format_info_block_height(info, 2) != 1);
> + FAIL_ON(image_format_info_block_height(info, 3) != 0);
> + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> +
> + /* Test a tiled format */
> + info = image_format_drm_lookup(DRM_FORMAT_X0L0);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_block_height(info, 0) != 2);
> + FAIL_ON(image_format_info_block_height(info, 1) != 0);
> + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> +
> + return 0;
> +}
> +
> +static int test_image_format_info_min_pitch(void)
> +{
> + const struct image_format_info *info = NULL;
> +
> + /* Test invalid arguments */
> + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> +
> + /* Test 1 plane 8 bits per pixel format */
> + info = image_format_drm_lookup(DRM_FORMAT_RGB332);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> +
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 1);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 2);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 640);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 1024);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 1920);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 4096);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 671);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> + (uint64_t)UINT_MAX);
> + FAIL_ON(image_format_info_min_pitch(info, 0, (UINT_MAX - 1)) !=
> + (uint64_t)(UINT_MAX - 1));
> +
> + /* Test 1 plane 16 bits per pixel format */
> + info = image_format_drm_lookup(DRM_FORMAT_XRGB4444);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> +
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 2);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 4);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 1280);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 2048);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 3840);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 8192);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 1342);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> + (uint64_t)UINT_MAX * 2);
> + FAIL_ON(image_format_info_min_pitch(info, 0, (UINT_MAX - 1)) !=
> + (uint64_t)(UINT_MAX - 1) * 2);
> +
> + /* Test 1 plane 24 bits per pixel format */
> + info = image_format_drm_lookup(DRM_FORMAT_RGB888);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> +
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 3);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 6);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 1920);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 3072);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 5760);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 12288);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 2013);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> + (uint64_t)UINT_MAX * 3);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX - 1) !=
> + (uint64_t)(UINT_MAX - 1) * 3);
> +
> + /* Test 1 plane 32 bits per pixel format */
> + info = image_format_drm_lookup(DRM_FORMAT_ABGR8888);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> +
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 4);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 8);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 2560);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 4096);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 7680);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 16384);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 2684);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> + (uint64_t)UINT_MAX * 4);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX - 1) !=
> + (uint64_t)(UINT_MAX - 1) * 4);
> +
> + /* Test 2 planes format */
> + info = image_format_drm_lookup(DRM_FORMAT_NV12);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 2, 0) != 0);
> +
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 1);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 1) != 2);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 2);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 1) != 2);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 640);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 320) != 640);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 1024);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 512) != 1024);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 1920);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 960) != 1920);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 4096);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 2048) != 4096);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 671);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 336) != 672);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> + (uint64_t)UINT_MAX);
> + FAIL_ON(image_format_info_min_pitch(info, 1, UINT_MAX / 2 + 1) !=
> + (uint64_t)UINT_MAX + 1);
> + FAIL_ON(image_format_info_min_pitch(info, 0, (UINT_MAX - 1)) !=
> + (uint64_t)(UINT_MAX - 1));
> + FAIL_ON(image_format_info_min_pitch(info, 1, (UINT_MAX - 1) / 2) !=
> + (uint64_t)(UINT_MAX - 1));
> +
> + /* Test 3 planes 8 bits per pixel format */
> + info = image_format_drm_lookup(DRM_FORMAT_YUV422);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 2, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 3, 0) != 0);
> +
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 1);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 1) != 1);
> + FAIL_ON(image_format_info_min_pitch(info, 2, 1) != 1);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 2);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 2) != 2);
> + FAIL_ON(image_format_info_min_pitch(info, 2, 2) != 2);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 640);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 320) != 320);
> + FAIL_ON(image_format_info_min_pitch(info, 2, 320) != 320);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 1024);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 512) != 512);
> + FAIL_ON(image_format_info_min_pitch(info, 2, 512) != 512);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 1920);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 960) != 960);
> + FAIL_ON(image_format_info_min_pitch(info, 2, 960) != 960);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 4096);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 2048) != 2048);
> + FAIL_ON(image_format_info_min_pitch(info, 2, 2048) != 2048);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 671);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 336) != 336);
> + FAIL_ON(image_format_info_min_pitch(info, 2, 336) != 336);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> + (uint64_t)UINT_MAX);
> + FAIL_ON(image_format_info_min_pitch(info, 1, UINT_MAX / 2 + 1) !=
> + (uint64_t)UINT_MAX / 2 + 1);
> + FAIL_ON(image_format_info_min_pitch(info, 2, UINT_MAX / 2 + 1) !=
> + (uint64_t)UINT_MAX / 2 + 1);
> + FAIL_ON(image_format_info_min_pitch(info, 0, (UINT_MAX - 1) / 2) !=
> + (uint64_t)(UINT_MAX - 1) / 2);
> + FAIL_ON(image_format_info_min_pitch(info, 1, (UINT_MAX - 1) / 2) !=
> + (uint64_t)(UINT_MAX - 1) / 2);
> + FAIL_ON(image_format_info_min_pitch(info, 2, (UINT_MAX - 1) / 2) !=
> + (uint64_t)(UINT_MAX - 1) / 2);
> +
> + /* Test tiled format */
> + info = image_format_drm_lookup(DRM_FORMAT_X0L2);
> + FAIL_ON(!info);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> +
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 2);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 4);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 1280);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 2048);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 3840);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 8192);
> + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 1342);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> + (uint64_t)UINT_MAX * 2);
> + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX - 1) !=
> + (uint64_t)(UINT_MAX - 1) * 2);
> +
> + return 0;
> +}
> +
> +#define selftest(test) { .name = #test, .func = test, }
> +
> +static struct image_format_test {
> + char *name;
> + int (*func)(void);
> +} tests[] = {
> + selftest(test_image_format_info_block_height),
> + selftest(test_image_format_info_block_width),
> + selftest(test_image_format_info_min_pitch),
> +};
> +
> +static int __init image_format_test_init(void)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> + struct image_format_test *test = &tests[i];
> + int ret;
> +
> + ret = test->func();
> + if (ret) {
> + pr_err("Failed test %s\n", test->name);
> + return ret;
> + }
> + }
> +
> + pr_info("All tests executed properly.\n");
> + return 0;
> +}
> +module_init(image_format_test_init);
> +
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_AUTHOR("Maxime Ripard <[email protected]>");
> +MODULE_LICENSE("GPL");
> diff --git a/lib/image-formats.c b/lib/image-formats.c
> new file mode 100644
> index 000000000000..1e52a7410222
> --- /dev/null
> +++ b/lib/image-formats.c
> @@ -0,0 +1,655 @@
> +/*
> + * Copyright (c) 2016 Laurent Pinchart <[email protected]>
> + * Copyright (c) 2019 Maxime Ripard <[email protected]>
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission. The copyright holders make no representations
> + * about the suitability of this software for any purpose. It is provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#include <linux/bug.h>
> +#include <linux/image-formats.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +
> +#include <uapi/drm/drm_fourcc.h>
> +
> +static const struct image_format_info formats[] = {
> + {
> + .drm_fmt = DRM_FORMAT_C8,
> + .depth = 8,
> + .num_planes = 1,
> + .cpp = { 1, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGB332,
> + .depth = 8,
> + .num_planes = 1,
> + .cpp = { 1, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGR233,
> + .depth = 8,
> + .num_planes = 1,
> + .cpp = { 1, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_XRGB4444,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_XBGR4444,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGBX4444,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGRX4444,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_ARGB4444,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_ABGR4444,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGBA4444,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGRA4444,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_XRGB1555,
> + .depth = 15,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_XBGR1555,
> + .depth = 15,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGBX5551,
> + .depth = 15,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGRX5551,
> + .depth = 15,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_ARGB1555,
> + .depth = 15,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_ABGR1555,
> + .depth = 15,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGBA5551,
> + .depth = 15,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGRA5551,
> + .depth = 15,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGB565,
> + .depth = 16,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGR565,
> + .depth = 16,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGB888,
> + .depth = 24,
> + .num_planes = 1,
> + .cpp = { 3, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGR888,
> + .depth = 24,
> + .num_planes = 1,
> + .cpp = { 3, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_XRGB8888,
> + .depth = 24,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_XBGR8888,
> + .depth = 24,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGBX8888,
> + .depth = 24,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGRX8888,
> + .depth = 24,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGB565_A8,
> + .depth = 24,
> + .num_planes = 2,
> + .cpp = { 2, 1, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGR565_A8,
> + .depth = 24,
> + .num_planes = 2,
> + .cpp = { 2, 1, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_XRGB2101010,
> + .depth = 30,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_XBGR2101010,
> + .depth = 30,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGBX1010102,
> + .depth = 30,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGRX1010102,
> + .depth = 30,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + }, {
> + .drm_fmt = DRM_FORMAT_ARGB2101010,
> + .depth = 30,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_ABGR2101010,
> + .depth = 30,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGBA1010102,
> + .depth = 30,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGRA1010102,
> + .depth = 30,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_ARGB8888,
> + .depth = 32,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_ABGR8888,
> + .depth = 32,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGBA8888,
> + .depth = 32,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGRA8888,
> + .depth = 32,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGB888_A8,
> + .depth = 32,
> + .num_planes = 2,
> + .cpp = { 3, 1, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGR888_A8,
> + .depth = 32,
> + .num_planes = 2,
> + .cpp = { 3, 1, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_XRGB8888_A8,
> + .depth = 32,
> + .num_planes = 2,
> + .cpp = { 4, 1, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_XBGR8888_A8,
> + .depth = 32,
> + .num_planes = 2,
> + .cpp = { 4, 1, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_RGBX8888_A8,
> + .depth = 32,
> + .num_planes = 2,
> + .cpp = { 4, 1, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_BGRX8888_A8,
> + .depth = 32,
> + .num_planes = 2,
> + .cpp = { 4, 1, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YUV410,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 4,
> + .vsub = 4,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YVU410,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 4,
> + .vsub = 4,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YUV411,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 4,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YVU411,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 4,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YUV420,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 2,
> + .vsub = 2,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YVU420,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 2,
> + .vsub = 2,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YUV422,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 2,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YVU422,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 2,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YUV444,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 1,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YVU444,
> + .depth = 0,
> + .num_planes = 3,
> + .cpp = { 1, 1, 1 },
> + .hsub = 1,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_NV12,
> + .depth = 0,
> + .num_planes = 2,
> + .cpp = { 1, 2, 0 },
> + .hsub = 2,
> + .vsub = 2,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_NV21,
> + .depth = 0,
> + .num_planes = 2,
> + .cpp = { 1, 2, 0 },
> + .hsub = 2,
> + .vsub = 2,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_NV16,
> + .depth = 0,
> + .num_planes = 2,
> + .cpp = { 1, 2, 0 },
> + .hsub = 2,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_NV61,
> + .depth = 0,
> + .num_planes = 2,
> + .cpp = { 1, 2, 0 },
> + .hsub = 2,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_NV24,
> + .depth = 0,
> + .num_planes = 2,
> + .cpp = { 1, 2, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_NV42,
> + .depth = 0,
> + .num_planes = 2,
> + .cpp = { 1, 2, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YUYV,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 2,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_YVYU,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 2,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_UYVY,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 2,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_VYUY,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 2, 0, 0 },
> + .hsub = 2,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_XYUV8888,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_AYUV,
> + .depth = 0,
> + .num_planes = 1,
> + .cpp = { 4, 0, 0 },
> + .hsub = 1,
> + .vsub = 1,
> + .has_alpha = true,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_Y0L0,
> + .depth = 0,
> + .num_planes = 1,
> + .char_per_block = { 8, 0, 0 },
> + .block_w = { 2, 0, 0 },
> + .block_h = { 2, 0, 0 },
> + .hsub = 2,
> + .vsub = 2,
> + .has_alpha = true,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_X0L0,
> + .depth = 0,
> + .num_planes = 1,
> + .char_per_block = { 8, 0, 0 },
> + .block_w = { 2, 0, 0 },
> + .block_h = { 2, 0, 0 },
> + .hsub = 2,
> + .vsub = 2,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_Y0L2,
> + .depth = 0,
> + .num_planes = 1,
> + .char_per_block = { 8, 0, 0 },
> + .block_w = { 2, 0, 0 },
> + .block_h = { 2, 0, 0 },
> + .hsub = 2,
> + .vsub = 2,
> + .has_alpha = true,
> + .is_yuv = true,
> + }, {
> + .drm_fmt = DRM_FORMAT_X0L2,
> + .depth = 0,
> + .num_planes = 1,
> + .char_per_block = { 8, 0, 0 },
> + .block_w = { 2, 0, 0 },
> + .block_h = { 2, 0, 0 },
> + .hsub = 2,
> + .vsub = 2,
> + .is_yuv = true,
> + },
> +};
> +
> +#define __image_format_lookup(_field, _fmt) \
> + ({ \
> + const struct image_format_info *format = NULL; \
> + unsigned i; \
> + \
> + for (i = 0; i < ARRAY_SIZE(formats); i++) \
> + if (formats[i]._field == _fmt) \
> + format = &formats[i]; \
> + \
> + format; \
> + })
> +
> +/**
> + * __image_format_drm_lookup - query information for a given format
> + * @drm: DRM fourcc pixel format (DRM_FORMAT_*)
> + *
> + * The caller should only pass a supported pixel format to this function.
> + *
> + * Returns:
> + * The instance of struct image_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct image_format_info *__image_format_drm_lookup(u32 drm)
> +{
> + return __image_format_lookup(drm_fmt, drm);
> +}
> +EXPORT_SYMBOL(__image_format_drm_lookup);
> +
> +/**
> + * image_format_drm_lookup - query information for a given format
> + * @drm: DRM fourcc pixel format (DRM_FORMAT_*)
> + *
> + * The caller should only pass a supported pixel format to this function.
> + * Unsupported pixel formats will generate a warning in the kernel log.
> + *
> + * Returns:
> + * The instance of struct image_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct image_format_info *image_format_drm_lookup(u32 drm)
> +{
> + const struct image_format_info *format;
> +
> + format = __image_format_drm_lookup(drm);
> +
> + WARN_ON(!format);
> + return format;
> +}
> +EXPORT_SYMBOL(image_format_drm_lookup);
>

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Attachments:

signature.asc (499.00 B)
OpenPGP digital signature

2019-04-23 12:36:03

by Paul Kocialkowski

[permalink] [raw]

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

Hi,

On Tue, 2019-04-23 at 09:30 +0200, Daniel Vetter wrote:
> 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.

The one I had in mind is V4L2_PIX_FMT_SUNXI_TILED_NV12 which translates
to DRM_FORMAT_NV12 + DRM_FORMAT_MOD_ALLWINNER_TILED. Seems to be a
pretty similar case to the Mediatek one indeed.

In our cause, that's because the video decoding engine produces its
destination buffers in a specific tiled format, that the display engine
can take in directly.

Cheers,

Paul

> -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
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

2019-04-23 14:29:52

Hi,

Le mardi 23 avril 2019 à 13:22 +0200, Thomas Zimmermann a écrit :
> Hi
>
> Am 17.04.19 um 09:54 schrieb Maxime Ripard:
> > Move the DRM formats API to turn this into a more generic image formats API
> > to be able to leverage it into some other places of the kernel, such as
> > v4l2 drivers.
>
> Here's some nitpicking: 'image_format_*' appears slightly vague; it
> could refer to a disk or ROM image as well. Something like
> 'pixel_format_*', 'pixbuf_format_*', etc. would clearly indicate graphics.

Yeah I'm with you on this one. It's bike-shedding, but I think it's
fine as long as more fundamental topics are moving as well.

Cheers,

Paul

> Best regards
> Thomas
>
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > include/linux/image-formats.h | 387 +++++++++++++++++++++-
> > lib/Kconfig | 7 +-
> > lib/Makefile | 3 +-
> > lib/image-formats-selftests.c | 325 +++++++++++++++++-
> > lib/image-formats.c | 655 +++++++++++++++++++++++++++++++++++-
> > 5 files changed, 1377 insertions(+)
> > create mode 100644 include/linux/image-formats.h
> > create mode 100644 lib/image-formats-selftests.c
> > create mode 100644 lib/image-formats.c
> >
> > diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> > new file mode 100644
> > index 000000000000..ec43d9f9a527
> > --- /dev/null
> > +++ b/include/linux/image-formats.h
> > @@ -0,0 +1,387 @@
> > +/*
> > + * Copyright (c) 2016 Laurent Pinchart <[email protected]>
> > + * Copyright (c) 2019 Maxime Ripard <[email protected]>
> > + *
> > + * Permission to use, copy, modify, distribute, and sell this software and its
> > + * documentation for any purpose is hereby granted without fee, provided that
> > + * the above copyright notice appear in all copies and that both that copyright
> > + * notice and this permission notice appear in supporting documentation, and
> > + * that the name of the copyright holders not be used in advertising or
> > + * publicity pertaining to distribution of the software without specific,
> > + * written prior permission. The copyright holders make no representations
> > + * about the suitability of this software for any purpose. It is provided "as
> > + * is" without express or implied warranty.
> > + *
> > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > + * OF THIS SOFTWARE.
> > + */
> > +
> > +#ifndef _IMAGE_FORMATS_H_
> > +#define _IMAGE_FORMATS_H_
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/types.h>
> > +
> > +/**
> > + * struct image_format_info - information about a image format
> > + */
> > +struct image_format_info {
> > + union {
> > + /**
> > + * @drm_fmt:
> > + *
> > + * DRM 4CC format identifier (DRM_FORMAT_*)
> > + */
> > + u32 drm_fmt;
> > +
> > + /**
> > + * @format:
> > + *
> > + * DRM 4CC format identifier (DRM_FORMAT_*). Kept
> > + * around for compatibility reasons with the current
> > + * DRM drivers.
> > + */
> > + u32 format;
> > + };
> > +
> > + /**
> > + * @depth:
> > + *
> > + * Color depth (number of bits per pixel excluding padding bits),
> > + * valid for a subset of RGB formats only. This is a legacy field, do
> > + * not use in new code and set to 0 for new formats.
> > + */
> > + u8 depth;
> > +
> > + /** @num_planes: Number of color planes (1 to 3) */
> > + u8 num_planes;
> > +
> > + union {
> > + /**
> > + * @cpp:
> > + *
> > + * Number of bytes per pixel (per plane), this is aliased with
> > + * @char_per_block. It is deprecated in favour of using the
> > + * triplet @char_per_block, @block_w, @block_h for better
> > + * describing the pixel format.
> > + */
> > + u8 cpp[3];
> > +
> > + /**
> > + * @char_per_block:
> > + *
> > + * Number of bytes per block (per plane), where blocks are
> > + * defined as a rectangle of pixels which are stored next to
> > + * each other in a byte aligned memory region. Together with
> > + * @block_w and @block_h this is used to properly describe tiles
> > + * in tiled formats or to describe groups of pixels in packed
> > + * formats for which the memory needed for a single pixel is not
> > + * byte aligned.
> > + *
> > + * @cpp has been kept for historical reasons because there are
> > + * a lot of places in drivers where it's used. In drm core for
> > + * generic code paths the preferred way is to use
> > + * @char_per_block, image_format_info_block_width() and
> > + * image_format_info_block_height() which allows handling both
> > + * block and non-block formats in the same way.
> > + *
> > + * For formats that are intended to be used only with non-linear
> > + * modifiers both @cpp and @char_per_block must be 0 in the
> > + * generic format table. Drivers could supply accurate
> > + * information from their drm_mode_config.get_format_info hook
> > + * if they want the core to be validating the pitch.
> > + */
> > + u8 char_per_block[3];
> > + };
> > +
> > + /**
> > + * @block_w:
> > + *
> > + * Block width in pixels, this is intended to be accessed through
> > + * image_format_info_block_width()
> > + */
> > + u8 block_w[3];
> > +
> > + /**
> > + * @block_h:
> > + *
> > + * Block height in pixels, this is intended to be accessed through
> > + * image_format_info_block_height()
> > + */
> > + u8 block_h[3];
> > +
> > + /** @hsub: Horizontal chroma subsampling factor */
> > + u8 hsub;
> > + /** @vsub: Vertical chroma subsampling factor */
> > + u8 vsub;
> > +
> > + /** @has_alpha: Does the format embeds an alpha component? */
> > + bool has_alpha;
> > +
> > + /** @is_yuv: Is it a YUV format? */
> > + bool is_yuv;
> > +};
> > +
> > +/**
> > + * image_format_info_is_yuv_packed - check that the format info matches a YUV
> > + * format with data laid in a single plane
> > + * @info: format info
> > + *
> > + * Returns:
> > + * A boolean indicating whether the format info matches a packed YUV format.
> > + */
> > +static inline bool
> > +image_format_info_is_yuv_packed(const struct image_format_info *info)
> > +{
> > + return info->is_yuv && info->num_planes == 1;
> > +}
> > +
> > +/**
> > + * image_format_info_is_yuv_semiplanar - check that the format info matches a YUV
> > + * format with data laid in two planes (luminance and chrominance)
> > + * @info: format info
> > + *
> > + * Returns:
> > + * A boolean indicating whether the format info matches a semiplanar YUV format.
> > + */
> > +static inline bool
> > +image_format_info_is_yuv_semiplanar(const struct image_format_info *info)
> > +{
> > + return info->is_yuv && info->num_planes == 2;
> > +}
> > +
> > +/**
> > + * image_format_info_is_yuv_planar - check that the format info matches a YUV
> > + * format with data laid in three planes (one for each YUV component)
> > + * @info: format info
> > + *
> > + * Returns:
> > + * A boolean indicating whether the format info matches a planar YUV format.
> > + */
> > +static inline bool
> > +image_format_info_is_yuv_planar(const struct image_format_info *info)
> > +{
> > + return info->is_yuv && info->num_planes == 3;
> > +}
> > +
> > +/**
> > + * image_format_info_is_yuv_sampling_410 - check that the format info matches a
> > + * YUV format with 4:1:0 sub-sampling
> > + * @info: format info
> > + *
> > + * Returns:
> > + * A boolean indicating whether the format info matches a YUV format with 4:1:0
> > + * sub-sampling.
> > + */
> > +static inline bool
> > +image_format_info_is_yuv_sampling_410(const struct image_format_info *info)
> > +{
> > + return info->is_yuv && info->hsub == 4 && info->vsub == 4;
> > +}
> > +
> > +/**
> > + * image_format_info_is_yuv_sampling_411 - check that the format info matches a
> > + * YUV format with 4:1:1 sub-sampling
> > + * @info: format info
> > + *
> > + * Returns:
> > + * A boolean indicating whether the format info matches a YUV format with 4:1:1
> > + * sub-sampling.
> > + */
> > +static inline bool
> > +image_format_info_is_yuv_sampling_411(const struct image_format_info *info)
> > +{
> > + return info->is_yuv && info->hsub == 4 && info->vsub == 1;
> > +}
> > +
> > +/**
> > + * image_format_info_is_yuv_sampling_420 - check that the format info matches a
> > + * YUV format with 4:2:0 sub-sampling
> > + * @info: format info
> > + *
> > + * Returns:
> > + * A boolean indicating whether the format info matches a YUV format with 4:2:0
> > + * sub-sampling.
> > + */
> > +static inline bool
> > +image_format_info_is_yuv_sampling_420(const struct image_format_info *info)
> > +{
> > + return info->is_yuv && info->hsub == 2 && info->vsub == 2;
> > +}
> > +
> > +/**
> > + * image_format_info_is_yuv_sampling_422 - check that the format info matches a
> > + * YUV format with 4:2:2 sub-sampling
> > + * @info: format info
> > + *
> > + * Returns:
> > + * A boolean indicating whether the format info matches a YUV format with 4:2:2
> > + * sub-sampling.
> > + */
> > +static inline bool
> > +image_format_info_is_yuv_sampling_422(const struct image_format_info *info)
> > +{
> > + return info->is_yuv && info->hsub == 2 && info->vsub == 1;
> > +}
> > +
> > +/**
> > + * image_format_info_is_yuv_sampling_444 - check that the format info matches a
> > + * YUV format with 4:4:4 sub-sampling
> > + * @info: format info
> > + *
> > + * Returns:
> > + * A boolean indicating whether the format info matches a YUV format with 4:4:4
> > + * sub-sampling.
> > + */
> > +static inline bool
> > +image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
> > +{
> > + return info->is_yuv && info->hsub == 1 && info->vsub == 1;
> > +}
> > +
> > +/**
> > + * image_format_info_plane_cpp - determine the bytes per pixel value
> > + * @format: pixel format info
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The bytes per pixel value for the specified plane.
> > + */
> > +static inline
> > +int image_format_info_plane_cpp(const struct image_format_info *info, int plane)
> > +{
> > + if (!info || plane >= info->num_planes)
> > + return 0;
> > +
> > + return info->cpp[plane];
> > +}
> > +
> > +/**
> > + * image_format_info_plane_width - width of the plane given the first plane
> > + * @format: pixel format info
> > + * @width: width of the first plane
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The width of @plane, given that the width of the first plane is @width.
> > + */
> > +static inline
> > +int image_format_info_plane_width(const struct image_format_info *info, int width,
> > + int plane)
> > +{
> > + if (!info || plane >= info->num_planes)
> > + return 0;
> > +
> > + if (plane == 0)
> > + return width;
> > +
> > + return width / info->hsub;
> > +}
> > +
> > +/**
> > + * image_format_info_plane_height - height of the plane given the first plane
> > + * @format: pixel format info
> > + * @height: height of the first plane
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The height of @plane, given that the height of the first plane is @height.
> > + */
> > +static inline
> > +int image_format_info_plane_height(const struct image_format_info *info, int height,
> > + int plane)
> > +{
> > + if (!info || plane >= info->num_planes)
> > + return 0;
> > +
> > + if (plane == 0)
> > + return height;
> > +
> > + return height / info->vsub;
> > +}
> > +
> > +/**
> > + * image_format_info_block_width - width in pixels of block.
> > + * @format: pointer to the image_format_info
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The width in pixels of a block, depending on the plane index.
> > + */
> > +static inline
> > +unsigned int image_format_info_block_width(const struct image_format_info *format,
> > + int plane)
> > +{
> > + if (!format)
> > + return 0;
> > +
> > + if (plane < 0 || plane >= ARRAY_SIZE(format->block_w))
> > + return 0;
> > +
> > + if (plane >= format->num_planes)
> > + return 0;
> > +
> > + if (!format->block_w[plane])
> > + return 1;
> > +
> > + return format->block_w[plane];
> > +}
> > +
> > +/**
> > + * image_format_info_block_height - height in pixels of a block
> > + * @info: pointer to the image_format_info
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The height in pixels of a block, depending on the plane index.
> > + */
> > +static inline
> > +unsigned int image_format_info_block_height(const struct image_format_info *format,
> > + int plane)
> > +{
> > + if (!format)
> > + return 0;
> > +
> > + if (plane < 0 || plane >= ARRAY_SIZE(format->block_w))
> > + return 0;
> > +
> > + if (plane >= format->num_planes)
> > + return 0;
> > +
> > + if (!format->block_h[plane])
> > + return 1;
> > +
> > + return format->block_h[plane];
> > +}
> > +
> > +/**
> > + * image_format_info_min_pitch - computes the minimum required pitch in bytes
> > + * @info: pixel format info
> > + * @plane: plane index
> > + * @buffer_width: buffer width in pixels
> > + *
> > + * Returns:
> > + * The minimum required pitch in bytes for a buffer by taking into consideration
> > + * the pixel format information and the buffer width.
> > + */
> > +static inline
> > +uint64_t image_format_info_min_pitch(const struct image_format_info *info,
> > + int plane, unsigned int buffer_width)
> > +{
> > + if (!info || plane < 0 || plane >= info->num_planes)
> > + return 0;
> > +
> > + return DIV_ROUND_UP_ULL((u64)buffer_width * info->char_per_block[plane],
> > + image_format_info_block_width(info, plane) *
> > + image_format_info_block_height(info, plane));
> > +}
> > +
> > +const struct image_format_info *__image_format_drm_lookup(u32 drm);
> > +const struct image_format_info *image_format_drm_lookup(u32 drm);
> > +
> > +#endif /* _IMAGE_FORMATS_H_ */
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index fb453afff32e..9a0160d3123b 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -625,3 +625,10 @@ config GENERIC_LIB_UCMPDI2
> >
> > config OBJAGG
> > tristate "objagg" if COMPILE_TEST
> > +
> > +config IMAGE_FORMATS
> > + bool
> > +
> > +config IMAGE_FORMATS_SELFTESTS
> > + tristate "Test image format functions"
> > + depends on IMAGE_FORMATS
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 6996d2b9f401..203336b91248 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -280,3 +280,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
> > obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
> > obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
> > obj-$(CONFIG_OBJAGG) += objagg.o
> > +
> > +obj-$(CONFIG_IMAGE_FORMATS) += image-formats.o
> > +obj-$(CONFIG_IMAGE_FORMATS_SELFTESTS) += image-formats-selftests.o
> > diff --git a/lib/image-formats-selftests.c b/lib/image-formats-selftests.c
> > new file mode 100644
> > index 000000000000..d0f0011b535e
> > --- /dev/null
> > +++ b/lib/image-formats-selftests.c
> > @@ -0,0 +1,325 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Test cases for the image_format functions
> > + */
> > +
> > +#define pr_fmt(fmt) "image_format: " fmt
> > +
> > +#include <linux/errno.h>
> > +#include <linux/image-formats.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +
> > +#include <drm/drm_fourcc.h>
> > +
> > +#define FAIL(test, msg, ...) \
> > + do { \
> > + if (test) { \
> > + pr_err("%s/%u: " msg, __FUNCTION__, __LINE__, ##__VA_ARGS__); \
> > + return -EINVAL; \
> > + } \
> > + } while (0)
> > +
> > +#define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")
> > +
> > +static int test_image_format_info_block_width(void)
> > +{
> > + const struct image_format_info *info = NULL;
> > +
> > + /* Test invalid arguments */
> > + FAIL_ON(image_format_info_block_width(info, 0) != 0);
> > + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> > + FAIL_ON(image_format_info_block_width(info, 1) != 0);
> > +
> > + /* Test 1 plane format */
> > + info = image_format_drm_lookup(DRM_FORMAT_XRGB4444);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_block_width(info, 0) != 1);
> > + FAIL_ON(image_format_info_block_width(info, 1) != 0);
> > + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> > +
> > + /* Test 2 planes format */
> > + info = image_format_drm_lookup(DRM_FORMAT_NV12);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_block_width(info, 0) != 1);
> > + FAIL_ON(image_format_info_block_width(info, 1) != 1);
> > + FAIL_ON(image_format_info_block_width(info, 2) != 0);
> > + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> > +
> > + /* Test 3 planes format */
> > + info = image_format_drm_lookup(DRM_FORMAT_YUV422);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_block_width(info, 0) != 1);
> > + FAIL_ON(image_format_info_block_width(info, 1) != 1);
> > + FAIL_ON(image_format_info_block_width(info, 2) != 1);
> > + FAIL_ON(image_format_info_block_width(info, 3) != 0);
> > + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> > +
> > + /* Test a tiled format */
> > + info = image_format_drm_lookup(DRM_FORMAT_X0L0);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_block_width(info, 0) != 2);
> > + FAIL_ON(image_format_info_block_width(info, 1) != 0);
> > + FAIL_ON(image_format_info_block_width(info, -1) != 0);
> > +
> > + return 0;
> > +}
> > +
> > +static int test_image_format_info_block_height(void)
> > +{
> > + const struct image_format_info *info = NULL;
> > +
> > + /* Test invalid arguments */
> > + FAIL_ON(image_format_info_block_height(info, 0) != 0);
> > + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> > + FAIL_ON(image_format_info_block_height(info, 1) != 0);
> > +
> > + /* Test 1 plane format */
> > + info = image_format_drm_lookup(DRM_FORMAT_XRGB4444);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_block_height(info, 0) != 1);
> > + FAIL_ON(image_format_info_block_height(info, 1) != 0);
> > + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> > +
> > + /* Test 2 planes format */
> > + info = image_format_drm_lookup(DRM_FORMAT_NV12);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_block_height(info, 0) != 1);
> > + FAIL_ON(image_format_info_block_height(info, 1) != 1);
> > + FAIL_ON(image_format_info_block_height(info, 2) != 0);
> > + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> > +
> > + /* Test 3 planes format */
> > + info = image_format_drm_lookup(DRM_FORMAT_YUV422);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_block_height(info, 0) != 1);
> > + FAIL_ON(image_format_info_block_height(info, 1) != 1);
> > + FAIL_ON(image_format_info_block_height(info, 2) != 1);
> > + FAIL_ON(image_format_info_block_height(info, 3) != 0);
> > + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> > +
> > + /* Test a tiled format */
> > + info = image_format_drm_lookup(DRM_FORMAT_X0L0);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_block_height(info, 0) != 2);
> > + FAIL_ON(image_format_info_block_height(info, 1) != 0);
> > + FAIL_ON(image_format_info_block_height(info, -1) != 0);
> > +
> > + return 0;
> > +}
> > +
> > +static int test_image_format_info_min_pitch(void)
> > +{
> > + const struct image_format_info *info = NULL;
> > +
> > + /* Test invalid arguments */
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> > +
> > + /* Test 1 plane 8 bits per pixel format */
> > + info = image_format_drm_lookup(DRM_FORMAT_RGB332);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> > +
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 1);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 640);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 1024);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 1920);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 4096);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 671);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> > + (uint64_t)UINT_MAX);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, (UINT_MAX - 1)) !=
> > + (uint64_t)(UINT_MAX - 1));
> > +
> > + /* Test 1 plane 16 bits per pixel format */
> > + info = image_format_drm_lookup(DRM_FORMAT_XRGB4444);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> > +
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 4);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 1280);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 2048);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 3840);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 8192);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 1342);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> > + (uint64_t)UINT_MAX * 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, (UINT_MAX - 1)) !=
> > + (uint64_t)(UINT_MAX - 1) * 2);
> > +
> > + /* Test 1 plane 24 bits per pixel format */
> > + info = image_format_drm_lookup(DRM_FORMAT_RGB888);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> > +
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 3);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 6);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 1920);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 3072);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 5760);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 12288);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 2013);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> > + (uint64_t)UINT_MAX * 3);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX - 1) !=
> > + (uint64_t)(UINT_MAX - 1) * 3);
> > +
> > + /* Test 1 plane 32 bits per pixel format */
> > + info = image_format_drm_lookup(DRM_FORMAT_ABGR8888);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> > +
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 4);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 8);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 2560);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 4096);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 7680);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 16384);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 2684);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> > + (uint64_t)UINT_MAX * 4);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX - 1) !=
> > + (uint64_t)(UINT_MAX - 1) * 4);
> > +
> > + /* Test 2 planes format */
> > + info = image_format_drm_lookup(DRM_FORMAT_NV12);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, 0) != 0);
> > +
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 1);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 1) != 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 1) != 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 640);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 320) != 640);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 1024);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 512) != 1024);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 1920);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 960) != 1920);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 4096);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 2048) != 4096);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 671);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 336) != 672);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> > + (uint64_t)UINT_MAX);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, UINT_MAX / 2 + 1) !=
> > + (uint64_t)UINT_MAX + 1);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, (UINT_MAX - 1)) !=
> > + (uint64_t)(UINT_MAX - 1));
> > + FAIL_ON(image_format_info_min_pitch(info, 1, (UINT_MAX - 1) / 2) !=
> > + (uint64_t)(UINT_MAX - 1));
> > +
> > + /* Test 3 planes 8 bits per pixel format */
> > + info = image_format_drm_lookup(DRM_FORMAT_YUV422);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 3, 0) != 0);
> > +
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 1);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 1) != 1);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, 1) != 1);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 2) != 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, 2) != 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 640);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 320) != 320);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, 320) != 320);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 1024);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 512) != 512);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, 512) != 512);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 1920);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 960) != 960);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, 960) != 960);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 4096);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 2048) != 2048);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, 2048) != 2048);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 671);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 336) != 336);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, 336) != 336);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> > + (uint64_t)UINT_MAX);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, UINT_MAX / 2 + 1) !=
> > + (uint64_t)UINT_MAX / 2 + 1);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, UINT_MAX / 2 + 1) !=
> > + (uint64_t)UINT_MAX / 2 + 1);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, (UINT_MAX - 1) / 2) !=
> > + (uint64_t)(UINT_MAX - 1) / 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, (UINT_MAX - 1) / 2) !=
> > + (uint64_t)(UINT_MAX - 1) / 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 2, (UINT_MAX - 1) / 2) !=
> > + (uint64_t)(UINT_MAX - 1) / 2);
> > +
> > + /* Test tiled format */
> > + info = image_format_drm_lookup(DRM_FORMAT_X0L2);
> > + FAIL_ON(!info);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, -1, 0) != 0);
> > + FAIL_ON(image_format_info_min_pitch(info, 1, 0) != 0);
> > +
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1) != 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 2) != 4);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 640) != 1280);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1024) != 2048);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 1920) != 3840);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 4096) != 8192);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, 671) != 1342);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX) !=
> > + (uint64_t)UINT_MAX * 2);
> > + FAIL_ON(image_format_info_min_pitch(info, 0, UINT_MAX - 1) !=
> > + (uint64_t)(UINT_MAX - 1) * 2);
> > +
> > + return 0;
> > +}
> > +
> > +#define selftest(test) { .name = #test, .func = test, }
> > +
> > +static struct image_format_test {
> > + char *name;
> > + int (*func)(void);
> > +} tests[] = {
> > + selftest(test_image_format_info_block_height),
> > + selftest(test_image_format_info_block_width),
> > + selftest(test_image_format_info_min_pitch),
> > +};
> > +
> > +static int __init image_format_test_init(void)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > + struct image_format_test *test = &tests[i];
> > + int ret;
> > +
> > + ret = test->func();
> > + if (ret) {
> > + pr_err("Failed test %s\n", test->name);
> > + return ret;
> > + }
> > + }
> > +
> > + pr_info("All tests executed properly.\n");
> > + return 0;
> > +}
> > +module_init(image_format_test_init);
> > +
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_AUTHOR("Maxime Ripard <[email protected]>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/lib/image-formats.c b/lib/image-formats.c
> > new file mode 100644
> > index 000000000000..1e52a7410222
> > --- /dev/null
> > +++ b/lib/image-formats.c
> > @@ -0,0 +1,655 @@
> > +/*
> > + * Copyright (c) 2016 Laurent Pinchart <[email protected]>
> > + * Copyright (c) 2019 Maxime Ripard <[email protected]>
> > + *
> > + * Permission to use, copy, modify, distribute, and sell this software and its
> > + * documentation for any purpose is hereby granted without fee, provided that
> > + * the above copyright notice appear in all copies and that both that copyright
> > + * notice and this permission notice appear in supporting documentation, and
> > + * that the name of the copyright holders not be used in advertising or
> > + * publicity pertaining to distribution of the software without specific,
> > + * written prior permission. The copyright holders make no representations
> > + * about the suitability of this software for any purpose. It is provided "as
> > + * is" without express or implied warranty.
> > + *
> > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > + * OF THIS SOFTWARE.
> > + */
> > +
> > +#include <linux/bug.h>
> > +#include <linux/image-formats.h>
> > +#include <linux/kernel.h>
> > +#include <linux/math64.h>
> > +
> > +#include <uapi/drm/drm_fourcc.h>
> > +
> > +static const struct image_format_info formats[] = {
> > + {
> > + .drm_fmt = DRM_FORMAT_C8,
> > + .depth = 8,
> > + .num_planes = 1,
> > + .cpp = { 1, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGB332,
> > + .depth = 8,
> > + .num_planes = 1,
> > + .cpp = { 1, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGR233,
> > + .depth = 8,
> > + .num_planes = 1,
> > + .cpp = { 1, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XRGB4444,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XBGR4444,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGBX4444,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGRX4444,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_ARGB4444,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_ABGR4444,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGBA4444,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGRA4444,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XRGB1555,
> > + .depth = 15,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XBGR1555,
> > + .depth = 15,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGBX5551,
> > + .depth = 15,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGRX5551,
> > + .depth = 15,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_ARGB1555,
> > + .depth = 15,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_ABGR1555,
> > + .depth = 15,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGBA5551,
> > + .depth = 15,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGRA5551,
> > + .depth = 15,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGB565,
> > + .depth = 16,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGR565,
> > + .depth = 16,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGB888,
> > + .depth = 24,
> > + .num_planes = 1,
> > + .cpp = { 3, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGR888,
> > + .depth = 24,
> > + .num_planes = 1,
> > + .cpp = { 3, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XRGB8888,
> > + .depth = 24,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XBGR8888,
> > + .depth = 24,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGBX8888,
> > + .depth = 24,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGRX8888,
> > + .depth = 24,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGB565_A8,
> > + .depth = 24,
> > + .num_planes = 2,
> > + .cpp = { 2, 1, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGR565_A8,
> > + .depth = 24,
> > + .num_planes = 2,
> > + .cpp = { 2, 1, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XRGB2101010,
> > + .depth = 30,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XBGR2101010,
> > + .depth = 30,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGBX1010102,
> > + .depth = 30,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGRX1010102,
> > + .depth = 30,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_ARGB2101010,
> > + .depth = 30,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_ABGR2101010,
> > + .depth = 30,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGBA1010102,
> > + .depth = 30,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGRA1010102,
> > + .depth = 30,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_ARGB8888,
> > + .depth = 32,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_ABGR8888,
> > + .depth = 32,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGBA8888,
> > + .depth = 32,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGRA8888,
> > + .depth = 32,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGB888_A8,
> > + .depth = 32,
> > + .num_planes = 2,
> > + .cpp = { 3, 1, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGR888_A8,
> > + .depth = 32,
> > + .num_planes = 2,
> > + .cpp = { 3, 1, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XRGB8888_A8,
> > + .depth = 32,
> > + .num_planes = 2,
> > + .cpp = { 4, 1, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XBGR8888_A8,
> > + .depth = 32,
> > + .num_planes = 2,
> > + .cpp = { 4, 1, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_RGBX8888_A8,
> > + .depth = 32,
> > + .num_planes = 2,
> > + .cpp = { 4, 1, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_BGRX8888_A8,
> > + .depth = 32,
> > + .num_planes = 2,
> > + .cpp = { 4, 1, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YUV410,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 4,
> > + .vsub = 4,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YVU410,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 4,
> > + .vsub = 4,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YUV411,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 4,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YVU411,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 4,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YUV420,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 2,
> > + .vsub = 2,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YVU420,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 2,
> > + .vsub = 2,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YUV422,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 2,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YVU422,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 2,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YUV444,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YVU444,
> > + .depth = 0,
> > + .num_planes = 3,
> > + .cpp = { 1, 1, 1 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_NV12,
> > + .depth = 0,
> > + .num_planes = 2,
> > + .cpp = { 1, 2, 0 },
> > + .hsub = 2,
> > + .vsub = 2,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_NV21,
> > + .depth = 0,
> > + .num_planes = 2,
> > + .cpp = { 1, 2, 0 },
> > + .hsub = 2,
> > + .vsub = 2,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_NV16,
> > + .depth = 0,
> > + .num_planes = 2,
> > + .cpp = { 1, 2, 0 },
> > + .hsub = 2,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_NV61,
> > + .depth = 0,
> > + .num_planes = 2,
> > + .cpp = { 1, 2, 0 },
> > + .hsub = 2,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_NV24,
> > + .depth = 0,
> > + .num_planes = 2,
> > + .cpp = { 1, 2, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_NV42,
> > + .depth = 0,
> > + .num_planes = 2,
> > + .cpp = { 1, 2, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YUYV,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 2,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_YVYU,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 2,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_UYVY,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 2,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_VYUY,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 2, 0, 0 },
> > + .hsub = 2,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_XYUV8888,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_AYUV,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .cpp = { 4, 0, 0 },
> > + .hsub = 1,
> > + .vsub = 1,
> > + .has_alpha = true,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_Y0L0,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .char_per_block = { 8, 0, 0 },
> > + .block_w = { 2, 0, 0 },
> > + .block_h = { 2, 0, 0 },
> > + .hsub = 2,
> > + .vsub = 2,
> > + .has_alpha = true,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_X0L0,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .char_per_block = { 8, 0, 0 },
> > + .block_w = { 2, 0, 0 },
> > + .block_h = { 2, 0, 0 },
> > + .hsub = 2,
> > + .vsub = 2,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_Y0L2,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .char_per_block = { 8, 0, 0 },
> > + .block_w = { 2, 0, 0 },
> > + .block_h = { 2, 0, 0 },
> > + .hsub = 2,
> > + .vsub = 2,
> > + .has_alpha = true,
> > + .is_yuv = true,
> > + }, {
> > + .drm_fmt = DRM_FORMAT_X0L2,
> > + .depth = 0,
> > + .num_planes = 1,
> > + .char_per_block = { 8, 0, 0 },
> > + .block_w = { 2, 0, 0 },
> > + .block_h = { 2, 0, 0 },
> > + .hsub = 2,
> > + .vsub = 2,
> > + .is_yuv = true,
> > + },
> > +};
> > +
> > +#define __image_format_lookup(_field, _fmt) \
> > + ({ \
> > + const struct image_format_info *format = NULL; \
> > + unsigned i; \
> > + \
> > + for (i = 0; i < ARRAY_SIZE(formats); i++) \
> > + if (formats[i]._field == _fmt) \
> > + format = &formats[i]; \
> > + \
> > + format; \
> > + })
> > +
> > +/**
> > + * __image_format_drm_lookup - query information for a given format
> > + * @drm: DRM fourcc pixel format (DRM_FORMAT_*)
> > + *
> > + * The caller should only pass a supported pixel format to this function.
> > + *
> > + * Returns:
> > + * The instance of struct image_format_info that describes the pixel format, or
> > + * NULL if the format is unsupported.
> > + */
> > +const struct image_format_info *__image_format_drm_lookup(u32 drm)
> > +{
> > + return __image_format_lookup(drm_fmt, drm);
> > +}
> > +EXPORT_SYMBOL(__image_format_drm_lookup);
> > +
> > +/**
> > + * image_format_drm_lookup - query information for a given format
> > + * @drm: DRM fourcc pixel format (DRM_FORMAT_*)
> > + *
> > + * The caller should only pass a supported pixel format to this function.
> > + * Unsupported pixel formats will generate a warning in the kernel log.
> > + *
> > + * Returns:
> > + * The instance of struct image_format_info that describes the pixel format, or
> > + * NULL if the format is unsupported.
> > + */
> > +const struct image_format_info *image_format_drm_lookup(u32 drm)
> > +{
> > + const struct image_format_info *format;
> > +
> > + format = __image_format_drm_lookup(drm);
> > +
> > + WARN_ON(!format);
> > + return format;
> > +}
> > +EXPORT_SYMBOL(image_format_drm_lookup);
> >

2019-04-23 17:19:45

Hi Hans

On Thu, May 02, 2019 at 10:24:00AM +0200, Hans Verkuil wrote:
> On 4/17/19 9:54 AM, Maxime Ripard wrote:
> > V4L2 uses different fourcc's than DRM, and has a different set of formats.
> > For now, let's add the v4l2 fourcc's for the already existing formats.
>
> For this lib to be more useful for V4L2, would it be a good idea to add
> Bayer formats as well? This can be done in a separate patch.
>
> Those formats are V4L specific, but are very common.

Yeah, this was mostly to support the formats that are already
supported as of today, but eventually more are going to be supported,
and the bayer formats seems like a natural choice :)

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (765.00 B)
signature.asc (235.00 B)
Download all attachments

2019-05-11 19:21:03

by Laurent Pinchart

[permalink] [raw]

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

Hi Paul,

On Tue, Apr 23, 2019 at 06:54:49PM +0200, Paul Kocialkowski wrote:
> Le dimanche 21 avril 2019 à 01:40 +0300, Laurent Pinchart a écrit :
> > 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.
>
> Well, I don't see how we could maintain backward compatibility with
> some DRM and V4L2 fourccs that are compatible and some that aren't.
> Since both descriptions have diverged already, one would need explicit
> checking of whether the format at hand is a compatible one or not
> before passing-it along as-is to the other subsystem or going through a
> format conversion step (in userspace, duplicating the information).
> So it feels like it kind of defeats the purpose.

Yes and no. If a V4L2-only application needs to deal with two sets of
4CCs for old and new APIs, it's a burden. However, if an application
that shares buffers between V4L2 and DRM/KMS can use a single set of
4CCs, that's a win, as those applications already need to convert. In
the end this shifts the problem from one set of applications to another,
but as soon as the new APIs get old enough to mandate their usage (which
would roughly be when the corresponding kernel ships in all supported
versions of all major distros), then applications will be able to drop
the conversion code, and everybody will be happier. Short term pain for
long term gain.

> If we're going to use a unified 4CC representation in the future, I
> think we should do it by using the new formats that this proposal is
> introducing instead of subsystem-specific formats. At which point I
> believe we will need an internal conversion step between that format
> and what the subsystem uses internally. Or do it the other way round
> and use the unified format all around the subsystem, with a legacy
> layer for the previous subsystem-specific format.

I agree with you. I would like new APIs to use unified 4CCs (probably
coming from DRM), and have an internal conversion from and to
subsystem-specific 4CCs where needed. That's quite a lot of work of
course, and won't happen overnight.

--
Regards,

Laurent Pinchart

2019-05-11 19:28:00

by Laurent Pinchart

[permalink] [raw]

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

Hi Daniel,

On Tue, Apr 23, 2019 at 09:18:52PM +0200, Daniel Vetter wrote:
> On Tue, Apr 23, 2019 at 5:45 PM Laurent Pinchart wrote:
> > 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.
>
> I think Daniel Stone explains fairly well why some of our additional
> metadata is included as a plane, while a lot of the other metadata
> involved in rendering/compute the framebuffer isn't. Not really
> anything to add here.
>
> >> 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 ?
>
> hw offers enormous amounts of tricks to make glClear O(1), or at least
> close enough. glClear is usually what's done at the start of every
> frame, and clears the entire framebuffer to a uniform color. This is
> achieved usually through 3 pieces:
> - actual framebuffer plane with the pixel data
> - a 2nd plane that (usually, but there's lots of tricks here) contains
> a bit of metadata per cacheline/block/whatever in the framebuffer to
> indicate how/if those pixels are compressed, or whether they are still
> the uniform color supplied through glClear. For actual O(1) this
> metadata is hierarchical, so that a glClear really only sets the
> top-level metadata to "all subordinate blocks still have are the clear
> color". hw tends to have some pretty strong opinions on where it's
> going to look for that 2nd plane.
> - curiously on most hw the actual clear color is supplied separately
> (and our plan is to just stuff it into a 3rd plane)

Just to clarify, is this needed for display engines ? Does the GPU
render to a 3 planes buffer with glClear()-related data in planes 2 and
3, with the buffer then being passed to the display engine that knows
how to interpret this, or are those extra planes only needed for GPU
rendering ?

> >> 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 small boutique trees are a problem themselves, not a solution.
> So if you're creating a new small boutique tree to fix a problem, you
> then have 2. Yes, assuming sufficient expenditure of energy it can be
> made to work, but I'd prefer to make my own life as easy and lazy as
> possible :-) And I think I've been fairly successful at that within
> drivers/gpu at least.
>
> Imo the proper fix is to merge v4l and drm, at a process/maintainer
> level. That would solve both the original issue and the 2ndary one of
> the permanent topic branch.

Proposals are welcome ;-)

--
Regards,

Laurent Pinchart

2019-05-13 16:42:49

by Mauro Carvalho Chehab

[permalink] [raw]

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

Em Mon, 13 May 2019 16:57:19 +0200
Daniel Vetter <[email protected]> escreveu:

> > > I think small boutique trees are a problem themselves, not a solution.
> > > So if you're creating a new small boutique tree to fix a problem, you
> > > then have 2. Yes, assuming sufficient expenditure of energy it can be
> > > made to work, but I'd prefer to make my own life as easy and lazy as
> > > possible :-) And I think I've been fairly successful at that within
> > > drivers/gpu at least.
> > >
> > > Imo the proper fix is to merge v4l and drm, at a process/maintainer
> > > level. That would solve both the original issue and the 2ndary one of
> > > the permanent topic branch.
> >
> > Proposals are welcome ;-)
>
> I'm already somewhat unpopular at LPC/lkml/kernel-at-large, I don't want
> to make this worse. That's why I don't want to officially push for
> anything here myself, nor be in any other way involved in an effort to
> figure out v4l governance and maintainership rules.
>
> What I think is required for efficient collaboration with drm (no matter
> how we implement that in the details once we're ready for that step) is
> some kind of group maintainership model. Doesn't need to be as extreme as
> drm-misc, but I think at least something like the soc tree (while it was
> still a bit more limited as arm-soc). Otherwise the impendence mismatch
> between how drm rolls and how v4l rolls is probably way too much. From my
> understanding v4l is working differently.
>
> Also, through sheer inertia of size, I don't think it'll be easier to
> align drm with the v4l model. So that option is not realistic.

I don't think it is realistic trying to align V4L2 model to every single
other subsystems we use. We have a lot of alignment with alsa, with even
increased during this merge window due to some drivers on media sharing
media controller resources with usb-audio. We also have lots of alignment
with i2c, as we use a lot of stuff there, and from time to time they
need to add new features due to our needs. The same is true also for
input and for other subsystems and arch/sub-arch trees.

That doesn't mean at all that everybody should use the same maintainership
model. Each subsystem should use whatever suits best.

That's said, after following this thread for a while, I'm starting to
convince that it wouldn't even be realistic to have a common fourcc
API for both subsystems. The internal requirements from each subsystem
are different, and, as it was already pointed in this thread, that's
basically why DRM ended by having their own way of doing that.

Yet, it would make sense to have at least a single nomenclature for
both systems to use, but it could be a simple as what we already do
with other common resources, like:

Documentation/ioctl/ioctl-number.txt

If we keep the fourcc codes there sorted, if one subsystem would
add a conflicting code, it would be easy to notice at linux-next.

Thanks,
Mauro

2019-05-13 18:35:27

by Daniel Vetter

[permalink] [raw]

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

On Sat, May 11, 2019 at 10:26:32PM +0300, Laurent Pinchart wrote:
> On Tue, Apr 23, 2019 at 09:18:52PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 23, 2019 at 5:45 PM Laurent Pinchart wrote:
> > > 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.
> >
> > I think Daniel Stone explains fairly well why some of our additional
> > metadata is included as a plane, while a lot of the other metadata
> > involved in rendering/compute the framebuffer isn't. Not really
> > anything to add here.
> >
> > >> 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 ?
> >
> > hw offers enormous amounts of tricks to make glClear O(1), or at least
> > close enough. glClear is usually what's done at the start of every
> > frame, and clears the entire framebuffer to a uniform color. This is
> > achieved usually through 3 pieces:
> > - actual framebuffer plane with the pixel data
> > - a 2nd plane that (usually, but there's lots of tricks here) contains
> > a bit of metadata per cacheline/block/whatever in the framebuffer to
> > indicate how/if those pixels are compressed, or whether they are still
> > the uniform color supplied through glClear. For actual O(1) this
> > metadata is hierarchical, so that a glClear really only sets the
> > top-level metadata to "all subordinate blocks still have are the clear
> > color". hw tends to have some pretty strong opinions on where it's
> > going to look for that 2nd plane.
> > - curiously on most hw the actual clear color is supplied separately
> > (and our plan is to just stuff it into a 3rd plane)
>
> Just to clarify, is this needed for display engines ? Does the GPU
> render to a 3 planes buffer with glClear()-related data in planes 2 and
> 3, with the buffer then being passed to the display engine that knows
> how to interpret this, or are those extra planes only needed for GPU
> rendering ?

I haven't yet seen a display engine that can scan out fast-cleared buffers
like these. Everyone can scan out compressed buffers, i.e. rgb + 2nd
auxialliary buffer with some compression metadata. But not yet with the
3rd plane.

So not relevant for display. But it is very much relevant for
uapi/drm_fourcc.h, because we'll need it for userspace interop between
different apis (vk, gl, ...) and different processes (client, compositor).
And in these standard extension texts we've officially made the kernel's
copy of that file the official registry. So these fourcc+modifier codes
will end up at least in that namespace (but maybe not in the same
library the kernel uses internally).

> > >> 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 small boutique trees are a problem themselves, not a solution.
> > So if you're creating a new small boutique tree to fix a problem, you
> > then have 2. Yes, assuming sufficient expenditure of energy it can be
> > made to work, but I'd prefer to make my own life as easy and lazy as
> > possible :-) And I think I've been fairly successful at that within
> > drivers/gpu at least.
> >
> > Imo the proper fix is to merge v4l and drm, at a process/maintainer
> > level. That would solve both the original issue and the 2ndary one of
> > the permanent topic branch.
>
> Proposals are welcome ;-)

I'm already somewhat unpopular at LPC/lkml/kernel-at-large, I don't want
to make this worse. That's why I don't want to officially push for
anything here myself, nor be in any other way involved in an effort to
figure out v4l governance and maintainership rules.

What I think is required for efficient collaboration with drm (no matter
how we implement that in the details once we're ready for that step) is
some kind of group maintainership model. Doesn't need to be as extreme as
drm-misc, but I think at least something like the soc tree (while it was
still a bit more limited as arm-soc). Otherwise the impendence mismatch
between how drm rolls and how v4l rolls is probably way too much. From my
understanding v4l is working differently.

Also, through sheer inertia of size, I don't think it'll be easier to
align drm with the v4l model. So that option is not realistic.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch