Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 12, 2018 at 3:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote:
>>
>> On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote:
>>>
>>> Hi,
>>>
>>> On Fri, Sep 07, 2018 at 09:28:44PM +0200, Daniel Vetter wrote:
>>> > On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote:
>>> > > Hi Daniel,
>>> > >
>>> > > On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
>>> > > > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
>>> > > > > Some formats have a non-integer number of bytes per pixel, which
>>> > > > > can't
>>> > > > > be handled with the existing 'cpp' field in drm_format_info. To
>>> > > > > handle
>>> > > > > these formats, add a 'bpp' field, which is only used if cpp[0] ==
>>> > > > > 0.
>>> > > > >
>>> > > > > This updates all the users of format->cpp in the core DRM code,
>>> > > > > converting them to use a new function to get the bits-per-pixel
>>> > > > > for any
>>> > > > > format.
>>> > > > >
>>> > > > > It's assumed that drivers will use the 'bpp' field when they add
>>> > > > > support
>>> > > > > for pixel formats with non-integer bytes-per-pixel.
>>> > > > >
>>> > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>>> > > >
>>> > > > I assume you still require that stuff is eventually aligned to
>>> > > > bytes? In
>>> > > > that case, can we subsume this into the tile work Alex is doing?
>>> > > > It's
>>> > > > essentially just another special case of having storage-size units
>>> > > > measured in bytes which span more than 1x1 pixel. And I kinda don't
>>> > > > want a
>>> > > > metric pile of special cases here in the format code, because that
>>> > > > just
>>> > > > means every driver handles a different subset, with different bugs.
>>> > > > -Daniel
>>> > >
>>> > > Sorry for the delay, been struggling to free some cycles to think
>>> > > about this.
>>> > >
>>> > > I'm not sure how to pull this in with the tiling stuff. In the AFBC
>>> > > case then our AFBC superblocks are always nice round numbers (256
>>> > > pixels), and so it does end up being a multiple of bytes.
>>> > >
>>> > > However, AFBC supports different superblock sizes, so picking just
>>> > > one
>>> > > doesn't really work out, and putting AFBC in the core format table
>>> > > which reflects AFBC doesn't seem good.
>>> > >
>>> > > We could make something up (e.g. call these formats "tiled" with 2x4
>>> > > tiles, which guarantees a multiple of 8), but it would be an
>>> > > arbitrarily-selected lie, which often seems to spell trouble. If we
>>> > > did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
>>> > > we still need to add a new field anyway.
>>> > >
>>> > > What's the pile of special cases you're worried about? The helper
>>> > > I've
>>> > > added here means that drivers which need to care can use one API and
>>> > > not implement their own bugs.
>>> >
>>> > I'm confused ... the new bits-per-pixel stuff you're adding here is for
>>> > yuv formats, not afbc. I'm just suggesting we have only 1 way of
>>> > describing such formats that need more descriptive power than cpp,
>>> > whether
>>> > they have some kind of pixel-groups or small tiles.
>>>
>>> Well, not really. The three formats which have non-integer cpp are:
>>> DRM_FORMAT_VUY101010, DRM_FORMAT_YUV420_8BIT and
>>> DRM_FORMAT_YUV420_10BIT. These formats are only valid with non-linear
>>> modifiers (no linear encoding is defined). Mali only supports them
>>> with AFBC.
>>>
>>> The formats themselves have no notion of tiling or grouping - the
>>> modifier adds that. I'm not aware of any non-AFBC uses of these
>>> formats, so I don't want to "make up" a small-tile layout restriction
>>> for them.
>>
>>
>> Ah, I missed that.
>>
>>> > For very special stuff like afbc you need to validate in the driver
>>> > anyway, too complicated. So I have no idea why you bring this up here?
>>>
>>> Sure, we can just let drivers provide their own format_info's for
>>> these, if that's what you prefer. The core format checking code can
>>> error out if it ever encounters them.
>>
>>
>> It's format_info we're talking about. What I mean is that you just set all
>> these to 0 and let the format_info code ignore it. And then having a
>> bespoke drm_format_check_afbc helper function or similar, which checks all
>> the layout restrictions of afbc.
>>
>> I still maintain that bpp and tile_size are equavalent, and we really
>> don't need both. Both are defacto a form of numerator/denumerator. If you
>> don't like that you have to introduce "fake" tiles for afbc, then we can
>> rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't
>> change one bit of the math. bpp simply hardcodes a denumerator of 8, and I
>> don't see why we need that special case. Except if you love to write
>> redundant self tests for all the math :-)
>>
>> So two options that I think are reasonable:
>> - one common numerator/denumerator. I don't care how you call that
>>  bikeshed.
>
>
> Sorry for being dense, but I'm still struggling to get my head around
> what you're suggesting. In particular "bpp simply hardcodes a
> denumerator of 8" didn't make any sense to me. Could you give concrete
> examples for how you think this would look for e.g.
>
> - DRM_FORMAT_VUY101010. 30-bits per pixel, no tiling.
> - DRM_FORMAT_Y0L2. 16-bits per pixel, 2x2 pixel tiles

Ok, a few examples:

4 cpp: tile_size = 4, tile_h/w = 1

30 bpp: In cpp that's 30 / 8 = 15 / 4. So would be a tile_size = 15,
tile_w = 4, tile_h = 1. If you check the math, this matches exactly
all the same addfb values as what you have in your bpp computation
(but only if you simplify the quotient).

16 bpp, 2x2 tiles: No real math needed here, tile_size = 2 * 2 * 2 =
8, tile_h/w = 2.


> I think we need two things:
> - The size, in bits, of a tile

Nope, you only need it in bytes, because in the end all buffers must
align to bytes. The generic formula is:

X bpp: tile_size = X / gcd(X, 8), tile_w = 8 / gcd(X, 8), tile_h = 1.

No bits needed anywhere.

> - The width and height, in pixels, of a tile (currently implicitly
>   1x1)

Yeah, but only if you use the cpp thing. We could even throw that out,
and entirely replace it with tile_size, with the rule that if tile_h/w
= 0, then you assume they're both 1.

> Do you disagree? Are you just saying that instead of adding .bpp I
> should be replacing .cpp with .bpp wholesale?

Hopefully the above explains what I mean, and demonstrates that you
can express any bpp in terms of tile_size/tile_w. Because bpp is just
a special quotient, with a fixed 8 divisor.
-Daniel

>> - don't check afbc using format_info, have your own helper that does that
>>  using custom code.
>
>
> We can do this, no problem.
>
> Thanks,
> -Brian
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch