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@xxxxxxx>
> > >
> > > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
