| Message ID | 20190206131457.1072-2-yuq825@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Lima DRM driver | expand |
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. 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
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
On Thu, Feb 14, 2019 at 11:27 PM Brian Starkey <Brian.Starkey@arm.com> 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). Just for conformation, is the non-compressed tile format + BLOCK_SIZE_16x16 super block format same as mali gpu used tile format? So what name would you suggest to name this mali gpu tile format? Or maybe it's better that you can send out a patch to add the tile format (better also other ones) and how it's assembled with super block format in the modifier, as I don't have a whole picture of this. > > 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. > Yes we need it. It's used in the userspace mesa driver and applications which need to share mali driver allocated buffers across process. So that one process can tell another process which format to interpret the passed buffer. Thanks, Qiang
On Fri, Feb 15, 2019 at 09:48:47AM +0800, Qiang Yu wrote: > On Thu, Feb 14, 2019 at 11:27 PM Brian Starkey <Brian.Starkey@arm.com> 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). > Just for conformation, is the non-compressed tile format + BLOCK_SIZE_16x16 > super block format same as mali gpu used tile format? > > So what name would you suggest to name this mali gpu tile format? > Or maybe it's better that you can send out a patch to add the tile > format (better also other ones) and how it's assembled with super block > format in the modifier, as I don't have a whole picture of this. Honestly, you know more about it than I do (ironic, I know). I'm not working on Mali GPUs and I've never run anything on Utgard. All I really know is that what you're describing here definitely _isn't_ AFBC. I've asked internally, so if I have anything useful to add, I will update you. If we can get a patch from Arm, I agree that would be a good solution. My (personal) main concern here is only that you don't conflict with AFBC. > > > > > 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. > > > Yes we need it. It's used in the userspace mesa driver and applications > which need to share mali driver allocated buffers across process. So > that one process can tell another process which format to interpret the > passed buffer. OK, thanks to both of you for educating me :-) Cheers, -Brian > > Thanks, > Qiang
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) + #if defined(__cplusplus) } #endif
Signed-off-by: Qiang Yu <yuq825@gmail.com> --- include/uapi/drm/drm_fourcc.h | 9 +++++++++ 1 file changed, 9 insertions(+)