Messages in this thread | | | Date | Wed, 12 Sep 2018 14:52:07 +0100 | From | Brian Starkey <> | Subject | Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel |
| |
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
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
| |