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>
In-Reply-To: <CAKMK7uEjh+GrSy5AgbVLVQd1S5oJ8KFiWEUmxtMMVdcMSBtdCQ@mail.gmail.com>
From:   Qiang Yu <yuq825@gmail.com>
Date:   Sun, 23 Jun 2019 11:18:10 +0800
Message-ID: <CAKGbVbs2UB-nfwUAJxBhyuVg22a39Tx5BraUBvB5N7+PT=eQLA@mail.gmail.com>
Subject: Re: [PATCH] drm/fourcc: Add Arm 16x16 block modifier
To:     Daniel Vetter <daniel@ffwll.ch>
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>,
        Ayan Halder <Ayan.Halder@arm.com>,
        Brian Starkey <Brian.Starkey@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 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