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 <andrze...@samsung.com>
>>
>> Add support for 16x16 tiled formats: NV12/NV21, YUYV and YUV420.
>>
>> Signed-off-by: Andrzej Pietrasiewicz <andrze...@samsung.com>
>> Signed-off-by: Marek Szyprowski <m.szyprow...@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
