linux-kernel - Re: [PATCH v2] drm/fourcc: Add Arm 16x16 block modifier

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191001091920.GA28248@arm.com>
Date:   Tue, 1 Oct 2019 09:19:21 +0000
From:   Ayan Halder <Ayan.Halder@....com>
To:     Brian Starkey <Brian.Starkey@....com>
CC:     Liviu Dudau <Liviu.Dudau@....com>,
        "malidp@...s.arm.com" <malidp@...s.arm.com>,
        "airlied@...ux.ie" <airlied@...ux.ie>,
        "maarten.lankhorst@...ux.intel.com" 
        <maarten.lankhorst@...ux.intel.com>,
        "maxime.ripard@...tlin.com" <maxime.ripard@...tlin.com>,
        "sean@...rly.run" <sean@...rly.run>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "yuq825@...il.com" <yuq825@...il.com>, nd <nd@....com>,
        Raymond Smith <Raymond.Smith@....com>
Subject: Re: [PATCH v2] drm/fourcc: Add Arm 16x16 block modifier

On Mon, Sep 30, 2019 at 05:02:37PM +0000, Brian Starkey wrote:
> Hi Ayan,
> 
> Could we preserve Ray's authorship on this patch?
Apologies for this, I will definitely preserve Ray's authorship in the
v3 patch.

> 
> On Mon, Sep 30, 2019 at 04:44:38PM +0000, Ayan Halder 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.
> > 
> > Changes from v1:-
> > 1. Reserved the upper four bits (out of the 56 bits assigned to each vendor)
> > to denote the category of Arm specific modifiers. Currently, we have two
> > categories ie AFBC and MISC.
> > 
> > Signed-off-by: Raymond Smith <raymond.smith@....com>
> > Signed-off-by: Ayan kumar halder <ayan.halder@....com>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 3feeaa3f987a..b1d3de961109 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -648,7 +648,21 @@ extern "C" {
> >   * Further information on the use of AFBC modifiers can be found in
> >   * Documentation/gpu/afbc.rst
> >   */
> > -#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode)	fourcc_mod_code(ARM, __afbc_mode)
> > +
> > +/*
> > + * The top 4 bits (out of the 56 bits alloted for specifying vendor specific
> > + * modifiers) denote the category for modifiers. Currently we have only two
> > + * categories of modifiers ie AFBC and MISC. We can have a maximum of sixteen
> > + * different categories.
> > + */
> 
> Yeah, this makes more sense than getting crazy with saving bits. Sorry
> Qiang/Daniel for not just going with this in the first instance when
> you both suggested it.
> 
> > +#define DRM_FORMAT_MOD_ARM_CODE(type, val) \
> > +	fourcc_mod_code(ARM, ((__u64)type << 52) | ((val) & 0x000fffffffffffffULL))
> 
> Not a big deal, but I might prefix type and val with "__" to reduce
> the chance of name collisions with code using the macro:
> DRM_FORMAT_MOD_ARM_CODE(__type, __val).
> 
> > +
> > +#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00
> > +#define DRM_FORMAT_MOD_ARM_TYPE_MISC 0x01
> > +
> > +#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \
> > +	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode)
> >  
> >  /*
> >   * AFBC superblock size
> > @@ -742,6 +756,17 @@ 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 \
> > +	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_MISC, 1ULL)
> > +
> > +
> 
> I think you can drop this newline, there's no extra space between any
> of the other definitions.
> 
> With this line dropped, and if you fix up the author:
> 
> Reviewed-by: Brian Starkey <brian.starkey@....com>
> 
> Thanks for the respin,

I will wait for daniel@...ll.ch and yuq825@...il.com comments before
respinning the patch.

> -Brian
> 
> >  /*
> >   * Allwinner tiled modifier
> >   *
> > -- 
> > 2.23.0
> >