* Re: [PATCH v2] drm/exynos: ipp: fix image broken issue
2018-05-24 1:04 ` [PATCH v2] drm/exynos: ipp: fix image broken issue Inki Dae
@ 2018-05-29 2:30 ` Inki Dae
0 siblings, 0 replies; 2+ messages in thread
From: Inki Dae @ 2018-05-29 2:30 UTC (permalink / raw)
To: dri-devel; +Cc: linux-samsung-soc, m.szyprowski
This patch changes userspace parameter but Kernel driver shouldn't touch such parameter.
Please ignore this patch. Marek will post a generic solution soon, which fixes the image broken issue without touching userspace parameter.
Thanks,
Inki Dae
2018년 05월 24일 10:04에 Inki Dae 이(가) 쓴 글:
> Fixed image broken issue while video play back with 854x480.
>
> GScaler device of Exynos5433 has IN_ID_MAX field in GSC_IN_CON
> register, which determines how much GScaler DMA has to drain
> data from system memory at once.
>
> Therefore, image size should be fixed up before IPP driver verifies
> whether gem buffer is enough for it or not.
>
> Changelog v2:
> - Fix align limit of image width size to 16bytes because with other values
> , 4 and 8bytes, it doesn't work.
> - Change the function name from gst_get_align_size to gst_set_align_limit.
> gst_set_align_limit function name is more reasonable.
> - Call fixup buffer function before calling size limit check.
> - Remove checking align of image offset, x and y. The offest values have
> no any limit but only size.
>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
> drivers/gpu/drm/exynos/exynos_drm_gsc.c | 94 +++++++++++++++++++++++++++------
> drivers/gpu/drm/exynos/exynos_drm_ipp.c | 18 +++++--
> drivers/gpu/drm/exynos/exynos_drm_ipp.h | 12 +++++
> 3 files changed, 106 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
> index e99dd1e..8bc31b5 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
> @@ -86,6 +86,28 @@ struct gsc_scaler {
> unsigned long main_vratio;
> };
>
> +/**
> + * struct gsc_driverdata - per device type driver data for init time.
> + *
> + * @limits: picture size limits array
> + * @clk_names: names of clocks needed by this variant
> + * @num_clocks: the number of clocks needed by this variant
> + * @has_in_ic_max: indicate whether GSCALER_IN_CON register has
> + * IN_IC_MAX field which means maxinum AXI
> + * read capability.
> + * @in_ic_max_shift: indicate which position IN_IC_MAX field is located.
> + * @in_ic_max_mask: A mask value to IN_IC_MAX field.
> + */
> +struct gsc_driverdata {
> + const struct drm_exynos_ipp_limit *limits;
> + int num_limits;
> + const char *clk_names[GSC_MAX_CLOCKS];
> + int num_clocks;
> + unsigned int has_in_ic_max:1;
> + unsigned int in_ic_max_shift;
> + unsigned int in_ic_max_mask;
> +};
> +
> /*
> * A structure of gsc context.
> *
> @@ -96,6 +118,9 @@ struct gsc_scaler {
> * @id: gsc id.
> * @irq: irq number.
> * @rotation: supports rotation of src.
> + * @align_size: A size that GSC_SRCIMG_WIDTH value of GSC_SRCIMG_SIZE
> + * register should be aligned with(in byte).
> + * @driver_data: a pointer to gsc_driverdata.
> */
> struct gsc_context {
> struct exynos_drm_ipp ipp;
> @@ -114,20 +139,8 @@ struct gsc_context {
> int id;
> int irq;
> bool rotation;
> -};
> -
> -/**
> - * struct gsc_driverdata - per device type driver data for init time.
> - *
> - * @limits: picture size limits array
> - * @clk_names: names of clocks needed by this variant
> - * @num_clocks: the number of clocks needed by this variant
> - */
> -struct gsc_driverdata {
> - const struct drm_exynos_ipp_limit *limits;
> - int num_limits;
> - const char *clk_names[GSC_MAX_CLOCKS];
> - int num_clocks;
> + unsigned int align_size;
> + struct gsc_driverdata *driver_data;
> };
>
> /* 8-tap Filter Coefficient */
> @@ -1095,6 +1108,15 @@ static void gsc_start(struct gsc_context *ctx)
> gsc_write(cfg, GSC_ENABLE);
> }
>
> +static void gsc_fixup(struct exynos_drm_ipp *ipp,
> + struct exynos_drm_ipp_task *task)
> +{
> + struct gsc_context *ctx = container_of(ipp, struct gsc_context, ipp);
> + struct exynos_drm_ipp_buffer *src = &task->src;
> +
> + src->buf.width = ALIGN(src->buf.width, ctx->align_size);
> +}
> +
> static int gsc_commit(struct exynos_drm_ipp *ipp,
> struct exynos_drm_ipp_task *task)
> {
> @@ -1124,6 +1146,41 @@ static int gsc_commit(struct exynos_drm_ipp *ipp,
> return 0;
> }
>
> +static void gsc_set_align_limit(struct gsc_context *ctx)
> +{
> + const struct drm_exynos_ipp_limit *limits = ctx->driver_data->limits;
> +
> + if (ctx->driver_data->has_in_ic_max) {
> + u32 cfg, mask, shift;
> +
> + mask = ctx->driver_data->in_ic_max_mask;
> + shift = ctx->driver_data->in_ic_max_shift;
> +
> + pm_runtime_get_sync(ctx->dev);
> +
> + cfg = gsc_read(GSC_IN_CON);
> +
> + /*
> + * fix align limit to 16bytes. With other limit values, 4
> + * and 8, it doesn't work.
> + */
> + cfg = (cfg & ~(mask << shift));
> + cfg |= 2 << shift;
> +
> + gsc_write(cfg, GSC_IN_CON);
> +
> + pm_runtime_mark_last_busy(ctx->dev);
> + pm_runtime_put_autosuspend(ctx->dev);
> +
> + ctx->align_size = 16;
> + } else {
> + /* No HW register to get the align limit so use fixed value. */
> + ctx->align_size = limits->h.align;
> + }
> +
> + DRM_DEBUG_KMS("align_size = %d\n", ctx->align_size);
> +}
> +
> static void gsc_abort(struct exynos_drm_ipp *ipp,
> struct exynos_drm_ipp_task *task)
> {
> @@ -1142,6 +1199,7 @@ static void gsc_abort(struct exynos_drm_ipp *ipp,
> }
>
> static struct exynos_drm_ipp_funcs ipp_funcs = {
> + .fixup = gsc_fixup,
> .commit = gsc_commit,
> .abort = gsc_abort,
> };
> @@ -1155,6 +1213,8 @@ static int gsc_bind(struct device *dev, struct device *master, void *data)
> ctx->drm_dev = drm_dev;
> drm_iommu_attach_device(drm_dev, dev);
>
> + gsc_set_align_limit(ctx);
> +
> exynos_drm_ipp_register(drm_dev, ipp, &ipp_funcs,
> DRM_EXYNOS_IPP_CAP_CROP | DRM_EXYNOS_IPP_CAP_ROTATE |
> DRM_EXYNOS_IPP_CAP_SCALE | DRM_EXYNOS_IPP_CAP_CONVERT,
> @@ -1221,6 +1281,7 @@ static int gsc_probe(struct platform_device *pdev)
> }
> ctx->formats = formats;
> ctx->num_formats = ARRAY_SIZE(gsc_formats);
> + ctx->driver_data = driver_data;
>
> /* clock control */
> for (i = 0; i < ctx->num_clocks; i++) {
> @@ -1340,7 +1401,7 @@ static int __maybe_unused gsc_runtime_resume(struct device *dev)
> };
>
> static const struct drm_exynos_ipp_limit gsc_5433_limits[] = {
> - { IPP_SIZE_LIMIT(BUFFER, .h = { 32, 8191, 2 }, .v = { 16, 8191, 2 }) },
> + { IPP_SIZE_LIMIT(BUFFER, .h = { 32, 8191, 16 }, .v = { 16, 8191, 2 }) },
> { IPP_SIZE_LIMIT(AREA, .h = { 16, 4800, 1 }, .v = { 8, 3344, 1 }) },
> { IPP_SIZE_LIMIT(ROTATED, .h = { 32, 2047 }, .v = { 8, 8191 }) },
> { IPP_SCALE_LIMIT(.h = { (1 << 16) / 16, (1 << 16) * 8 },
> @@ -1366,6 +1427,9 @@ static int __maybe_unused gsc_runtime_resume(struct device *dev)
> .num_clocks = 4,
> .limits = gsc_5433_limits,
> .num_limits = ARRAY_SIZE(gsc_5433_limits),
> + .has_in_ic_max = 1,
> + .in_ic_max_shift = 24,
> + .in_ic_max_mask = 0x3,
> };
>
> static const struct of_device_id exynos_drm_gsc_of_match[] = {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> index 26374e5..2319c12 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> @@ -510,9 +510,7 @@ static int exynos_drm_ipp_check_size_limits(struct exynos_drm_ipp_buffer *buf,
> }
> __get_size_limit(limits, num_limits, id, &l);
> if (!__size_limit_check(buf->rect.w, lh) ||
> - !__align_check(buf->rect.x, lh->align) ||
> - !__size_limit_check(buf->rect.h, lv) ||
> - !__align_check(buf->rect.y, lv->align))
> + !__size_limit_check(buf->rect.h, lv))
> return -EINVAL;
>
> return 0;
> @@ -560,6 +558,14 @@ static int exynos_drm_ipp_check_scale_limits(
> return 0;
> }
>
> +static void exynos_drm_ipp_fixup_buffer(struct exynos_drm_ipp_task *task)
> +{
> + struct exynos_drm_ipp *ipp = task->ipp;
> +
> + if (ipp->funcs->fixup)
> + ipp->funcs->fixup(ipp, task);
> +}
> +
> static int exynos_drm_ipp_task_check(struct exynos_drm_ipp_task *task)
> {
> struct exynos_drm_ipp *ipp = task->ipp;
> @@ -607,6 +613,12 @@ static int exynos_drm_ipp_task_check(struct exynos_drm_ipp_task *task)
> return -EINVAL;
> }
>
> + /*
> + * image size should be fixed up before setup buffer call
> + * which verifies whether image size exceeds gem buffer size or not.
> + */
> + exynos_drm_ipp_fixup_buffer(task);
> +
> src_fmt = __ipp_format_get(ipp, src->buf.fourcc, src->buf.modifier,
> DRM_EXYNOS_IPP_FORMAT_SOURCE);
> if (!src_fmt) {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.h b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
> index 0b27d4a..5c2059f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
> @@ -20,6 +20,18 @@
> */
> struct exynos_drm_ipp_funcs {
> /**
> + * @fixup:
> + *
> + * Correct buffer size according to hardware limit of each DMA device.
> + * Some DMA device has maximum memory read capability through AXI bus,
> + * which reads data from memory as a given bytes.
> + * Therefore, IPP driver needs to check if the buffer size meets
> + * the HW limlit of each DMA device such as GScaler, Scaler,
> + * Rotator and FIMC.
> + */
> + void (*fixup)(struct exynos_drm_ipp *ipp,
> + struct exynos_drm_ipp_task *task);
> + /**
> * @commit:
> *
> * This is the main entry point to start framebuffer processing
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 2+ messages in thread