Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1561112433-5308-1-git-send-email-raymond.smith@arm.com>
 <CAKMK7uEjh+GrSy5AgbVLVQd1S5oJ8KFiWEUmxtMMVdcMSBtdCQ@mail.gmail.com> <20190624093233.73f3tcshewlbogli@DESKTOP-E1NTVVP.localdomain>
In-Reply-To: <20190624093233.73f3tcshewlbogli@DESKTOP-E1NTVVP.localdomain>
From:   Daniel Vetter <daniel@ffwll.ch>
Date:   Mon, 24 Jun 2019 11:58:59 +0200
Message-ID: <CAKMK7uG02qAqH8MMpE6kzRO99HTjnadTFDrY1vVr9RmAiFPvJA@mail.gmail.com>
Subject: Re: [PATCH] drm/fourcc: Add Arm 16x16 block modifier
To:     Brian Starkey <Brian.Starkey@arm.com>
Cc:     Raymond Smith <Raymond.Smith@arm.com>,
        "maarten.lankhorst@linux.intel.com" 
        <maarten.lankhorst@linux.intel.com>,
        "maxime.ripard@bootlin.com" <maxime.ripard@bootlin.com>,
        "sean@poorly.run" <sean@poorly.run>,
        "airlied@linux.ie" <airlied@linux.ie>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "yuq825@gmail.com" <yuq825@gmail.com>,
        Ayan Halder <Ayan.Halder@arm.com>,
        "malidp@foss.arm.com" <malidp@foss.arm.com>, nd <nd@arm.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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