Re: [PATCH v4 2/9] drm/exynos: ipp: Add IPP v2 framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marek,

Thanks for your effort and sorry for late. Below are trivial my comments,

2017년 10월 23일 16:54에 Marek Szyprowski 이(가) 쓴 글:
> This patch adds Exynos IPP v2 subsystem and userspace API.
> 
> New userspace API is focused ONLY on memory-to-memory image processing.
> The two remainging IPP operation modes (framebuffer writeback and
> local-path output with image processing) can be implemented using
> standard DRM features: writeback connectors and additional DRM planes with
> scaling features.
> 
> V2 IPP userspace API is not compatible with old IPP ioctls. This is a
> significant change, but the old IPP subsystem in mainline Linux kernel was
> partially disfunctional anyway and not used in any open-source project.
> 
> V2 IPP userspace API is based on stateless approach, which much better fits
> to memory-to-memory image processing model. It also provides support for
> all image formats, which are both already defined in DRM API and supported
> by the existing IPP hardware modules.
> 
> The API consists of the following ioctls:
> - DRM_IOCTL_EXYNOS_IPP_GET_RESOURCES: to enumerate all available image
>   processing modules,
> - DRM_IOCTL_EXYNOS_IPP_GET_CAPS: to query capabilities and supported image
>   formats of given IPP module,
> - DRM_IOCTL_EXYNOS_IPP_GET_LIMITS: to query hardware limitiations for
>   selected image format of given IPP module,
> - DRM_IOCTL_EXYNOS_IPP_COMMIT: to perform operation described by the
>   provided structures (source and destination buffers, operation rectangle,
>   transformation, etc).
> 
> The proposed userspace API is extensible. In the future more advanced image
> processing operations can be defined to support for example blending.
> 
> Userspace API is fully functional also on DRM render nodes, so it is not
> limited to the root/privileged client.
> 
> Internal driver API also has been completely rewritten. New IPP core
> performs all possible input validation, checks and object life-time
> control. The drivers can focus only on writing configuration to hardware
> registers. Stateless nature of DRM_IOCTL_EXYNOS_IPP_COMMIT ioctl simplifies
> the driver API. Minimal driver needs to provide a single callback for
> starting processing and an array with supported image formats.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/exynos/Kconfig          |   3 +
>  drivers/gpu/drm/exynos/Makefile         |   1 +
>  drivers/gpu/drm/exynos/exynos_drm_drv.c |   9 +
>  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 911 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_ipp.h | 195 +++++++
>  include/uapi/drm/exynos_drm.h           | 238 +++++++++
>  6 files changed, 1357 insertions(+)
>  create mode 100644 drivers/gpu/drm/exynos/exynos_drm_ipp.c
>  create mode 100644 drivers/gpu/drm/exynos/exynos_drm_ipp.h
> 
> diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
> index 88cff0e039b6..2e097a81df12 100644
> --- a/drivers/gpu/drm/exynos/Kconfig
> +++ b/drivers/gpu/drm/exynos/Kconfig
> @@ -94,6 +94,9 @@ config DRM_EXYNOS_G2D
>  	help
>  	  Choose this option if you want to use Exynos G2D for DRM.
>  
> +config DRM_EXYNOS_IPP
> +	bool
> +
>  config DRM_EXYNOS_FIMC
>  	bool "FIMC"
>  	depends on BROKEN && MFD_SYSCON
> diff --git a/drivers/gpu/drm/exynos/Makefile b/drivers/gpu/drm/exynos/Makefile
> index 09bb002c9555..f663490e949d 100644
> --- a/drivers/gpu/drm/exynos/Makefile
> +++ b/drivers/gpu/drm/exynos/Makefile
> @@ -17,6 +17,7 @@ exynosdrm-$(CONFIG_DRM_EXYNOS_MIXER)	+= exynos_mixer.o
>  exynosdrm-$(CONFIG_DRM_EXYNOS_HDMI)	+= exynos_hdmi.o
>  exynosdrm-$(CONFIG_DRM_EXYNOS_VIDI)	+= exynos_drm_vidi.o
>  exynosdrm-$(CONFIG_DRM_EXYNOS_G2D)	+= exynos_drm_g2d.o
> +exynosdrm-$(CONFIG_DRM_EXYNOS_IPP)	+= exynos_drm_ipp.o
>  exynosdrm-$(CONFIG_DRM_EXYNOS_FIMC)	+= exynos_drm_fimc.o
>  exynosdrm-$(CONFIG_DRM_EXYNOS_ROTATOR)	+= exynos_drm_rotator.o
>  exynosdrm-$(CONFIG_DRM_EXYNOS_GSC)	+= exynos_drm_gsc.o
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 2fc5d3c01390..de4cce258a5b 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -26,6 +26,7 @@
>  #include "exynos_drm_fb.h"
>  #include "exynos_drm_gem.h"
>  #include "exynos_drm_plane.h"
> +#include "exynos_drm_ipp.h"
>  #include "exynos_drm_vidi.h"
>  #include "exynos_drm_g2d.h"
>  #include "exynos_drm_iommu.h"
> @@ -114,6 +115,14 @@ static const struct drm_ioctl_desc exynos_ioctls[] = {
>  			DRM_AUTH | DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(EXYNOS_G2D_EXEC, exynos_g2d_exec_ioctl,
>  			DRM_AUTH | DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(EXYNOS_IPP_GET_RESOURCES, exynos_drm_ipp_get_res_ioctl,
> +			DRM_AUTH | DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(EXYNOS_IPP_GET_CAPS, exynos_drm_ipp_get_caps_ioctl,
> +			DRM_AUTH | DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(EXYNOS_IPP_GET_LIMITS, exynos_drm_ipp_get_limits_ioctl,
> +			DRM_AUTH | DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(EXYNOS_IPP_COMMIT, exynos_drm_ipp_commit_ioctl,
> +			DRM_AUTH | DRM_RENDER_ALLOW),
>  };
>  
>  static const struct file_operations exynos_drm_driver_fops = {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> new file mode 100644
> index 000000000000..d54d23c6f2b0
> --- /dev/null
> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> @@ -0,0 +1,911 @@
> +/*
> + * Copyright (C) 2017 Samsung Electronics Co.Ltd
> + * Authors:
> + *	Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> + *
> + * Exynos DRM Image Post Processing (IPP) related functions
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + */
> +
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_mode.h>
> +#include <uapi/drm/exynos_drm.h>
> +
> +#include "exynos_drm_drv.h"
> +#include "exynos_drm_gem.h"
> +#include "exynos_drm_ipp.h"
> +
> +static int num_ipp;
> +static LIST_HEAD(ipp_list);
> +
> +struct drm_pending_exynos_ipp_event {
> +	struct drm_pending_event base;
> +	struct drm_exynos_ipp_event event;
> +};
> +
> +/**
> + * exynos_drm_ipp_register - Register a new picture processor hardware module
> + * @dev: DRM device
> + * @ipp: ipp module to init
> + * @funcs: callbacks for the new ipp object
> + * @caps: bitmask of ipp capabilities (%DRM_EXYNOS_IPP_CAP_*)
> + * @formats: array of supported formats (%DRM_FORMAT_*)
> + * @name: name (for debugging purposes)
> + *
> + * Initializes a ipp module.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int exynos_drm_ipp_register(struct drm_device *dev, struct exynos_drm_ipp *ipp,
> +		const struct exynos_drm_ipp_funcs *funcs, unsigned int caps,
> +		const struct exynos_drm_ipp_formats *formats, const char *name)
> +{
> +	WARN_ON(!ipp);
> +	WARN_ON(!funcs);
> +	WARN_ON(!formats);
> +
> +	spin_lock_init(&ipp->lock);
> +	INIT_LIST_HEAD(&ipp->todo_list);
> +	init_waitqueue_head(&ipp->done_wq);
> +	ipp->dev = dev;
> +	ipp->funcs = funcs;
> +	ipp->capabilities = caps;
> +	ipp->name = name;
> +	ipp->num_formats = 0;
> +	ipp->formats = formats;
> +	while (formats++->fourcc)
> +		ipp->num_formats++;
> +
> +	list_add_tail(&ipp->head, &ipp_list);

mutex lock/unlock would be required as component framework did.

> +	ipp->id = num_ipp++;
> +
> +	DRM_DEBUG_DRIVER("Registered ipp %d\n", ipp->id);
> +
> +	return 0;
> +}
> +
> +/**
> + * exynos_drm_ipp_unregister - Unregister the picture processor module
> + * @dev: DRM device
> + * @ipp: ipp module
> + */
> +void exynos_drm_ipp_unregister(struct drm_device *dev,
> +			       struct exynos_drm_ipp *ipp)
> +{
> +	BUG_ON(ipp->task);
> +	BUG_ON(!list_empty(&ipp->todo_list));
> +	list_del(&ipp->head);

ditto.

> +}
> +
> +/**
> + * exynos_drm_ipp_ioctl_get_res_ioctl - enumerate all ipp modules
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * Construct a list of ipp ids.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int exynos_drm_ipp_get_res_ioctl(struct drm_device *dev, void *data,
> +				 struct drm_file *file_priv)
> +{
> +	struct drm_exynos_ioctl_ipp_get_res *resp = data;
> +	struct exynos_drm_ipp *ipp;
> +	uint32_t __user *ipp_ptr = (uint32_t __user *)
> +						(unsigned long)resp->ipp_id_ptr;
> +	unsigned int count = num_ipp, copied = 0;
> +
> +	/*
> +	 * This ioctl is called twice, once to determine how much space is
> +	 * needed, and the 2nd time to fill it.

This is a same way as drm_mode_getresources function did. I'm not sure whether calling this function twice is best or not but no problem.

> +	 */
> +	if (count && resp->count_ipps >= count) {
> +		list_for_each_entry(ipp, &ipp_list, head) {
> +			if (put_user(ipp->id, ipp_ptr + copied))
> +				return -EFAULT;
> +			copied++;
> +		}
> +	}
> +	resp->count_ipps = count;
> +
> +	return 0;
> +}
> +
> +static inline struct exynos_drm_ipp *__ipp_get(uint32_t id)
> +{
> +	struct exynos_drm_ipp *ipp;
> +
> +	list_for_each_entry(ipp, &ipp_list, head)

mutex lock here. ipp could be deleted by exynos_drm_ipp_unregister function.

> +		if (ipp->id == id)
> +			return ipp;
> +	return NULL;
> +}
> +
> +/**
> + * exynos_drm_ipp_ioctl_get_caps - get ipp module capabilities and formats
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * Construct a structure describing ipp module capabilities.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int exynos_drm_ipp_get_caps_ioctl(struct drm_device *dev, void *data,
> +				  struct drm_file *file_priv)
> +{
> +	struct drm_exynos_ioctl_ipp_get_caps *resp = data;
> +	void __user *ptr = (void __user *)(unsigned long)resp->formats_ptr;
> +	struct exynos_drm_ipp *ipp;
> +	int i;
> +
> +	ipp = __ipp_get(resp->ipp_id);
> +	if (!ipp)
> +		return -ENOENT;
> +
> +	resp->ipp_id = ipp->id;
> +	resp->capabilities = ipp->capabilities;
> +
> +	/*
> +	 * This ioctl is called twice, once to determine how much space is
> +	 * needed, and the 2nd time to fill it.
> +	 */
> +	if (resp->formats_count >= ipp->num_formats) {
> +		for (i = 0; i < ipp->num_formats; i++) {
> +			struct drm_exynos_ipp_format tmp = {
> +				.fourcc = ipp->formats[i].fourcc,
> +				.type = ipp->formats[i].type,
> +				.modifier = ipp->formats[i].modifier,
> +			};
> +
> +			if (copy_to_user(ptr, &tmp, sizeof(tmp)))
> +				return -EFAULT;
> +			ptr += sizeof(tmp);
> +		}
> +	}
> +	resp->formats_count = ipp->num_formats;
> +
> +	return 0;
> +}
> +
> +static inline const struct exynos_drm_ipp_formats *__ipp_format_get(
> +			uint32_t fourcc, uint64_t mod, unsigned long type,

Should argument, 'type', be 'unsigned long' data type? not 'unsigned int'?

> +			const struct exynos_drm_ipp_formats *f)
> +{
> +	for (; f->fourcc; f++)
> +		if ((f->type & type) && f->fourcc == fourcc &&

Here 'f->type' has 'unsigned int' data type.

> +		    f->modifier == mod)
> +			return f;
> +	return NULL;
> +}
> +
> +/**
> + * exynos_drm_ipp_get_limits_ioctl - get ipp module limits
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * Construct a structure describing ipp module limitations for provided
> + * picture format.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int exynos_drm_ipp_get_limits_ioctl(struct drm_device *dev, void *data,
> +				    struct drm_file *file_priv)
> +{
> +	struct drm_exynos_ioctl_ipp_get_limits *resp = data;
> +	void __user *ptr = (void __user *)(unsigned long)resp->limits_ptr;
> +	const struct exynos_drm_ipp_formats *format;
> +	const struct drm_exynos_ipp_limit *limits, *l;
> +	struct exynos_drm_ipp *ipp;
> +	int count = 0;
> +
> +	if (resp->type != DRM_EXYNOS_IPP_FORMAT_SOURCE &&
> +	    resp->type != DRM_EXYNOS_IPP_FORMAT_DESTINATION)
> +		return -EINVAL;
> +
> +	ipp = __ipp_get(resp->ipp_id);
> +	if (!ipp)
> +		return -ENOENT;
> +
> +	format = __ipp_format_get(resp->fourcc, resp->modifier, resp->type,
> +			      ipp->formats);
> +	if (!format)
> +		return -EINVAL;
> +
> +	limits = format->limits;
> +	if (limits)
> +		for (l = limits; l->type; l++)
> +			count++;

It seems no need to use loop to count. How about this?
at device driver,
static struct gsc_driverdata gsc_exynosxxxx_drvdata = {
	.clk_name...
	..
	.limits = gsc_xxxx_limits,
	.limit_cnt = ARRAY_SIZE(gsc_xxx_limits),
};

> +
> +	/*
> +	 * This ioctl is called twice, once to determine how much space is
> +	 * needed, and the 2nd time to fill it.
> +	 */
> +	if (count && resp->limits_count >= count) {
> +		for (l = limits; l->type; l++, ptr += sizeof(*l))
> +			if (copy_to_user((void __user *)ptr, l, sizeof(*l)))
> +				return -EFAULT;
> +	}
> +	resp->limits_count = count;
> +
> +	return 0;
> +}
> +
> +static inline struct exynos_drm_ipp_task *
> +	exynos_drm_ipp_task_alloc(struct exynos_drm_ipp *ipp)
> +{
> +	struct exynos_drm_ipp_task *task;
> +
> +	task = kzalloc(sizeof(*task), GFP_KERNEL);
> +	if (!task)
> +		return NULL;
> +
> +	task->dev = ipp->dev;
> +	task->ipp = ipp;
> +
> +	/* some defaults */
> +	task->src.rect.w = task->dst.rect.w = UINT_MAX;
> +	task->src.rect.h = task->dst.rect.h = UINT_MAX;
> +	task->transform.rotation = DRM_MODE_ROTATE_0;
> +
> +	DRM_DEBUG_DRIVER("Allocated task %pK\n", task);
> +
> +	return task;
> +}
> +
> +struct exynos_drm_param_map {
> +	unsigned int id;
> +	unsigned int size;
> +	unsigned int offset;
> +} exynos_drm_ipp_params_maps[] = {
> +	{
> +		DRM_EXYNOS_IPP_TASK_BUFFER | DRM_EXYNOS_IPP_TASK_TYPE_SOURCE,
> +		sizeof(struct drm_exynos_ipp_task_buffer),
> +		offsetof(struct exynos_drm_ipp_task, src.buf),
> +	}, {
> +		DRM_EXYNOS_IPP_TASK_BUFFER | DRM_EXYNOS_IPP_TASK_TYPE_DESTINATION,
> +		sizeof(struct drm_exynos_ipp_task_buffer),
> +		offsetof(struct exynos_drm_ipp_task, dst.buf),
> +	}, {
> +		DRM_EXYNOS_IPP_TASK_RECTANGLE | DRM_EXYNOS_IPP_TASK_TYPE_SOURCE,
> +		sizeof(struct drm_exynos_ipp_task_rect),
> +		offsetof(struct exynos_drm_ipp_task, src.rect),
> +	}, {
> +		DRM_EXYNOS_IPP_TASK_RECTANGLE | DRM_EXYNOS_IPP_TASK_TYPE_DESTINATION,
> +		sizeof(struct drm_exynos_ipp_task_rect),
> +		offsetof(struct exynos_drm_ipp_task, dst.rect),
> +	}, {
> +		DRM_EXYNOS_IPP_TASK_TRANSFORM,
> +		sizeof(struct drm_exynos_ipp_task_transform),
> +		offsetof(struct exynos_drm_ipp_task, transform),
> +	}, {
> +		DRM_EXYNOS_IPP_TASK_ALPHA,
> +		sizeof(struct drm_exynos_ipp_task_alpha),
> +		offsetof(struct exynos_drm_ipp_task, alpha),
> +	},
> +};
> +
> +static int exynos_drm_ipp_task_set(struct exynos_drm_ipp_task *task,
> +				  struct drm_exynos_ioctl_ipp_commit *arg)
> +{
> +	unsigned int size = arg->params_size;
> +	void *p, *params;
> +	int ret = 0;
> +
> +	if (size > PAGE_SIZE)
> +		return -ERANGE;

I think you would need to check whether commands - which are passed by user space and arg->params_ptr points to - are valid or not here.

With this verification, checking if size > PAGE_SIZE wouldn't be needed because valid commands include only below data structures,
struct drm_exynos_ipp_task_buffer, struct drm_exynos_ipp_task_rect, struct drm_exynos_ipp_task_transform, and struct drm_exynos_ipp_task_alpha.

This would be done by exynos_drm_ipp_task_check function which is called at exynos_drm_ipp_commit_ioctl function after exynos_drm_ipp_task_set funcion call.
However, if task checking could be done prior to exynos_drm_ipp_set function and the commands passed by user-space have wrong data then you could skip unnecessary operation - copying commands from user-space to kernel.

> +
> +	p = kmalloc(size, GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(p, (void __user *)(unsigned long)arg->params_ptr,
> +			   size)) {
> +		ret = -EFAULT;
> +		DRM_DEBUG_DRIVER("Failed to copy configuration from userspace\n");
> +		goto free;
> +	}
> +
> +	params = p;
> +	while (size) {
> +		struct exynos_drm_param_map *map = exynos_drm_ipp_params_maps;
> +		u32 *id = params;
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(exynos_drm_ipp_params_maps); i++)
> +			if (map[i].id == *id)
> +				break;
> +
> +		if (i < ARRAY_SIZE(exynos_drm_ipp_params_maps) &&
> +		    map[i].size <= size) {
> +			memcpy((void *)task + map[i].offset, params,
> +			       map[i].size);
> +			params += map[i].size;
> +			size -= map[i].size;
> +		} else {
> +			ret = -EINVAL;
> +			goto free;
> +		}
> +	}
> +
> +	DRM_DEBUG_DRIVER("Got task %pK configuration from userspace\n", task);
> +free:
> +	kfree(p);
> +	return ret;
> +}
> +
> +static int exynos_drm_ipp_task_setup_buffer(struct exynos_drm_ipp_buffer *buf,
> +					    struct drm_file *filp)
> +{
> +	int ret = 0;
> +	int i;
> +
> +	/* basic checks */
> +	if (buf->buf.width == 0 || buf->buf.height == 0)
> +		return -EINVAL;
> +	buf->format = drm_format_info(buf->buf.fourcc);
> +	for (i = 0; i < buf->format->num_planes; i++) {
> +		unsigned int width = (i == 0) ? buf->buf.width :
> +			     DIV_ROUND_UP(buf->buf.width, buf->format->hsub);
> +
> +		if (buf->buf.pitch[i] == 0)
> +			buf->buf.pitch[i] = width * buf->format->cpp[i];
> +		if (buf->buf.pitch[i] < width * buf->format->cpp[i])
> +			return -EINVAL;
> +		if (!buf->buf.gem_id[i])
> +			return -ENOENT;
> +	}
> +
> +	/* get GEM buffers and check their size */
> +	for (i = 0; i < buf->format->num_planes; i++) {
> +		unsigned int height = (i == 0) ? buf->buf.height :
> +			     DIV_ROUND_UP(buf->buf.height, buf->format->vsub);
> +		unsigned long size = height * buf->buf.pitch[i] +
> +				     buf->buf.offset[i];
> +		struct drm_gem_object *obj = drm_gem_object_lookup(filp,
> +							    buf->buf.gem_id[i]);
> +		if (!obj) {
> +			ret = -ENOENT;
> +			goto gem_free;
> +		}
> +		buf->exynos_gem[i] = to_exynos_gem(obj);
> +
> +		if (size > buf->exynos_gem[i]->size) {
> +			i++;
> +			ret = -EINVAL;
> +			goto gem_free;
> +		}
> +		buf->dma_addr[i] = buf->exynos_gem[i]->dma_addr +
> +				   buf->buf.offset[i];
> +	}
> +
> +	return 0;
> +gem_free:
> +	while (i--) {
> +		drm_gem_object_put_unlocked(&buf->exynos_gem[i]->base);
> +		buf->exynos_gem[i] = NULL;
> +	}
> +	return ret;
> +}
> +
> +static void exynos_drm_ipp_task_release_buffer(struct exynos_drm_ipp_buffer *buf)
> +{
> +	int i;
> +
> +	if (!buf->exynos_gem[0])
> +		return;
> +	for (i = 0; i < buf->format->num_planes; i++)
> +		drm_gem_object_put_unlocked(&buf->exynos_gem[i]->base);
> +}
> +
> +static void exynos_drm_ipp_task_free(struct exynos_drm_ipp *ipp,
> +				 struct exynos_drm_ipp_task *task)
> +{
> +	DRM_DEBUG_DRIVER("Freeing task %pK\n", task);
> +
> +	exynos_drm_ipp_task_release_buffer(&task->src);
> +	exynos_drm_ipp_task_release_buffer(&task->dst);
> +	if (task->event)
> +		drm_event_cancel_free(ipp->dev, &task->event->base);
> +	kfree(task);
> +}
> +
> +struct drm_ipp_limit {
> +	struct drm_exynos_ipp_limit_val h;
> +	struct drm_exynos_ipp_limit_val v;
> +};
> +
> +enum drm_ipp_size_id {
> +	IPP_LIMIT_BUFFER, IPP_LIMIT_AREA, IPP_LIMIT_ROTATED, IPP_LIMIT_MAX
> +};
> +
> +static const enum drm_ipp_size_id limit_id_fallback[IPP_LIMIT_MAX][4] = {
> +	[IPP_LIMIT_BUFFER]  = { DRM_EXYNOS_IPP_LIMIT_SIZE_BUFFER },
> +	[IPP_LIMIT_AREA]    = { DRM_EXYNOS_IPP_LIMIT_SIZE_AREA,
> +				DRM_EXYNOS_IPP_LIMIT_SIZE_BUFFER },
> +	[IPP_LIMIT_ROTATED] = { DRM_EXYNOS_IPP_LIMIT_SIZE_ROTATED,
> +				DRM_EXYNOS_IPP_LIMIT_SIZE_AREA,
> +				DRM_EXYNOS_IPP_LIMIT_SIZE_BUFFER },
> +};
> +
> +static inline void __limit_set_val(unsigned int *ptr, unsigned int val)
> +{
> +	if (!*ptr)
> +		*ptr = val;
> +}
> +
> +static void __get_size_limit(const struct drm_exynos_ipp_limit *limits,
> +			     enum drm_ipp_size_id id, struct drm_ipp_limit *res)
> +{
> +	const struct drm_exynos_ipp_limit *l = limits;
> +	int i = 0;
> +
> +	memset(res, 0, sizeof(*res));
> +	for (i = 0; limit_id_fallback[id][i]; i++)
> +		for (l = limits; l->type; l++) {
> +			if (((l->type & DRM_EXYNOS_IPP_LIMIT_TYPE_MASK) !=
> +			      DRM_EXYNOS_IPP_LIMIT_TYPE_SIZE) ||
> +			    ((l->type & DRM_EXYNOS_IPP_LIMIT_SIZE_MASK) !=
> +						     limit_id_fallback[id][i]))
> +				continue;
> +			__limit_set_val(&res->h.min, l->h.min);
> +			__limit_set_val(&res->h.max, l->h.max);
> +			__limit_set_val(&res->h.align, l->h.align);
> +			__limit_set_val(&res->v.min, l->v.min);
> +			__limit_set_val(&res->v.max, l->v.max);
> +			__limit_set_val(&res->v.align, l->v.align);
> +		}
> +}
> +
> +static inline bool __align_check(unsigned int val, unsigned int align)
> +{
> +	if (align && (val & (align - 1))) {
> +		DRM_DEBUG_DRIVER("Value %d exceeds hw limits (align %d)\n",
> +				 val, align);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +static inline bool __size_limit_check(unsigned int val,
> +				 struct drm_exynos_ipp_limit_val *l)
> +{
> +	if ((l->min && val < l->min) || (l->max && val > l->max)) {
> +		DRM_DEBUG_DRIVER("Value %d exceeds hw limits (min %d, max %d)\n",
> +				 val, l->min, l->max);
> +		return false;
> +	}
> +	return __align_check(val, l->align);
> +}
> +
> +static int exynos_drm_ipp_check_size_limits(struct exynos_drm_ipp_buffer *buf,
> +	const struct drm_exynos_ipp_limit *limits, bool rotate, bool swap)
> +{
> +	enum drm_ipp_size_id id = rotate ? IPP_LIMIT_ROTATED : IPP_LIMIT_AREA;
> +	struct drm_ipp_limit l;
> +	struct drm_exynos_ipp_limit_val *lh = &l.h, *lv = &l.v;
> +
> +	if (!limits)
> +		return 0;
> +
> +	__get_size_limit(limits, IPP_LIMIT_BUFFER, &l);
> +	if (!__size_limit_check(buf->buf.width, &l.h) ||
> +	    !__size_limit_check(buf->buf.height, &l.v))
> +		return -EINVAL;

Printing out warning message here would be better for developers as a hint.

> +
> +	if (swap) {
> +		lv = &l.h;
> +		lh = &l.v;
> +	}
> +	__get_size_limit(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))
> +		return -EINVAL;

ditto.

> +
> +	return 0;
> +}
> +
> +static inline bool __scale_limit_check(unsigned int src, unsigned int dst,
> +				       unsigned int min, unsigned int max)
> +{
> +	if ((max && (dst << 16) > src * max) ||
> +	    (min && (dst << 16) < src * min)) {
> +		DRM_DEBUG_DRIVER("Scale from %d to %d exceeds hw limits (ratio min %d.%05d, max %d.%05d)\n",
> +				 src, dst,
> +				 min >> 16, 100000 * (min & 0xffff) / (1 << 16),
> +				 max >> 16, 100000 * (max & 0xffff) / (1 << 16));
> +		return false;
> +	}
> +	return true;
> +}
> +
> +static int exynos_drm_ipp_check_scale_limits(struct drm_exynos_ipp_task_rect *src,
> +	struct drm_exynos_ipp_task_rect *dst,
> +	const struct drm_exynos_ipp_limit *limits, bool swap)
> +{
> +	const struct drm_exynos_ipp_limit_val *lh, *lv;
> +
> +	if (!limits)
> +		return 0;
> +
> +	for (; limits->type; limits++)
> +		if ((limits->type & DRM_EXYNOS_IPP_LIMIT_TYPE_MASK) ==
> +		    DRM_EXYNOS_IPP_LIMIT_TYPE_SCALE)
> +			break;
> +	if (!limits->type)
> +		return 0;
> +
> +	lh = (!swap) ? &limits->h : &limits->v;
> +	lv = (!swap) ? &limits->v : &limits->h;
> +
> +	if (!__scale_limit_check(src->w, dst->w, lh->min, lh->max) ||
> +	    !__scale_limit_check(src->h, dst->h, lv->min, lv->max))
> +		return -EINVAL;

ditto, hint as a warning.

> +
> +	return 0;
> +}
> +
> +static int exynos_drm_ipp_task_check(struct exynos_drm_ipp_task *task,
> +				    struct drm_file *filp)
> +{
> +	struct exynos_drm_ipp *ipp = task->ipp;
> +	const struct exynos_drm_ipp_formats *src_format, *dst_format;
> +	struct exynos_drm_ipp_buffer *src = &task->src, *dst = &task->dst;
> +	unsigned int rotation = task->transform.rotation;
> +	int ret = 0;
> +	bool scale = false, swap = false;
> +
> +	DRM_DEBUG_DRIVER("Checking task %pK\n", task);
> +
> +	if (src->rect.w == UINT_MAX)
> +		src->rect.w = src->buf.width;
> +	if (src->rect.h == UINT_MAX)
> +		src->rect.h = src->buf.height;
> +	if (dst->rect.w == UINT_MAX)
> +		dst->rect.w = dst->buf.width;
> +	if (dst->rect.h == UINT_MAX)
> +		dst->rect.h = dst->buf.height;
> +
> +	if (src->rect.x + src->rect.w > (src->buf.width) ||
> +	    src->rect.y + src->rect.h > (src->buf.height) ||
> +	    dst->rect.x + dst->rect.w > (dst->buf.width) ||
> +	    dst->rect.y + dst->rect.h > (dst->buf.height)) {
> +		DRM_DEBUG_DRIVER("Task %pK: defined area is outside provided buffers\n",
> +				 task);
> +		return -EINVAL;
> +	}
> +
> +	if (drm_rotation_90_or_270(rotation))
> +		swap = true;
> +
> +	if ((!swap && (src->rect.w != dst->rect.w || src->rect.h != dst->rect.h)) ||
> +	    (swap && (src->rect.w != dst->rect.h || src->rect.h != dst->rect.w)))
> +		scale = true;
> +
> +	if ((!(ipp->capabilities & DRM_EXYNOS_IPP_CAP_CROP) &&
> +	     (src->rect.x || src->rect.y || dst->rect.x || dst->rect.y)) ||
> +	    (!(ipp->capabilities & DRM_EXYNOS_IPP_CAP_ROTATE) &&
> +	     rotation != DRM_MODE_ROTATE_0) ||
> +	    (!(ipp->capabilities & DRM_EXYNOS_IPP_CAP_SCALE) && scale) ||
> +	    (!(ipp->capabilities & DRM_EXYNOS_IPP_CAP_CONVERT) &&
> +	     src->buf.fourcc != dst->buf.fourcc)) {
> +		DRM_DEBUG_DRIVER("Task %pK: hw capabilities exceeded\n", task);
> +		return -EINVAL;
> +	}
> +
> +	src_format = __ipp_format_get(src->buf.fourcc, src->buf.modifier,
> +				  DRM_EXYNOS_IPP_FORMAT_SOURCE, ipp->formats);
> +	if (!src_format) {
> +		DRM_DEBUG_DRIVER("Task %pK: src format not supported\n", task);
> +		return -EINVAL;
> +	}
> +	ret = exynos_drm_ipp_check_size_limits(src, src_format->limits,
> +					  rotation != DRM_MODE_ROTATE_0, false);
> +	if (ret)
> +		return ret;
> +	ret = exynos_drm_ipp_check_scale_limits(&src->rect, &dst->rect,
> +						src_format->limits, swap);
> +	if (ret)
> +		return ret;
> +
> +	dst_format = __ipp_format_get(dst->buf.fourcc, dst->buf.modifier,
> +			       DRM_EXYNOS_IPP_FORMAT_DESTINATION, ipp->formats);
> +	if (!dst_format) {
> +		DRM_DEBUG_DRIVER("Task %pK: dst format not supported\n", task);
> +		return -EINVAL;
> +	}
> +	ret = exynos_drm_ipp_check_size_limits(dst, dst_format->limits,
> +					   rotation != DRM_MODE_ROTATE_0, swap);
> +	if (ret)
> +		return ret;
> +	ret = exynos_drm_ipp_check_scale_limits(&src->rect, &dst->rect,
> +						dst_format->limits, swap);
> +	if (ret)
> +		return ret;
> +
> +	ret = exynos_drm_ipp_task_setup_buffer(src, filp);
> +	if (ret) {
> +		DRM_DEBUG_DRIVER("Task %pK: src buffer setup failed\n", task);
> +		return ret;
> +	}
> +	ret = exynos_drm_ipp_task_setup_buffer(dst, filp);
> +	if (ret) {
> +		DRM_DEBUG_DRIVER("Task %pK: dst buffer setup failed\n", task);
> +		return ret;
> +	}
> +
> +	if (ipp->funcs->check) {
> +		ret = ipp->funcs->check(ipp, task);

It would be better to check HW or driver specific things prior to buffer setup.

> +		if (ret) {
> +			DRM_DEBUG_DRIVER("Task %pK: driver specific check failed\n",
> +					 task);
> +			return ret;
> +		}
> +	}
> +
> +	DRM_DEBUG_DRIVER("Task %pK: all checks done.\n", task);
> +
> +	return ret;
> +}
> +
> +static int exynos_drm_ipp_event_create(struct exynos_drm_ipp_task *task,
> +			struct drm_file *file_priv, uint64_t user_data)
> +{
> +	struct drm_pending_exynos_ipp_event *e = NULL;
> +	int ret;
> +
> +	e = kzalloc(sizeof(*e), GFP_KERNEL);
> +	if (!e)
> +		return -ENOMEM;
> +
> +	e->event.base.type = DRM_EXYNOS_IPP_EVENT;
> +	e->event.base.length = sizeof(e->event);
> +	e->event.user_data = user_data;
> +
> +	ret = drm_event_reserve_init(task->dev, file_priv, &e->base,
> +				     &e->event.base);
> +	if (ret)
> +		goto free;
> +
> +	task->event = e;
> +	return 0;
> +free:
> +	kfree(e);
> +	return ret;
> +}
> +
> +static void exynos_drm_ipp_event_send(struct exynos_drm_ipp_task *task)
> +{
> +	struct timeval now = ktime_to_timeval(ktime_get());
> +
> +	task->event->event.tv_sec = now.tv_sec;
> +	task->event->event.tv_usec = now.tv_usec;
> +	task->event->event.sequence = atomic_inc_return(&task->ipp->sequence);
> +
> +	drm_send_event(task->dev, &task->event->base);
> +}
> +
> +static int exynos_drm_ipp_task_cleanup(struct exynos_drm_ipp_task *task)
> +{
> +	int ret = task->ret;
> +
> +	if (ret == 0 && task->event) {
> +		exynos_drm_ipp_event_send(task);
> +		/* ensure event won't be canceled on task free */
> +		task->event = NULL;
> +	}
> +
> +	exynos_drm_ipp_task_free(task->ipp, task);
> +	return ret;
> +}
> +
> +static void exynos_drm_ipp_cleanup_work(struct work_struct *work)
> +{
> +	struct exynos_drm_ipp_task *task = container_of(work,
> +				struct exynos_drm_ipp_task, cleanup_work);
> +
> +	exynos_drm_ipp_task_cleanup(task);
> +}
> +
> +static void exynos_drm_ipp_next_task(struct exynos_drm_ipp *ipp);

It's not good. How about just moving implementaion of this function above side?

Thanks,
Inki Dae

> +
> +/**
> + * exynos_drm_ipp_task_done - finish given task and set return code
> + * @task: ipp task to finish
> + * @ret: error code or 0 if operation has been performed successfully
> + */
> +void exynos_drm_ipp_task_done(struct exynos_drm_ipp_task *task, int ret)
> +{
> +	struct exynos_drm_ipp *ipp = task->ipp;
> +	unsigned long flags;
> +
> +	DRM_DEBUG_DRIVER("ipp: %d, task %pK done\n", ipp->id, task);
> +
> +	spin_lock_irqsave(&ipp->lock, flags);
> +	if (ipp->task == task)
> +		ipp->task = NULL;
> +	task->flags |= DRM_EXYNOS_IPP_TASK_DONE;
> +	task->ret = ret;
> +	spin_unlock_irqrestore(&ipp->lock, flags);
> +
> +	exynos_drm_ipp_next_task(ipp);
> +	wake_up(&ipp->done_wq);
> +
> +	if (task->flags & DRM_EXYNOS_IPP_TASK_ASYNC) {
> +		INIT_WORK(&task->cleanup_work, exynos_drm_ipp_cleanup_work);
> +		schedule_work(&task->cleanup_work);
> +	}
> +}
> +
> +static void exynos_drm_ipp_next_task(struct exynos_drm_ipp *ipp)
> +{
> +	struct exynos_drm_ipp_task *task;
> +	unsigned long flags;
> +	int ret;
> +
> +	DRM_DEBUG_DRIVER("ipp: %d, try to run new task\n", ipp->id);
> +
> +	spin_lock_irqsave(&ipp->lock, flags);
> +
> +	if (ipp->task || list_empty(&ipp->todo_list)) {
> +		spin_unlock_irqrestore(&ipp->lock, flags);
> +		return;
> +	}
> +
> +	task = list_first_entry(&ipp->todo_list, struct exynos_drm_ipp_task,
> +				head);
> +	list_del_init(&task->head);
> +	ipp->task = task;
> +
> +	spin_unlock_irqrestore(&ipp->lock, flags);
> +
> +	DRM_DEBUG_DRIVER("ipp: %d, selected task %pK to run\n", ipp->id, task);
> +
> +	ret = ipp->funcs->commit(ipp, task);
> +	if (ret)
> +		exynos_drm_ipp_task_done(task, ret);
> +}
> +
> +static void exynos_drm_ipp_schedule_task(struct exynos_drm_ipp *ipp,
> +				     struct exynos_drm_ipp_task *task)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ipp->lock, flags);
> +	list_add(&task->head, &ipp->todo_list);
> +	spin_unlock_irqrestore(&ipp->lock, flags);
> +
> +	exynos_drm_ipp_next_task(ipp);
> +}
> +
> +static void exynos_drm_ipp_task_abort(struct exynos_drm_ipp *ipp,
> +				  struct exynos_drm_ipp_task *task)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ipp->lock, flags);
> +	if (task->flags & DRM_EXYNOS_IPP_TASK_DONE) {
> +		/* already completed task */
> +		exynos_drm_ipp_task_cleanup(task);
> +	} else if (ipp->task != task) {
> +		/* task has not been scheduled for execution yet */
> +		list_del_init(&task->head);
> +		exynos_drm_ipp_task_cleanup(task);
> +	} else {
> +		/*
> +		 * currently processed task, call abort() and perform
> +		 * cleanup with async worker
> +		 */
> +		task->flags |= DRM_EXYNOS_IPP_TASK_ASYNC;
> +		spin_unlock_irqrestore(&ipp->lock, flags);
> +		if (ipp->funcs->abort)
> +			ipp->funcs->abort(ipp, task);
> +		return;
> +	}
> +	spin_unlock_irqrestore(&ipp->lock, flags);
> +}
> +
> +/**
> + * exynos_drm_ipp_commit_ioctl - perform image processing operation
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * Construct a ipp task from the set of properties provided from the user
> + * and try to schedule it to framebuffer processor hardware.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int exynos_drm_ipp_commit_ioctl(struct drm_device *dev, void *data,
> +			  struct drm_file *file_priv)
> +{
> +	struct drm_exynos_ioctl_ipp_commit *arg = data;
> +	struct exynos_drm_ipp *ipp;
> +	struct exynos_drm_ipp_task *task;
> +	int ret = 0;
> +
> +	if ((arg->flags & ~DRM_EXYNOS_IPP_FLAGS) || arg->reserved)
> +		return -EINVAL;
> +
> +	/* can't test and expect an event at the same time */
> +	if ((arg->flags & DRM_EXYNOS_IPP_FLAG_TEST_ONLY) &&
> +			(arg->flags & DRM_EXYNOS_IPP_FLAG_EVENT))
> +		return -EINVAL;
> +
> +	ipp = __ipp_get(arg->ipp_id);
> +	if (!ipp)
> +		return -ENOENT;
> +
> +	task = exynos_drm_ipp_task_alloc(ipp);
> +	if (!task)
> +		return -ENOMEM;
> +
> +	ret = exynos_drm_ipp_task_set(task, arg);
> +	if (ret)
> +		goto free;
> +
> +	ret = exynos_drm_ipp_task_check(task, file_priv);
> +	if (ret || arg->flags & DRM_EXYNOS_IPP_FLAG_TEST_ONLY)
> +		goto free;
> +
> +	if (arg->flags & DRM_EXYNOS_IPP_FLAG_EVENT) {
> +		ret = exynos_drm_ipp_event_create(task, file_priv,
> +						 arg->user_data);
> +		if (ret)
> +			goto free;
> +	}
> +
> +	/*
> +	 * Queue task for processing on the hardware. task object will be
> +	 * then freed after exynos_drm_ipp_task_done()
> +	 */
> +	if (arg->flags & DRM_EXYNOS_IPP_FLAG_NONBLOCK) {
> +		DRM_DEBUG_DRIVER("ipp: %d, nonblocking processing task %pK\n",
> +				 ipp->id, task);
> +
> +		task->flags |= DRM_EXYNOS_IPP_TASK_ASYNC;
> +		exynos_drm_ipp_schedule_task(task->ipp, task);
> +		ret = 0;
> +	} else {
> +		DRM_DEBUG_DRIVER("ipp: %d, processing task %pK\n", ipp->id, task);
> +		exynos_drm_ipp_schedule_task(ipp, task);
> +		ret = wait_event_interruptible(ipp->done_wq,
> +					task->flags & DRM_EXYNOS_IPP_TASK_DONE);
> +		if (ret)
> +			exynos_drm_ipp_task_abort(ipp, task);
> +		else
> +			ret = exynos_drm_ipp_task_cleanup(task);
> +	}
> +	return ret;
> +free:
> +	exynos_drm_ipp_task_free(ipp, task);
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.h b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
> new file mode 100644
> index 000000000000..199eb2006410
> --- /dev/null
> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
> @@ -0,0 +1,195 @@
> +/*
> + * Copyright (c) 2017 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#ifndef _EXYNOS_DRM_IPP_H_
> +#define _EXYNOS_DRM_IPP_H_
> +
> +#include <drm/drmP.h>
> +
> +struct exynos_drm_ipp;
> +struct exynos_drm_ipp_task;
> +
> +/**
> + * struct exynos_drm_ipp_funcs - exynos_drm_ipp control functions
> + */
> +struct exynos_drm_ipp_funcs {
> +	/**
> +	 * @check:
> +	 *
> +	 * This is the optional hook to validate an ipp task. This function
> +	 * must reject any task which the hardware or driver doesn't support.
> +	 * This includes but is of course not limited to:
> +	 *
> +	 *  - Checking that the buffers, scaling and placement requirements
> +	 *    and so on are within the limits of the hardware not specified by
> +	 *    limits array.
> +	 *
> +	 *  - The driver does not need to repeat basic input validation like
> +	 *    done in the exynos_drm_ipp_task_check() function. The core does
> +	 *    that before calling this hook.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or one of the below negative error codes:
> +	 *
> +	 *  - -EINVAL, if any of the above constraints are violated.
> +	 */
> +	int (*check)(struct exynos_drm_ipp *ipp,
> +		     struct exynos_drm_ipp_task *task);
> +
> +	/**
> +	 * @commit:
> +	 *
> +	 * This is the main entry point to start framebuffer processing
> +	 * in the hardware. The exynos_drm_ipp_task has been already validated.
> +	 * This function must not wait until the device finishes processing.
> +	 * When the driver finishes processing, it has to call
> +	 * exynos_exynos_drm_ipp_task_done() function.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or negative error codes in case of failure.
> +	 */
> +	int (*commit)(struct exynos_drm_ipp *ipp,
> +		      struct exynos_drm_ipp_task *task);
> +
> +	/**
> +	 * @abort:
> +	 *
> +	 * Informs the driver that it has to abort the currently running
> +	 * task as soon as possible (i.e. as soon as it can stop the device
> +	 * safely), even if the task would not have been finished by then.
> +	 * After the driver performs the necessary steps, it has to call
> +	 * exynos_drm_ipp_task_done() (as if the task ended normally).
> +	 * This function does not have to (and will usually not) wait
> +	 * until the device enters a state when it can be stopped.
> +	 */
> +	void (*abort)(struct exynos_drm_ipp *ipp,
> +		      struct exynos_drm_ipp_task *task);
> +};
> +
> +/**
> + * struct exynos_drm_ipp - central picture processor module structure
> + */
> +struct exynos_drm_ipp {
> +	struct drm_device *dev;
> +	struct list_head head;
> +	unsigned int id;
> +
> +	const char *name;
> +	const struct exynos_drm_ipp_funcs *funcs;
> +	unsigned int capabilities;
> +	const struct exynos_drm_ipp_formats *formats;
> +	unsigned int num_formats;
> +	atomic_t sequence;
> +
> +	spinlock_t lock;
> +	struct exynos_drm_ipp_task *task;
> +	struct list_head todo_list;
> +	wait_queue_head_t done_wq;
> +};
> +
> +struct exynos_drm_ipp_buffer {
> +	struct drm_exynos_ipp_task_buffer buf;
> +	struct drm_exynos_ipp_task_rect rect;
> +
> +	struct exynos_drm_gem *exynos_gem[MAX_FB_BUFFER];
> +	const struct drm_format_info *format;
> +	dma_addr_t dma_addr[MAX_FB_BUFFER];
> +};
> +
> +/**
> + * struct exynos_drm_ipp_task - a structure describing transformation that
> + * has to be performed by the picture processor hardware module
> + */
> +struct exynos_drm_ipp_task {
> +	struct drm_device *dev;
> +	struct exynos_drm_ipp *ipp;
> +	struct list_head head;
> +
> +	struct exynos_drm_ipp_buffer src;
> +	struct exynos_drm_ipp_buffer dst;
> +
> +	struct drm_exynos_ipp_task_transform transform;
> +	struct drm_exynos_ipp_task_alpha alpha;
> +
> +	struct work_struct cleanup_work;
> +	unsigned int flags;
> +	int ret;
> +
> +	struct drm_pending_exynos_ipp_event *event;
> +};
> +
> +#define DRM_EXYNOS_IPP_TASK_DONE	(1 << 0)
> +#define DRM_EXYNOS_IPP_TASK_ASYNC	(1 << 1)
> +
> +struct exynos_drm_ipp_formats {
> +	u32 fourcc;
> +	u32 type;
> +	u64 modifier;
> +	const struct drm_exynos_ipp_limit *limits;
> +};
> +
> +/* helper macros to set exynos_drm_ipp_formats structure and limits*/
> +#define IPP_SRCDST_MFORMAT(f, m, l) \
> +     .fourcc = DRM_FORMAT_##f, .modifier = m, .limits = l, \
> +     .type = (DRM_EXYNOS_IPP_FORMAT_SOURCE | DRM_EXYNOS_IPP_FORMAT_DESTINATION)
> +
> +#define IPP_SRCDST_FORMAT(f, l) IPP_SRCDST_MFORMAT(f, 0, l)
> +
> +#define IPP_SIZE_LIMIT(l, val...)	\
> +     .type = (DRM_EXYNOS_IPP_LIMIT_TYPE_SIZE | DRM_EXYNOS_IPP_LIMIT_SIZE_##l), \
> +     val
> +
> +#define IPP_SCALE_LIMIT(val...)	\
> +	.type = (DRM_EXYNOS_IPP_LIMIT_TYPE_SCALE),\
> +	val
> +
> +int exynos_drm_ipp_register(struct drm_device *dev, struct exynos_drm_ipp *ipp,
> +		const struct exynos_drm_ipp_funcs *funcs, unsigned int caps,
> +		const struct exynos_drm_ipp_formats *formats, const char *name);
> +void exynos_drm_ipp_unregister(struct drm_device *dev, struct exynos_drm_ipp *ipp);
> +
> +void exynos_drm_ipp_task_done(struct exynos_drm_ipp_task *task, int ret);
> +
> +#ifdef CONFIG_DRM_EXYNOS_IPP
> +int exynos_drm_ipp_get_res_ioctl(struct drm_device *dev, void *data,
> +				 struct drm_file *file_priv);
> +int exynos_drm_ipp_get_caps_ioctl(struct drm_device *dev, void *data,
> +				  struct drm_file *file_priv);
> +int exynos_drm_ipp_get_limits_ioctl(struct drm_device *dev, void *data,
> +				    struct drm_file *file_priv);
> +int exynos_drm_ipp_commit_ioctl(struct drm_device *dev,
> +				void *data, struct drm_file *file_priv);
> +#else
> +static inline int exynos_drm_ipp_get_res_ioctl(struct drm_device *dev,
> +	 void *data, struct drm_file *file_priv)
> +{
> +	struct drm_exynos_ioctl_ipp_get_res *resp = data;
> +
> +	resp->count_ipps = 0;
> +	return 0;
> +}
> +static inline int exynos_drm_ipp_get_caps_ioctl(struct drm_device *dev,
> +	 void *data, struct drm_file *file_priv)
> +{
> +	return -ENODEV;
> +}
> +static inline int exynos_drm_ipp_get_limits_ioctl(struct drm_device *dev,
> +	 void *data, struct drm_file *file_priv)
> +{
> +	return -ENODEV;
> +}
> +static inline int exynos_drm_ipp_commit_ioctl(struct drm_device *dev,
> +	 void *data, struct drm_file *file_priv)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +#endif
> diff --git a/include/uapi/drm/exynos_drm.h b/include/uapi/drm/exynos_drm.h
> index 5d9a08d69ba7..974cbb579f90 100644
> --- a/include/uapi/drm/exynos_drm.h
> +++ b/include/uapi/drm/exynos_drm.h
> @@ -134,6 +134,219 @@ struct drm_exynos_g2d_exec {
>  	__u64					async;
>  };
>  
> +/* Exynos DRM IPP v2 API */
> +
> +/**
> + * Enumerate available IPP hardware modules.
> + *
> + * @count_ipps: size of ipp_id array / number of ipp modules (set by driver)
> + * @reserved: padding
> + * @ipp_id_ptr: pointer to ipp_id array or NULL
> + */
> +struct drm_exynos_ioctl_ipp_get_res {
> +	__u32 count_ipps;
> +	__u32 reserved;
> +	__u64 ipp_id_ptr;
> +};
> +
> +enum drm_exynos_ipp_format_type {
> +	DRM_EXYNOS_IPP_FORMAT_SOURCE		= 0x01,
> +	DRM_EXYNOS_IPP_FORMAT_DESTINATION	= 0x02,
> +};
> +
> +struct drm_exynos_ipp_format {
> +	__u32 fourcc;
> +	__u32 type;
> +	__u64 modifier;
> +};
> +
> +enum drm_exynos_ipp_capability {
> +	DRM_EXYNOS_IPP_CAP_CROP		= 0x01,
> +	DRM_EXYNOS_IPP_CAP_ROTATE	= 0x02,
> +	DRM_EXYNOS_IPP_CAP_SCALE	= 0x04,
> +	DRM_EXYNOS_IPP_CAP_CONVERT	= 0x08,
> +};
> +
> +/**
> + * Get IPP hardware capabilities and supported image formats.
> + *
> + * @ipp_id: id of IPP module to query
> + * @capabilities: bitmask of drm_exynos_ipp_capability (set by driver)
> + * @reserved: padding
> + * @formats_count: size of formats array (in entries) / number of filled
> + *		   formats (set by driver)
> + * @formats_ptr: pointer to formats array or NULL
> + */
> +struct drm_exynos_ioctl_ipp_get_caps {
> +	__u32 ipp_id;
> +	__u32 capabilities;
> +	__u32 reserved;
> +	__u32 formats_count;
> +	__u64 formats_ptr;
> +};
> +
> +enum drm_exynos_ipp_limit_type {
> +	/* size (horizontal/vertial) limits, in pixels (min, max, alignment) */
> +	DRM_EXYNOS_IPP_LIMIT_TYPE_SIZE		= 0x0001,
> +	/* scale ratio (horizonta/vertial), 16.16 fixed point (min, max) */
> +	DRM_EXYNOS_IPP_LIMIT_TYPE_SCALE		= 0x0002,
> +
> +	/* image buffer area */
> +	DRM_EXYNOS_IPP_LIMIT_SIZE_BUFFER	= 0x0001 << 16,
> +	/* src/dst rectangle area */
> +	DRM_EXYNOS_IPP_LIMIT_SIZE_AREA		= 0x0002 << 16,
> +	/* src/dst rectangle area when rotation enabled */
> +	DRM_EXYNOS_IPP_LIMIT_SIZE_ROTATED	= 0x0003 << 16,
> +
> +	DRM_EXYNOS_IPP_LIMIT_TYPE_MASK		= 0x000f,
> +	DRM_EXYNOS_IPP_LIMIT_SIZE_MASK		= 0x000f << 16,
> +};
> +
> +struct drm_exynos_ipp_limit_val {
> +	__u32 min;
> +	__u32 max;
> +	__u32 align;
> +	__u32 reserved;
> +};
> +
> +/**
> + * IPP module limitation.
> + *
> + * @type: limit type (see drm_exynos_ipp_limit_type enum)
> + * @reserved: padding
> + * @h: horizontal limits
> + * @v: vertical limits
> + */
> +struct drm_exynos_ipp_limit {
> +	__u32 type;
> +	__u32 reserved;
> +	struct drm_exynos_ipp_limit_val h;
> +	struct drm_exynos_ipp_limit_val v;
> +};
> +
> +/**
> + * Get IPP limits for given image format.
> + *
> + * @ipp_id: id of IPP module to query
> + * @fourcc: image format code (see DRM_FORMAT_* in drm_fourcc.h)
> + * @modifier: image format modifier (see DRM_FORMAT_MOD_* in drm_fourcc.h)
> + * @type: source/destination identifier (drm_exynos_ipp_format_flag enum)
> + * @limits_count: size of limits array (in entries) / number of filled entries
> + *		 (set by driver)
> + * @limits_ptr: pointer to limits array or NULL
> + */
> +struct drm_exynos_ioctl_ipp_get_limits {
> +	__u32 ipp_id;
> +	__u32 fourcc;
> +	__u64 modifier;
> +	__u32 type;
> +	__u32 limits_count;
> +	__u64 limits_ptr;
> +};
> +
> +enum drm_exynos_ipp_task_id {
> +	/* buffer described by struct drm_exynos_ipp_task_buffer */
> +	DRM_EXYNOS_IPP_TASK_BUFFER		= 0x0001,
> +	/* rectangle described by struct drm_exynos_ipp_task_rect */
> +	DRM_EXYNOS_IPP_TASK_RECTANGLE		= 0x0002,
> +	/* transformation described by struct drm_exynos_ipp_task_transform */
> +	DRM_EXYNOS_IPP_TASK_TRANSFORM		= 0x0003,
> +	/* alpha configuration described by struct drm_exynos_ipp_task_alpha */
> +	DRM_EXYNOS_IPP_TASK_ALPHA		= 0x0004,
> +
> +	/* source image data (for buffer and rectangle chunks) */
> +	DRM_EXYNOS_IPP_TASK_TYPE_SOURCE		= 0x0001 << 16,
> +	/* destination image data (for buffer and rectangle chunks) */
> +	DRM_EXYNOS_IPP_TASK_TYPE_DESTINATION	= 0x0002 << 16,
> +};
> +
> +/**
> + * Memory buffer with image data.
> + *
> + * @id: must be DRM_EXYNOS_IPP_TASK_BUFFER
> + * other parameters are same as for AddFB2 generic DRM ioctl
> + */
> +struct drm_exynos_ipp_task_buffer {
> +	__u32	id;
> +	__u32	fourcc;
> +	__u32	width, height;
> +	__u32	gem_id[4];
> +	__u32	offset[4];
> +	__u32	pitch[4];
> +	__u64	modifier;
> +};
> +
> +/**
> + * Rectangle for processing.
> + *
> + * @id: must be DRM_EXYNOS_IPP_TASK_RECTANGLE
> + * @reserved: padding
> + * @x,@y: left corner in pixels
> + * @w,@h: width/height in pixels
> + */
> +struct drm_exynos_ipp_task_rect {
> +	__u32	id;
> +	__u32	reserved;
> +	__u32	x;
> +	__u32	y;
> +	__u32	w;
> +	__u32	h;
> +};
> +
> +/**
> + * Image tranformation description.
> + *
> + * @id: must be DRM_EXYNOS_IPP_TASK_TRANSFORM
> + * @rotation: DRM_MODE_ROTATE_* and DRM_MODE_REFLECT_* values
> + */
> +struct drm_exynos_ipp_task_transform {
> +	__u32	id;
> +	__u32	rotation;
> +};
> +
> +/**
> + * Image global alpha configuration for formats without alpha values.
> + *
> + * @id: must be DRM_EXYNOS_IPP_TASK_ALPHA
> + * @value: global alpha value (0-255)
> + */
> +struct drm_exynos_ipp_task_alpha {
> +	__u32	id;
> +	__u32	value;
> +};
> +
> +enum drm_exynos_ipp_flag {
> +	/* generate DRM event after processing */
> +	DRM_EXYNOS_IPP_FLAG_EVENT	= 0x01,
> +	/* dry run, only check task parameters */
> +	DRM_EXYNOS_IPP_FLAG_TEST_ONLY	= 0x02,
> +	/* non-blocking processing */
> +	DRM_EXYNOS_IPP_FLAG_NONBLOCK	= 0x04,
> +};
> +
> +#define DRM_EXYNOS_IPP_FLAGS (DRM_EXYNOS_IPP_FLAG_EVENT |\
> +		DRM_EXYNOS_IPP_FLAG_TEST_ONLY | DRM_EXYNOS_IPP_FLAG_NONBLOCK)
> +
> +/**
> + * Perform image processing described by array of drm_exynos_ipp_task_*
> + * structures (parameters array).
> + *
> + * @ipp_id: id of IPP module to run the task
> + * @flags: bitmask of drm_exynos_ipp_flag values
> + * @reserved: padding
> + * @params_size: size of parameters array (in bytes)
> + * @params_ptr: pointer to parameters array or NULL
> + * @user_data: (optional) data for drm event
> + */
> +struct drm_exynos_ioctl_ipp_commit {
> +	__u32 ipp_id;
> +	__u32 flags;
> +	__u32 reserved;
> +	__u32 params_size;
> +	__u64 params_ptr;
> +	__u64 user_data;
> +};
> +
>  #define DRM_EXYNOS_GEM_CREATE		0x00
>  #define DRM_EXYNOS_GEM_MAP		0x01
>  /* Reserved 0x03 ~ 0x05 for exynos specific gem ioctl */
> @@ -146,6 +359,11 @@ struct drm_exynos_g2d_exec {
>  #define DRM_EXYNOS_G2D_EXEC		0x22
>  
>  /* Reserved 0x30 ~ 0x33 for obsolete Exynos IPP ioctls */
> +/* IPP - Image Post Processing */
> +#define DRM_EXYNOS_IPP_GET_RESOURCES	0x40
> +#define DRM_EXYNOS_IPP_GET_CAPS		0x41
> +#define DRM_EXYNOS_IPP_GET_LIMITS	0x42
> +#define DRM_EXYNOS_IPP_COMMIT		0x43
>  
>  #define DRM_IOCTL_EXYNOS_GEM_CREATE		DRM_IOWR(DRM_COMMAND_BASE + \
>  		DRM_EXYNOS_GEM_CREATE, struct drm_exynos_gem_create)
> @@ -164,8 +382,18 @@ struct drm_exynos_g2d_exec {
>  #define DRM_IOCTL_EXYNOS_G2D_EXEC		DRM_IOWR(DRM_COMMAND_BASE + \
>  		DRM_EXYNOS_G2D_EXEC, struct drm_exynos_g2d_exec)
>  
> +#define DRM_IOCTL_EXYNOS_IPP_GET_RESOURCES	DRM_IOWR(DRM_COMMAND_BASE + \
> +		DRM_EXYNOS_IPP_GET_RESOURCES, struct drm_exynos_ioctl_ipp_get_res)
> +#define DRM_IOCTL_EXYNOS_IPP_GET_CAPS		DRM_IOWR(DRM_COMMAND_BASE + \
> +		DRM_EXYNOS_IPP_GET_CAPS, struct drm_exynos_ioctl_ipp_get_caps)
> +#define DRM_IOCTL_EXYNOS_IPP_GET_LIMITS		DRM_IOWR(DRM_COMMAND_BASE + \
> +		DRM_EXYNOS_IPP_GET_LIMITS, struct drm_exynos_ioctl_ipp_get_limits)
> +#define DRM_IOCTL_EXYNOS_IPP_COMMIT		DRM_IOWR(DRM_COMMAND_BASE + \
> +		DRM_EXYNOS_IPP_COMMIT, struct drm_exynos_ioctl_ipp_commit)
> +
>  /* EXYNOS specific events */
>  #define DRM_EXYNOS_G2D_EVENT		0x80000000
> +#define DRM_EXYNOS_IPP_EVENT		0x80000002
>  
>  struct drm_exynos_g2d_event {
>  	struct drm_event	base;
> @@ -176,6 +404,16 @@ struct drm_exynos_g2d_event {
>  	__u32			reserved;
>  };
>  
> +struct drm_exynos_ipp_event {
> +	struct drm_event	base;
> +	__u64			user_data;
> +	__u32			tv_sec;
> +	__u32			tv_usec;
> +	__u32			ipp_id;
> +	__u32			sequence;
> +	__u64			reserved;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux