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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
> 
> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux