| Message ID | 20190314111348.2218-1-yuq825@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/fourcc: add ARM GPU tile modifier | expand |
"AGTB" is jargon-y, but then again, so is "AFBC"... Unless Arm wants to
publish the actual name for the format, this works :)
Thank you for the clarification (in the other emails)
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Hello Qiang, Sorry for weighing in late on this, I've quite a backlog to work through. Since your first patch set, I did raise this internally. The request has been making it's way through: - GPU engineering, to determine what exactly this format is, and what other variants there might be which are supported on different GPU generations, so that we can determine a sane naming convention. - Our legal team, to determine what detail we are happy to share from an IP perspective. I can't imagine there being an issue here, but process is process, and there's not a lot I can do to move that along. There was talk on the legal side last week. I will ask for an update. I realise this is taking a very long time, and for that I can only apologise; I really am trying to get you an answer. On Thu, Mar 14, 2019 at 07:13:48PM +0800, Qiang Yu wrote: > v2: separate AFBC and GPU encoding > > v3: rename device to type and GPU modifer name > > Cc: Brian Starkey <Brian.Starkey@arm.com> > Cc: Rob Herring <robh@kernel.org> > Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> > Cc: Ayan Halder <Ayan.Halder@arm.com> > Signed-off-by: Qiang Yu <yuq825@gmail.com> > --- > include/uapi/drm/drm_fourcc.h | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > index 9fa7cf7bb274..f80a675cb09a 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -601,6 +601,19 @@ extern "C" { > */ > #define DRM_FORMAT_MOD_BROADCOM_UIF fourcc_mod_code(BROADCOM, 6) > > +/* > + * Arm Buffer Layout Type Code > + * > + * Arm has multiple types of buffer layout which are not totally shared > + * across devices, so add a type field at the MSB of the format field > + * to separate each type's encoding. > + */ > +#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00 > +#define DRM_FORMAT_MOD_ARM_TYPE_AGTB 0x01 > + I don't think we need a whole byte here. We can just keep adding individual bits as needed. In any case, I'm not that keen on adding this "AGTB" category. That effectively reserves 55 (or 47) bits just to describe a single layout. We don't know if this is a "category" per-se, or just a single Utgard tiling format - as discussed I'm trying to get an answer for that. AFBC only uses bit-fields and the "__afbc_mode" helper macro because it has many different "features" which can be combined quite freely. That leads to combinatorial expansion, so when a new feature is introduced, this can approximately double the list of possible AFBC modifiers. That's rather unmanageable if we list them all exhaustively. For other layouts which don't have this kind of combinatorial effect, I'd rather have #defines for the specific layouts, all individually setting the top bit, without giving that top bit any kind of "name". The top bit would effectively mean "not AFBC", rather than meaning "AGTB", allowing us to use the lower bits more freely. In the future, maybe Arm comes up with another layout with tons of combinatorial variants (let's call it "AFBC2") - in which case I think we'd add a new "__afbc2_mode" helper macro, which sets the top *two* bits - but *only* if listing all of the AFBC2 variants directly wasn't practical. If you can hang on a while longer, I hope Arm can push a patch which you can just use directly, and then handling the bit assignments will be our mess rather than yours :-) Thanks, -Brian > +#define DRM_FORMAT_MOD_ARM_CODE(type, val) \ > + fourcc_mod_code(ARM, ((__u64)type << 48) | ((val) & 0x0000ffffffffffffULL)) > + > /* > * Arm Framebuffer Compression (AFBC) modifiers > * > @@ -615,7 +628,8 @@ 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) > +#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \ > + DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode) > > /* > * AFBC superblock size > @@ -709,6 +723,21 @@ extern "C" { > */ > #define AFBC_FORMAT_MOD_BCH (1ULL << 11) > > +/* > + * Arm Graphics Tiled Buffer (AGTB) modifiers > + */ > +#define DRM_FORMAT_MOD_ARM_AGTB(mode) \ > + DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AGTB, mode) > + > +/* > + * AGTB mode 0 modifier > + * > + * 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_AGTB_MODE0 DRM_FORMAT_MOD_ARM_AGTB(1) > + > /* > * Allwinner tiled modifier > * > -- > 2.17.1 >
Hi Brian, > Since your first patch set, I did raise this internally. The request > has been making it's way through: > > - GPU engineering, to determine what exactly this format is, and > what other variants there might be which are supported on different > GPU generations, so that we can determine a sane naming convention. > > - Our legal team, to determine what detail we are happy to share from > an IP perspective. I can't imagine there being an issue here, but > process is process, and there's not a lot I can do to move that > along. > > There was talk on the legal side last week. I will ask for an update. > I realise this is taking a very long time, and for that I can only > apologise; I really am trying to get you an answer. Thanks for the update and your effort to make a generic solution on the ARM side. It's really some time passed since last time we talked about this, but I think it's worth to wait for more time to get a reliable solution with more internal information known by us. > For other layouts which don't have this kind of combinatorial effect, > I'd rather have #defines for the specific layouts, all individually > setting the top bit, without giving that top bit any kind of "name". > The top bit would effectively mean "not AFBC", rather than meaning > "AGTB", allowing us to use the lower bits more freely. > Seems like another kind of category, but with different meaning. > If you can hang on a while longer, I hope Arm can push a patch which > you can just use directly, and then handling the bit assignments will > be our mess rather than yours :-) > That's better, I can wait :-) Thanks, Qiang > > +#define DRM_FORMAT_MOD_ARM_CODE(type, val) \ > > + fourcc_mod_code(ARM, ((__u64)type << 48) | ((val) & 0x0000ffffffffffffULL)) > > + > > /* > > * Arm Framebuffer Compression (AFBC) modifiers > > * > > @@ -615,7 +628,8 @@ 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) > > +#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \ > > + DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode) > > > > /* > > * AFBC superblock size > > @@ -709,6 +723,21 @@ extern "C" { > > */ > > #define AFBC_FORMAT_MOD_BCH (1ULL << 11) > > > > +/* > > + * Arm Graphics Tiled Buffer (AGTB) modifiers > > + */ > > +#define DRM_FORMAT_MOD_ARM_AGTB(mode) \ > > + DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AGTB, mode) > > + > > +/* > > + * AGTB mode 0 modifier > > + * > > + * 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_AGTB_MODE0 DRM_FORMAT_MOD_ARM_AGTB(1) > > + > > /* > > * Allwinner tiled modifier > > * > > -- > > 2.17.1 > >
On Tue, Mar 19, 2019 at 08:05:32PM +0800, Qiang Yu wrote: > Hi Brian, > > > Since your first patch set, I did raise this internally. The request > > has been making it's way through: > > > > - GPU engineering, to determine what exactly this format is, and > > what other variants there might be which are supported on different > > GPU generations, so that we can determine a sane naming convention. > > > > - Our legal team, to determine what detail we are happy to share from > > an IP perspective. I can't imagine there being an issue here, but > > process is process, and there's not a lot I can do to move that > > along. > > > > There was talk on the legal side last week. I will ask for an update. > > I realise this is taking a very long time, and for that I can only > > apologise; I really am trying to get you an answer. > > Thanks for the update and your effort to make a generic solution on > the ARM side. It's really some time passed since last time we talked > about this, but I think it's worth to wait for more time to get a reliable > solution with more internal information known by us. Thank you for your patience :-) > > > For other layouts which don't have this kind of combinatorial effect, > > I'd rather have #defines for the specific layouts, all individually > > setting the top bit, without giving that top bit any kind of "name". > > The top bit would effectively mean "not AFBC", rather than meaning > > "AGTB", allowing us to use the lower bits more freely. > > > Seems like another kind of category, but with different meaning. Indeed, just a category without a name ;-) Thanks, -Brian
> We don't know if this is a "category" per-se, or just a single Utgard > tiling format - as discussed I'm trying to get an answer for that. FWIW, as I think I mentioned on an message, this format is used on Midgard as well, and presumably also Bifrost. On Midgard, when a texture is uploaded (just as a plain bitmap), the driver tiles it in this format and uploads that, to improve cache locality. That said, unlike on Lima/Utgard, Panfrost does not need this exposed in kernel space, since I don't believe the format is renderable on Midgard. Any time Qiang would need to import/export something in this tiled format, I would just use AFBC for the same effect + bw savings.
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 9fa7cf7bb274..f80a675cb09a 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -601,6 +601,19 @@ extern "C" { */ #define DRM_FORMAT_MOD_BROADCOM_UIF fourcc_mod_code(BROADCOM, 6) +/* + * Arm Buffer Layout Type Code + * + * Arm has multiple types of buffer layout which are not totally shared + * across devices, so add a type field at the MSB of the format field + * to separate each type's encoding. + */ +#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00 +#define DRM_FORMAT_MOD_ARM_TYPE_AGTB 0x01 + +#define DRM_FORMAT_MOD_ARM_CODE(type, val) \ + fourcc_mod_code(ARM, ((__u64)type << 48) | ((val) & 0x0000ffffffffffffULL)) + /* * Arm Framebuffer Compression (AFBC) modifiers * @@ -615,7 +628,8 @@ 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) +#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \ + DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode) /* * AFBC superblock size @@ -709,6 +723,21 @@ extern "C" { */ #define AFBC_FORMAT_MOD_BCH (1ULL << 11) +/* + * Arm Graphics Tiled Buffer (AGTB) modifiers + */ +#define DRM_FORMAT_MOD_ARM_AGTB(mode) \ + DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AGTB, mode) + +/* + * AGTB mode 0 modifier + * + * 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_AGTB_MODE0 DRM_FORMAT_MOD_ARM_AGTB(1) + /* * Allwinner tiled modifier *
v2: separate AFBC and GPU encoding v3: rename device to type and GPU modifer name Cc: Brian Starkey <Brian.Starkey@arm.com> Cc: Rob Herring <robh@kernel.org> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> Cc: Ayan Halder <Ayan.Halder@arm.com> Signed-off-by: Qiang Yu <yuq825@gmail.com> --- include/uapi/drm/drm_fourcc.h | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)