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@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.
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
