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

[Inki Dae <inki.dae@samsung.com>]

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