Re: [PATCH 1/2] drm/fourcc: add ARM tiled format modifier

From: Daniel Vetter <daniel@ffwll.ch>
To: Brian Starkey <Brian.Starkey@arm.com>
Cc: Marek Vasut <marex@denx.de>, Simon Shields <simon@lineageos.org>,
	"lima@lists.freedesktop.org" <lima@lists.freedesktop.org>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Vasily Khoruzhick <anarsoul@gmail.com>,
	David Airlie <airlied@linux.ie>, Qiang Yu <yuq825@gmail.com>,
	nd <nd@arm.com>, Sean Paul <sean@poorly.run>,
	Andreas Baierl <ichgeh@imkreisrum.de>,
	Erico Nunes <nunes.erico@gmail.com>
Subject: Re: [PATCH 1/2] drm/fourcc: add ARM tiled format modifier
Date: Thu, 14 Feb 2019 16:34:55 +0100	[thread overview]
Message-ID: <20190214153455.GG2665@phenom.ffwll.local> (raw)
In-Reply-To: <20190214152700.e7f42dfeniu2oirn@DESKTOP-E1NTVVP.localdomain>

On Thu, Feb 14, 2019 at 03:26:59PM +0000, Brian Starkey wrote:
> Hi,
> 
> On Wed, Feb 06, 2019 at 09:14:56PM +0800, Qiang Yu wrote:
> > Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 0b44260a5ee9..953b59eb3fd2 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -644,6 +644,15 @@ extern "C" {
> >   */
> >  #define AFBC_FORMAT_MOD_SC      (1ULL <<  9)
> >  
> > +/*
> > + * ARM tiled format
> > + *
> > + * This is used by ARM Mali Utgard/Midgard GPU. It divides buffer into
> > + * 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
> > + * in the block are reordered.
> > + */
> > +#define DRM_FORMAT_MOD_ARM_TILED fourcc_mod_code(ARM, 1)
> > +
> 
> This conflicts with the already-defined AFBC modifiers. It has the
> same value as:
> 
> DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16);
> 
> I would suggest we use the top bit(s) to namespace between the
> different types of tiling formats Arm has (e.g. AFBC vs
> non-compressed).
> 
> I think this would be an internal-only layout though. Do we need
> modifiers for internal-only formats? I thought they were mainly for
> sharing cross-device. I also didn't see this macro getting used
> anywhere in the driver; I suppose at a minimum you should validate the
> modifier value provided by userspace.

Modifiers are also used in dri2/3 and wayland protocols, plus for buffer
sharing on the gl side. And for render drivers it's pretty common to not
validate at all what userspace is doing with it's buffers, as long as you
can make sure it can't escape it's sandbox (iommu pagetables generally
considered good enough, better if per-process).
-Daniel

> 
> Thanks,
> -Brian
> 
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel