On 22.08.2018 12:40, Daniel Vetter wrote:

On Tue, Aug 21, 2018 at 8:30 PM, Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com wrote:

...

Malidp implements a number of yuv buffer formats which are not currently described in drm_fourcc.h. This adds those definitions and describes their memory layout.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

We're working on adding at least some of these for i915 too. Adding the relevant people, to make sure we end up with agreeing drm_fourcc.h codes.

JP, Swati, please take a look at this and review.

Thanks, Daniel

...

---

drivers/gpu/drm/drm_fourcc.c | 7 +++++++ include/uapi/drm/drm_fourcc.h | 27 ++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index 35c1e2742c27..18bb960e9943 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -173,6 +173,13 @@ const struct drm_format_info *__drm_format_info(u32 format) { .format = DRM_FORMAT_UYVY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, { .format = DRM_FORMAT_VYUY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, { .format = DRM_FORMAT_AYUV, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true },

•           { .format = DRM_FORMAT_XYUV8888,        .depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

•           { .format = DRM_FORMAT_XVYU2101010,     .depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

•           { .format = DRM_FORMAT_Y0L0,            .depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
  

•           { .format = DRM_FORMAT_X0L0,            .depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

•           { .format = DRM_FORMAT_Y0L2,            .depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
  

•           { .format = DRM_FORMAT_X0L2,            .depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

•           { .format = DRM_FORMAT_P010,            .depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv  = true },
     };
  
     unsigned int i;
  

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 21c50b39596f..5e01fbcd7a30 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -141,13 +141,31 @@ extern "C" { #define DRM_FORMAT_RGBA1010102 fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 little endian */ #define DRM_FORMAT_BGRA1010102 fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 little endian */

-/* packed YCbCr */ +/* packed YCbCr422 */ #define DRM_FORMAT_YUYV fourcc_code('Y', 'U', 'Y', 'V') /* [31:0] Cr0:Y1:Cb0:Y0 8:8:8:8 little endian */ #define DRM_FORMAT_YVYU fourcc_code('Y', 'V', 'Y', 'U') /* [31:0] Cb0:Y1:Cr0:Y0 8:8:8:8 little endian */ #define DRM_FORMAT_UYVY fourcc_code('U', 'Y', 'V', 'Y') /* [31:0] Y1:Cr0:Y0:Cb0 8:8:8:8 little endian */ #define DRM_FORMAT_VYUY fourcc_code('V', 'Y', 'U', 'Y') /* [31:0] Y1:Cb0:Y0:Cr0 8:8:8:8 little endian */

+/* packed YCbCr444 */ #define DRM_FORMAT_AYUV fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */ +#define DRM_FORMAT_XYUV8888 fourcc_code('X', 'Y', 'U', 'V') /* [31:0] X:Y:Cb:Cr 8:8:8:8 little endian */ +#define DRM_FORMAT_XVYU2101010 fourcc_code('X', 'V', '3', '0') /* [31:0] X:Cr:Y:Cb 2:10:10:10 little endian */

+/*

• • packed YCbCr420 2x2 tiled formats
• • first 64 bits will contain Y,Cb,Cr components for a 2x2 tile
• */

+/* [63:0] A3:A2:Y3:0:Cr0:0:Y2:0:A1:A0:Y1:0:Cb0:0:Y0:0 1:1:8:2:8:2:8:2:1:1:8:2:8:2:8:2 little endian */ +#define DRM_FORMAT_Y0L0 fourcc_code('Y', '0', 'L', '0') +/* [63:0] X3:X2:Y3:0:Cr0:0:Y2:0:X1:X0:Y1:0:Cb0:0:Y0:0 1:1:8:2:8:2:8:2:1:1:8:2:8:2:8:2 little endian */ +#define DRM_FORMAT_X0L0 fourcc_code('X', '0', 'L', '0')

+/* [63:0] A3:A2:Y3:Cr0:Y2:A1:A0:Y1:Cb0:Y0 1:1:10:10:10:1:1:10:10:10 little endian */ +#define DRM_FORMAT_Y0L2 fourcc_code('Y', '0', 'L', '2') +/* [63:0] X3:X2:Y3:Cr0:Y2:X1:X0:Y1:Cb0:Y0 1:1:10:10:10:1:1:10:10:10 little endian */ +#define DRM_FORMAT_X0L2 fourcc_code('X', '0', 'L', '2')

/*

• 2 plane RGB + A

@@ -177,6 +195,13 @@ extern "C" { #define DRM_FORMAT_NV24 fourcc_code('N', 'V', '2', '4') /* non-subsampled Cr:Cb plane */ #define DRM_FORMAT_NV42 fourcc_code('N', 'V', '4', '2') /* non-subsampled Cb:Cr plane */

+/*

• • Each sample packed into the top 10 bits of a 16-bit word.
• • Y plane: [63:0] Y3:0:Y2:0:Y1:0:Y0:0, 10:6:10:6:10:6:10:6
• • CrCb plane: [63:0] Cr2:0:Cb2:0:Cr0:0:Cb0:0, 10:6:10:6:10:6:10:6
• */

There are more of these P01X formats than just P010 so above description should be written taking that into account. You can see my version here: https://lists.freedesktop.org/archives/dri-devel/2018-August/186590.html

Maybe you can use my patch for this part?

...

+#define DRM_FORMAT_P010 fourcc_code('P', '0', '1', '0') /* 2x2 subsampled Cr:Cb plane */

• /*
  • 3 plane YCbCr
  • index 0: Y plane, [7:0] Y

-- 2.18.0

/Juha-Pekka --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

On 22.08.2018 13:50, Alexandru-Cosmin Gheorghe wrote:

Hi,

On Wed, Aug 22, 2018 at 01:39:16PM +0300, Juha-Pekka Heikkilä wrote:

...

On 22.08.2018 12:40, Daniel Vetter wrote:

...

On Tue, Aug 21, 2018 at 8:30 PM, Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com wrote:

...

Malidp implements a number of yuv buffer formats which are not currently described in drm_fourcc.h. This adds those definitions and describes their memory layout.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

We're working on adding at least some of these for i915 too. Adding the relevant people, to make sure we end up with agreeing drm_fourcc.h codes.

JP, Swati, please take a look at this and review.

Thanks, Daniel

...

---

drivers/gpu/drm/drm_fourcc.c | 7 +++++++ include/uapi/drm/drm_fourcc.h | 27 ++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index 35c1e2742c27..18bb960e9943 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -173,6 +173,13 @@ const struct drm_format_info *__drm_format_info(u32 format) { .format = DRM_FORMAT_UYVY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, { .format = DRM_FORMAT_VYUY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, { .format = DRM_FORMAT_AYUV, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true },

•           { .format = DRM_FORMAT_XYUV8888,        .depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

•           { .format = DRM_FORMAT_XVYU2101010,     .depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

•           { .format = DRM_FORMAT_Y0L0,            .depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
  

•           { .format = DRM_FORMAT_X0L0,            .depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

•           { .format = DRM_FORMAT_Y0L2,            .depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
  

•           { .format = DRM_FORMAT_X0L2,            .depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

•           { .format = DRM_FORMAT_P010,            .depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv  = true },
     };
  
     unsigned int i;
  

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 21c50b39596f..5e01fbcd7a30 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -141,13 +141,31 @@ extern "C" { #define DRM_FORMAT_RGBA1010102 fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 little endian */ #define DRM_FORMAT_BGRA1010102 fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 little endian */

-/* packed YCbCr */ +/* packed YCbCr422 */ #define DRM_FORMAT_YUYV fourcc_code('Y', 'U', 'Y', 'V') /* [31:0] Cr0:Y1:Cb0:Y0 8:8:8:8 little endian */ #define DRM_FORMAT_YVYU fourcc_code('Y', 'V', 'Y', 'U') /* [31:0] Cb0:Y1:Cr0:Y0 8:8:8:8 little endian */ #define DRM_FORMAT_UYVY fourcc_code('U', 'Y', 'V', 'Y') /* [31:0] Y1:Cr0:Y0:Cb0 8:8:8:8 little endian */ #define DRM_FORMAT_VYUY fourcc_code('V', 'Y', 'U', 'Y') /* [31:0] Y1:Cb0:Y0:Cr0 8:8:8:8 little endian */

+/* packed YCbCr444 */ #define DRM_FORMAT_AYUV fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */ +#define DRM_FORMAT_XYUV8888 fourcc_code('X', 'Y', 'U', 'V') /* [31:0] X:Y:Cb:Cr 8:8:8:8 little endian */ +#define DRM_FORMAT_XVYU2101010 fourcc_code('X', 'V', '3', '0') /* [31:0] X:Cr:Y:Cb 2:10:10:10 little endian */

+/*

• • packed YCbCr420 2x2 tiled formats
• • first 64 bits will contain Y,Cb,Cr components for a 2x2 tile
• */

+/* [63:0] A3:A2:Y3:0:Cr0:0:Y2:0:A1:A0:Y1:0:Cb0:0:Y0:0 1:1:8:2:8:2:8:2:1:1:8:2:8:2:8:2 little endian */ +#define DRM_FORMAT_Y0L0 fourcc_code('Y', '0', 'L', '0') +/* [63:0] X3:X2:Y3:0:Cr0:0:Y2:0:X1:X0:Y1:0:Cb0:0:Y0:0 1:1:8:2:8:2:8:2:1:1:8:2:8:2:8:2 little endian */ +#define DRM_FORMAT_X0L0 fourcc_code('X', '0', 'L', '0')

+/* [63:0] A3:A2:Y3:Cr0:Y2:A1:A0:Y1:Cb0:Y0 1:1:10:10:10:1:1:10:10:10 little endian */ +#define DRM_FORMAT_Y0L2 fourcc_code('Y', '0', 'L', '2') +/* [63:0] X3:X2:Y3:Cr0:Y2:X1:X0:Y1:Cb0:Y0 1:1:10:10:10:1:1:10:10:10 little endian */ +#define DRM_FORMAT_X0L2 fourcc_code('X', '0', 'L', '2')

/*

• 2 plane RGB + A

@@ -177,6 +195,13 @@ extern "C" { #define DRM_FORMAT_NV24 fourcc_code('N', 'V', '2', '4') /* non-subsampled Cr:Cb plane */ #define DRM_FORMAT_NV42 fourcc_code('N', 'V', '4', '2') /* non-subsampled Cb:Cr plane */

+/*

• • Each sample packed into the top 10 bits of a 16-bit word.
• • Y plane: [63:0] Y3:0:Y2:0:Y1:0:Y0:0, 10:6:10:6:10:6:10:6
• • CrCb plane: [63:0] Cr2:0:Cb2:0:Cr0:0:Cb0:0, 10:6:10:6:10:6:10:6
• */

There are more of these P01X formats than just P010 so above description should be written taking that into account. You can see my version here: https://lists.freedesktop.org/archives/dri-devel/2018-August/186590.html

Maybe you can use my patch for this part?

Nothing against that, I'm more than happy to rebase my series when that gets merged.

I see your patch had been already reviewed, anything holding it back from getting merged in drm-misc-next ?

I don't think there's anything really blocking it. Nobody just acted on it and I don't have access to do that. :)

/Juha-Pekka

--------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

On Tue, Aug 21, 2018 at 07:30:01PM +0100, Alexandru Gheorghe wrote:

Add two new fields(tile_w and tile_h) to drm_format_info, which represent the horizontal and vertical sizes of a tile in tiled formats. This fields will be used by the next patch to add support in drm core for handling framebuffer restrictions and to correctly handle source cropping.

Additionally, since I was touching drm_format_info table I decided to break the lines to the 80 characters limit, they were already getting out of hand, but let's not focus on that, I don't have any problem going back to the original line sizes or style them other way, if someone wants me to.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

drivers/gpu/drm/drm_fourcc.c | 352 +++++++++++++++++++++++++++-------- include/drm/drm_fourcc.h | 4 + 2 files changed, 281 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index 18bb960e9943..f55cd93ba2d0 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -105,81 +105,283 @@ EXPORT_SYMBOL(drm_get_format_name); const struct drm_format_info *__drm_format_info(u32 format) { static const struct drm_format_info formats[] = {

• { .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_RGB332,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_BGR233,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_XRGB4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_XBGR4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_RGBX4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_BGRX4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_ARGB4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_ABGR4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_RGBA4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_BGRA4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_XRGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_XBGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_RGBX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_BGRX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_ARGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_ABGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_RGBA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_BGRA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_RGB565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_BGR565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_RGB888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_BGR888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_XRGB8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_XBGR8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_RGBX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_BGRX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_RGB565_A8,	.depth = 24, .num_planes = 2, .cpp = { 2, 1, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_BGR565_A8,	.depth = 24, .num_planes = 2, .cpp = { 2, 1, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_XRGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_XBGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_RGBX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_BGRX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
  

• { .format = DRM_FORMAT_ARGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_ABGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_RGBA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_BGRA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_ARGB8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_ABGR8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_RGBA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_BGRA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_RGB888_A8,	.depth = 32, .num_planes = 2, .cpp = { 3, 1, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_BGR888_A8,	.depth = 32, .num_planes = 2, .cpp = { 3, 1, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_XRGB8888_A8,	.depth = 32, .num_planes = 2, .cpp = { 4, 1, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_XBGR8888_A8,	.depth = 32, .num_planes = 2, .cpp = { 4, 1, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_RGBX8888_A8,	.depth = 32, .num_planes = 2, .cpp = { 4, 1, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_BGRX8888_A8,	.depth = 32, .num_planes = 2, .cpp = { 4, 1, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
  

• { .format = DRM_FORMAT_YUV410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4, .is_yuv = true },
  

• { .format = DRM_FORMAT_YVU410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4, .is_yuv = true },
  

• { .format = DRM_FORMAT_YUV411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_YVU411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_YUV420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

• { .format = DRM_FORMAT_YVU420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

• { .format = DRM_FORMAT_YUV422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_YVU422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_YUV444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_YVU444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_NV12,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

• { .format = DRM_FORMAT_NV21,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

• { .format = DRM_FORMAT_NV16,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_NV61,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_NV24,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_NV42,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_YUYV,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_YVYU,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_VYUY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_AYUV,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true },
  

• { .format = DRM_FORMAT_XYUV8888,	.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_XVYU2101010,	.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
  

• { .format = DRM_FORMAT_Y0L0,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
  

• { .format = DRM_FORMAT_X0L0,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

• { .format = DRM_FORMAT_Y0L2,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
  

• { .format = DRM_FORMAT_X0L2,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
  

• { .format = DRM_FORMAT_P010,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv  = true },
  

• { .format = DRM_FORMAT_C8,
  

• 		.depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_RGB332,
  

• 		.depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_BGR233,
  

• 		.depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_XRGB4444,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_XBGR4444,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_RGBX4444,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_BGRX4444,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_ARGB4444,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_ABGR4444,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_RGBA4444,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_BGRA4444,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_XRGB1555,
  

• 		.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_XBGR1555,
  

• 		.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_RGBX5551,
  

• 		.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_BGRX5551,
  

• 		.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1, },
  

• { .format = DRM_FORMAT_ARGB1555,
  

• 		.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_ABGR1555,
  

• 		.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_RGBA5551,
  

• 		.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_BGRA5551,
  

• 		.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_RGB565,
  

• 		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_BGR565,
  

• 		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_RGB888,
  

• 		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_BGR888,
  

• 		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_XRGB8888,
  

• 		.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_XBGR8888,
  

• 		.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_RGBX8888,
  

• 		.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_BGRX8888,
  

• 		.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_RGB565_A8,
  

• 		.depth = 24, .num_planes = 2, .cpp = { 2, 1, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_BGR565_A8,
  

• 		.depth = 24, .num_planes = 2, .cpp = { 2, 1, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_XRGB2101010,
  

• 		.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_XBGR2101010,
  

• 		.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_RGBX1010102,
  

• 		.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_BGRX1010102,
  

• 		.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1 },
  

• { .format = DRM_FORMAT_ARGB2101010,
  

• 		.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_ABGR2101010,
  

• 		.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_RGBA1010102,
  

• 		.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_BGRA1010102,
  

• 		.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_ARGB8888,
  

• 		.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_ABGR8888,
  

• 		.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_RGBA8888,
  

• 		.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_BGRA8888,
  

• 		.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_RGB888_A8,
  

• 		.depth = 32, .num_planes = 2, .cpp = { 3, 1, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_BGR888_A8,
  

• 		.depth = 32, .num_planes = 2, .cpp = { 3, 1, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_XRGB8888_A8,
  

• 		.depth = 32, .num_planes = 2, .cpp = { 4, 1, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_XBGR8888_A8,
  

• 		.depth = 32, .num_planes = 2, .cpp = { 4, 1, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_RGBX8888_A8,
  

• 		.depth = 32, .num_planes = 2, .cpp = { 4, 1, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_BGRX8888_A8,
  

• 		.depth = 32, .num_planes = 2, .cpp = { 4, 1, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true },
  

• { .format = DRM_FORMAT_YUV410,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 4, .vsub = 4, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YVU410,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 4, .vsub = 4, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YUV411,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 4, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YVU411,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 4, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YUV420,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 2, .vsub = 2, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YVU420,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 2, .vsub = 2, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YUV422,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 2, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YVU422,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 2, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YUV444,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YVU444,
  

• 		.depth = 0, .num_planes = 3, .cpp = { 1, 1, 1 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_NV12,
  

• 		.depth = 0, .num_planes = 2, .cpp = { 1, 2, 0 },
  

• 		.hsub = 2, .vsub = 2, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_NV21,
  

• 		.depth = 0, .num_planes = 2, .cpp = { 1, 2, 0 },
  

• 		.hsub = 2, .vsub = 2, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_NV16,
  

• 		.depth = 0, .num_planes = 2, .cpp = { 1, 2, 0 },
  

• 		.hsub = 2, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_NV61,
  

• 		.depth = 0, .num_planes = 2, .cpp = { 1, 2, 0 },
  

• 		.hsub = 2, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_NV24,
  

• 		.depth = 0, .num_planes = 2, .cpp = { 1, 2, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_NV42,
  

• 		.depth = 0, .num_planes = 2, .cpp = { 1, 2, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YUYV,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 2, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_YVYU,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 2, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_UYVY,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 2, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_VYUY,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 2, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_AYUV,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.has_alpha = true, .is_yuv = true },
  

• { .format = DRM_FORMAT_XYUV8888,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_XVYU2101010,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 },
  

• 		.hsub = 1, .vsub = 1, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_Y0L0,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 2, .vsub = 2, .tile_w = 2, .tile_h = 2,
  

• 		.has_alpha = true, .is_yuv = true },
  

• { .format = DRM_FORMAT_X0L0,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 2, .vsub = 2, .tile_w = 2, .tile_h = 2,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_Y0L2,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 2, .vsub = 2, .tile_w = 2, .tile_h = 2,
  

• 		.has_alpha = true, .is_yuv = true },
  

• { .format = DRM_FORMAT_X0L2,
  

• 		.depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 },
  

• 		.hsub = 2, .vsub = 2, .tile_w = 2, .tile_h = 2,
  

• 		.is_yuv = true },
  

• { .format = DRM_FORMAT_P010,
  

• 		.depth = 0, .num_planes = 2, .cpp = { 2, 4, 0 },
  

• 		.hsub = 2, .vsub = 2, .tile_w = 1, .tile_h = 1,
  

• 		.is_yuv  = true },
  

  };

  unsigned int i;

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index f9c15845f465..41681cf2b140 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -38,6 +38,8 @@ struct drm_mode_fb_cmd2;

• @cpp: Number of bytes per pixel (per plane)
• @hsub: Horizontal chroma subsampling factor
• @vsub: Vertical chroma subsampling factor

• • @tile_w: Width of a tile in pixels, for non-tiled formats this should be 1
• • @tile_h: Height of a tile in pixels, for non-tiled formats this should be 1

I think switching to inline struct members would be good. Also I think we need more details here about the units. Is this in pixels or in bytes? Also would be good to be really clear in how tiles related to cpp. Or should we want to redefine cpp as characters_per_tile?

I also think that we should encode that "0 means 1", simply for a more compact table. -Daniel

• @has_alpha: Does the format embeds an alpha component?
• @is_yuv: Is it a YUV format?

*/ @@ -48,6 +50,8 @@ struct drm_format_info { u8 cpp[3]; u8 hsub; u8 vsub;

• u8 tile_w;
• u8 tile_h; bool has_alpha; bool is_yuv;

};

2.18.0

On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote:

The previous patch added tile_w and tile_h, which represent the horizontal and vertical sizes of a tile.

This one uses that to plumb through drm core in order to be able to handle linear tile formats without the need for drivers to roll up their own implementation.

This patch had been written with Mali-dp X0L2 and X0L0 in mind which is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 bytes per pixel and where tiles are laid out in a linear manner.

Now what are the restrictions:

1. Pitch in bytes is expected to cover at least tile_h * width in

pixels. Due to this the places where the pitch is checked/used need to be updated to take into consideration the tile_w, tile_h and tile_size. tile_size = cpp * tile_w * tile_h

2. When doing source cropping plane_src_x/y need to be a multiple of

tile_w/tile_h and we need to take into consideration the tile_w/tile_h when computing the start address.

For all non-tiled formats the tile_w and tile_h will be 1, so if I didn't miss anything nothing should change.

Regarding multi-planar linear tile formats, I'm not sure how those should be handle I kind of assumed that tile_h/tile_w will have to be divided by horizontal/subsampling. Anyway, I think it's best to just put an warning in there and handle it when someone tries to add support for them.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

drivers/gpu/drm/drm_atomic.c | 8 +++ drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- include/drm/drm_fourcc.h | 2 + 6 files changed, 94 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e11e2e..7a3e893a4cd1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }

• /* Make sure source coordinates are a multiple of tile sizes */
• if ((state->src_x >> 16) % state->fb->format->tile_w ||

•    (state->src_y >> 16) % state->fb->format->tile_h) {
  

• DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions",
  

• 		 plane->base.id, plane->name);
  

• return -EINVAL;
  

• }

Feels rather wrong to me to put such hardware specific limitations into the core.

• if (plane_switching_crtc(state->state, plane, state)) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", plane->base.id, plane->name);

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..4d8052adce67 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, struct drm_gem_cma_object *obj; dma_addr_t paddr; u8 h_div = 1, v_div = 1;

• u32 tile_w = drm_format_tile_width(fb->format, plane);

• u32 tile_h = drm_format_tile_height(fb->format, plane);

  obj = drm_fb_cma_get_gem_obj(fb, plane); if (!obj)

@@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, v_div = fb->format->vsub; }

• paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
• paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;

• paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16))

• 	/ h_div;
  

• /*

• * For tile formats pitches are expected to cover at least
  

• * width * tile_h pixels
  

• */
  

• paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;

  return paddr;

} diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index f55cd93ba2d0..d6c9c5aa4036 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) return height / info->vsub; } EXPORT_SYMBOL(drm_format_plane_height);

+/**

• • drm_format_tile_width - width of a tile for tile formats, should be 1 for all
• • non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The width of a tile, depending on the plane index and horizontal sub-sampling
• */

+uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_w);
• if (plane == 0 || info->tile_w == 1)

• return info->tile_w;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_w % info->hsub);
• return info->tile_w / info->hsub;

+} +EXPORT_SYMBOL(drm_format_tile_width);

+/**

• • drm_format_tile_height - height of a tile for tile formats, should be 1 for
• • all non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The height of a tile, depending on the plane index and vertical sub-sampling
• */

+uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_h);
• if (plane == 0 || info->tile_h == 1)

• return info->tile_h;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_h % info->vsub);
• return info->tile_h / info->vsub;

+} +EXPORT_SYMBOL(drm_format_tile_height); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..57509e51cb80 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i); unsigned int cpp = info->cpp[i];

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = cpp * tile_w * tile_h;
  

• unsigned int num_htiles;
  

• unsigned int num_vtiles;
  
  

  if (!r->handles[i]) { DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i); return -EINVAL; }

• if ((uint64_t) width * cpp > UINT_MAX)
  

• if ((width % tile_w) || (height % tile_h)) {
  

• 	DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n");
  

• 	return -EINVAL;
  

• }
  

• num_htiles = width / tile_w;
  

• num_vtiles = height / tile_h;
  

• if ((uint64_t)num_htiles * tile_size > UINT_MAX)
  return -ERANGE;
  
  

• if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
  

• if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX)
  return -ERANGE;
  
  

• if (r->pitches[i] < width * cpp) {
  

• if (r->pitches[i] < num_htiles * tile_size) {
  DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
  return -EINVAL;
  

  }

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 2810d4131411..3d01a1a9d5d2 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -161,6 +161,11 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, unsigned int width = mode_cmd->width / (i ? info->hsub : 1); unsigned int height = mode_cmd->height / (i ? info->vsub : 1); unsigned int min_size;

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = info->cpp[i] * tile_w * tile_h;
  

• unsigned int num_htiles = width / tile_w;
  

• unsigned int num_vtiles = height / tile_h;
  
  

  objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]); if (!objs[i]) {

@@ -169,9 +174,8 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, goto err_gem_object_put; }

• min_size = (height - 1) * mode_cmd->pitches[i]
  

• 	 + width * info->cpp[i]
  

• 	 + mode_cmd->offsets[i];
  

• min_size = (num_vtiles - 1) * mode_cmd->pitches[i]
  

• 	 + num_htiles * tile_size + mode_cmd->offsets[i];
  
  

  if (objs[i]->size < min_size) { drm_gem_object_put_unlocked(objs[i]);

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 41681cf2b140..001afca9bcff 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -76,6 +76,8 @@ int drm_format_horz_chroma_subsampling(uint32_t format); int drm_format_vert_chroma_subsampling(uint32_t format); int drm_format_plane_width(int width, uint32_t format, int plane); int drm_format_plane_height(int height, uint32_t format, int plane); +uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane); +uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane); const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);

#endif /* __DRM_FOURCC_H__ */

2.18.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:

On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä wrote:

...

On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote:

...

The previous patch added tile_w and tile_h, which represent the horizontal and vertical sizes of a tile.

This one uses that to plumb through drm core in order to be able to handle linear tile formats without the need for drivers to roll up their own implementation.

This patch had been written with Mali-dp X0L2 and X0L0 in mind which is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 bytes per pixel and where tiles are laid out in a linear manner.

Now what are the restrictions:

1. Pitch in bytes is expected to cover at least tile_h * width in

pixels. Due to this the places where the pitch is checked/used need to be updated to take into consideration the tile_w, tile_h and tile_size. tile_size = cpp * tile_w * tile_h

2. When doing source cropping plane_src_x/y need to be a multiple of

tile_w/tile_h and we need to take into consideration the tile_w/tile_h when computing the start address.

For all non-tiled formats the tile_w and tile_h will be 1, so if I didn't miss anything nothing should change.

Regarding multi-planar linear tile formats, I'm not sure how those should be handle I kind of assumed that tile_h/tile_w will have to be divided by horizontal/subsampling. Anyway, I think it's best to just put an warning in there and handle it when someone tries to add support for them.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

drivers/gpu/drm/drm_atomic.c | 8 +++ drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- include/drm/drm_fourcc.h | 2 + 6 files changed, 94 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e11e2e..7a3e893a4cd1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }

• /* Make sure source coordinates are a multiple of tile sizes */
• if ((state->src_x >> 16) % state->fb->format->tile_w ||

•    (state->src_y >> 16) % state->fb->format->tile_h) {
  

• DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions",
  

• 		 plane->base.id, plane->name);
  

• return -EINVAL;
  

• }

Feels rather wrong to me to put such hardware specific limitations into the core.

Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't work.

If that guy is supposed to give you a tile aligned address then it could just do that and leave it up to the driver to deal with the remainder of the coordinates?

An alternative, would be to include it in the driver and put an WARN in drm_fb_cma_get_gem_addr in case someone else uses it with a src_x/src_y tile multiples.

What do you think about that ?

...

...

• if (plane_switching_crtc(state->state, plane, state)) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", plane->base.id, plane->name);

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..4d8052adce67 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, struct drm_gem_cma_object *obj; dma_addr_t paddr; u8 h_div = 1, v_div = 1;

• u32 tile_w = drm_format_tile_width(fb->format, plane);

• u32 tile_h = drm_format_tile_height(fb->format, plane);

  obj = drm_fb_cma_get_gem_obj(fb, plane); if (!obj)

@@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, v_div = fb->format->vsub; }

• paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
• paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;

• paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16))

• 	/ h_div;
  

• /*

• * For tile formats pitches are expected to cover at least
  

• * width * tile_h pixels
  

• */
  

• paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;

Presumably this thing should be using the clipped coordinates instead of the raw user provided ones?

...

...

return paddr; } diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index f55cd93ba2d0..d6c9c5aa4036 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) return height / info->vsub; } EXPORT_SYMBOL(drm_format_plane_height);

+/**

• • drm_format_tile_width - width of a tile for tile formats, should be 1 for all
• • non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The width of a tile, depending on the plane index and horizontal sub-sampling
• */

+uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_w);
• if (plane == 0 || info->tile_w == 1)

• return info->tile_w;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_w % info->hsub);
• return info->tile_w / info->hsub;

+} +EXPORT_SYMBOL(drm_format_tile_width);

+/**

• • drm_format_tile_height - height of a tile for tile formats, should be 1 for
• • all non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The height of a tile, depending on the plane index and vertical sub-sampling
• */

+uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_h);
• if (plane == 0 || info->tile_h == 1)

• return info->tile_h;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_h % info->vsub);
• return info->tile_h / info->vsub;

+} +EXPORT_SYMBOL(drm_format_tile_height); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..57509e51cb80 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i); unsigned int cpp = info->cpp[i];

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = cpp * tile_w * tile_h;
  

• unsigned int num_htiles;
  

• unsigned int num_vtiles;
  
  

  if (!r->handles[i]) { DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i); return -EINVAL; }

• if ((uint64_t) width * cpp > UINT_MAX)
  

• if ((width % tile_w) || (height % tile_h)) {
  

• 	DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n");
  

• 	return -EINVAL;
  

• }
  

• num_htiles = width / tile_w;
  

• num_vtiles = height / tile_h;
  

• if ((uint64_t)num_htiles * tile_size > UINT_MAX)
  return -ERANGE;
  
  

• if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
  

• if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX)
  return -ERANGE;
  
  

• if (r->pitches[i] < width * cpp) {
  

• if (r->pitches[i] < num_htiles * tile_size) {
  DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
  return -EINVAL;
  

  }

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 2810d4131411..3d01a1a9d5d2 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -161,6 +161,11 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, unsigned int width = mode_cmd->width / (i ? info->hsub : 1); unsigned int height = mode_cmd->height / (i ? info->vsub : 1); unsigned int min_size;

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = info->cpp[i] * tile_w * tile_h;
  

• unsigned int num_htiles = width / tile_w;
  

• unsigned int num_vtiles = height / tile_h;
  
  

  objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]); if (!objs[i]) {

@@ -169,9 +174,8 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, goto err_gem_object_put; }

• min_size = (height - 1) * mode_cmd->pitches[i]
  

• 	 + width * info->cpp[i]
  

• 	 + mode_cmd->offsets[i];
  

• min_size = (num_vtiles - 1) * mode_cmd->pitches[i]
  

• 	 + num_htiles * tile_size + mode_cmd->offsets[i];
  
  

  if (objs[i]->size < min_size) { drm_gem_object_put_unlocked(objs[i]);

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 41681cf2b140..001afca9bcff 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -76,6 +76,8 @@ int drm_format_horz_chroma_subsampling(uint32_t format); int drm_format_vert_chroma_subsampling(uint32_t format); int drm_format_plane_width(int width, uint32_t format, int plane); int drm_format_plane_height(int height, uint32_t format, int plane); +uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane); +uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane); const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);

#endif /* __DRM_FOURCC_H__ */

2.18.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Cheers, Alex G

On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe Alexandru-Cosmin.Gheorghe@arm.com wrote:

On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:

...

On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:

...

On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä wrote:

...

On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote:

...

The previous patch added tile_w and tile_h, which represent the horizontal and vertical sizes of a tile.

This one uses that to plumb through drm core in order to be able to handle linear tile formats without the need for drivers to roll up their own implementation.

This patch had been written with Mali-dp X0L2 and X0L0 in mind which is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 bytes per pixel and where tiles are laid out in a linear manner.

Now what are the restrictions:

1. Pitch in bytes is expected to cover at least tile_h * width in

pixels. Due to this the places where the pitch is checked/used need to be updated to take into consideration the tile_w, tile_h and tile_size. tile_size = cpp * tile_w * tile_h

2. When doing source cropping plane_src_x/y need to be a multiple of

tile_w/tile_h and we need to take into consideration the tile_w/tile_h when computing the start address.

For all non-tiled formats the tile_w and tile_h will be 1, so if I didn't miss anything nothing should change.

Regarding multi-planar linear tile formats, I'm not sure how those should be handle I kind of assumed that tile_h/tile_w will have to be divided by horizontal/subsampling. Anyway, I think it's best to just put an warning in there and handle it when someone tries to add support for them.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

drivers/gpu/drm/drm_atomic.c | 8 +++ drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- include/drm/drm_fourcc.h | 2 + 6 files changed, 94 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e11e2e..7a3e893a4cd1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }

•   /* Make sure source coordinates are a multiple of tile sizes */
  

•   if ((state->src_x >> 16) % state->fb->format->tile_w ||
  

•       (state->src_y >> 16) % state->fb->format->tile_h) {
  

•           DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions",
  

•                            plane->base.id, plane->name);
  

•           return -EINVAL;
  

•   }
  

Feels rather wrong to me to put such hardware specific limitations into the core.

Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't work.

If that guy is supposed to give you a tile aligned address then it could just do that and leave it up to the driver to deal with the remainder of the coordinates?

...

An alternative, would be to include it in the driver and put an WARN in drm_fb_cma_get_gem_addr in case someone else uses it with a src_x/src_y tile multiples.

What do you think about that ?

...

...

•   if (plane_switching_crtc(state->state, plane, state)) {
            DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n",
                             plane->base.id, plane->name);
  

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..4d8052adce67 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, struct drm_gem_cma_object *obj; dma_addr_t paddr; u8 h_div = 1, v_div = 1;

•   u32 tile_w = drm_format_tile_width(fb->format, plane);
  

•   u32 tile_h = drm_format_tile_height(fb->format, plane);
  
    obj = drm_fb_cma_get_gem_obj(fb, plane);
    if (!obj)
  

@@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, v_div = fb->format->vsub; }

•   paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
  

•   paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
  

•   paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16))
  

•                   / h_div;
  

•   /*
  

•    * For tile formats pitches are expected to cover at least
  

•    * width * tile_h pixels
  

•    */
  

•   paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;
  

Presumably this thing should be using the clipped coordinates instead of the raw user provided ones?

So you expect the driver to thinker with state->src_x and state->src_y to be tiled aligned?

What do you expect to happen if this function gets passed a src_x/src_y that's not tile aligned, WARN about it or just silently return the beginning of the tile and document that behaviour.

WARN really hard and return NULL or something for any tiled format. Imo a plain address just doesn't make sense for a tile layout: To get a good starting point you need both a base address (which will be aligned to tile rows ofc), plus a x/y offset within. The driver can then decided whether it must have a tile offset of 0, or what exactly it can handle. So yeah I think we need a new helper here that gets this right. -Daniel

...

...

...

...

    return paddr;

} diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index f55cd93ba2d0..d6c9c5aa4036 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) return height / info->vsub; } EXPORT_SYMBOL(drm_format_plane_height);

+/**

• • drm_format_tile_width - width of a tile for tile formats, should be 1 for all
• • non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The width of a tile, depending on the plane index and horizontal sub-sampling
• */

+uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) +{

•   WARN_ON(!info->tile_w);
  

•   if (plane == 0 || info->tile_w == 1)
  

•           return info->tile_w;
  

•   /*
  

•    * Multi planar tiled formats have never been tested, check that
  

•    * buffer restrictions and source cropping meet the format layout
  

•    * expectations.
  

•    */
  

•   WARN_ON("Multi-planar tiled formats unsupported");
  

•   WARN_ON(info->tile_w % info->hsub);
  

•   return info->tile_w / info->hsub;
  

+} +EXPORT_SYMBOL(drm_format_tile_width);

+/**

• • drm_format_tile_height - height of a tile for tile formats, should be 1 for
• • all non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The height of a tile, depending on the plane index and vertical sub-sampling
• */

+uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) +{

•   WARN_ON(!info->tile_h);
  

•   if (plane == 0 || info->tile_h == 1)
  

•           return info->tile_h;
  

•   /*
  

•    * Multi planar tiled formats have never been tested, check that
  

•    * buffer restrictions and source cropping meet the format layout
  

•    * expectations.
  

•    */
  

•   WARN_ON("Multi-planar tiled formats unsupported");
  

•   WARN_ON(info->tile_h % info->vsub);
  

•   return info->tile_h / info->vsub;
  

+} +EXPORT_SYMBOL(drm_format_tile_height); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..57509e51cb80 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i); unsigned int cpp = info->cpp[i];

•           unsigned int tile_w = drm_format_tile_width(info, i);
  

•           unsigned int tile_h = drm_format_tile_height(info, i);
  

•           unsigned int tile_size = cpp * tile_w * tile_h;
  

•           unsigned int num_htiles;
  

•           unsigned int num_vtiles;
  
            if (!r->handles[i]) {
                    DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
                    return -EINVAL;
            }
  
  

•           if ((uint64_t) width * cpp > UINT_MAX)
  

•           if ((width % tile_w) || (height % tile_h)) {
  

•                   DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n");
  

•                   return -EINVAL;
  

•           }
  

•           num_htiles = width / tile_w;
  

•           num_vtiles = height / tile_h;
  

•           if ((uint64_t)num_htiles * tile_size > UINT_MAX)
                    return -ERANGE;
  
  

•           if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
  

•           if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX)
                    return -ERANGE;
  
  

•           if (r->pitches[i] < width * cpp) {
  

•           if (r->pitches[i] < num_htiles * tile_size) {
                    DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
                    return -EINVAL;
            }
  

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 2810d4131411..3d01a1a9d5d2 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -161,6 +161,11 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, unsigned int width = mode_cmd->width / (i ? info->hsub : 1); unsigned int height = mode_cmd->height / (i ? info->vsub : 1); unsigned int min_size;

•           unsigned int tile_w = drm_format_tile_width(info, i);
  

•           unsigned int tile_h = drm_format_tile_height(info, i);
  

•           unsigned int tile_size = info->cpp[i] * tile_w * tile_h;
  

•           unsigned int num_htiles = width / tile_w;
  

•           unsigned int num_vtiles = height / tile_h;
  
            objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
            if (!objs[i]) {
  

@@ -169,9 +174,8 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, goto err_gem_object_put; }

•           min_size = (height - 1) * mode_cmd->pitches[i]
  

•                    + width * info->cpp[i]
  

•                    + mode_cmd->offsets[i];
  

•           min_size = (num_vtiles - 1) * mode_cmd->pitches[i]
  

•                    + num_htiles * tile_size + mode_cmd->offsets[i];
  
            if (objs[i]->size < min_size) {
                    drm_gem_object_put_unlocked(objs[i]);
  

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 41681cf2b140..001afca9bcff 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -76,6 +76,8 @@ int drm_format_horz_chroma_subsampling(uint32_t format); int drm_format_vert_chroma_subsampling(uint32_t format); int drm_format_plane_width(int width, uint32_t format, int plane); int drm_format_plane_height(int height, uint32_t format, int plane); +uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane); +uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane); const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);

#endif /* __DRM_FOURCC_H__ */

2.18.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Cheers, Alex G

-- Ville Syrjälä Intel

-- Cheers, Alex G

On Thu, Aug 23, 2018 at 05:30:36PM +0300, Ville Syrjälä wrote:

On Wed, Aug 22, 2018 at 09:48:29PM +0200, Daniel Vetter wrote:

...

On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe Alexandru-Cosmin.Gheorghe@arm.com wrote:

...

On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:

...

On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:

...

On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä wrote:

...

On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote: > The previous patch added tile_w and tile_h, which represent the > horizontal and vertical sizes of a tile. > > This one uses that to plumb through drm core in order to be able to > handle linear tile formats without the need for drivers to roll up > their own implementation. > > This patch had been written with Mali-dp X0L2 and X0L0 in mind which > is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 > bytes per pixel and where tiles are laid out in a linear manner. > > Now what are the restrictions: > > 1. Pitch in bytes is expected to cover at least tile_h * width in > pixels. Due to this the places where the pitch is checked/used need to > be updated to take into consideration the tile_w, tile_h and > tile_size. > tile_size = cpp * tile_w * tile_h > > 2. When doing source cropping plane_src_x/y need to be a multiple of > tile_w/tile_h and we need to take into consideration the tile_w/tile_h > when computing the start address. > > For all non-tiled formats the tile_w and tile_h will be 1, so if I > didn't miss anything nothing should change. > > Regarding multi-planar linear tile formats, I'm not sure how those > should be handle I kind of assumed that tile_h/tile_w will have to be > divided by horizontal/subsampling. Anyway, I think it's best to just > put an warning in there and handle it when someone tries to add > support for them. > > Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com > --- > drivers/gpu/drm/drm_atomic.c | 8 +++ > drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- > drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ > drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- > include/drm/drm_fourcc.h | 2 + > 6 files changed, 94 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 3eb061e11e2e..7a3e893a4cd1 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, > return -ENOSPC; > } > > + /* Make sure source coordinates are a multiple of tile sizes */ > + if ((state->src_x >> 16) % state->fb->format->tile_w || > + (state->src_y >> 16) % state->fb->format->tile_h) { > + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions", > + plane->base.id, plane->name); > + return -EINVAL; > + }

Feels rather wrong to me to put such hardware specific limitations into the core.

Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't work.

If that guy is supposed to give you a tile aligned address then it could just do that and leave it up to the driver to deal with the remainder of the coordinates?

...

An alternative, would be to include it in the driver and put an WARN in drm_fb_cma_get_gem_addr in case someone else uses it with a src_x/src_y tile multiples.

What do you think about that ?

...

> + > if (plane_switching_crtc(state->state, plane, state)) { > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", > plane->base.id, plane->name); > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > index 47e0e2f6642d..4d8052adce67 100644 > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, > struct drm_gem_cma_object *obj; > dma_addr_t paddr; > u8 h_div = 1, v_div = 1; > + u32 tile_w = drm_format_tile_width(fb->format, plane); > + u32 tile_h = drm_format_tile_height(fb->format, plane); > > obj = drm_fb_cma_get_gem_obj(fb, plane); > if (!obj) > @@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, > v_div = fb->format->vsub; > } > > - paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div; > - paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div; > + paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16)) > + / h_div; > + /* > + * For tile formats pitches are expected to cover at least > + * width * tile_h pixels > + */ > + paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;

Presumably this thing should be using the clipped coordinates instead of the raw user provided ones?

So you expect the driver to thinker with state->src_x and state->src_y to be tiled aligned?

I was referring to using state->src.x1 etc. Using src_x/y will cause you to scan out the wrong stuff if the plane top/left edge got clipped. And as a side note: drivers are not even allowed to muck with src_x/y etc. because that would corrupt the state that user has set up.

Yes, it seems this function should have used src.x1, instead of src_x/y, but I'm not sure it can be easily fix since not all the drivers calling it call drm_atomic_helper_check_plane_state that populates src/dst rectangles.

...

...

What do you expect to happen if this function gets passed a src_x/src_y that's not tile aligned, WARN about it or just silently return the beginning of the tile and document that behaviour.

WARN really hard and return NULL or something for any tiled format. Imo a plain address just doesn't make sense for a tile layout: To get a good starting point you need both a base address (which will be aligned to tile rows ofc), plus a x/y offset within. The driver can then decided whether it must have a tile offset of 0, or what exactly it can handle. So yeah I think we need a new helper here that gets this right.

Yeah, something that either returns the remaining intra-tile x/y offsets, or the tile aligned x/y offsets and lets the driver deal with the remainder in whatever way is appropriate (which could even be -EINVAL in case the hw can't simply handle it). The latter being pretty much how i915 does things.

Looking at DRM_FORMAT_MOD_SAMSUNG_64_32_TILE with its Z shape ordering of the tiles, it doesn't seem there could be any generic way of getting the address of a pixel, especially using just the tile_w and tile_h.

-- Ville Syrjälä Intel

On Thu, Aug 23, 2018 at 08:25:46PM +0300, Ville Syrjälä wrote:

On Thu, Aug 23, 2018 at 06:19:31PM +0100, Alexandru-Cosmin Gheorghe wrote:

...

On Thu, Aug 23, 2018 at 05:30:36PM +0300, Ville Syrjälä wrote:

...

On Wed, Aug 22, 2018 at 09:48:29PM +0200, Daniel Vetter wrote:

...

On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe Alexandru-Cosmin.Gheorghe@arm.com wrote:

...

On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:

...

On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote: > On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä wrote: > > On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote: > > > The previous patch added tile_w and tile_h, which represent the > > > horizontal and vertical sizes of a tile. > > > > > > This one uses that to plumb through drm core in order to be able to > > > handle linear tile formats without the need for drivers to roll up > > > their own implementation. > > > > > > This patch had been written with Mali-dp X0L2 and X0L0 in mind which > > > is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 > > > bytes per pixel and where tiles are laid out in a linear manner. > > > > > > Now what are the restrictions: > > > > > > 1. Pitch in bytes is expected to cover at least tile_h * width in > > > pixels. Due to this the places where the pitch is checked/used need to > > > be updated to take into consideration the tile_w, tile_h and > > > tile_size. > > > tile_size = cpp * tile_w * tile_h > > > > > > 2. When doing source cropping plane_src_x/y need to be a multiple of > > > tile_w/tile_h and we need to take into consideration the tile_w/tile_h > > > when computing the start address. > > > > > > For all non-tiled formats the tile_w and tile_h will be 1, so if I > > > didn't miss anything nothing should change. > > > > > > Regarding multi-planar linear tile formats, I'm not sure how those > > > should be handle I kind of assumed that tile_h/tile_w will have to be > > > divided by horizontal/subsampling. Anyway, I think it's best to just > > > put an warning in there and handle it when someone tries to add > > > support for them. > > > > > > Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com > > > --- > > > drivers/gpu/drm/drm_atomic.c | 8 +++ > > > drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- > > > drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ > > > drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- > > > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- > > > include/drm/drm_fourcc.h | 2 + > > > 6 files changed, 94 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index 3eb061e11e2e..7a3e893a4cd1 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, > > > return -ENOSPC; > > > } > > > > > > + /* Make sure source coordinates are a multiple of tile sizes */ > > > + if ((state->src_x >> 16) % state->fb->format->tile_w || > > > + (state->src_y >> 16) % state->fb->format->tile_h) { > > > + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions", > > > + plane->base.id, plane->name); > > > + return -EINVAL; > > > + } > > > > Feels rather wrong to me to put such hardware specific limitations into > > the core. > > Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't > work.

If that guy is supposed to give you a tile aligned address then it could just do that and leave it up to the driver to deal with the remainder of the coordinates?

> > An alternative, would be to include it in the driver and put an WARN > in drm_fb_cma_get_gem_addr in case someone else uses it with a > src_x/src_y tile multiples. > > What do you think about that ? > > > > > > + > > > if (plane_switching_crtc(state->state, plane, state)) { > > > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", > > > plane->base.id, plane->name); > > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > > > index 47e0e2f6642d..4d8052adce67 100644 > > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > > > @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, > > > struct drm_gem_cma_object *obj; > > > dma_addr_t paddr; > > > u8 h_div = 1, v_div = 1; > > > + u32 tile_w = drm_format_tile_width(fb->format, plane); > > > + u32 tile_h = drm_format_tile_height(fb->format, plane); > > > > > > obj = drm_fb_cma_get_gem_obj(fb, plane); > > > if (!obj) > > > @@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, > > > v_div = fb->format->vsub; > > > } > > > > > > - paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div; > > > - paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div; > > > + paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16)) > > > + / h_div; > > > + /* > > > + * For tile formats pitches are expected to cover at least > > > + * width * tile_h pixels > > > + */ > > > + paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;

Presumably this thing should be using the clipped coordinates instead of the raw user provided ones?

So you expect the driver to thinker with state->src_x and state->src_y to be tiled aligned?

I was referring to using state->src.x1 etc. Using src_x/y will cause you to scan out the wrong stuff if the plane top/left edge got clipped. And as a side note: drivers are not even allowed to muck with src_x/y etc. because that would corrupt the state that user has set up.

Yes, it seems this function should have used src.x1, instead of src_x/y, but I'm not sure it can be easily fix since not all the drivers calling it call drm_atomic_helper_check_plane_state that populates src/dst rectangles.

Time to fix those drivers?

I'm bit reluctant to add restrictions/checks in other peoples drivers without being able to test it.

-- Ville Syrjälä Intel

On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote:

The previous patch added tile_w and tile_h, which represent the horizontal and vertical sizes of a tile.

This one uses that to plumb through drm core in order to be able to handle linear tile formats without the need for drivers to roll up their own implementation.

This patch had been written with Mali-dp X0L2 and X0L0 in mind which is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 bytes per pixel and where tiles are laid out in a linear manner.

Now what are the restrictions:

1. Pitch in bytes is expected to cover at least tile_h * width in

pixels. Due to this the places where the pitch is checked/used need to be updated to take into consideration the tile_w, tile_h and tile_size. tile_size = cpp * tile_w * tile_h

2. When doing source cropping plane_src_x/y need to be a multiple of

tile_w/tile_h and we need to take into consideration the tile_w/tile_h when computing the start address.

For all non-tiled formats the tile_w and tile_h will be 1, so if I didn't miss anything nothing should change.

Regarding multi-planar linear tile formats, I'm not sure how those should be handle I kind of assumed that tile_h/tile_w will have to be divided by horizontal/subsampling. Anyway, I think it's best to just put an warning in there and handle it when someone tries to add support for them.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

Just general comment: I think it'd be awesome (and also a great demonstration that the tile_h/w approach is sound) if we could subsume the DRM_FORMAT_MOD_SAMSUNG_64_32_TILE stuff into this.

Probably need a core change to drm_get_format_info for this pair (and reject it for everything else), with the correct tile sizes set. -Daniel

---

drivers/gpu/drm/drm_atomic.c | 8 +++ drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- include/drm/drm_fourcc.h | 2 + 6 files changed, 94 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e11e2e..7a3e893a4cd1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }

• /* Make sure source coordinates are a multiple of tile sizes */
• if ((state->src_x >> 16) % state->fb->format->tile_w ||

•    (state->src_y >> 16) % state->fb->format->tile_h) {
  

• DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions",
  

• 		 plane->base.id, plane->name);
  

• return -EINVAL;
  

• }
• if (plane_switching_crtc(state->state, plane, state)) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", plane->base.id, plane->name);

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..4d8052adce67 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, struct drm_gem_cma_object *obj; dma_addr_t paddr; u8 h_div = 1, v_div = 1;

• u32 tile_w = drm_format_tile_width(fb->format, plane);

• u32 tile_h = drm_format_tile_height(fb->format, plane);

  obj = drm_fb_cma_get_gem_obj(fb, plane); if (!obj)

@@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, v_div = fb->format->vsub; }

• paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
• paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;

• paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16))

• 	/ h_div;
  

• /*

• * For tile formats pitches are expected to cover at least
  

• * width * tile_h pixels
  

• */
  

• paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;

  return paddr;

} diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index f55cd93ba2d0..d6c9c5aa4036 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) return height / info->vsub; } EXPORT_SYMBOL(drm_format_plane_height);

+/**

• • drm_format_tile_width - width of a tile for tile formats, should be 1 for all
• • non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The width of a tile, depending on the plane index and horizontal sub-sampling
• */

+uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_w);
• if (plane == 0 || info->tile_w == 1)

• return info->tile_w;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_w % info->hsub);
• return info->tile_w / info->hsub;

+} +EXPORT_SYMBOL(drm_format_tile_width);

+/**

• • drm_format_tile_height - height of a tile for tile formats, should be 1 for
• • all non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The height of a tile, depending on the plane index and vertical sub-sampling
• */

+uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_h);
• if (plane == 0 || info->tile_h == 1)

• return info->tile_h;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_h % info->vsub);
• return info->tile_h / info->vsub;

+} +EXPORT_SYMBOL(drm_format_tile_height); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..57509e51cb80 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i); unsigned int cpp = info->cpp[i];

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = cpp * tile_w * tile_h;
  

• unsigned int num_htiles;
  

• unsigned int num_vtiles;
  
  

  if (!r->handles[i]) { DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i); return -EINVAL; }

• if ((uint64_t) width * cpp > UINT_MAX)
  

• if ((width % tile_w) || (height % tile_h)) {
  

• 	DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n");
  

• 	return -EINVAL;
  

• }
  

• num_htiles = width / tile_w;
  

• num_vtiles = height / tile_h;
  

• if ((uint64_t)num_htiles * tile_size > UINT_MAX)
  return -ERANGE;
  
  

• if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
  

• if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX)
  return -ERANGE;
  
  

• if (r->pitches[i] < width * cpp) {
  

• if (r->pitches[i] < num_htiles * tile_size) {
  DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
  return -EINVAL;
  

  }

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 2810d4131411..3d01a1a9d5d2 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -161,6 +161,11 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, unsigned int width = mode_cmd->width / (i ? info->hsub : 1); unsigned int height = mode_cmd->height / (i ? info->vsub : 1); unsigned int min_size;

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = info->cpp[i] * tile_w * tile_h;
  

• unsigned int num_htiles = width / tile_w;
  

• unsigned int num_vtiles = height / tile_h;
  
  

  objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]); if (!objs[i]) {

@@ -169,9 +174,8 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, goto err_gem_object_put; }

• min_size = (height - 1) * mode_cmd->pitches[i]
  

• 	 + width * info->cpp[i]
  

• 	 + mode_cmd->offsets[i];
  

• min_size = (num_vtiles - 1) * mode_cmd->pitches[i]
  

• 	 + num_htiles * tile_size + mode_cmd->offsets[i];
  
  

  if (objs[i]->size < min_size) { drm_gem_object_put_unlocked(objs[i]);

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 41681cf2b140..001afca9bcff 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -76,6 +76,8 @@ int drm_format_horz_chroma_subsampling(uint32_t format); int drm_format_vert_chroma_subsampling(uint32_t format); int drm_format_plane_width(int width, uint32_t format, int plane); int drm_format_plane_height(int height, uint32_t format, int plane); +uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane); +uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane); const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);

#endif /* __DRM_FOURCC_H__ */

2.18.0

On Wed, Aug 22, 2018 at 10:18:51PM +0200, Daniel Vetter wrote:

On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote:

...

The previous patch added tile_w and tile_h, which represent the horizontal and vertical sizes of a tile.

This one uses that to plumb through drm core in order to be able to handle linear tile formats without the need for drivers to roll up their own implementation.

This patch had been written with Mali-dp X0L2 and X0L0 in mind which is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 bytes per pixel and where tiles are laid out in a linear manner.

Now what are the restrictions:

1. Pitch in bytes is expected to cover at least tile_h * width in

pixels. Due to this the places where the pitch is checked/used need to be updated to take into consideration the tile_w, tile_h and tile_size. tile_size = cpp * tile_w * tile_h

2. When doing source cropping plane_src_x/y need to be a multiple of

tile_w/tile_h and we need to take into consideration the tile_w/tile_h when computing the start address.

For all non-tiled formats the tile_w and tile_h will be 1, so if I didn't miss anything nothing should change.

Regarding multi-planar linear tile formats, I'm not sure how those should be handle I kind of assumed that tile_h/tile_w will have to be divided by horizontal/subsampling. Anyway, I think it's best to just put an warning in there and handle it when someone tries to add support for them.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

drivers/gpu/drm/drm_atomic.c | 8 +++ drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- include/drm/drm_fourcc.h | 2 + 6 files changed, 94 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e11e2e..7a3e893a4cd1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }

• /* Make sure source coordinates are a multiple of tile sizes */
• if ((state->src_x >> 16) % state->fb->format->tile_w ||

•    (state->src_y >> 16) % state->fb->format->tile_h) {
  

• DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions",
  

• 		 plane->base.id, plane->name);
  

• return -EINVAL;
  

• }
• if (plane_switching_crtc(state->state, plane, state)) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", plane->base.id, plane->name);

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..4d8052adce67 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, struct drm_gem_cma_object *obj; dma_addr_t paddr; u8 h_div = 1, v_div = 1;

• u32 tile_w = drm_format_tile_width(fb->format, plane);

• u32 tile_h = drm_format_tile_height(fb->format, plane);

  obj = drm_fb_cma_get_gem_obj(fb, plane); if (!obj)

@@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, v_div = fb->format->vsub; }

• paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
• paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;

• paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16))

• 	/ h_div;
  

• /*

• * For tile formats pitches are expected to cover at least
  

• * width * tile_h pixels
  

• */
  

• paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;

  return paddr;

} diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index f55cd93ba2d0..d6c9c5aa4036 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) return height / info->vsub; } EXPORT_SYMBOL(drm_format_plane_height);

+/**

• • drm_format_tile_width - width of a tile for tile formats, should be 1 for all
• • non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The width of a tile, depending on the plane index and horizontal sub-sampling
• */

+uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_w);
• if (plane == 0 || info->tile_w == 1)

• return info->tile_w;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_w % info->hsub);
• return info->tile_w / info->hsub;

+} +EXPORT_SYMBOL(drm_format_tile_width);

+/**

• • drm_format_tile_height - height of a tile for tile formats, should be 1 for
• • all non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The height of a tile, depending on the plane index and vertical sub-sampling
• */

+uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_h);
• if (plane == 0 || info->tile_h == 1)

• return info->tile_h;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_h % info->vsub);
• return info->tile_h / info->vsub;

+} +EXPORT_SYMBOL(drm_format_tile_height); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..57509e51cb80 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i); unsigned int cpp = info->cpp[i];

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = cpp * tile_w * tile_h;
  

• unsigned int num_htiles;
  

• unsigned int num_vtiles;
  
  

  if (!r->handles[i]) { DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i); return -EINVAL; }

• if ((uint64_t) width * cpp > UINT_MAX)
  

• if ((width % tile_w) || (height % tile_h)) {
  

I think this is too strict. You can carve out a sub-part of anything really with a drm_framebuffer. See below, we only checked that width * cpp < pitches, so leftover bytes/bits was always ok.

Your driver might have additional constraints, but that's a different story.

This seem like the only thing that we have in common with DRM_FORMAT_MOD_SAMSUNG_64_32_TILE/NV12MT, which is even more restrictive than that and it wants the width to be a multiple of 128.

...

• 	DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n");
  

• 	return -EINVAL;
  

• }
  

• num_htiles = width / tile_w;
  

• num_vtiles = height / tile_h;
  

• if ((uint64_t)num_htiles * tile_size > UINT_MAX)
  return -ERANGE;
  
  

• if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
  

• if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX)
  return -ERANGE;
  
  

• if (r->pitches[i] < width * cpp) {
  

• if (r->pitches[i] < num_htiles * tile_size) {
  

Essentially you define ->pitches now to mean a row of tiles. At least for bigger tiled formats (using modifiers) we don't use it like that. But it also gets awkward quickly if we keep pitches to mean bytes in one row of pixels. For that case we'd have

That's how everything started, we have already internal userspace and drivers that expect and pass pitch as a row of tiles in bytes.

For the tiles format in this patchset(X0L2/X0L0) which orders the tiles in linear manner the pitch will at least get you at the beginning of a row.

I'm not sure I understand what's the meaning of a pitch for DRM_FORMAT_MOD_SAMSUNG_64_32_TILE which orders the tiles in Z shape. How far will a pitch for this format get you, it seems that using just witdth * cpp will get you somewhere in the middle.

pitches % (cpp * tile_w) == 0 pitches * align(height, tile_h) < size

That's at least the definition of tiling we've used in i915. It also matches what we've done for DRM_FORMAT_MOD_SAMSUNG_64_32_TILE.

This definition of pitches becomes a bit nonsense for fractional cpp, but well, cpp is by definition not fractional, so we can figure that out when we actually have that problem. -Daniel

...

	DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
	return -EINVAL;
}

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 2810d4131411..3d01a1a9d5d2 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -161,6 +161,11 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, unsigned int width = mode_cmd->width / (i ? info->hsub : 1); unsigned int height = mode_cmd->height / (i ? info->vsub : 1); unsigned int min_size;

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = info->cpp[i] * tile_w * tile_h;
  

• unsigned int num_htiles = width / tile_w;
  

• unsigned int num_vtiles = height / tile_h;
  
  

  objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]); if (!objs[i]) {

@@ -169,9 +174,8 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, goto err_gem_object_put; }

• min_size = (height - 1) * mode_cmd->pitches[i]
  

• 	 + width * info->cpp[i]
  

• 	 + mode_cmd->offsets[i];
  

• min_size = (num_vtiles - 1) * mode_cmd->pitches[i]
  

• 	 + num_htiles * tile_size + mode_cmd->offsets[i];
  
  

  if (objs[i]->size < min_size) { drm_gem_object_put_unlocked(objs[i]);

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 41681cf2b140..001afca9bcff 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -76,6 +76,8 @@ int drm_format_horz_chroma_subsampling(uint32_t format); int drm_format_vert_chroma_subsampling(uint32_t format); int drm_format_plane_width(int width, uint32_t format, int plane); int drm_format_plane_height(int height, uint32_t format, int plane); +uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane); +uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane); const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);

#endif /* __DRM_FOURCC_H__ */

2.18.0

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:

On Thu, Aug 23, 2018 at 06:43:40PM +0100, Alexandru-Cosmin Gheorghe wrote:

...

On Wed, Aug 22, 2018 at 10:18:51PM +0200, Daniel Vetter wrote:

...

On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote:

...

The previous patch added tile_w and tile_h, which represent the horizontal and vertical sizes of a tile.

This one uses that to plumb through drm core in order to be able to handle linear tile formats without the need for drivers to roll up their own implementation.

This patch had been written with Mali-dp X0L2 and X0L0 in mind which is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 bytes per pixel and where tiles are laid out in a linear manner.

Now what are the restrictions:

1. Pitch in bytes is expected to cover at least tile_h * width in

pixels. Due to this the places where the pitch is checked/used need to be updated to take into consideration the tile_w, tile_h and tile_size. tile_size = cpp * tile_w * tile_h

2. When doing source cropping plane_src_x/y need to be a multiple of

tile_w/tile_h and we need to take into consideration the tile_w/tile_h when computing the start address.

For all non-tiled formats the tile_w and tile_h will be 1, so if I didn't miss anything nothing should change.

Regarding multi-planar linear tile formats, I'm not sure how those should be handle I kind of assumed that tile_h/tile_w will have to be divided by horizontal/subsampling. Anyway, I think it's best to just put an warning in there and handle it when someone tries to add support for them.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

drivers/gpu/drm/drm_atomic.c | 8 +++ drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- include/drm/drm_fourcc.h | 2 + 6 files changed, 94 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e11e2e..7a3e893a4cd1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }

• /* Make sure source coordinates are a multiple of tile sizes */
• if ((state->src_x >> 16) % state->fb->format->tile_w ||

•    (state->src_y >> 16) % state->fb->format->tile_h) {
  

• DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions",
  

• 		 plane->base.id, plane->name);
  

• return -EINVAL;
  

• }
• if (plane_switching_crtc(state->state, plane, state)) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", plane->base.id, plane->name);

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..4d8052adce67 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, struct drm_gem_cma_object *obj; dma_addr_t paddr; u8 h_div = 1, v_div = 1;

• u32 tile_w = drm_format_tile_width(fb->format, plane);

• u32 tile_h = drm_format_tile_height(fb->format, plane);

  obj = drm_fb_cma_get_gem_obj(fb, plane); if (!obj)

@@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, v_div = fb->format->vsub; }

• paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
• paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;

• paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16))

• 	/ h_div;
  

• /*

• * For tile formats pitches are expected to cover at least
  

• * width * tile_h pixels
  

• */
  

• paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;

  return paddr;

} diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index f55cd93ba2d0..d6c9c5aa4036 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) return height / info->vsub; } EXPORT_SYMBOL(drm_format_plane_height);

+/**

• • drm_format_tile_width - width of a tile for tile formats, should be 1 for all
• • non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The width of a tile, depending on the plane index and horizontal sub-sampling
• */

+uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_w);
• if (plane == 0 || info->tile_w == 1)

• return info->tile_w;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_w % info->hsub);
• return info->tile_w / info->hsub;

+} +EXPORT_SYMBOL(drm_format_tile_width);

+/**

• • drm_format_tile_height - height of a tile for tile formats, should be 1 for
• • all non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The height of a tile, depending on the plane index and vertical sub-sampling
• */

+uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_h);
• if (plane == 0 || info->tile_h == 1)

• return info->tile_h;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_h % info->vsub);
• return info->tile_h / info->vsub;

+} +EXPORT_SYMBOL(drm_format_tile_height); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..57509e51cb80 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i); unsigned int cpp = info->cpp[i];

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = cpp * tile_w * tile_h;
  

• unsigned int num_htiles;
  

• unsigned int num_vtiles;
  
  

  if (!r->handles[i]) { DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i); return -EINVAL; }

• if ((uint64_t) width * cpp > UINT_MAX)
  

• if ((width % tile_w) || (height % tile_h)) {
  

I think this is too strict. You can carve out a sub-part of anything really with a drm_framebuffer. See below, we only checked that width * cpp < pitches, so leftover bytes/bits was always ok.

Your driver might have additional constraints, but that's a different story.

This seem like the only thing that we have in common with DRM_FORMAT_MOD_SAMSUNG_64_32_TILE/NV12MT, which is even more restrictive than that and it wants the width to be a multiple of 128.

...

...

• 	DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n");
  

• 	return -EINVAL;
  

• }
  

• num_htiles = width / tile_w;
  

• num_vtiles = height / tile_h;
  

• if ((uint64_t)num_htiles * tile_size > UINT_MAX)
  return -ERANGE;
  
  

• if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
  

• if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX)
  return -ERANGE;
  
  

• if (r->pitches[i] < width * cpp) {
  

• if (r->pitches[i] < num_htiles * tile_size) {
  

Essentially you define ->pitches now to mean a row of tiles. At least for bigger tiled formats (using modifiers) we don't use it like that. But it also gets awkward quickly if we keep pitches to mean bytes in one row of pixels. For that case we'd have

That's how everything started, we have already internal userspace and drivers that expect and pass pitch as a row of tiles in bytes.

For the tiles format in this patchset(X0L2/X0L0) which orders the tiles in linear manner the pitch will at least get you at the beginning of a row.

I'm not sure I understand what's the meaning of a pitch for DRM_FORMAT_MOD_SAMSUNG_64_32_TILE which orders the tiles in Z shape. How far will a pitch for this format get you, it seems that using just witdth * cpp will get you somewhere in the middle.

So there's 2 interpretations of pitch if you have a tile/block layout:

• What you do, where pitch = num_tiles_per_row * tile_size. Adding pitch to the base address will then advance tile_h pixels. Let's call this alex_pitch.

• What I'm proposing, and what NV12MT does, and what every other driver does (afaik, I didn't carefully check them all): Pretend there's no tile, and compute the pitch as if it's a linear format. The reason behind this is that fairly often for tiled layouts (but not always), you have magic remapping hardware, which can present a linear layout to you. So for NV12MT that would resolve the Z-shape and macroblock layout, and give you a normal NV12 framebuffer. Let's call this drm_pitch. Note that the drm pitch does _not_ make any sense when you add it to the physical base address, only if you add mutliple's of tile_h, to get an entire tile row.

The relationship is drm_pitch = alex_pitch / tile_h. I'm not aware of any format where that doesn't devide evenly, so in practice it doesn't matter which one you pick. Imo either has up/downsides.

Since everyone is already using the linear pitch approach I'd suggest sticking with that. Might make life a little easier for generic userspace (though it still needs to know other details of the tiling format of course).

Hi,

On Fri, Aug 31, 2018 at 05:12:24PM +0200, Daniel Vetter wrote:

On Fri, Aug 31, 2018 at 02:20:31PM +0300, Ville Syrjälä wrote:

...

On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:

...

On Thu, Aug 23, 2018 at 06:43:40PM +0100, Alexandru-Cosmin Gheorghe wrote:

...

On Wed, Aug 22, 2018 at 10:18:51PM +0200, Daniel Vetter wrote:

...

On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote:

...

The previous patch added tile_w and tile_h, which represent the horizontal and vertical sizes of a tile.

This one uses that to plumb through drm core in order to be able to handle linear tile formats without the need for drivers to roll up their own implementation.

This patch had been written with Mali-dp X0L2 and X0L0 in mind which is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 bytes per pixel and where tiles are laid out in a linear manner.

Now what are the restrictions:

1. Pitch in bytes is expected to cover at least tile_h * width in

pixels. Due to this the places where the pitch is checked/used need to be updated to take into consideration the tile_w, tile_h and tile_size. tile_size = cpp * tile_w * tile_h

2. When doing source cropping plane_src_x/y need to be a multiple of

tile_w/tile_h and we need to take into consideration the tile_w/tile_h when computing the start address.

For all non-tiled formats the tile_w and tile_h will be 1, so if I didn't miss anything nothing should change.

Regarding multi-planar linear tile formats, I'm not sure how those should be handle I kind of assumed that tile_h/tile_w will have to be divided by horizontal/subsampling. Anyway, I think it's best to just put an warning in there and handle it when someone tries to add support for them.

Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com

drivers/gpu/drm/drm_atomic.c | 8 +++ drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- include/drm/drm_fourcc.h | 2 + 6 files changed, 94 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e11e2e..7a3e893a4cd1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }

• /* Make sure source coordinates are a multiple of tile sizes */
• if ((state->src_x >> 16) % state->fb->format->tile_w ||

•    (state->src_y >> 16) % state->fb->format->tile_h) {
  

• DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions",
  

• 		 plane->base.id, plane->name);
  

• return -EINVAL;
  

• }
• if (plane_switching_crtc(state->state, plane, state)) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", plane->base.id, plane->name);

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..4d8052adce67 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, struct drm_gem_cma_object *obj; dma_addr_t paddr; u8 h_div = 1, v_div = 1;

• u32 tile_w = drm_format_tile_width(fb->format, plane);

• u32 tile_h = drm_format_tile_height(fb->format, plane);

  obj = drm_fb_cma_get_gem_obj(fb, plane); if (!obj)

@@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, v_div = fb->format->vsub; }

• paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
• paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;

• paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16))

• 	/ h_div;
  

• /*

• * For tile formats pitches are expected to cover at least
  

• * width * tile_h pixels
  

• */
  

• paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;

  return paddr;

} diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index f55cd93ba2d0..d6c9c5aa4036 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) return height / info->vsub; } EXPORT_SYMBOL(drm_format_plane_height);

+/**

• • drm_format_tile_width - width of a tile for tile formats, should be 1 for all
• • non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The width of a tile, depending on the plane index and horizontal sub-sampling
• */

+uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_w);
• if (plane == 0 || info->tile_w == 1)

• return info->tile_w;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_w % info->hsub);
• return info->tile_w / info->hsub;

+} +EXPORT_SYMBOL(drm_format_tile_width);

+/**

• • drm_format_tile_height - height of a tile for tile formats, should be 1 for
• • all non-tiled formats.
• • @format: pixel format
• • @plane: plane index
• • Returns:
• • The height of a tile, depending on the plane index and vertical sub-sampling
• */

+uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) +{

• WARN_ON(!info->tile_h);
• if (plane == 0 || info->tile_h == 1)

• return info->tile_h;
  

• /*

• * Multi planar tiled formats have never been tested, check that
  

• * buffer restrictions and source cropping meet the format layout
  

• * expectations.
  

• */
  

• WARN_ON("Multi-planar tiled formats unsupported");
• WARN_ON(info->tile_h % info->vsub);
• return info->tile_h / info->vsub;

+} +EXPORT_SYMBOL(drm_format_tile_height); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..57509e51cb80 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i); unsigned int cpp = info->cpp[i];

• unsigned int tile_w = drm_format_tile_width(info, i);
  

• unsigned int tile_h = drm_format_tile_height(info, i);
  

• unsigned int tile_size = cpp * tile_w * tile_h;
  

• unsigned int num_htiles;
  

• unsigned int num_vtiles;
  
  

  if (!r->handles[i]) { DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i); return -EINVAL; }

• if ((uint64_t) width * cpp > UINT_MAX)
  

• if ((width % tile_w) || (height % tile_h)) {
  

I think this is too strict. You can carve out a sub-part of anything really with a drm_framebuffer. See below, we only checked that width * cpp < pitches, so leftover bytes/bits was always ok.

Your driver might have additional constraints, but that's a different story.

This seem like the only thing that we have in common with DRM_FORMAT_MOD_SAMSUNG_64_32_TILE/NV12MT, which is even more restrictive than that and it wants the width to be a multiple of 128.

...

...

• 	DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n");
  

• 	return -EINVAL;
  

• }
  

• num_htiles = width / tile_w;
  

• num_vtiles = height / tile_h;
  

• if ((uint64_t)num_htiles * tile_size > UINT_MAX)
  return -ERANGE;
  
  

• if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
  

• if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX)
  return -ERANGE;
  
  

• if (r->pitches[i] < width * cpp) {
  

• if (r->pitches[i] < num_htiles * tile_size) {
  

Essentially you define ->pitches now to mean a row of tiles. At least for bigger tiled formats (using modifiers) we don't use it like that. But it also gets awkward quickly if we keep pitches to mean bytes in one row of pixels. For that case we'd have

That's how everything started, we have already internal userspace and drivers that expect and pass pitch as a row of tiles in bytes.

For the tiles format in this patchset(X0L2/X0L0) which orders the tiles in linear manner the pitch will at least get you at the beginning of a row.

I'm not sure I understand what's the meaning of a pitch for DRM_FORMAT_MOD_SAMSUNG_64_32_TILE which orders the tiles in Z shape. How far will a pitch for this format get you, it seems that using just witdth * cpp will get you somewhere in the middle.

So there's 2 interpretations of pitch if you have a tile/block layout:

• What you do, where pitch = num_tiles_per_row * tile_size. Adding pitch to the base address will then advance tile_h pixels. Let's call this alex_pitch.

Is more or less mali_pitch, rather than alex_pitch.

...

...

• What I'm proposing, and what NV12MT does, and what every other driver does (afaik, I didn't carefully check them all): Pretend there's no tile, and compute the pitch as if it's a linear format. The reason behind this is that fairly often for tiled layouts (but not always), you have magic remapping hardware, which can present a linear layout to you. So for NV12MT that would resolve the Z-shape and macroblock layout, and give you a normal NV12 framebuffer. Let's call this drm_pitch. Note that the drm pitch does _not_ make any sense when you add it to the physical base address, only if you add mutliple's of tile_h, to get an entire tile row.

That's exactly what I'm proposing, for linear tile formats we should use a pitch that naturaly has sense which is drm_pitch * tile_h.

For any other shape of arranging arranging the tiles like the Z shape of NV12MT, the fake pitch meaning would remain unchanged.

...

...

The relationship is drm_pitch = alex_pitch / tile_h. I'm not aware of any format where that doesn't devide evenly, so in practice it doesn't matter which one you pick. Imo either has up/downsides.

Since everyone is already using the linear pitch approach I'd suggest sticking with that. Might make life a little easier for generic userspace (though it still needs to know other details of the tiling format of course).

Yeah, that's what I meant to add/clarify. The fake linear pitch is, to the best of my knowledge, already the standard we're using in drm. -Daniel

Everyone except Mali. Still, it seems like a good idea to transform something "fake" in something with meaning, of course without breaking the userspace :).

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Mon, Sep 03, 2018 at 09:26:24AM +0200, Daniel Vetter wrote:

On Fri, Aug 31, 2018 at 05:26:37PM +0100, Alexandru-Cosmin Gheorghe wrote:

...

Hi,

On Fri, Aug 31, 2018 at 05:12:24PM +0200, Daniel Vetter wrote:

...

On Fri, Aug 31, 2018 at 02:20:31PM +0300, Ville Syrjälä wrote:

...

On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:

...

On Thu, Aug 23, 2018 at 06:43:40PM +0100, Alexandru-Cosmin Gheorghe wrote:

...

On Wed, Aug 22, 2018 at 10:18:51PM +0200, Daniel Vetter wrote: > On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote: > > The previous patch added tile_w and tile_h, which represent the > > horizontal and vertical sizes of a tile. > > > > This one uses that to plumb through drm core in order to be able to > > handle linear tile formats without the need for drivers to roll up > > their own implementation. > > > > This patch had been written with Mali-dp X0L2 and X0L0 in mind which > > is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 > > bytes per pixel and where tiles are laid out in a linear manner. > > > > Now what are the restrictions: > > > > 1. Pitch in bytes is expected to cover at least tile_h * width in > > pixels. Due to this the places where the pitch is checked/used need to > > be updated to take into consideration the tile_w, tile_h and > > tile_size. > > tile_size = cpp * tile_w * tile_h > > > > 2. When doing source cropping plane_src_x/y need to be a multiple of > > tile_w/tile_h and we need to take into consideration the tile_w/tile_h > > when computing the start address. > > > > For all non-tiled formats the tile_w and tile_h will be 1, so if I > > didn't miss anything nothing should change. > > > > Regarding multi-planar linear tile formats, I'm not sure how those > > should be handle I kind of assumed that tile_h/tile_w will have to be > > divided by horizontal/subsampling. Anyway, I think it's best to just > > put an warning in there and handle it when someone tries to add > > support for them. > > > > Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com > > --- > > drivers/gpu/drm/drm_atomic.c | 8 +++ > > drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- > > drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ > > drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- > > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- > > include/drm/drm_fourcc.h | 2 + > > 6 files changed, 94 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 3eb061e11e2e..7a3e893a4cd1 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, > > return -ENOSPC; > > } > > > > + /* Make sure source coordinates are a multiple of tile sizes */ > > + if ((state->src_x >> 16) % state->fb->format->tile_w || > > + (state->src_y >> 16) % state->fb->format->tile_h) { > > + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions", > > + plane->base.id, plane->name); > > + return -EINVAL; > > + } > > + > > if (plane_switching_crtc(state->state, plane, state)) { > > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", > > plane->base.id, plane->name); > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > > index 47e0e2f6642d..4d8052adce67 100644 > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > > @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, > > struct drm_gem_cma_object *obj; > > dma_addr_t paddr; > > u8 h_div = 1, v_div = 1; > > + u32 tile_w = drm_format_tile_width(fb->format, plane); > > + u32 tile_h = drm_format_tile_height(fb->format, plane); > > > > obj = drm_fb_cma_get_gem_obj(fb, plane); > > if (!obj) > > @@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, > > v_div = fb->format->vsub; > > } > > > > - paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div; > > - paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div; > > + paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16)) > > + / h_div; > > + /* > > + * For tile formats pitches are expected to cover at least > > + * width * tile_h pixels > > + */ > > + paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div; > > > > return paddr; > > } > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > > index f55cd93ba2d0..d6c9c5aa4036 100644 > > --- a/drivers/gpu/drm/drm_fourcc.c > > +++ b/drivers/gpu/drm/drm_fourcc.c > > @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) > > return height / info->vsub; > > } > > EXPORT_SYMBOL(drm_format_plane_height); > > + > > +/** > > + * drm_format_tile_width - width of a tile for tile formats, should be 1 for all > > + * non-tiled formats. > > + * @format: pixel format > > + * @plane: plane index > > + * > > + * Returns: > > + * The width of a tile, depending on the plane index and horizontal sub-sampling > > + */ > > +uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) > > +{ > > + WARN_ON(!info->tile_w); > > + if (plane == 0 || info->tile_w == 1) > > + return info->tile_w; > > + > > + /* > > + * Multi planar tiled formats have never been tested, check that > > + * buffer restrictions and source cropping meet the format layout > > + * expectations. > > + */ > > + WARN_ON("Multi-planar tiled formats unsupported"); > > + WARN_ON(info->tile_w % info->hsub); > > + return info->tile_w / info->hsub; > > +} > > +EXPORT_SYMBOL(drm_format_tile_width); > > + > > +/** > > + * drm_format_tile_height - height of a tile for tile formats, should be 1 for > > + * all non-tiled formats. > > + * @format: pixel format > > + * @plane: plane index > > + * > > + * Returns: > > + * The height of a tile, depending on the plane index and vertical sub-sampling > > + */ > > +uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) > > +{ > > + WARN_ON(!info->tile_h); > > + if (plane == 0 || info->tile_h == 1) > > + return info->tile_h; > > + > > + /* > > + * Multi planar tiled formats have never been tested, check that > > + * buffer restrictions and source cropping meet the format layout > > + * expectations. > > + */ > > + WARN_ON("Multi-planar tiled formats unsupported"); > > + WARN_ON(info->tile_h % info->vsub); > > + return info->tile_h / info->vsub; > > +} > > +EXPORT_SYMBOL(drm_format_tile_height); > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > index 781af1d42d76..57509e51cb80 100644 > > --- a/drivers/gpu/drm/drm_framebuffer.c > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, > > unsigned int width = fb_plane_width(r->width, info, i); > > unsigned int height = fb_plane_height(r->height, info, i); > > unsigned int cpp = info->cpp[i]; > > + unsigned int tile_w = drm_format_tile_width(info, i); > > + unsigned int tile_h = drm_format_tile_height(info, i); > > + unsigned int tile_size = cpp * tile_w * tile_h; > > + unsigned int num_htiles; > > + unsigned int num_vtiles; > > > > if (!r->handles[i]) { > > DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i); > > return -EINVAL; > > } > > > > - if ((uint64_t) width * cpp > UINT_MAX) > > + if ((width % tile_w) || (height % tile_h)) { > > I think this is too strict. You can carve out a sub-part of anything > really with a drm_framebuffer. See below, we only checked that width * cpp > < pitches, so leftover bytes/bits was always ok. > > Your driver might have additional constraints, but that's a different > story.

This seem like the only thing that we have in common with DRM_FORMAT_MOD_SAMSUNG_64_32_TILE/NV12MT, which is even more restrictive than that and it wants the width to be a multiple of 128.

> > > + DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n"); > > + return -EINVAL; > > + } > > + > > + num_htiles = width / tile_w; > > + num_vtiles = height / tile_h; > > + > > + if ((uint64_t)num_htiles * tile_size > UINT_MAX) > > return -ERANGE; > > > > - if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX) > > + if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX) > > return -ERANGE; > > > > - if (r->pitches[i] < width * cpp) { > > + if (r->pitches[i] < num_htiles * tile_size) { > > Essentially you define ->pitches now to mean a row of tiles. At least for > bigger tiled formats (using modifiers) we don't use it like that. But it > also gets awkward quickly if we keep pitches to mean bytes in one row of > pixels. For that case we'd have

That's how everything started, we have already internal userspace and drivers that expect and pass pitch as a row of tiles in bytes.

For the tiles format in this patchset(X0L2/X0L0) which orders the tiles in linear manner the pitch will at least get you at the beginning of a row.

I'm not sure I understand what's the meaning of a pitch for DRM_FORMAT_MOD_SAMSUNG_64_32_TILE which orders the tiles in Z shape. How far will a pitch for this format get you, it seems that using just witdth * cpp will get you somewhere in the middle.

So there's 2 interpretations of pitch if you have a tile/block layout:

• What you do, where pitch = num_tiles_per_row * tile_size. Adding pitch to the base address will then advance tile_h pixels. Let's call this alex_pitch.

Is more or less mali_pitch, rather than alex_pitch.

...

...

...

• What I'm proposing, and what NV12MT does, and what every other driver does (afaik, I didn't carefully check them all): Pretend there's no tile, and compute the pitch as if it's a linear format. The reason behind this is that fairly often for tiled layouts (but not always), you have magic remapping hardware, which can present a linear layout to you. So for NV12MT that would resolve the Z-shape and macroblock layout, and give you a normal NV12 framebuffer. Let's call this drm_pitch. Note that the drm pitch does _not_ make any sense when you add it to the physical base address, only if you add mutliple's of tile_h, to get an entire tile row.

That's exactly what I'm proposing, for linear tile formats we should use a pitch that naturaly has sense which is drm_pitch * tile_h.

For any other shape of arranging arranging the tiles like the Z shape of NV12MT, the fake pitch meaning would remain unchanged.

...

...

...

The relationship is drm_pitch = alex_pitch / tile_h. I'm not aware of any format where that doesn't devide evenly, so in practice it doesn't matter which one you pick. Imo either has up/downsides.

Since everyone is already using the linear pitch approach I'd suggest sticking with that. Might make life a little easier for generic userspace (though it still needs to know other details of the tiling format of course).

Yeah, that's what I meant to add/clarify. The fake linear pitch is, to the best of my knowledge, already the standard we're using in drm. -Daniel

Everyone except Mali. Still, it seems like a good idea to transform something "fake" in something with meaning, of course without breaking the userspace :).

Do you mean this is already used like this in drm/malidp code? Or does "mali" here stand for the blob? Need to know, so we know what we'd break.

Blob and a bunch of internal stuff, nothing in the mainline. That's why I was trying to use the same convention in drm/malidp.

As Ville points out in his reply, drm_pitch is pretty much what the drm_framebuffer pitch is defined as, so we can't change that.

-Daniel

Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch