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

[Brian Starkey]

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

I think we need two things:
 - The size, in bits, of a tile
 - The width and height, in pixels, of a tile (currently implicitly
   1x1)

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

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