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

Message ID 20180810132915eucas1p18c60ec0bc72fb40b53d0f41a3d43c565~JiPohZpxV1504615046eucas1p1-@eucas1p1.samsung.com
State New
Headers show
Series
  • Exynos DRM IPP: add support for 16x16 tiled formats
Related show

Commit Message

Marek Szyprowski Aug. 10, 2018, 1:29 p.m.
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(-)

-- 
2.17.1

Comments

Inki Dae Sept. 11, 2018, 11:54 p.m. | #1
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 = {

>
Andrzej Pietrasiewicz Sept. 12, 2018, 6:59 a.m. | #2
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
Inki Dae Sept. 13, 2018, 5:14 a.m. | #3
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

> 

>
Marek Szyprowski Sept. 13, 2018, 8:03 a.m. | #4
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
Inki Dae Sept. 13, 2018, 8:23 a.m. | #5
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

>
Marek Szyprowski Sept. 13, 2018, 8:49 a.m. | #6
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
Inki Dae Sept. 13, 2018, 9:18 a.m. | #7
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

>
Inki Dae Sept. 21, 2018, 3:20 a.m. | #8
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 = {

>
Marek Szyprowski Sept. 21, 2018, 8:22 a.m. | #9
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

Patch

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 = {