[PATCH 1/3] drm: drm_fourcc: add Samsung 16x16 tile format

[PATCH 1/3] drm: drm_fourcc: add Samsung 16x16 tile format

[Marek Szyprowski]

From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

Add modifier for tiled formats used by graphics modules found in Samsung
Exynos5250/542x/5433 SoCs. This is a simple tiled layout using tiles
of 16x16 pixels in a row-major layout.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 include/uapi/drm/drm_fourcc.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 721ab7e54d96..5631b196c07a 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -299,6 +299,15 @@ extern "C" {
  */
 #define DRM_FORMAT_MOD_SAMSUNG_64_32_TILE	fourcc_mod_code(SAMSUNG, 1)
 
+/*
+ * Tiled, 16 (pixels) x 16 (lines) - sized macroblocks
+ *
+ * This is a simple tiled layout using tiles of 16x16 pixels in a row-major
+ * layout. For YCbCr formats Cb/Cr components are taken in such a way that
+ * they correspond to their 16x16 luma block.
+ */
+#define DRM_FORMAT_MOD_SAMSUNG_16_16_TILE	fourcc_mod_code(SAMSUNG, 2)
+
 /*
  * Qualcomm Compressed Format
  *
-- 
2.17.1

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

[PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Marek Szyprowski]

From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_scaler.c | 133 ++++++++++++---------
 1 file changed, 75 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_scaler.c b/drivers/gpu/drm/exynos/exynos_drm_scaler.c
index 0ddb6eec7b11..8e761ef63eac 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_scaler.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_scaler.c
@@ -49,56 +49,46 @@ struct scaler_context {
 	const struct scaler_data	*scaler_data;
 };
 
-static u32 scaler_get_format(u32 drm_fmt)
+struct scaler_format {
+	u32	drm_fmt;
+	u32	internal_fmt;
+	u32	chroma_tile_w;
+	u32	chroma_tile_h;
+};
+
+static const struct scaler_format scaler_formats[] = {
+	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
+	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
+	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
+	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
+	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
+	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
+	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
+	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
+	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
+	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
+	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
+	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
+	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
+	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
+	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
+	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
+	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
+	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
+	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
+	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
+	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },
+};
+
+static const struct scaler_format *scaler_get_format(u32 drm_fmt)
 {
-	switch (drm_fmt) {
-	case DRM_FORMAT_NV12:
-		return SCALER_YUV420_2P_UV;
-	case DRM_FORMAT_NV21:
-		return SCALER_YUV420_2P_VU;
-	case DRM_FORMAT_YUV420:
-		return SCALER_YUV420_3P;
-	case DRM_FORMAT_YUYV:
-		return SCALER_YUV422_1P_YUYV;
-	case DRM_FORMAT_UYVY:
-		return SCALER_YUV422_1P_UYVY;
-	case DRM_FORMAT_YVYU:
-		return SCALER_YUV422_1P_YVYU;
-	case DRM_FORMAT_NV16:
-		return SCALER_YUV422_2P_UV;
-	case DRM_FORMAT_NV61:
-		return SCALER_YUV422_2P_VU;
-	case DRM_FORMAT_YUV422:
-		return SCALER_YUV422_3P;
-	case DRM_FORMAT_NV24:
-		return SCALER_YUV444_2P_UV;
-	case DRM_FORMAT_NV42:
-		return SCALER_YUV444_2P_VU;
-	case DRM_FORMAT_YUV444:
-		return SCALER_YUV444_3P;
-	case DRM_FORMAT_RGB565:
-		return SCALER_RGB_565;
-	case DRM_FORMAT_XRGB1555:
-		return SCALER_ARGB1555;
-	case DRM_FORMAT_ARGB1555:
-		return SCALER_ARGB1555;
-	case DRM_FORMAT_XRGB4444:
-		return SCALER_ARGB4444;
-	case DRM_FORMAT_ARGB4444:
-		return SCALER_ARGB4444;
-	case DRM_FORMAT_XRGB8888:
-		return SCALER_ARGB8888;
-	case DRM_FORMAT_ARGB8888:
-		return SCALER_ARGB8888;
-	case DRM_FORMAT_RGBX8888:
-		return SCALER_RGBA8888;
-	case DRM_FORMAT_RGBA8888:
-		return SCALER_RGBA8888;
-	default:
-		break;
-	}
+	int i;
 
-	return 0;
+	for (i = 0; i < ARRAY_SIZE(scaler_formats); i++)
+		if (scaler_formats[i].drm_fmt == drm_fmt)
+			return &scaler_formats[i];
+
+	return NULL;
 }
 
 static inline int scaler_reset(struct scaler_context *scaler)
@@ -152,11 +142,11 @@ static inline void scaler_enable_int(struct scaler_context *scaler)
 }
 
 static inline void scaler_set_src_fmt(struct scaler_context *scaler,
-	u32 src_fmt)
+	u32 src_fmt, u32 tile)
 {
 	u32 val;
 
-	val = SCALER_SRC_CFG_SET_COLOR_FORMAT(src_fmt);
+	val = SCALER_SRC_CFG_SET_COLOR_FORMAT(src_fmt) | (tile << 10);
 	scaler_write(val, SCALER_SRC_CFG);
 }
 
@@ -188,15 +178,19 @@ static inline void scaler_set_src_span(struct scaler_context *scaler,
 	scaler_write(val, SCALER_SRC_SPAN);
 }
 
-static inline void scaler_set_src_luma_pos(struct scaler_context *scaler,
-	struct drm_exynos_ipp_task_rect *src_pos)
+static inline void scaler_set_src_luma_chroma_pos(struct scaler_context *scaler,
+	struct drm_exynos_ipp_task_rect *src_pos, const struct scaler_format *fmt)
 {
 	u32 val;
 
 	val = SCALER_SRC_Y_POS_SET_YH_POS(src_pos->x << 2);
 	val |=  SCALER_SRC_Y_POS_SET_YV_POS(src_pos->y << 2);
 	scaler_write(val, SCALER_SRC_Y_POS);
-	scaler_write(val, SCALER_SRC_C_POS); /* ATTENTION! */
+	val = SCALER_SRC_C_POS_SET_CH_POS(
+		(src_pos->x * fmt->chroma_tile_w / 16) << 2);
+	val |=  SCALER_SRC_C_POS_SET_CV_POS(
+		(src_pos->y * fmt->chroma_tile_h / 16) << 2);
+	scaler_write(val, SCALER_SRC_C_POS);
 }
 
 static inline void scaler_set_src_wh(struct scaler_context *scaler,
@@ -366,10 +360,10 @@ static int scaler_commit(struct exynos_drm_ipp *ipp,
 	struct scaler_context *scaler =
 			container_of(ipp, struct scaler_context, ipp);
 
-	u32 src_fmt = scaler_get_format(task->src.buf.fourcc);
+	const struct scaler_format *src_fmt = scaler_get_format(task->src.buf.fourcc);
 	struct drm_exynos_ipp_task_rect *src_pos = &task->src.rect;
 
-	u32 dst_fmt = scaler_get_format(task->dst.buf.fourcc);
+	const struct scaler_format *dst_fmt = scaler_get_format(task->dst.buf.fourcc);
 	struct drm_exynos_ipp_task_rect *dst_pos = &task->dst.rect;
 
 	pm_runtime_get_sync(scaler->dev);
@@ -380,13 +374,14 @@ static int scaler_commit(struct exynos_drm_ipp *ipp,
 
 	scaler->task = task;
 
-	scaler_set_src_fmt(scaler, src_fmt);
+	scaler_set_src_fmt(
+		scaler, src_fmt->internal_fmt, task->src.buf.modifier != 0);
 	scaler_set_src_base(scaler, &task->src);
 	scaler_set_src_span(scaler, &task->src);
-	scaler_set_src_luma_pos(scaler, src_pos);
+	scaler_set_src_luma_chroma_pos(scaler, src_pos, src_fmt);
 	scaler_set_src_wh(scaler, src_pos);
 
-	scaler_set_dst_fmt(scaler, dst_fmt);
+	scaler_set_dst_fmt(scaler, dst_fmt->internal_fmt);
 	scaler_set_dst_base(scaler, &task->dst);
 	scaler_set_dst_span(scaler, &task->dst);
 	scaler_set_dst_luma_pos(scaler, dst_pos);
@@ -617,6 +612,16 @@ static const struct drm_exynos_ipp_limit scaler_5420_one_pixel_limits[] = {
 			  .v = { 65536 * 1 / 4, 65536 * 16 }) },
 };
 
+static const struct drm_exynos_ipp_limit scaler_5420_tile_limits[] = {
+	{ IPP_SIZE_LIMIT(BUFFER, .h = { 16, SZ_8K }, .v = { 16, SZ_8K })},
+	{ IPP_SIZE_LIMIT(AREA, .h.align = 16, .v.align = 16) },
+	{ IPP_SCALE_LIMIT(.h = {1, 1}, .v = {1, 1})},
+	{ }
+};
+
+#define IPP_SRCDST_TILE_FORMAT(f, l)	\
+	IPP_SRCDST_MFORMAT(f, DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, (l))
+
 static const struct exynos_drm_ipp_formats exynos5420_formats[] = {
 	/* SCALER_YUV420_2P_UV */
 	{ IPP_SRCDST_FORMAT(NV21, scaler_5420_two_pixel_hv_limits) },
@@ -680,6 +685,18 @@ static const struct exynos_drm_ipp_formats exynos5420_formats[] = {
 
 	/* SCALER_RGBA8888 */
 	{ IPP_SRCDST_FORMAT(RGBA8888, scaler_5420_one_pixel_limits) },
+
+	/* SCALER_YUV420_2P_UV TILE */
+	{ IPP_SRCDST_TILE_FORMAT(NV21, scaler_5420_tile_limits) },
+
+	/* SCALER_YUV420_2P_VU TILE */
+	{ IPP_SRCDST_TILE_FORMAT(NV12, scaler_5420_tile_limits) },
+
+	/* SCALER_YUV420_3P TILE */
+	{ IPP_SRCDST_TILE_FORMAT(YUV420, scaler_5420_tile_limits) },
+
+	/* SCALER_YUV422_1P_YUYV TILE */
+	{ IPP_SRCDST_TILE_FORMAT(YUYV, scaler_5420_tile_limits) },
 };
 
 static const struct scaler_data exynos5420_data = {
-- 
2.17.1

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

[PATCH 3/3] drm/exynos: gsc: Add support for tiled formats

[Marek Szyprowski]

Add support for 16x16 tiled NV12 and NV21 formats.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gsc.c | 46 ++++++++++++++++++-------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
index 7ba414b52faa..ce15d46bfce8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
@@ -448,7 +448,7 @@ static void gsc_handle_irq(struct gsc_context *ctx, bool enable,
 }
 
 
-static void gsc_src_set_fmt(struct gsc_context *ctx, u32 fmt)
+static void gsc_src_set_fmt(struct gsc_context *ctx, u32 fmt, bool tiled)
 {
 	u32 cfg;
 
@@ -514,6 +514,9 @@ static void gsc_src_set_fmt(struct gsc_context *ctx, u32 fmt)
 		break;
 	}
 
+	if (tiled)
+		cfg |= (GSC_IN_TILE_C_16x8 | GSC_IN_TILE_MODE);
+
 	gsc_write(cfg, GSC_IN_CON);
 }
 
@@ -632,7 +635,7 @@ static void gsc_src_set_addr(struct gsc_context *ctx, u32 buf_id,
 	gsc_src_set_buf_seq(ctx, buf_id, true);
 }
 
-static void gsc_dst_set_fmt(struct gsc_context *ctx, u32 fmt)
+static void gsc_dst_set_fmt(struct gsc_context *ctx, u32 fmt, bool tiled)
 {
 	u32 cfg;
 
@@ -698,6 +701,9 @@ static void gsc_dst_set_fmt(struct gsc_context *ctx, u32 fmt)
 		break;
 	}
 
+	if (tiled)
+		cfg |= (GSC_IN_TILE_C_16x8 | GSC_OUT_TILE_MODE);
+
 	gsc_write(cfg, GSC_OUT_CON);
 }
 
@@ -1122,11 +1128,11 @@ static int gsc_commit(struct exynos_drm_ipp *ipp,
 		return ret;
 	}
 
-	gsc_src_set_fmt(ctx, task->src.buf.fourcc);
+	gsc_src_set_fmt(ctx, task->src.buf.fourcc, task->src.buf.modifier);
 	gsc_src_set_transf(ctx, task->transform.rotation);
 	gsc_src_set_size(ctx, &task->src);
 	gsc_src_set_addr(ctx, 0, &task->src);
-	gsc_dst_set_fmt(ctx, task->dst.buf.fourcc);
+	gsc_dst_set_fmt(ctx, task->dst.buf.fourcc, task->dst.buf.modifier);
 	gsc_dst_set_size(ctx, &task->dst);
 	gsc_dst_set_addr(ctx, 0, &task->dst);
 	gsc_set_prescaler(ctx, &ctx->sc, &task->src.rect, &task->dst.rect);
@@ -1200,6 +1206,10 @@ static const unsigned int gsc_formats[] = {
 	DRM_FORMAT_YUV420, DRM_FORMAT_YVU420, DRM_FORMAT_YUV422,
 };
 
+static const unsigned int gsc_tiled_formats[] = {
+	DRM_FORMAT_NV12, DRM_FORMAT_NV21,
+};
+
 static int gsc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1207,23 +1217,24 @@ static int gsc_probe(struct platform_device *pdev)
 	struct exynos_drm_ipp_formats *formats;
 	struct gsc_context *ctx;
 	struct resource *res;
-	int ret, i;
+	int num_formats, ret, i, j;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
-	formats = devm_kcalloc(dev,
-			       ARRAY_SIZE(gsc_formats), sizeof(*formats),
-			       GFP_KERNEL);
-	if (!formats)
-		return -ENOMEM;
-
 	driver_data = (struct gsc_driverdata *)of_device_get_match_data(dev);
 	ctx->dev = dev;
 	ctx->num_clocks = driver_data->num_clocks;
 	ctx->clk_names = driver_data->clk_names;
 
+	/* construct formats/limits array */
+	num_formats = ARRAY_SIZE(gsc_formats) + ARRAY_SIZE(gsc_tiled_formats);
+	formats = devm_kcalloc(dev, num_formats, sizeof(*formats), GFP_KERNEL);
+	if (!formats)
+		return -ENOMEM;
+
+	/* linear formats */
 	for (i = 0; i < ARRAY_SIZE(gsc_formats); i++) {
 		formats[i].fourcc = gsc_formats[i];
 		formats[i].type = DRM_EXYNOS_IPP_FORMAT_SOURCE |
@@ -1231,8 +1242,19 @@ static int gsc_probe(struct platform_device *pdev)
 		formats[i].limits = driver_data->limits;
 		formats[i].num_limits = driver_data->num_limits;
 	}
+
+	/* tiled formats */
+	for (j = i, i = 0; i < ARRAY_SIZE(gsc_tiled_formats); j++, i++) {
+		formats[j].fourcc = gsc_tiled_formats[i];
+		formats[j].modifier = DRM_FORMAT_MOD_SAMSUNG_16_16_TILE;
+		formats[j].type = DRM_EXYNOS_IPP_FORMAT_SOURCE |
+				  DRM_EXYNOS_IPP_FORMAT_DESTINATION;
+		formats[j].limits = driver_data->limits;
+		formats[j].num_limits = driver_data->num_limits;
+	}
+
 	ctx->formats = formats;
-	ctx->num_formats = ARRAY_SIZE(gsc_formats);
+	ctx->num_formats = num_formats;
 
 	/* clock control */
 	for (i = 0; i < ctx->num_clocks; i++) {
-- 
2.17.1

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

Re: [PATCH 1/3] drm: drm_fourcc: add Samsung 16x16 tile format

[Inki Dae]

2018년 08월 10일 22:28에 Marek Szyprowski 이(가) 쓴 글:
> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> 
> Add modifier for tiled formats used by graphics modules found in Samsung
> Exynos5250/542x/5433 SoCs. This is a simple tiled layout using tiles
> of 16x16 pixels in a row-major layout.

Reviewed-by: Inki Dae <inki.dae@samsung.com>

Thanks,
Inki Dae

> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 721ab7e54d96..5631b196c07a 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -299,6 +299,15 @@ extern "C" {
>   */
>  #define DRM_FORMAT_MOD_SAMSUNG_64_32_TILE	fourcc_mod_code(SAMSUNG, 1)
>  
> +/*
> + * Tiled, 16 (pixels) x 16 (lines) - sized macroblocks
> + *
> + * This is a simple tiled layout using tiles of 16x16 pixels in a row-major
> + * layout. For YCbCr formats Cb/Cr components are taken in such a way that
> + * they correspond to their 16x16 luma block.
> + */
> +#define DRM_FORMAT_MOD_SAMSUNG_16_16_TILE	fourcc_mod_code(SAMSUNG, 2)
> +
>  /*
>   * Qualcomm Compressed Format
>   *
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: drm_fourcc: add Samsung 16x16 tile format

[Inki Dae]

Hi,

2018년 08월 27일 09:38에 Inki Dae 이(가) 쓴 글:
> 
> 
> 2018년 08월 10일 22:28에 Marek Szyprowski 이(가) 쓴 글:
>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>
>> Add modifier for tiled formats used by graphics modules found in Samsung
>> Exynos5250/542x/5433 SoCs. This is a simple tiled layout using tiles
>> of 16x16 pixels in a row-major layout.
> 
> Reviewed-by: Inki Dae <inki.dae@samsung.com>
> 

This patch is required by below two patches which add Samsung 16x16 tile format support to gsc and scaler drivers.
https://patchwork.kernel.org/patch/10562707/
https://patchwork.kernel.org/patch/10562713/

Could you give me ack-by so that I could request GIT-PULL together?

Thanks,
InkI Dae

> Thanks,
> Inki Dae
> 
>>
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  include/uapi/drm/drm_fourcc.h | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>> index 721ab7e54d96..5631b196c07a 100644
>> --- a/include/uapi/drm/drm_fourcc.h
>> +++ b/include/uapi/drm/drm_fourcc.h
>> @@ -299,6 +299,15 @@ extern "C" {
>>   */
>>  #define DRM_FORMAT_MOD_SAMSUNG_64_32_TILE	fourcc_mod_code(SAMSUNG, 1)
>>  
>> +/*
>> + * Tiled, 16 (pixels) x 16 (lines) - sized macroblocks
>> + *
>> + * This is a simple tiled layout using tiles of 16x16 pixels in a row-major
>> + * layout. For YCbCr formats Cb/Cr components are taken in such a way that
>> + * they correspond to their 16x16 luma block.
>> + */
>> +#define DRM_FORMAT_MOD_SAMSUNG_16_16_TILE	fourcc_mod_code(SAMSUNG, 2)
>> +
>>  /*
>>   * Qualcomm Compressed Format
>>   *
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Inki Dae]

Hi Marek and Andrzej,

2018년 08월 10일 22:29에 Marek Szyprowski 이(가) 쓴 글:
> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> 
> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_scaler.c | 133 ++++++++++++---------
>  1 file changed, 75 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_scaler.c b/drivers/gpu/drm/exynos/exynos_drm_scaler.c
> index 0ddb6eec7b11..8e761ef63eac 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_scaler.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_scaler.c
> @@ -49,56 +49,46 @@ struct scaler_context {
>  	const struct scaler_data	*scaler_data;
>  };
>  
> -static u32 scaler_get_format(u32 drm_fmt)
> +struct scaler_format {
> +	u32	drm_fmt;
> +	u32	internal_fmt;
> +	u32	chroma_tile_w;
> +	u32	chroma_tile_h;
> +};
> +
> +static const struct scaler_format scaler_formats[] = {
> +	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
> +	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
> +	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
> +	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
> +	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
> +	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
> +	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
> +	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
> +	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
> +	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
> +	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
> +	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
> +	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
> +	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
> +	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
> +	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
> +	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
> +	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
> +	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
> +	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
> +	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },

Seems the tile size of each format you declared above is wrong.
According to data sheet for Exynos5420/5422/5433, each plane has different tile size.
I.e., SACLE_YUV420_2P_UV/VU has two planes, and Y plane has 16 x 16 tile size but U and V plane have 8 x 8 tile size each other.

However, above declaration has only one tile size and it means that each plane has always same tile size.
And also RGB formats have 16 x 16 tile size in tile mode but you declared it as 0.

Could you check it out again?

Thanks,
Inki Dae

> +};
> +
> +static const struct scaler_format *scaler_get_format(u32 drm_fmt)
>  {
> -	switch (drm_fmt) {
> -	case DRM_FORMAT_NV12:
> -		return SCALER_YUV420_2P_UV;
> -	case DRM_FORMAT_NV21:
> -		return SCALER_YUV420_2P_VU;
> -	case DRM_FORMAT_YUV420:
> -		return SCALER_YUV420_3P;
> -	case DRM_FORMAT_YUYV:
> -		return SCALER_YUV422_1P_YUYV;
> -	case DRM_FORMAT_UYVY:
> -		return SCALER_YUV422_1P_UYVY;
> -	case DRM_FORMAT_YVYU:
> -		return SCALER_YUV422_1P_YVYU;
> -	case DRM_FORMAT_NV16:
> -		return SCALER_YUV422_2P_UV;
> -	case DRM_FORMAT_NV61:
> -		return SCALER_YUV422_2P_VU;
> -	case DRM_FORMAT_YUV422:
> -		return SCALER_YUV422_3P;
> -	case DRM_FORMAT_NV24:
> -		return SCALER_YUV444_2P_UV;
> -	case DRM_FORMAT_NV42:
> -		return SCALER_YUV444_2P_VU;
> -	case DRM_FORMAT_YUV444:
> -		return SCALER_YUV444_3P;
> -	case DRM_FORMAT_RGB565:
> -		return SCALER_RGB_565;
> -	case DRM_FORMAT_XRGB1555:
> -		return SCALER_ARGB1555;
> -	case DRM_FORMAT_ARGB1555:
> -		return SCALER_ARGB1555;
> -	case DRM_FORMAT_XRGB4444:
> -		return SCALER_ARGB4444;
> -	case DRM_FORMAT_ARGB4444:
> -		return SCALER_ARGB4444;
> -	case DRM_FORMAT_XRGB8888:
> -		return SCALER_ARGB8888;
> -	case DRM_FORMAT_ARGB8888:
> -		return SCALER_ARGB8888;
> -	case DRM_FORMAT_RGBX8888:
> -		return SCALER_RGBA8888;
> -	case DRM_FORMAT_RGBA8888:
> -		return SCALER_RGBA8888;
> -	default:
> -		break;
> -	}
> +	int i;
>  
> -	return 0;
> +	for (i = 0; i < ARRAY_SIZE(scaler_formats); i++)
> +		if (scaler_formats[i].drm_fmt == drm_fmt)
> +			return &scaler_formats[i];
> +
> +	return NULL;
>  }
>  
>  static inline int scaler_reset(struct scaler_context *scaler)
> @@ -152,11 +142,11 @@ static inline void scaler_enable_int(struct scaler_context *scaler)
>  }
>  
>  static inline void scaler_set_src_fmt(struct scaler_context *scaler,
> -	u32 src_fmt)
> +	u32 src_fmt, u32 tile)
>  {
>  	u32 val;
>  
> -	val = SCALER_SRC_CFG_SET_COLOR_FORMAT(src_fmt);
> +	val = SCALER_SRC_CFG_SET_COLOR_FORMAT(src_fmt) | (tile << 10);
>  	scaler_write(val, SCALER_SRC_CFG);
>  }
>  
> @@ -188,15 +178,19 @@ static inline void scaler_set_src_span(struct scaler_context *scaler,
>  	scaler_write(val, SCALER_SRC_SPAN);
>  }
>  
> -static inline void scaler_set_src_luma_pos(struct scaler_context *scaler,
> -	struct drm_exynos_ipp_task_rect *src_pos)
> +static inline void scaler_set_src_luma_chroma_pos(struct scaler_context *scaler,
> +	struct drm_exynos_ipp_task_rect *src_pos, const struct scaler_format *fmt)
>  {
>  	u32 val;
>  
>  	val = SCALER_SRC_Y_POS_SET_YH_POS(src_pos->x << 2);
>  	val |=  SCALER_SRC_Y_POS_SET_YV_POS(src_pos->y << 2);
>  	scaler_write(val, SCALER_SRC_Y_POS);
> -	scaler_write(val, SCALER_SRC_C_POS); /* ATTENTION! */
> +	val = SCALER_SRC_C_POS_SET_CH_POS(
> +		(src_pos->x * fmt->chroma_tile_w / 16) << 2);
> +	val |=  SCALER_SRC_C_POS_SET_CV_POS(
> +		(src_pos->y * fmt->chroma_tile_h / 16) << 2);
> +	scaler_write(val, SCALER_SRC_C_POS);
>  }
>  
>  static inline void scaler_set_src_wh(struct scaler_context *scaler,
> @@ -366,10 +360,10 @@ static int scaler_commit(struct exynos_drm_ipp *ipp,
>  	struct scaler_context *scaler =
>  			container_of(ipp, struct scaler_context, ipp);
>  
> -	u32 src_fmt = scaler_get_format(task->src.buf.fourcc);
> +	const struct scaler_format *src_fmt = scaler_get_format(task->src.buf.fourcc);
>  	struct drm_exynos_ipp_task_rect *src_pos = &task->src.rect;
>  
> -	u32 dst_fmt = scaler_get_format(task->dst.buf.fourcc);
> +	const struct scaler_format *dst_fmt = scaler_get_format(task->dst.buf.fourcc);
>  	struct drm_exynos_ipp_task_rect *dst_pos = &task->dst.rect;
>  
>  	pm_runtime_get_sync(scaler->dev);
> @@ -380,13 +374,14 @@ static int scaler_commit(struct exynos_drm_ipp *ipp,
>  
>  	scaler->task = task;
>  
> -	scaler_set_src_fmt(scaler, src_fmt);
> +	scaler_set_src_fmt(
> +		scaler, src_fmt->internal_fmt, task->src.buf.modifier != 0);
>  	scaler_set_src_base(scaler, &task->src);
>  	scaler_set_src_span(scaler, &task->src);
> -	scaler_set_src_luma_pos(scaler, src_pos);
> +	scaler_set_src_luma_chroma_pos(scaler, src_pos, src_fmt);
>  	scaler_set_src_wh(scaler, src_pos);
>  
> -	scaler_set_dst_fmt(scaler, dst_fmt);
> +	scaler_set_dst_fmt(scaler, dst_fmt->internal_fmt);
>  	scaler_set_dst_base(scaler, &task->dst);
>  	scaler_set_dst_span(scaler, &task->dst);
>  	scaler_set_dst_luma_pos(scaler, dst_pos);
> @@ -617,6 +612,16 @@ static const struct drm_exynos_ipp_limit scaler_5420_one_pixel_limits[] = {
>  			  .v = { 65536 * 1 / 4, 65536 * 16 }) },
>  };
>  
> +static const struct drm_exynos_ipp_limit scaler_5420_tile_limits[] = {
> +	{ IPP_SIZE_LIMIT(BUFFER, .h = { 16, SZ_8K }, .v = { 16, SZ_8K })},
> +	{ IPP_SIZE_LIMIT(AREA, .h.align = 16, .v.align = 16) },
> +	{ IPP_SCALE_LIMIT(.h = {1, 1}, .v = {1, 1})},
> +	{ }
> +};
> +
> +#define IPP_SRCDST_TILE_FORMAT(f, l)	\
> +	IPP_SRCDST_MFORMAT(f, DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, (l))
> +
>  static const struct exynos_drm_ipp_formats exynos5420_formats[] = {
>  	/* SCALER_YUV420_2P_UV */
>  	{ IPP_SRCDST_FORMAT(NV21, scaler_5420_two_pixel_hv_limits) },
> @@ -680,6 +685,18 @@ static const struct exynos_drm_ipp_formats exynos5420_formats[] = {
>  
>  	/* SCALER_RGBA8888 */
>  	{ IPP_SRCDST_FORMAT(RGBA8888, scaler_5420_one_pixel_limits) },
> +
> +	/* SCALER_YUV420_2P_UV TILE */
> +	{ IPP_SRCDST_TILE_FORMAT(NV21, scaler_5420_tile_limits) },
> +
> +	/* SCALER_YUV420_2P_VU TILE */
> +	{ IPP_SRCDST_TILE_FORMAT(NV12, scaler_5420_tile_limits) },
> +
> +	/* SCALER_YUV420_3P TILE */
> +	{ IPP_SRCDST_TILE_FORMAT(YUV420, scaler_5420_tile_limits) },
> +
> +	/* SCALER_YUV422_1P_YUYV TILE */
> +	{ IPP_SRCDST_TILE_FORMAT(YUYV, scaler_5420_tile_limits) },
>  };
>  
>  static const struct scaler_data exynos5420_data = {
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Andrzej Pietrasiewicz]

Hi Inki,

W dniu 12.09.2018 o 01:54, Inki Dae pisze:
> Hi Marek and Andrzej,
> 
> 2018년 08월 10일 22:29에 Marek Szyprowski 이(가) 쓴 글:
>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>
>> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
>>

<snip>

>>   
>> -static u32 scaler_get_format(u32 drm_fmt)
>> +struct scaler_format {
>> +	u32	drm_fmt;
>> +	u32	internal_fmt;
>> +	u32	chroma_tile_w;
>> +	u32	chroma_tile_h;
>> +};
>> +
>> +static const struct scaler_format scaler_formats[] = {
>> +	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
>> +	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
>> +	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
>> +	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
>> +	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
>> +	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
>> +	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
>> +	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
>> +	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
>> +	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
>> +	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
>> +	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
>> +	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
>> +	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
>> +	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
>> +	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
>> +	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
>> +	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
>> +	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
>> +	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
>> +	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },
> 
> Seems the tile size of each format you declared above is wrong.
> According to data sheet for Exynos5420/5422/5433, each plane has different tile size.
> I.e., SACLE_YUV420_2P_UV/VU has two planes, and Y plane has 16 x 16 tile size but U and V plane have 8 x 8 tile size each other.
> 
> However, above declaration has only one tile size

true, but the members of struct scaler_format are called

_____chroma_____tile_w/h

> and it means that each plane has always same tile size.

this conclusion is not justified

> And also RGB formats have 16 x 16 tile size in tile mode but you declared it as 0.

The data sheet says, that all formats have the same Y/RGB tile size
and it is always 16x16. That is why only chroma tile size is remembered,
because only chroma tile size can be different between formats.

RGB formats don't have any chroma component, hence zero chroma_tile_w/h.

Regards,

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

Re: [PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Inki Dae]

2018년 09월 12일 15:59에 Andrzej Pietrasiewicz 이(가) 쓴 글:
> Hi Inki,
> 
> W dniu 12.09.2018 o 01:54, Inki Dae pisze:
>> Hi Marek and Andrzej,
>>
>> 2018년 08월 10일 22:29에 Marek Szyprowski 이(가) 쓴 글:
>>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>>
>>> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
>>>
> 
> <snip>
> 
>>>   
>>> -static u32 scaler_get_format(u32 drm_fmt)
>>> +struct scaler_format {
>>> +	u32	drm_fmt;
>>> +	u32	internal_fmt;
>>> +	u32	chroma_tile_w;
>>> +	u32	chroma_tile_h;
>>> +};
>>> +
>>> +static const struct scaler_format scaler_formats[] = {
>>> +	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
>>> +	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
>>> +	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
>>> +	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
>>> +	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
>>> +	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
>>> +	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
>>> +	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
>>> +	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
>>> +	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
>>> +	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
>>> +	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
>>> +	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
>>> +	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
>>> +	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
>>> +	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
>>> +	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
>>> +	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
>>> +	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
>>> +	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
>>> +	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },
>>
>> Seems the tile size of each format you declared above is wrong.
>> According to data sheet for Exynos5420/5422/5433, each plane has different tile size.
>> I.e., SACLE_YUV420_2P_UV/VU has two planes, and Y plane has 16 x 16 tile size but U and V plane have 8 x 8 tile size each other.
>>
>> However, above declaration has only one tile size
> 
> true, but the members of struct scaler_format are called
> 
> _____chroma_____tile_w/h
> 
>> and it means that each plane has always same tile size.
> 
> this conclusion is not justified
> 
>> And also RGB formats have 16 x 16 tile size in tile mode but you declared it as 0.
> 
> The data sheet says, that all formats have the same Y/RGB tile size
> and it is always 16x16. That is why only chroma tile size is remembered,
> because only chroma tile size can be different between formats.

Ok, chroma_tile_h/w are used to calculate only the position of the chroma buffer.

By the way, user space would want to know the size limit in tiled mode so that they can allocate each buffer for Y(luma) and CbCr(chroma).
However, below code would provide Y/RGB tile size limit - 16 - to user space,
static const struct drm_exynos_ipp_limit scaler_5420_tile_limits[] = {          
         { IPP_SIZE_LIMIT(BUFFER, .h = { 16, SZ_8K }, .v = { 16, SZ_8K })},      
         { IPP_SIZE_LIMIT(AREA, .h.align = 16, .v.align = 16) },     

It would mean that user space will allocate 16 pixels aligned buffer even for chroma but the actual size limit of it would be 8 pixels in case of SCALE_YUV420_WP_UV/VU foramt. 
Is there some code I'm missing?

Thanks,
Inki Dae

> 
> RGB formats don't have any chroma component, hence zero chroma_tile_w/h.
> 
> Regards,
> 
> Andrzej
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Marek Szyprowski]

Hi Inki,

On 2018-09-13 07:14, Inki Dae wrote:
> 2018년 09월 12일 15:59에 Andrzej Pietrasiewicz 이(가) 쓴 글:
>> W dniu 12.09.2018 o 01:54, Inki Dae pisze:
>>> Hi Marek and Andrzej,
>>>
>>> 2018년 08월 10일 22:29에 Marek Szyprowski 이(가) 쓴 글:
>>>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>>>
>>>> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
>> <snip>
>>
>>>> -static u32 scaler_get_format(u32 drm_fmt)
>>>> +struct scaler_format {
>>>> +	u32	drm_fmt;
>>>> +	u32	internal_fmt;
>>>> +	u32	chroma_tile_w;
>>>> +	u32	chroma_tile_h;
>>>> +};
>>>> +
>>>> +static const struct scaler_format scaler_formats[] = {
>>>> +	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
>>>> +	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
>>>> +	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
>>>> +	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
>>>> +	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
>>>> +	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
>>>> +	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
>>>> +	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
>>>> +	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
>>>> +	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
>>>> +	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
>>>> +	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
>>>> +	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
>>>> +	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
>>>> +	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
>>>> +	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
>>>> +	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
>>>> +	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
>>>> +	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
>>>> +	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
>>>> +	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },
>>> Seems the tile size of each format you declared above is wrong.
>>> According to data sheet for Exynos5420/5422/5433, each plane has different tile size.
>>> I.e., SACLE_YUV420_2P_UV/VU has two planes, and Y plane has 16 x 16 tile size but U and V plane have 8 x 8 tile size each other.
>>>
>>> However, above declaration has only one tile size
>> true, but the members of struct scaler_format are called
>>
>> _____chroma_____tile_w/h
>>
>>> and it means that each plane has always same tile size.
>> this conclusion is not justified
>>
>>> And also RGB formats have 16 x 16 tile size in tile mode but you declared it as 0.
>> The data sheet says, that all formats have the same Y/RGB tile size
>> and it is always 16x16. That is why only chroma tile size is remembered,
>> because only chroma tile size can be different between formats.
> Ok, chroma_tile_h/w are used to calculate only the position of the chroma buffer.
>
> By the way, user space would want to know the size limit in tiled mode so that they can allocate each buffer for Y(luma) and CbCr(chroma).
> However, below code would provide Y/RGB tile size limit - 16 - to user space,
> static const struct drm_exynos_ipp_limit scaler_5420_tile_limits[] = {
>           { IPP_SIZE_LIMIT(BUFFER, .h = { 16, SZ_8K }, .v = { 16, SZ_8K })},
>           { IPP_SIZE_LIMIT(AREA, .h.align = 16, .v.align = 16) },
>
> It would mean that user space will allocate 16 pixels aligned buffer even for chroma but the actual size limit of it would be 8 pixels in case of SCALE_YUV420_WP_UV/VU foramt.
> Is there some code I'm missing?

Userspace knows everything needed to allocate proper buffers. Those
limits describes size of the image buffers in pixels. The size of each
plane (luma or chroma if exists for given format) in bytes directly
comes out of the selected pixel format (fourcc).

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

Re: [PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Inki Dae]

Hi Marek,

2018년 09월 13일 17:03에 Marek Szyprowski 이(가) 쓴 글:
> Hi Inki,
> 
> On 2018-09-13 07:14, Inki Dae wrote:
>> 2018년 09월 12일 15:59에 Andrzej Pietrasiewicz 이(가) 쓴 글:
>>> W dniu 12.09.2018 o 01:54, Inki Dae pisze:
>>>> Hi Marek and Andrzej,
>>>>
>>>> 2018년 08월 10일 22:29에 Marek Szyprowski 이(가) 쓴 글:
>>>>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>>>>
>>>>> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
>>> <snip>
>>>
>>>>> -static u32 scaler_get_format(u32 drm_fmt)
>>>>> +struct scaler_format {
>>>>> +	u32	drm_fmt;
>>>>> +	u32	internal_fmt;
>>>>> +	u32	chroma_tile_w;
>>>>> +	u32	chroma_tile_h;
>>>>> +};
>>>>> +
>>>>> +static const struct scaler_format scaler_formats[] = {
>>>>> +	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
>>>>> +	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
>>>>> +	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
>>>>> +	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
>>>>> +	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
>>>>> +	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
>>>>> +	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
>>>>> +	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
>>>>> +	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
>>>>> +	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
>>>>> +	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
>>>>> +	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
>>>>> +	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
>>>>> +	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
>>>>> +	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
>>>>> +	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
>>>>> +	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
>>>>> +	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
>>>>> +	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
>>>>> +	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
>>>>> +	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },
>>>> Seems the tile size of each format you declared above is wrong.
>>>> According to data sheet for Exynos5420/5422/5433, each plane has different tile size.
>>>> I.e., SACLE_YUV420_2P_UV/VU has two planes, and Y plane has 16 x 16 tile size but U and V plane have 8 x 8 tile size each other.
>>>>
>>>> However, above declaration has only one tile size
>>> true, but the members of struct scaler_format are called
>>>
>>> _____chroma_____tile_w/h
>>>
>>>> and it means that each plane has always same tile size.
>>> this conclusion is not justified
>>>
>>>> And also RGB formats have 16 x 16 tile size in tile mode but you declared it as 0.
>>> The data sheet says, that all formats have the same Y/RGB tile size
>>> and it is always 16x16. That is why only chroma tile size is remembered,
>>> because only chroma tile size can be different between formats.
>> Ok, chroma_tile_h/w are used to calculate only the position of the chroma buffer.
>>
>> By the way, user space would want to know the size limit in tiled mode so that they can allocate each buffer for Y(luma) and CbCr(chroma).
>> However, below code would provide Y/RGB tile size limit - 16 - to user space,
>> static const struct drm_exynos_ipp_limit scaler_5420_tile_limits[] = {
>>           { IPP_SIZE_LIMIT(BUFFER, .h = { 16, SZ_8K }, .v = { 16, SZ_8K })},
>>           { IPP_SIZE_LIMIT(AREA, .h.align = 16, .v.align = 16) },
>>
>> It would mean that user space will allocate 16 pixels aligned buffer even for chroma but the actual size limit of it would be 8 pixels in case of SCALE_YUV420_WP_UV/VU foramt.
>> Is there some code I'm missing?
> 
> Userspace knows everything needed to allocate proper buffers. Those

How does user space know everything? Actually, the size of each plane in tiled mode would depend on Hardware, Scaler device in our case. Is there something user space can know it explictly? Or you mean they can know it implictly?


> limits describes size of the image buffers in pixels. The size of each
> plane (luma or chroma if exists for given format) in bytes directly
> comes out of the selected pixel format (fourcc).

fourcc would say the size not considered for the Hardware limit.

Thanks,
Inki Dae

> 
> Best regards
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Marek Szyprowski]

Hi Inki,

On 2018-09-13 10:23, Inki Dae wrote:
> 2018년 09월 13일 17:03에 Marek Szyprowski 이(가) 쓴 글:
>> On 2018-09-13 07:14, Inki Dae wrote:
>>> 2018년 09월 12일 15:59에 Andrzej Pietrasiewicz 이(가) 쓴 글:
>>>> W dniu 12.09.2018 o 01:54, Inki Dae pisze:
>>>>> Hi Marek and Andrzej,
>>>>>
>>>>> 2018년 08월 10일 22:29에 Marek Szyprowski 이(가) 쓴 글:
>>>>>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>>>>>
>>>>>> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
>>>> <snip>
>>>>
>>>>>> -static u32 scaler_get_format(u32 drm_fmt)
>>>>>> +struct scaler_format {
>>>>>> +	u32	drm_fmt;
>>>>>> +	u32	internal_fmt;
>>>>>> +	u32	chroma_tile_w;
>>>>>> +	u32	chroma_tile_h;
>>>>>> +};
>>>>>> +
>>>>>> +static const struct scaler_format scaler_formats[] = {
>>>>>> +	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
>>>>>> +	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
>>>>>> +	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
>>>>>> +	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
>>>>>> +	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
>>>>>> +	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
>>>>>> +	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
>>>>>> +	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
>>>>>> +	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
>>>>>> +	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
>>>>>> +	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
>>>>>> +	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
>>>>>> +	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
>>>>>> +	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
>>>>>> +	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
>>>>>> +	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
>>>>>> +	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
>>>>>> +	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
>>>>>> +	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
>>>>>> +	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
>>>>>> +	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },
>>>>> Seems the tile size of each format you declared above is wrong.
>>>>> According to data sheet for Exynos5420/5422/5433, each plane has different tile size.
>>>>> I.e., SACLE_YUV420_2P_UV/VU has two planes, and Y plane has 16 x 16 tile size but U and V plane have 8 x 8 tile size each other.
>>>>>
>>>>> However, above declaration has only one tile size
>>>> true, but the members of struct scaler_format are called
>>>>
>>>> _____chroma_____tile_w/h
>>>>
>>>>> and it means that each plane has always same tile size.
>>>> this conclusion is not justified
>>>>
>>>>> And also RGB formats have 16 x 16 tile size in tile mode but you declared it as 0.
>>>> The data sheet says, that all formats have the same Y/RGB tile size
>>>> and it is always 16x16. That is why only chroma tile size is remembered,
>>>> because only chroma tile size can be different between formats.
>>> Ok, chroma_tile_h/w are used to calculate only the position of the chroma buffer.
>>>
>>> By the way, user space would want to know the size limit in tiled mode so that they can allocate each buffer for Y(luma) and CbCr(chroma).
>>> However, below code would provide Y/RGB tile size limit - 16 - to user space,
>>> static const struct drm_exynos_ipp_limit scaler_5420_tile_limits[] = {
>>>            { IPP_SIZE_LIMIT(BUFFER, .h = { 16, SZ_8K }, .v = { 16, SZ_8K })},
>>>            { IPP_SIZE_LIMIT(AREA, .h.align = 16, .v.align = 16) },
>>>
>>> It would mean that user space will allocate 16 pixels aligned buffer even for chroma but the actual size limit of it would be 8 pixels in case of SCALE_YUV420_WP_UV/VU foramt.
>>> Is there some code I'm missing?
>> Userspace knows everything needed to allocate proper buffers. Those
> How does user space know everything? Actually, the size of each plane in tiled mode would depend on Hardware, Scaler device in our case. Is there something user space can know it explictly? Or you mean they can know it implictly?

The size of each plane depends on the selected pixel format only. If you 
select DRM_FORMAT_NV12 + DRM_FORMAT_MOD_SAMSUNG_16_16_TILE modifier, 
then this unambiguously defines buffer size for each plane. Hardware has 
nothing to change here. Userspace can even query if given IPP module 
supports such pixel+modifier combo and get alignment requirements for it.

>> limits describes size of the image buffers in pixels. The size of each
>> plane (luma or chroma if exists for given format) in bytes directly
>> comes out of the selected pixel format (fourcc).
> fourcc would say the size not considered for the Hardware limit.

Alignment can be queried by userspace via get EXYNOS_IPP_GET_LIMITS 
ioctl. It provides limits for given pixel format + tile modifier combo, 
although in case of Exynos Scaler, the alignment requirements are simple 
result of the tiled format definition (tiled formats are typically 
defined only for width/height matching multiples of single tile size).

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

Re: [PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Inki Dae]

2018년 09월 13일 17:49에 Marek Szyprowski 이(가) 쓴 글:
> Hi Inki,
> 
> On 2018-09-13 10:23, Inki Dae wrote:
>> 2018년 09월 13일 17:03에 Marek Szyprowski 이(가) 쓴 글:
>>> On 2018-09-13 07:14, Inki Dae wrote:
>>>> 2018년 09월 12일 15:59에 Andrzej Pietrasiewicz 이(가) 쓴 글:
>>>>> W dniu 12.09.2018 o 01:54, Inki Dae pisze:
>>>>>> Hi Marek and Andrzej,
>>>>>>
>>>>>> 2018년 08월 10일 22:29에 Marek Szyprowski 이(가) 쓴 글:
>>>>>>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>>>>>>
>>>>>>> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
>>>>> <snip>
>>>>>
>>>>>>> -static u32 scaler_get_format(u32 drm_fmt)
>>>>>>> +struct scaler_format {
>>>>>>> +	u32	drm_fmt;
>>>>>>> +	u32	internal_fmt;
>>>>>>> +	u32	chroma_tile_w;
>>>>>>> +	u32	chroma_tile_h;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static const struct scaler_format scaler_formats[] = {
>>>>>>> +	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
>>>>>>> +	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
>>>>>>> +	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
>>>>>>> +	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
>>>>>>> +	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
>>>>>>> +	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
>>>>>>> +	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
>>>>>>> +	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
>>>>>>> +	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
>>>>>>> +	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
>>>>>>> +	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
>>>>>>> +	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
>>>>>>> +	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
>>>>>>> +	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
>>>>>>> +	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
>>>>>>> +	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
>>>>>>> +	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
>>>>>>> +	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
>>>>>>> +	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
>>>>>>> +	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
>>>>>>> +	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },
>>>>>> Seems the tile size of each format you declared above is wrong.
>>>>>> According to data sheet for Exynos5420/5422/5433, each plane has different tile size.
>>>>>> I.e., SACLE_YUV420_2P_UV/VU has two planes, and Y plane has 16 x 16 tile size but U and V plane have 8 x 8 tile size each other.
>>>>>>
>>>>>> However, above declaration has only one tile size
>>>>> true, but the members of struct scaler_format are called
>>>>>
>>>>> _____chroma_____tile_w/h
>>>>>
>>>>>> and it means that each plane has always same tile size.
>>>>> this conclusion is not justified
>>>>>
>>>>>> And also RGB formats have 16 x 16 tile size in tile mode but you declared it as 0.
>>>>> The data sheet says, that all formats have the same Y/RGB tile size
>>>>> and it is always 16x16. That is why only chroma tile size is remembered,
>>>>> because only chroma tile size can be different between formats.
>>>> Ok, chroma_tile_h/w are used to calculate only the position of the chroma buffer.
>>>>
>>>> By the way, user space would want to know the size limit in tiled mode so that they can allocate each buffer for Y(luma) and CbCr(chroma).
>>>> However, below code would provide Y/RGB tile size limit - 16 - to user space,
>>>> static const struct drm_exynos_ipp_limit scaler_5420_tile_limits[] = {
>>>>            { IPP_SIZE_LIMIT(BUFFER, .h = { 16, SZ_8K }, .v = { 16, SZ_8K })},
>>>>            { IPP_SIZE_LIMIT(AREA, .h.align = 16, .v.align = 16) },
>>>>
>>>> It would mean that user space will allocate 16 pixels aligned buffer even for chroma but the actual size limit of it would be 8 pixels in case of SCALE_YUV420_WP_UV/VU foramt.
>>>> Is there some code I'm missing?
>>> Userspace knows everything needed to allocate proper buffers. Those
>> How does user space know everything? Actually, the size of each plane in tiled mode would depend on Hardware, Scaler device in our case. Is there something user space can know it explictly? Or you mean they can know it implictly?
> 
> The size of each plane depends on the selected pixel format only. If you 
> select DRM_FORMAT_NV12 + DRM_FORMAT_MOD_SAMSUNG_16_16_TILE modifier, 
> then this unambiguously defines buffer size for each plane. Hardware has 
> nothing to change here. Userspace can even query if given IPP module 
> supports such pixel+modifier combo and get alignment requirements for it.
> 
>>> limits describes size of the image buffers in pixels. The size of each
>>> plane (luma or chroma if exists for given format) in bytes directly
>>> comes out of the selected pixel format (fourcc).
>> fourcc would say the size not considered for the Hardware limit.
> 
> Alignment can be queried by userspace via get EXYNOS_IPP_GET_LIMITS 
> ioctl. It provides limits for given pixel format + tile modifier combo, 
> although in case of Exynos Scaler, the alignment requirements are simple 
> result of the tiled format definition (tiled formats are typically 
> defined only for width/height matching multiples of single tile size).

Right, this is why I commented like below before,
 "However, below code would provide Y/RGB tile size limit - 16 - to user space,
  static const struct drm_exynos_ipp_limit scaler_5420_tile_limits[] = {
            { IPP_SIZE_LIMIT(BUFFER, .h = { 16, SZ_8K }, .v = { 16, SZ_8K })},
            { IPP_SIZE_LIMIT(AREA, .h.align = 16, .v.align = 16) },

  It would mean that user space will allocate 16 pixels aligned buffer even for chroma but the actual size limit of it would be 8 pixels in case of SCALE_YUV420_WP_UV/VU foramt."

As you can see, only Y(luma) size limit will be provided to user space, and I'm saying about CbCr(chroma) size limit.
Anyway, this is a trivial thing. 16 pixels limit for CbCr would be no problem although really a little bit memory is wasted.

Thanks,
Inki Dae

> 
> Best regards
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Inki Dae]

Hi,

There are several warnings,
WARNING: line over 80 characters
#276: FILE: drivers/gpu/drm/exynos/exynos_drm_scaler.c:182:
+	struct drm_exynos_ipp_task_rect *src_pos, const struct scaler_format *fmt)

WARNING: line over 80 characters
#297: FILE: drivers/gpu/drm/exynos/exynos_drm_scaler.c:363:
+	const struct scaler_format *src_fmt = scaler_get_format(task->src.buf.fourcc);

WARNING: line over 80 characters
#301: FILE: drivers/gpu/drm/exynos/exynos_drm_scaler.c:366:
+	const struct scaler_format *dst_fmt = scaler_get_format(task->dst.buf.fourcc);

total: 0 errors, 3 warnings, 192 lines checked


And comment below.


On 2018년 08월 10일 22:29, Marek Szyprowski wrote:
> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> 
> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_scaler.c | 133 ++++++++++++---------
>  1 file changed, 75 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_scaler.c b/drivers/gpu/drm/exynos/exynos_drm_scaler.c
> index 0ddb6eec7b11..8e761ef63eac 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_scaler.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_scaler.c
> @@ -49,56 +49,46 @@ struct scaler_context {
>  	const struct scaler_data	*scaler_data;
>  };
>  
> -static u32 scaler_get_format(u32 drm_fmt)
> +struct scaler_format {
> +	u32	drm_fmt;
> +	u32	internal_fmt;
> +	u32	chroma_tile_w;
> +	u32	chroma_tile_h;
> +};
> +
> +static const struct scaler_format scaler_formats[] = {
> +	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
> +	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
> +	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
> +	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
> +	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
> +	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
> +	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
> +	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
> +	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
> +	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
> +	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
> +	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
> +	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
> +	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
> +	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
> +	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
> +	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
> +	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
> +	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
> +	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
> +	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },
> +};
> +
> +static const struct scaler_format *scaler_get_format(u32 drm_fmt)
>  {
> -	switch (drm_fmt) {
> -	case DRM_FORMAT_NV12:
> -		return SCALER_YUV420_2P_UV;
> -	case DRM_FORMAT_NV21:
> -		return SCALER_YUV420_2P_VU;
> -	case DRM_FORMAT_YUV420:
> -		return SCALER_YUV420_3P;
> -	case DRM_FORMAT_YUYV:
> -		return SCALER_YUV422_1P_YUYV;
> -	case DRM_FORMAT_UYVY:
> -		return SCALER_YUV422_1P_UYVY;
> -	case DRM_FORMAT_YVYU:
> -		return SCALER_YUV422_1P_YVYU;
> -	case DRM_FORMAT_NV16:
> -		return SCALER_YUV422_2P_UV;
> -	case DRM_FORMAT_NV61:
> -		return SCALER_YUV422_2P_VU;
> -	case DRM_FORMAT_YUV422:
> -		return SCALER_YUV422_3P;
> -	case DRM_FORMAT_NV24:
> -		return SCALER_YUV444_2P_UV;
> -	case DRM_FORMAT_NV42:
> -		return SCALER_YUV444_2P_VU;
> -	case DRM_FORMAT_YUV444:
> -		return SCALER_YUV444_3P;
> -	case DRM_FORMAT_RGB565:
> -		return SCALER_RGB_565;
> -	case DRM_FORMAT_XRGB1555:
> -		return SCALER_ARGB1555;
> -	case DRM_FORMAT_ARGB1555:
> -		return SCALER_ARGB1555;
> -	case DRM_FORMAT_XRGB4444:
> -		return SCALER_ARGB4444;
> -	case DRM_FORMAT_ARGB4444:
> -		return SCALER_ARGB4444;
> -	case DRM_FORMAT_XRGB8888:
> -		return SCALER_ARGB8888;
> -	case DRM_FORMAT_ARGB8888:
> -		return SCALER_ARGB8888;
> -	case DRM_FORMAT_RGBX8888:
> -		return SCALER_RGBA8888;
> -	case DRM_FORMAT_RGBA8888:
> -		return SCALER_RGBA8888;
> -	default:
> -		break;
> -	}
> +	int i;
>  
> -	return 0;
> +	for (i = 0; i < ARRAY_SIZE(scaler_formats); i++)
> +		if (scaler_formats[i].drm_fmt == drm_fmt)
> +			return &scaler_formats[i];
> +
> +	return NULL;
>  }
>  
>  static inline int scaler_reset(struct scaler_context *scaler)
> @@ -152,11 +142,11 @@ static inline void scaler_enable_int(struct scaler_context *scaler)
>  }
>  
>  static inline void scaler_set_src_fmt(struct scaler_context *scaler,
> -	u32 src_fmt)
> +	u32 src_fmt, u32 tile)
>  {
>  	u32 val;
>  
> -	val = SCALER_SRC_CFG_SET_COLOR_FORMAT(src_fmt);
> +	val = SCALER_SRC_CFG_SET_COLOR_FORMAT(src_fmt) | (tile << 10);
>  	scaler_write(val, SCALER_SRC_CFG);
>  }
>  
> @@ -188,15 +178,19 @@ static inline void scaler_set_src_span(struct scaler_context *scaler,
>  	scaler_write(val, SCALER_SRC_SPAN);
>  }
>  
> -static inline void scaler_set_src_luma_pos(struct scaler_context *scaler,
> -	struct drm_exynos_ipp_task_rect *src_pos)
> +static inline void scaler_set_src_luma_chroma_pos(struct scaler_context *scaler,
> +	struct drm_exynos_ipp_task_rect *src_pos, const struct scaler_format *fmt)
>  {
>  	u32 val;
>  
>  	val = SCALER_SRC_Y_POS_SET_YH_POS(src_pos->x << 2);
>  	val |=  SCALER_SRC_Y_POS_SET_YV_POS(src_pos->y << 2);
>  	scaler_write(val, SCALER_SRC_Y_POS);
> -	scaler_write(val, SCALER_SRC_C_POS); /* ATTENTION! */
> +	val = SCALER_SRC_C_POS_SET_CH_POS(
> +		(src_pos->x * fmt->chroma_tile_w / 16) << 2);
> +	val |=  SCALER_SRC_C_POS_SET_CV_POS(
> +		(src_pos->y * fmt->chroma_tile_h / 16) << 2);
> +	scaler_write(val, SCALER_SRC_C_POS);
>  }
>  
>  static inline void scaler_set_src_wh(struct scaler_context *scaler,
> @@ -366,10 +360,10 @@ static int scaler_commit(struct exynos_drm_ipp *ipp,
>  	struct scaler_context *scaler =
>  			container_of(ipp, struct scaler_context, ipp);
>  
> -	u32 src_fmt = scaler_get_format(task->src.buf.fourcc);
> +	const struct scaler_format *src_fmt = scaler_get_format(task->src.buf.fourcc);
>  	struct drm_exynos_ipp_task_rect *src_pos = &task->src.rect;
>  
> -	u32 dst_fmt = scaler_get_format(task->dst.buf.fourcc);
> +	const struct scaler_format *dst_fmt = scaler_get_format(task->dst.buf.fourcc);
>  	struct drm_exynos_ipp_task_rect *dst_pos = &task->dst.rect;
>  
>  	pm_runtime_get_sync(scaler->dev);
> @@ -380,13 +374,14 @@ static int scaler_commit(struct exynos_drm_ipp *ipp,
>  
>  	scaler->task = task;
>  
> -	scaler_set_src_fmt(scaler, src_fmt);
> +	scaler_set_src_fmt(
> +		scaler, src_fmt->internal_fmt, task->src.buf.modifier != 0);

You changed 'u32 src_fmt/dst_fmt' to 'struct src_fmt/dst_fmt' but never check null pointer.
So if there is no valid format then src_fmt and dst_fmt will have NULL, and which in turn, above code will incur null pointer access.

Thanks,
Inki Dae

>  	scaler_set_src_base(scaler, &task->src);
>  	scaler_set_src_span(scaler, &task->src);
> -	scaler_set_src_luma_pos(scaler, src_pos);
> +	scaler_set_src_luma_chroma_pos(scaler, src_pos, src_fmt);
>  	scaler_set_src_wh(scaler, src_pos);
>  
> -	scaler_set_dst_fmt(scaler, dst_fmt);
> +	scaler_set_dst_fmt(scaler, dst_fmt->internal_fmt);
>  	scaler_set_dst_base(scaler, &task->dst);
>  	scaler_set_dst_span(scaler, &task->dst);
>  	scaler_set_dst_luma_pos(scaler, dst_pos);
> @@ -617,6 +612,16 @@ static const struct drm_exynos_ipp_limit scaler_5420_one_pixel_limits[] = {
>  			  .v = { 65536 * 1 / 4, 65536 * 16 }) },
>  };
>  
> +static const struct drm_exynos_ipp_limit scaler_5420_tile_limits[] = {
> +	{ IPP_SIZE_LIMIT(BUFFER, .h = { 16, SZ_8K }, .v = { 16, SZ_8K })},
> +	{ IPP_SIZE_LIMIT(AREA, .h.align = 16, .v.align = 16) },
> +	{ IPP_SCALE_LIMIT(.h = {1, 1}, .v = {1, 1})},
> +	{ }
> +};
> +
> +#define IPP_SRCDST_TILE_FORMAT(f, l)	\
> +	IPP_SRCDST_MFORMAT(f, DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, (l))
> +
>  static const struct exynos_drm_ipp_formats exynos5420_formats[] = {
>  	/* SCALER_YUV420_2P_UV */
>  	{ IPP_SRCDST_FORMAT(NV21, scaler_5420_two_pixel_hv_limits) },
> @@ -680,6 +685,18 @@ static const struct exynos_drm_ipp_formats exynos5420_formats[] = {
>  
>  	/* SCALER_RGBA8888 */
>  	{ IPP_SRCDST_FORMAT(RGBA8888, scaler_5420_one_pixel_limits) },
> +
> +	/* SCALER_YUV420_2P_UV TILE */
> +	{ IPP_SRCDST_TILE_FORMAT(NV21, scaler_5420_tile_limits) },
> +
> +	/* SCALER_YUV420_2P_VU TILE */
> +	{ IPP_SRCDST_TILE_FORMAT(NV12, scaler_5420_tile_limits) },
> +
> +	/* SCALER_YUV420_3P TILE */
> +	{ IPP_SRCDST_TILE_FORMAT(YUV420, scaler_5420_tile_limits) },
> +
> +	/* SCALER_YUV422_1P_YUYV TILE */
> +	{ IPP_SRCDST_TILE_FORMAT(YUYV, scaler_5420_tile_limits) },
>  };
>  
>  static const struct scaler_data exynos5420_data = {
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: scaler: Add support for tiled formats

[Marek Szyprowski]

Hi Inki,

On 2018-09-21 05:20, Inki Dae wrote:
> There are several warnings,
> WARNING: line over 80 characters
> #276: FILE: drivers/gpu/drm/exynos/exynos_drm_scaler.c:182:
> +	struct drm_exynos_ipp_task_rect *src_pos, const struct scaler_format *fmt)
>
> WARNING: line over 80 characters
> #297: FILE: drivers/gpu/drm/exynos/exynos_drm_scaler.c:363:
> +	const struct scaler_format *src_fmt = scaler_get_format(task->src.buf.fourcc);
>
> WARNING: line over 80 characters
> #301: FILE: drivers/gpu/drm/exynos/exynos_drm_scaler.c:366:
> +	const struct scaler_format *dst_fmt = scaler_get_format(task->dst.buf.fourcc);
>
> total: 0 errors, 3 warnings, 192 lines checked
>
>
> And comment below.
>
>
> On 2018년 08월 10일 22:29, Marek Szyprowski wrote:
>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>
>> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
>>
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_scaler.c | 133 ++++++++++++---------
>>   1 file changed, 75 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_scaler.c b/drivers/gpu/drm/exynos/exynos_drm_scaler.c
>> index 0ddb6eec7b11..8e761ef63eac 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_scaler.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_scaler.c
>> @@ -49,56 +49,46 @@ struct scaler_context {
>>   	const struct scaler_data	*scaler_data;
>>   };
>>   
>> -static u32 scaler_get_format(u32 drm_fmt)
>> +struct scaler_format {
>> +	u32	drm_fmt;
>> +	u32	internal_fmt;
>> +	u32	chroma_tile_w;
>> +	u32	chroma_tile_h;
>> +};
>> +
>> +static const struct scaler_format scaler_formats[] = {
>> +	{ DRM_FORMAT_NV12, SCALER_YUV420_2P_UV, 8, 8 },
>> +	{ DRM_FORMAT_NV21, SCALER_YUV420_2P_VU, 8, 8 },
>> +	{ DRM_FORMAT_YUV420, SCALER_YUV420_3P, 8, 8 },
>> +	{ DRM_FORMAT_YUYV, SCALER_YUV422_1P_YUYV, 16, 16 },
>> +	{ DRM_FORMAT_UYVY, SCALER_YUV422_1P_UYVY, 16, 16 },
>> +	{ DRM_FORMAT_YVYU, SCALER_YUV422_1P_YVYU, 16, 16 },
>> +	{ DRM_FORMAT_NV16, SCALER_YUV422_2P_UV, 8, 16 },
>> +	{ DRM_FORMAT_NV61, SCALER_YUV422_2P_VU, 8, 16 },
>> +	{ DRM_FORMAT_YUV422, SCALER_YUV422_3P, 8, 16 },
>> +	{ DRM_FORMAT_NV24, SCALER_YUV444_2P_UV, 16, 16 },
>> +	{ DRM_FORMAT_NV42, SCALER_YUV444_2P_VU, 16, 16 },
>> +	{ DRM_FORMAT_YUV444, SCALER_YUV444_3P, 16, 16 },
>> +	{ DRM_FORMAT_RGB565, SCALER_RGB_565, 0, 0 },
>> +	{ DRM_FORMAT_XRGB1555, SCALER_ARGB1555, 0, 0 },
>> +	{ DRM_FORMAT_ARGB1555, SCALER_ARGB1555, 0, 0 },
>> +	{ DRM_FORMAT_XRGB4444, SCALER_ARGB4444, 0, 0 },
>> +	{ DRM_FORMAT_ARGB4444, SCALER_ARGB4444, 0, 0 },
>> +	{ DRM_FORMAT_XRGB8888, SCALER_ARGB8888, 0, 0 },
>> +	{ DRM_FORMAT_ARGB8888, SCALER_ARGB8888, 0, 0 },
>> +	{ DRM_FORMAT_RGBX8888, SCALER_RGBA8888, 0, 0 },
>> +	{ DRM_FORMAT_RGBA8888, SCALER_RGBA8888, 0, 0 },
>> +};
>> +
>> +static const struct scaler_format *scaler_get_format(u32 drm_fmt)
>>   {
>> -	switch (drm_fmt) {
>> -	case DRM_FORMAT_NV12:
>> -		return SCALER_YUV420_2P_UV;
>> -	case DRM_FORMAT_NV21:
>> -		return SCALER_YUV420_2P_VU;
>> -	case DRM_FORMAT_YUV420:
>> -		return SCALER_YUV420_3P;
>> -	case DRM_FORMAT_YUYV:
>> -		return SCALER_YUV422_1P_YUYV;
>> -	case DRM_FORMAT_UYVY:
>> -		return SCALER_YUV422_1P_UYVY;
>> -	case DRM_FORMAT_YVYU:
>> -		return SCALER_YUV422_1P_YVYU;
>> -	case DRM_FORMAT_NV16:
>> -		return SCALER_YUV422_2P_UV;
>> -	case DRM_FORMAT_NV61:
>> -		return SCALER_YUV422_2P_VU;
>> -	case DRM_FORMAT_YUV422:
>> -		return SCALER_YUV422_3P;
>> -	case DRM_FORMAT_NV24:
>> -		return SCALER_YUV444_2P_UV;
>> -	case DRM_FORMAT_NV42:
>> -		return SCALER_YUV444_2P_VU;
>> -	case DRM_FORMAT_YUV444:
>> -		return SCALER_YUV444_3P;
>> -	case DRM_FORMAT_RGB565:
>> -		return SCALER_RGB_565;
>> -	case DRM_FORMAT_XRGB1555:
>> -		return SCALER_ARGB1555;
>> -	case DRM_FORMAT_ARGB1555:
>> -		return SCALER_ARGB1555;
>> -	case DRM_FORMAT_XRGB4444:
>> -		return SCALER_ARGB4444;
>> -	case DRM_FORMAT_ARGB4444:
>> -		return SCALER_ARGB4444;
>> -	case DRM_FORMAT_XRGB8888:
>> -		return SCALER_ARGB8888;
>> -	case DRM_FORMAT_ARGB8888:
>> -		return SCALER_ARGB8888;
>> -	case DRM_FORMAT_RGBX8888:
>> -		return SCALER_RGBA8888;
>> -	case DRM_FORMAT_RGBA8888:
>> -		return SCALER_RGBA8888;
>> -	default:
>> -		break;
>> -	}
>> +	int i;
>>   
>> -	return 0;
>> +	for (i = 0; i < ARRAY_SIZE(scaler_formats); i++)
>> +		if (scaler_formats[i].drm_fmt == drm_fmt)
>> +			return &scaler_formats[i];
>> +
>> +	return NULL;
>>   }
>>   
>>   static inline int scaler_reset(struct scaler_context *scaler)
>> @@ -152,11 +142,11 @@ static inline void scaler_enable_int(struct scaler_context *scaler)
>>   }
>>   
>>   static inline void scaler_set_src_fmt(struct scaler_context *scaler,
>> -	u32 src_fmt)
>> +	u32 src_fmt, u32 tile)
>>   {
>>   	u32 val;
>>   
>> -	val = SCALER_SRC_CFG_SET_COLOR_FORMAT(src_fmt);
>> +	val = SCALER_SRC_CFG_SET_COLOR_FORMAT(src_fmt) | (tile << 10);
>>   	scaler_write(val, SCALER_SRC_CFG);
>>   }
>>   
>> @@ -188,15 +178,19 @@ static inline void scaler_set_src_span(struct scaler_context *scaler,
>>   	scaler_write(val, SCALER_SRC_SPAN);
>>   }
>>   
>> -static inline void scaler_set_src_luma_pos(struct scaler_context *scaler,
>> -	struct drm_exynos_ipp_task_rect *src_pos)
>> +static inline void scaler_set_src_luma_chroma_pos(struct scaler_context *scaler,
>> +	struct drm_exynos_ipp_task_rect *src_pos, const struct scaler_format *fmt)
>>   {
>>   	u32 val;
>>   
>>   	val = SCALER_SRC_Y_POS_SET_YH_POS(src_pos->x << 2);
>>   	val |=  SCALER_SRC_Y_POS_SET_YV_POS(src_pos->y << 2);
>>   	scaler_write(val, SCALER_SRC_Y_POS);
>> -	scaler_write(val, SCALER_SRC_C_POS); /* ATTENTION! */
>> +	val = SCALER_SRC_C_POS_SET_CH_POS(
>> +		(src_pos->x * fmt->chroma_tile_w / 16) << 2);
>> +	val |=  SCALER_SRC_C_POS_SET_CV_POS(
>> +		(src_pos->y * fmt->chroma_tile_h / 16) << 2);
>> +	scaler_write(val, SCALER_SRC_C_POS);
>>   }
>>   
>>   static inline void scaler_set_src_wh(struct scaler_context *scaler,
>> @@ -366,10 +360,10 @@ static int scaler_commit(struct exynos_drm_ipp *ipp,
>>   	struct scaler_context *scaler =
>>   			container_of(ipp, struct scaler_context, ipp);
>>   
>> -	u32 src_fmt = scaler_get_format(task->src.buf.fourcc);
>> +	const struct scaler_format *src_fmt = scaler_get_format(task->src.buf.fourcc);
>>   	struct drm_exynos_ipp_task_rect *src_pos = &task->src.rect;
>>   
>> -	u32 dst_fmt = scaler_get_format(task->dst.buf.fourcc);
>> +	const struct scaler_format *dst_fmt = scaler_get_format(task->dst.buf.fourcc);
>>   	struct drm_exynos_ipp_task_rect *dst_pos = &task->dst.rect;
>>   
>>   	pm_runtime_get_sync(scaler->dev);
>> @@ -380,13 +374,14 @@ static int scaler_commit(struct exynos_drm_ipp *ipp,
>>   
>>   	scaler->task = task;
>>   
>> -	scaler_set_src_fmt(scaler, src_fmt);
>> +	scaler_set_src_fmt(
>> +		scaler, src_fmt->internal_fmt, task->src.buf.modifier != 0);
> You changed 'u32 src_fmt/dst_fmt' to 'struct src_fmt/dst_fmt' but never check null pointer.
> So if there is no valid format then src_fmt and dst_fmt will have NULL, and which in turn, above code will incur null pointer access.

IPPv2 framework checks data provided by userspace before calling driver
functions, so they will be never called with a format, which was not
advertised in driver's capabilities, thus NULL checks can be avoided.

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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