lkml.org 
[lkml]   [2019]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] drm/fourcc: Add Arm 16x16 block modifier
    Date
    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

    \
     
     \ /
      Last update: 2019-06-24 13:24    [W:3.608 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site