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

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

[Raymond Smith]

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@arm.com>
---
 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))
+
+/*
  * Allwinner tiled modifier
  *
  * This tiling mode is implemented by the VPU found on all Allwinner platforms,
-- 
2.7.4

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

[Brian Starkey]

On Fri, Jun 21, 2019 at 11:21:08AM +0100, Raymond Smith 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@arm.com>

Reviewed-by: Brian Starkey <brian.starkey@arm.com>

Thanks for chasing this down!

> ---
>  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))
> +
> +/*
>   * Allwinner tiled modifier
>   *
>   * This tiling mode is implemented by the VPU found on all Allwinner platforms,
> -- 
> 2.7.4
> 

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

[Daniel Vetter]

On Fri, Jun 21, 2019 at 12:21 PM Raymond Smith <Raymond.Smith@arm.com> 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@arm.com>
> ---
>  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.

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

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

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

[Daniel Vetter]

On Fri, Jun 21, 2019 at 12:21 PM Raymond Smith <Raymond.Smith@arm.com> 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@arm.com>
> ---
>  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.

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

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

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

[Qiang Yu]

On Fri, Jun 21, 2019 at 11:27 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Jun 21, 2019 at 12:21 PM Raymond Smith <Raymond.Smith@arm.com> 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@arm.com>
> > ---
> >  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))
>
Thanks for the patch.

> 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.
>
This idea is like my previous patch:
https://patchwork.kernel.org/patch/10852619/

lima driver just need a unique modifier to represent this format, so this patch
is enough for lima needs.

But I'm also a little worry about the expansion of only reserve the top bit
(55) for classification from the ARM point of view. A few more bit (at least 2
for being able to represent 4 class of format) and more clear class/format
fields division would be better.

Thanks,
Qiang




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

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

[Qiang Yu]

On Fri, Jun 21, 2019 at 11:27 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Jun 21, 2019 at 12:21 PM Raymond Smith <Raymond.Smith@arm.com> 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@arm.com>
> > ---
> >  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))
>
Thanks for the patch.

> 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.
>
This idea is like my previous patch:
https://patchwork.kernel.org/patch/10852619/

lima driver just need a unique modifier to represent this format, so this patch
is enough for lima needs.

But I'm also a little worry about the expansion of only reserve the top bit
(55) for classification from the ARM point of view. A few more bit (at least 2
for being able to represent 4 class of format) and more clear class/format
fields division would be better.

Thanks,
Qiang




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

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

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@arm.com> 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@arm.com>
> > ---
> >  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

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

[Daniel Vetter]

On Mon, Jun 24, 2019 at 11:32 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> 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@arm.com> 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@arm.com>
> > > ---
> > >  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

Uh, so the index would be encoded with least-significant bit first,
starting from bit55 downwards? Clever idea, but I think this needs a
macro (or at least a comment). Not sure there's a ready-made bitmask
mirror function for this stuff, works case we can hand-code it and
extend every time we need one more bit encoded. Something like:

MIRROR_U32((u & (BIT(0)) << 31 | (u & BIT(1) << 30 | ...)

And then shift that to the correct place. Probably want an

ARM_MODIFIER_ENCODE(space_idx, flags) macro which assembles everything.

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

Yeah that's what I meant. Also better to explicitly reserve this, i.e.

#define ARM_FBC_MODIFIER_SPACE 0
#define ARM_MISC_MODIFIER_SPACE 1

and then encode with the mirror trickery.

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

:-) I mean full code exists in panfrost/lima, so this won't change
anything really ...

Cheers, Daniel

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Mon, Jun 24, 2019 at 11:32 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> 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@arm.com> 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@arm.com>
> > > ---
> > >  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

Uh, so the index would be encoded with least-significant bit first,
starting from bit55 downwards? Clever idea, but I think this needs a
macro (or at least a comment). Not sure there's a ready-made bitmask
mirror function for this stuff, works case we can hand-code it and
extend every time we need one more bit encoded. Something like:

MIRROR_U32((u & (BIT(0)) << 31 | (u & BIT(1) << 30 | ...)

And then shift that to the correct place. Probably want an

ARM_MODIFIER_ENCODE(space_idx, flags) macro which assembles everything.

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

Yeah that's what I meant. Also better to explicitly reserve this, i.e.

#define ARM_FBC_MODIFIER_SPACE 0
#define ARM_MISC_MODIFIER_SPACE 1

and then encode with the mirror trickery.

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

:-) I mean full code exists in panfrost/lima, so this won't change
anything really ...

Cheers, Daniel

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



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

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

[Brian Starkey]

On Mon, Jun 24, 2019 at 11:58:59AM +0200, Daniel Vetter wrote:
> On Mon, Jun 24, 2019 at 11:32 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
> >
> > 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@arm.com> 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@arm.com>
> > > > ---
> > > >  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
> 
> Uh, so the index would be encoded with least-significant bit first,
> starting from bit55 downwards? 

Yeah.

> Clever idea, but I think this needs a
> macro (or at least a comment). Not sure there's a ready-made bitmask
> mirror function for this stuff, works case we can hand-code it and
> extend every time we need one more bit encoded. Something like:
> 
> MIRROR_U32((u & (BIT(0)) << 31 | (u & BIT(1) << 30 | ...)
> 

Is it really worth it? People can just use the definitions as written
in drm_fourcc.h. I agree that we should have the high bits described
in a comment though.

> And then shift that to the correct place. Probably want an
> 
> ARM_MODIFIER_ENCODE(space_idx, flags) macro which assembles everything.
> 
> >         ...
> >
> > 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.
> 
> Yeah that's what I meant. Also better to explicitly reserve this, i.e.
> 
> #define ARM_FBC_MODIFIER_SPACE 0
> #define ARM_MISC_MODIFIER_SPACE 1
> 
> and then encode with the mirror trickery.
> 

I don't really see the value in that either, it's just giving
userspace the opportunity to depend on more stuff: more future
headaches. So long as the 64-bit values are stable, that should be
enough.

> > 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 ;-)
> 
> :-) I mean full code exists in panfrost/lima, so this won't change
> anything really ...

Yeah, so for us to provide a more detailed description would require
another lengthy loop through our legal approval process, and I'm not
sure we can make a strong business case (which is what we need) for
why this is needed.

Of course, if someone happens to know the layout and wants to
contribute to this file... Then I don't know how ack/r-b would work in
that case, but I imagine the subsystem maintainer(s) might take issue
with us attempting to block that contribution.

Thanks,
-Brian

> 
> Cheers, Daniel
> 
> >
> > -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
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Mon, Jun 24, 2019 at 1:23 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> On Mon, Jun 24, 2019 at 11:58:59AM +0200, Daniel Vetter wrote:
> > On Mon, Jun 24, 2019 at 11:32 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > >
> > > 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@arm.com> 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@arm.com>
> > > > > ---
> > > > >  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
> >
> > Uh, so the index would be encoded with least-significant bit first,
> > starting from bit55 downwards?
>
> Yeah.
>
> > Clever idea, but I think this needs a
> > macro (or at least a comment). Not sure there's a ready-made bitmask
> > mirror function for this stuff, works case we can hand-code it and
> > extend every time we need one more bit encoded. Something like:
> >
> > MIRROR_U32((u & (BIT(0)) << 31 | (u & BIT(1) << 30 | ...)
> >
>
> Is it really worth it? People can just use the definitions as written
> in drm_fourcc.h. I agree that we should have the high bits described
> in a comment though.
>
> > And then shift that to the correct place. Probably want an
> >
> > ARM_MODIFIER_ENCODE(space_idx, flags) macro which assembles everything.
> >
> > >         ...
> > >
> > > 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.
> >
> > Yeah that's what I meant. Also better to explicitly reserve this, i.e.
> >
> > #define ARM_FBC_MODIFIER_SPACE 0
> > #define ARM_MISC_MODIFIER_SPACE 1
> >
> > and then encode with the mirror trickery.
> >
>
> I don't really see the value in that either, it's just giving
> userspace the opportunity to depend on more stuff: more future
> headaches. So long as the 64-bit values are stable, that should be
> enough.

If you think you need to save the few bits this potentially saves you
over just encoding 8bit enum like in Qiang's original patch I think
you get to type a few macros and comments ...

> > > 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 ;-)
> >
> > :-) I mean full code exists in panfrost/lima, so this won't change
> > anything really ...
>
> Yeah, so for us to provide a more detailed description would require
> another lengthy loop through our legal approval process, and I'm not
> sure we can make a strong business case (which is what we need) for
> why this is needed.
>
> Of course, if someone happens to know the layout and wants to
> contribute to this file... Then I don't know how ack/r-b would work in
> that case, but I imagine the subsystem maintainer(s) might take issue
> with us attempting to block that contribution.

Well can't really take a modifier without knowing what it's for, I
guess this is up to lima/panfrost folks then to figure out :-P
-Daniel

>
> Thanks,
> -Brian
>
> >
> > Cheers, Daniel
> >
> > >
> > > -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
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Mon, Jun 24, 2019 at 1:23 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> On Mon, Jun 24, 2019 at 11:58:59AM +0200, Daniel Vetter wrote:
> > On Mon, Jun 24, 2019 at 11:32 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > >
> > > 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@arm.com> 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@arm.com>
> > > > > ---
> > > > >  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
> >
> > Uh, so the index would be encoded with least-significant bit first,
> > starting from bit55 downwards?
>
> Yeah.
>
> > Clever idea, but I think this needs a
> > macro (or at least a comment). Not sure there's a ready-made bitmask
> > mirror function for this stuff, works case we can hand-code it and
> > extend every time we need one more bit encoded. Something like:
> >
> > MIRROR_U32((u & (BIT(0)) << 31 | (u & BIT(1) << 30 | ...)
> >
>
> Is it really worth it? People can just use the definitions as written
> in drm_fourcc.h. I agree that we should have the high bits described
> in a comment though.
>
> > And then shift that to the correct place. Probably want an
> >
> > ARM_MODIFIER_ENCODE(space_idx, flags) macro which assembles everything.
> >
> > >         ...
> > >
> > > 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.
> >
> > Yeah that's what I meant. Also better to explicitly reserve this, i.e.
> >
> > #define ARM_FBC_MODIFIER_SPACE 0
> > #define ARM_MISC_MODIFIER_SPACE 1
> >
> > and then encode with the mirror trickery.
> >
>
> I don't really see the value in that either, it's just giving
> userspace the opportunity to depend on more stuff: more future
> headaches. So long as the 64-bit values are stable, that should be
> enough.

If you think you need to save the few bits this potentially saves you
over just encoding 8bit enum like in Qiang's original patch I think
you get to type a few macros and comments ...

> > > 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 ;-)
> >
> > :-) I mean full code exists in panfrost/lima, so this won't change
> > anything really ...
>
> Yeah, so for us to provide a more detailed description would require
> another lengthy loop through our legal approval process, and I'm not
> sure we can make a strong business case (which is what we need) for
> why this is needed.
>
> Of course, if someone happens to know the layout and wants to
> contribute to this file... Then I don't know how ack/r-b would work in
> that case, but I imagine the subsystem maintainer(s) might take issue
> with us attempting to block that contribution.

Well can't really take a modifier without knowing what it's for, I
guess this is up to lima/panfrost folks then to figure out :-P
-Daniel

>
> Thanks,
> -Brian
>
> >
> > Cheers, Daniel
> >
> > >
> > > -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
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch



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

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

[Qiang Yu]

Hi guys,

I'd like to know the status of this patch? I expect a v2 adding some
comments/macros about the high bit plan would be enough?

@Raymond & @Brian do you still need another long process to send out a
v2 patch? If so, I can help to prepare a v2 patch according to your
previous mail.

Thanks,
Qiang

On Thu, Jun 27, 2019 at 3:30 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jun 24, 2019 at 1:23 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> >
> > On Mon, Jun 24, 2019 at 11:58:59AM +0200, Daniel Vetter wrote:
> > > On Mon, Jun 24, 2019 at 11:32 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > > >
> > > > 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@arm.com> 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@arm.com>
> > > > > > ---
> > > > > >  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
> > >
> > > Uh, so the index would be encoded with least-significant bit first,
> > > starting from bit55 downwards?
> >
> > Yeah.
> >
> > > Clever idea, but I think this needs a
> > > macro (or at least a comment). Not sure there's a ready-made bitmask
> > > mirror function for this stuff, works case we can hand-code it and
> > > extend every time we need one more bit encoded. Something like:
> > >
> > > MIRROR_U32((u & (BIT(0)) << 31 | (u & BIT(1) << 30 | ...)
> > >
> >
> > Is it really worth it? People can just use the definitions as written
> > in drm_fourcc.h. I agree that we should have the high bits described
> > in a comment though.
> >
> > > And then shift that to the correct place. Probably want an
> > >
> > > ARM_MODIFIER_ENCODE(space_idx, flags) macro which assembles everything.
> > >
> > > >         ...
> > > >
> > > > 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.
> > >
> > > Yeah that's what I meant. Also better to explicitly reserve this, i.e.
> > >
> > > #define ARM_FBC_MODIFIER_SPACE 0
> > > #define ARM_MISC_MODIFIER_SPACE 1
> > >
> > > and then encode with the mirror trickery.
> > >
> >
> > I don't really see the value in that either, it's just giving
> > userspace the opportunity to depend on more stuff: more future
> > headaches. So long as the 64-bit values are stable, that should be
> > enough.
>
> If you think you need to save the few bits this potentially saves you
> over just encoding 8bit enum like in Qiang's original patch I think
> you get to type a few macros and comments ...
>
> > > > 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 ;-)
> > >
> > > :-) I mean full code exists in panfrost/lima, so this won't change
> > > anything really ...
> >
> > Yeah, so for us to provide a more detailed description would require
> > another lengthy loop through our legal approval process, and I'm not
> > sure we can make a strong business case (which is what we need) for
> > why this is needed.
> >
> > Of course, if someone happens to know the layout and wants to
> > contribute to this file... Then I don't know how ack/r-b would work in
> > that case, but I imagine the subsystem maintainer(s) might take issue
> > with us attempting to block that contribution.
>
> Well can't really take a modifier without knowing what it's for, I
> guess this is up to lima/panfrost folks then to figure out :-P
> -Daniel
>
> >
> > Thanks,
> > -Brian
> >
> > >
> > > Cheers, Daniel
> > >
> > > >
> > > > -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
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Qiang Yu]

Hi guys,

I'd like to know the status of this patch? I expect a v2 adding some
comments/macros about the high bit plan would be enough?

@Raymond & @Brian do you still need another long process to send out a
v2 patch? If so, I can help to prepare a v2 patch according to your
previous mail.

Thanks,
Qiang

On Thu, Jun 27, 2019 at 3:30 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jun 24, 2019 at 1:23 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> >
> > On Mon, Jun 24, 2019 at 11:58:59AM +0200, Daniel Vetter wrote:
> > > On Mon, Jun 24, 2019 at 11:32 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > > >
> > > > 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@arm.com> 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@arm.com>
> > > > > > ---
> > > > > >  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
> > >
> > > Uh, so the index would be encoded with least-significant bit first,
> > > starting from bit55 downwards?
> >
> > Yeah.
> >
> > > Clever idea, but I think this needs a
> > > macro (or at least a comment). Not sure there's a ready-made bitmask
> > > mirror function for this stuff, works case we can hand-code it and
> > > extend every time we need one more bit encoded. Something like:
> > >
> > > MIRROR_U32((u & (BIT(0)) << 31 | (u & BIT(1) << 30 | ...)
> > >
> >
> > Is it really worth it? People can just use the definitions as written
> > in drm_fourcc.h. I agree that we should have the high bits described
> > in a comment though.
> >
> > > And then shift that to the correct place. Probably want an
> > >
> > > ARM_MODIFIER_ENCODE(space_idx, flags) macro which assembles everything.
> > >
> > > >         ...
> > > >
> > > > 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.
> > >
> > > Yeah that's what I meant. Also better to explicitly reserve this, i.e.
> > >
> > > #define ARM_FBC_MODIFIER_SPACE 0
> > > #define ARM_MISC_MODIFIER_SPACE 1
> > >
> > > and then encode with the mirror trickery.
> > >
> >
> > I don't really see the value in that either, it's just giving
> > userspace the opportunity to depend on more stuff: more future
> > headaches. So long as the 64-bit values are stable, that should be
> > enough.
>
> If you think you need to save the few bits this potentially saves you
> over just encoding 8bit enum like in Qiang's original patch I think
> you get to type a few macros and comments ...
>
> > > > 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 ;-)
> > >
> > > :-) I mean full code exists in panfrost/lima, so this won't change
> > > anything really ...
> >
> > Yeah, so for us to provide a more detailed description would require
> > another lengthy loop through our legal approval process, and I'm not
> > sure we can make a strong business case (which is what we need) for
> > why this is needed.
> >
> > Of course, if someone happens to know the layout and wants to
> > contribute to this file... Then I don't know how ack/r-b would work in
> > that case, but I imagine the subsystem maintainer(s) might take issue
> > with us attempting to block that contribution.
>
> Well can't really take a modifier without knowing what it's for, I
> guess this is up to lima/panfrost folks then to figure out :-P
> -Daniel
>
> >
> > Thanks,
> > -Brian
> >
> > >
> > > Cheers, Daniel
> > >
> > > >
> > > > -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
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Ayan Halder]

On Fri, Sep 20, 2019 at 10:15:41AM +0800, Qiang Yu wrote:
> Hi guys,
> 
> I'd like to know the status of this patch? I expect a v2 adding some
> comments/macros about the high bit plan would be enough?
> 
> @Raymond & @Brian do you still need another long process to send out a
> v2 patch? If so, I can help to prepare a v2 patch according to your
> previous mail.

Apologies for the long wait.
@Raymond has left the company, so now I will be looking into it. I
will respin the patch in a day or two.
 
> Thanks,
> Qiang
> 
> On Thu, Jun 27, 2019 at 3:30 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jun 24, 2019 at 1:23 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > >
> > > On Mon, Jun 24, 2019 at 11:58:59AM +0200, Daniel Vetter wrote:
> > > > On Mon, Jun 24, 2019 at 11:32 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > > > >
> > > > > 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@arm.com> 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@arm.com>
> > > > > > > ---
> > > > > > >  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
> > > >
> > > > Uh, so the index would be encoded with least-significant bit first,
> > > > starting from bit55 downwards?
> > >
> > > Yeah.
> > >
> > > > Clever idea, but I think this needs a
> > > > macro (or at least a comment). Not sure there's a ready-made bitmask
> > > > mirror function for this stuff, works case we can hand-code it and
> > > > extend every time we need one more bit encoded. Something like:
> > > >
> > > > MIRROR_U32((u & (BIT(0)) << 31 | (u & BIT(1) << 30 | ...)
> > > >
> > >
> > > Is it really worth it? People can just use the definitions as written
> > > in drm_fourcc.h. I agree that we should have the high bits described
> > > in a comment though.
> > >
> > > > And then shift that to the correct place. Probably want an
> > > >
> > > > ARM_MODIFIER_ENCODE(space_idx, flags) macro which assembles everything.
> > > >
> > > > >         ...
> > > > >
> > > > > 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.
> > > >
> > > > Yeah that's what I meant. Also better to explicitly reserve this, i.e.
> > > >
> > > > #define ARM_FBC_MODIFIER_SPACE 0
> > > > #define ARM_MISC_MODIFIER_SPACE 1
> > > >
> > > > and then encode with the mirror trickery.
> > > >
> > >
> > > I don't really see the value in that either, it's just giving
> > > userspace the opportunity to depend on more stuff: more future
> > > headaches. So long as the 64-bit values are stable, that should be
> > > enough.
> >
> > If you think you need to save the few bits this potentially saves you
> > over just encoding 8bit enum like in Qiang's original patch I think
> > you get to type a few macros and comments ...
> >
> > > > > 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 ;-)
> > > >
> > > > :-) I mean full code exists in panfrost/lima, so this won't change
> > > > anything really ...
> > >
> > > Yeah, so for us to provide a more detailed description would require
> > > another lengthy loop through our legal approval process, and I'm not
> > > sure we can make a strong business case (which is what we need) for
> > > why this is needed.
> > >
> > > Of course, if someone happens to know the layout and wants to
> > > contribute to this file... Then I don't know how ack/r-b would work in
> > > that case, but I imagine the subsystem maintainer(s) might take issue
> > > with us attempting to block that contribution.
> >
> > Well can't really take a modifier without knowing what it's for, I
> > guess this is up to lima/panfrost folks then to figure out :-P
> > -Daniel
> >
> > >
> > > Thanks,
> > > -Brian
> > >
> > > >
> > > > Cheers, Daniel
> > > >
> > > > >
> > > > > -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
> > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ayan Halder]

On Fri, Sep 20, 2019 at 10:15:41AM +0800, Qiang Yu wrote:
> Hi guys,
> 
> I'd like to know the status of this patch? I expect a v2 adding some
> comments/macros about the high bit plan would be enough?
> 
> @Raymond & @Brian do you still need another long process to send out a
> v2 patch? If so, I can help to prepare a v2 patch according to your
> previous mail.

Apologies for the long wait.
@Raymond has left the company, so now I will be looking into it. I
will respin the patch in a day or two.
 
> Thanks,
> Qiang
> 
> On Thu, Jun 27, 2019 at 3:30 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jun 24, 2019 at 1:23 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > >
> > > On Mon, Jun 24, 2019 at 11:58:59AM +0200, Daniel Vetter wrote:
> > > > On Mon, Jun 24, 2019 at 11:32 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > > > >
> > > > > 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@arm.com> 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@arm.com>
> > > > > > > ---
> > > > > > >  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
> > > >
> > > > Uh, so the index would be encoded with least-significant bit first,
> > > > starting from bit55 downwards?
> > >
> > > Yeah.
> > >
> > > > Clever idea, but I think this needs a
> > > > macro (or at least a comment). Not sure there's a ready-made bitmask
> > > > mirror function for this stuff, works case we can hand-code it and
> > > > extend every time we need one more bit encoded. Something like:
> > > >
> > > > MIRROR_U32((u & (BIT(0)) << 31 | (u & BIT(1) << 30 | ...)
> > > >
> > >
> > > Is it really worth it? People can just use the definitions as written
> > > in drm_fourcc.h. I agree that we should have the high bits described
> > > in a comment though.
> > >
> > > > And then shift that to the correct place. Probably want an
> > > >
> > > > ARM_MODIFIER_ENCODE(space_idx, flags) macro which assembles everything.
> > > >
> > > > >         ...
> > > > >
> > > > > 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.
> > > >
> > > > Yeah that's what I meant. Also better to explicitly reserve this, i.e.
> > > >
> > > > #define ARM_FBC_MODIFIER_SPACE 0
> > > > #define ARM_MISC_MODIFIER_SPACE 1
> > > >
> > > > and then encode with the mirror trickery.
> > > >
> > >
> > > I don't really see the value in that either, it's just giving
> > > userspace the opportunity to depend on more stuff: more future
> > > headaches. So long as the 64-bit values are stable, that should be
> > > enough.
> >
> > If you think you need to save the few bits this potentially saves you
> > over just encoding 8bit enum like in Qiang's original patch I think
> > you get to type a few macros and comments ...
> >
> > > > > 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 ;-)
> > > >
> > > > :-) I mean full code exists in panfrost/lima, so this won't change
> > > > anything really ...
> > >
> > > Yeah, so for us to provide a more detailed description would require
> > > another lengthy loop through our legal approval process, and I'm not
> > > sure we can make a strong business case (which is what we need) for
> > > why this is needed.
> > >
> > > Of course, if someone happens to know the layout and wants to
> > > contribute to this file... Then I don't know how ack/r-b would work in
> > > that case, but I imagine the subsystem maintainer(s) might take issue
> > > with us attempting to block that contribution.
> >
> > Well can't really take a modifier without knowing what it's for, I
> > guess this is up to lima/panfrost folks then to figure out :-P
> > -Daniel
> >
> > >
> > > Thanks,
> > > -Brian
> > >
> > > >
> > > > Cheers, Daniel
> > > >
> > > > >
> > > > > -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
> > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch