| Message ID | 1561112433-5308-1-git-send-email-raymond.smith@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fourcc: Add Arm 16x16 block modifier | expand |
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 >
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 >
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
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
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
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
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
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
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
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,
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(+)