Re: [PATCH] drm/fourcc: Add Arm 16x16 block modifier

[Brian Starkey]

Hi Daniel,

On Fri, Jun 21, 2019 at 05:27:00PM +0200, Daniel Vetter wrote:
> On Fri, Jun 21, 2019 at 12:21 PM Raymond Smith <Raymond.Smith@xxxxxxx> wrote:
> >
> > Add the DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED modifier to
> > denote the 16x16 block u-interleaved format used in Arm Utgard and
> > Midgard GPUs.
> >
> > Signed-off-by: Raymond Smith <raymond.smith@xxxxxxx>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 3feeaa3..8ed7ecf 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -743,6 +743,16 @@ extern "C" {
> >  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
> >
> >  /*
> > + * Arm 16x16 Block U-Interleaved modifier
> > + *
> > + * This is used by Arm Mali Utgard and Midgard GPUs. It divides the image
> > + * into 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
> > + * in the block are reordered.
> > + */
> > +#define DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED \
> > +       fourcc_mod_code(ARM, ((1ULL << 55) | 1))
> 
> This seems to be an extremely random pick for a new number. What's the
> thinking here? Aside from "doesnt match any of the afbc combos" ofc.
> If you're already up to having thrown away 55bits, then it's not going
> to last long really :-)
> 
> I think a good idea would be to reserve a bunch of the high bits as
> some form of index (afbc would get index 0 for backwards compat). And
> then the lower bits would be for free use for a given index/mode. And
> the first mode is probably an enumeration, where possible modes simple
> get enumerated without further flags or anything.

Yup, that's the plan:

	(0 << 55): AFBC
	(1 << 55): This "non-category" for U-Interleaved
	(1 << 54): Whatever the next category is
	(3 << 54): Whatever comes after that
	(1 << 53): Maybe we'll get here someday
	...

I didn't want to explicitly reserve some high bits, because we've no
idea how many to reserve. This way, we can assign exactly as many
high bits as we need, when we need them. If any of the "modes" start
encroaching towards the high bits, we'll have to make a decision at
that point.

Also, this is the only U-Interleaved format (that I know of), so it's
not worth calling bit 55 "The U-Interleaved bit" because that would be
a waste of space. It's more like the "misc" bit, but that's not a
useful name to enshrine in UAPI.

Note that isn't the same as the "not-AFBC bit", because we may well
have something in the future which is neither AFBC nor "misc".

We've been very careful in our code to enforce all
undefined/unrecognised bits to be zero, to ensure that this works.

> 
> The other bit: Would be real good to define the format a bit more
> precisely, including the layout within the tile.

It's U-Interleaved, obviously ;-)

-Brian

> 
> Also ofc needs acks from lima/panfrost people since I assume they'll
> be using this, too.
> 
> Thanks, Daniel
> 
> > +
> > +/*
> >   * Allwinner tiled modifier
> >   *
> >   * This tiling mode is implemented by the VPU found on all Allwinner platforms,
> > --
> > 2.7.4
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch