[PATCH v2] drm/exynos: ipp: fix image broken issue

[PATCH v2] drm/exynos: ipp: fix image broken issue

[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
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/exynos: ipp: fix image broken issue

[Inki Dae]

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