[RFC v2 0/2] Exynos DRM: add Picture Processor extension

[RFC v2 0/2] Exynos DRM: add Picture Processor extension

[Marek Szyprowski]

Dear all,

This is an updated proposal for extending EXYNOS DRM API with generic support
for hardware modules, which can be used for processing image data from the
one memory buffer to another. Typical memory-to-memory operations are:
rotation, scaling, colour space conversion or mix of them. This is
a follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
processors", which has been rejected as "not really needed in the DRM core":
http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html

In this proposal I moved all the code to Exynos DRM driver, so now this
will be specific only to Exynos DRM. I've also changed the name from
framebuffer processor (fbproc) to picture processor (pp) to avoid confusion
with fbdev API.

Here is a bit more information what picture processors are:

Embedded SoCs are known to have a number of hardware blocks, which perform
such operations. They can be used in paralel to the main GPU module to
offload CPU from processing grapics or video data. One of example use of
such modules is implementing video overlay, which usually requires color
space conversion from NV12 (or similar) to RGB32 color space and scaling to
target window size.

The proposed API is ispired a bit by atomic KMS approach. Each picture
processor has a number of parameters, which describe operation to be
performed by respective hardware module. The list is open and can be
extended in the future. In typical case those parameters are a source fb
id and rectangle (x, y, width, height) and destination fb id and
rectangle as well as a rotation property. Parameters are provided by
their predefined IDs. To perform an operation on image data, userspace
provides a set of parameters and their values for given picture processor.

The proposed API consists of the 3 new ioctls:
- DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
  processors,
- DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
  processor,
- DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
  property set.

The proposed userspace API is extensible. Drivers can add their own,
custom parameters to add support for more advanced picture processing (for
example blending).

This proposal aims to replace Exynos DRM IPP (Image Post Processing)
subsystem. IPP API is over-engineered in general, but not really extensible
on the other side. It is also buggy, with significant design flaws - the
biggest issue is the fact that the API covers memory-2-memory picture
operations together with CRTC writeback and duplicating features, which
belongs to video plane. Comparing with IPP subsystem, the PP framework is
smaller (1807 vs 646 lines) and allows driver simplification (Exynos
rotator driver smaller by over 200 lines).

Open questions:
- How to expose supported pixel formats/modifiers? Currently this is done
  by the arrays of supported fourcc codes in the drm_exynos_pp_get structure
  and DRM_IOCTL_EXYNOS_PP_GET ioctl.
  I would like to switch to a structure similar to recently discussed
  format/modifier blob to avoid reinventing wheel:
  https://www.spinics.net/lists/intel-gfx/msg127094.html
- How to expose the range of the supported parameters (min/max width,
  rotation values)? Is it really needed?


TODO:
- convert remaining Exynos DRM IPP drivers (FIMC, GScaller)
- remove Exynos DRM IPP subsystem
- (optional) provide virtual V4L2 mem2mem device on top of Exynos PP framework

Patches were tested on Exynos 4412-based Odroid U3 board, on top of Linux v4.11
kernel.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Changelog:
v2:
- removed usage of DRM objects and properties - replaced them with simple list
  of parameters with predefined IDs

v1:
- moved this feature from DRM core to Exynos DRM driver
- changed name from framebuffer processor to picture processor
- simplified code to cover only things needed by Exynos drivers
- implemented simple fifo task scheduler
- cleaned up rotator driver conversion (removed IPP remainings)


v0: http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
- initial post of "[RFC 0/2] New feature: Framebuffer processors"
- generic approach implemented in DRM core, rejected


Patch summary:

Marek Szyprowski (2):
  drm/exynos: Add Picture Processor framework
  drm/exynos: Convert Exynos Rotator driver to Picture Processor
    interface

 drivers/gpu/drm/exynos/Kconfig              |   1 -
 drivers/gpu/drm/exynos/Makefile             |   3 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c     |   9 +
 drivers/gpu/drm/exynos/exynos_drm_drv.h     |   4 +
 drivers/gpu/drm/exynos/exynos_drm_pp.c      | 645 ++++++++++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_pp.h      | 153 +++++++
 drivers/gpu/drm/exynos/exynos_drm_rotator.c | 493 +++++----------------
 drivers/gpu/drm/exynos/exynos_drm_rotator.h |  19 -
 include/uapi/drm/exynos_drm.h               |  76 ++++
 9 files changed, 1006 insertions(+), 397 deletions(-)
 create mode 100644 drivers/gpu/drm/exynos/exynos_drm_pp.c
 create mode 100644 drivers/gpu/drm/exynos/exynos_drm_pp.h
 delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_rotator.h

-- 
1.9.1

[RFC v2 1/2] drm/exynos: Add Picture Processor framework

[Marek Szyprowski]

This patch extends Exynos DRM API with picture processor hardware modules.
Such modules can be used for processing image data from the one memory buffer
to another. Typical memory-to-memory operations are: rotation, scaling, colour
space conversion or mix of them.

The proposed API is heavily inspired by atomic KMS approach - it is also
based on DRM objects and their properties. A new DRM object is introduced:
picture processor (called pp for convenience). Such objects have a set of
standard DRM properties, which describes the operation to be performed by
respective hardware module. In typical case those properties are a source
fb id and rectangle (x, y, width, height) and destination fb id and
rectangle. Optionally a rotation property can be also specified if
supported by the given hardware. To perform an operation on image data,
userspace provides a set of properties and their values for given fbproc
object in a similar way as object and properties are provided for
performing atomic page flip / mode setting.

The proposed API consists of the 3 new ioctls:
- DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
  processors,
- DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
  processor,
- DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
  property set.

The proposed userspace API is extensible. Drivers can add their own,
custom parameters to add support for more advanced picture processing (for
example blending).

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/Makefile         |   3 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c |   8 +
 drivers/gpu/drm/exynos/exynos_drm_drv.h |   4 +
 drivers/gpu/drm/exynos/exynos_drm_pp.c  | 645 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_pp.h  | 153 ++++++++
 include/uapi/drm/exynos_drm.h           |  76 ++++
 6 files changed, 888 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/exynos/exynos_drm_pp.c
 create mode 100644 drivers/gpu/drm/exynos/exynos_drm_pp.h

diff --git a/drivers/gpu/drm/exynos/Makefile b/drivers/gpu/drm/exynos/Makefile
index f663490e949d..2632b0ee5d2d 100644
--- a/drivers/gpu/drm/exynos/Makefile
+++ b/drivers/gpu/drm/exynos/Makefile
@@ -3,7 +3,8 @@
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
 exynosdrm-y := exynos_drm_drv.o exynos_drm_crtc.o exynos_drm_fb.o \
-		exynos_drm_gem.o exynos_drm_core.o exynos_drm_plane.o
+		exynos_drm_gem.o exynos_drm_core.o exynos_drm_plane.o \
+		exynos_drm_pp.o
 
 exynosdrm-$(CONFIG_DRM_FBDEV_EMULATION) += exynos_drm_fbdev.o
 exynosdrm-$(CONFIG_DRM_EXYNOS_IOMMU) += exynos_drm_iommu.o
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 035d02ecffcd..386a9d325c5a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -27,6 +27,7 @@
 #include "exynos_drm_fb.h"
 #include "exynos_drm_gem.h"
 #include "exynos_drm_plane.h"
+#include "exynos_drm_pp.h"
 #include "exynos_drm_vidi.h"
 #include "exynos_drm_g2d.h"
 #include "exynos_drm_ipp.h"
@@ -53,6 +54,7 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
 
 	init_waitqueue_head(&private->wait);
 	spin_lock_init(&private->lock);
+	INIT_LIST_HEAD(&private->pp_list);
 
 	dev_set_drvdata(dev->dev, dev);
 	dev->dev_private = (void *)private;
@@ -241,6 +243,12 @@ static void exynos_drm_lastclose(struct drm_device *dev)
 			DRM_AUTH | DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(EXYNOS_IPP_CMD_CTRL, exynos_drm_ipp_cmd_ctrl,
 			DRM_AUTH | DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(EXYNOS_PP_GET_RESOURCES, exynos_drm_pp_get_res,
+			DRM_AUTH | DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(EXYNOS_PP_GET, exynos_drm_pp_get,
+			DRM_AUTH | DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(EXYNOS_PP_COMMIT, exynos_drm_pp_commit,
+			DRM_AUTH | DRM_RENDER_ALLOW),
 };
 
 static const struct file_operations exynos_drm_driver_fops = {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index cf6e08cb35a7..bdc049e8c94f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -220,6 +220,10 @@ struct exynos_drm_private {
 	u32			pending;
 	spinlock_t		lock;
 	wait_queue_head_t	wait;
+
+	/* for pp api */
+	int num_pp;
+	struct list_head pp_list;
 };
 
 static inline struct exynos_drm_crtc *
diff --git a/drivers/gpu/drm/exynos/exynos_drm_pp.c b/drivers/gpu/drm/exynos/exynos_drm_pp.c
new file mode 100644
index 000000000000..e94231dbcb9c
--- /dev/null
+++ b/drivers/gpu/drm/exynos/exynos_drm_pp.c
@@ -0,0 +1,645 @@
+/*
+ * Copyright (C) 2017 Samsung Electronics Co.Ltd
+ * Authors:
+ *	Marek Szyprowski <m.szyprowski@samsung.com>
+ *
+ * Exynos DRM Picture Processor (PP) 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.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN 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_pp.h"
+
+struct drm_pending_exynos_pp_event {
+	struct drm_pending_event base;
+	struct drm_exynos_pp_event event;
+};
+
+static const uint32_t exynos_drm_standard_parameters[] = {
+	DRM_EXYNOS_PP_PARAM_SRC_FB,
+	DRM_EXYNOS_PP_PARAM_SRC_X,
+	DRM_EXYNOS_PP_PARAM_SRC_Y,
+	DRM_EXYNOS_PP_PARAM_SRC_WIDTH,
+	DRM_EXYNOS_PP_PARAM_SRC_HEIGHT,
+	DRM_EXYNOS_PP_PARAM_DST_FB,
+	DRM_EXYNOS_PP_PARAM_DST_X,
+	DRM_EXYNOS_PP_PARAM_DST_Y,
+	DRM_EXYNOS_PP_PARAM_DST_WIDTH,
+	DRM_EXYNOS_PP_PARAM_DST_HEIGHT,
+	DRM_EXYNOS_PP_PARAM_ROTATION,
+};
+
+/**
+ * exynos_drm_pp_register - Register a new picture processor hardware module
+ * @dev: DRM device
+ * @pp: pp module to init
+ * @funcs: callbacks for the new pp object
+ * @caps: bitmask of pp capabilities (%DRM_EXYNOS_PP_CAP_*)
+ * @src_fmts: array of supported source fb formats (%DRM_FORMAT_*)
+ * @src_fmt_count: number of elements in @src_fmts
+ * @dst_fmts: array of supported destination fb formats (%DRM_FORMAT_*)
+ * @dst_fmt_count: number of elements in @dst_fmts
+ * @rotation: a set of supported rotation transformations
+ * @name: name (for debugging purposes)
+ *
+ * Initializes a pp module.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int exynos_drm_pp_register(struct drm_device *dev, struct exynos_drm_pp *pp,
+		    const struct exynos_drm_pp_funcs *funcs, unsigned int caps,
+		    const uint32_t *src_fmts, unsigned int src_fmt_count,
+		    const uint32_t *dst_fmts, unsigned int dst_fmt_count,
+		    unsigned int rotation, const char *name)
+{
+	struct exynos_drm_private *priv = dev->dev_private;
+
+	spin_lock_init(&pp->lock);
+	INIT_LIST_HEAD(&pp->todo_list);
+	init_waitqueue_head(&pp->done_wq);
+	pp->dev = dev;
+	pp->funcs = funcs;
+	pp->capabilities = caps;
+	pp->src_format_types = src_fmts;
+	pp->src_format_count = src_fmt_count;
+	pp->dst_format_types = dst_fmts;
+	pp->dst_format_count = dst_fmt_count;
+	pp->rotation = rotation;
+	pp->parameters = exynos_drm_standard_parameters;
+	pp->parameters_count = ARRAY_SIZE(exynos_drm_standard_parameters);
+	pp->name = name;
+
+	list_add_tail(&pp->head, &priv->pp_list);
+	pp->id = priv->num_pp++;
+
+	DRM_DEBUG_DRIVER("Registered pp %d\n", pp->id);
+
+	return 0;
+}
+
+/**
+ * exynos_drm_pp_unregister - Unregister the picture processor module
+ * @dev: DRM device
+ * @pp: pp module
+ */
+void exynos_drm_pp_unregister(struct drm_device *dev, struct exynos_drm_pp *pp)
+{
+	BUG_ON(pp->task);
+	BUG_ON(!list_empty(&pp->todo_list));
+	list_del(&pp->head);
+}
+
+/**
+ * exynos_drm_pp_get_res - enumerate all pp modules
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Construct a list of pp ids to return to the user.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int exynos_drm_pp_get_res(struct drm_device *dev, void *data,
+			  struct drm_file *file_priv)
+{
+	struct exynos_drm_private *priv = dev->dev_private;
+	struct drm_exynos_pp_get_res *resp = data;
+	struct exynos_drm_pp *pp;
+	uint32_t __user *pp_ptr;
+	unsigned int count = priv->num_pp, copied = 0;
+
+	/*
+	 * This ioctl is called twice, once to determine how much space is
+	 * needed, and the 2nd time to fill it.
+	 */
+	if (count && resp->count_pps >= count) {
+		pp_ptr = (uint32_t __user *)
+					(unsigned long)resp->pp_id_ptr;
+
+		list_for_each_entry(pp, &priv->pp_list, head) {
+			if (put_user(pp->id, pp_ptr + copied))
+				return -EFAULT;
+			copied++;
+		}
+	}
+	resp->count_pps = count;
+
+	return 0;
+}
+
+static inline struct exynos_drm_pp *exynos_drm_pp_find(struct drm_device *dev,
+						       uint32_t id)
+{
+	struct exynos_drm_private *priv = dev->dev_private;
+	struct exynos_drm_pp *pp;
+
+	list_for_each_entry(pp, &priv->pp_list, head) {
+		if (pp->id == id)
+			return pp;
+	}
+	return NULL;
+}
+
+/**
+ * exynos_drm_pp_get - get picture processor module parameters
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Construct a pp configuration structure to return to the user.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int exynos_drm_pp_get(struct drm_device *dev, void *data,
+		      struct drm_file *file_priv)
+{
+	struct drm_exynos_pp_get *resp = data;
+	struct exynos_drm_pp *pp;
+	uint32_t __user *format_ptr;
+
+	pp = exynos_drm_pp_find(dev, resp->pp_id);
+	if (!pp)
+		return -ENOENT;
+
+	resp->pp_id = pp->id;
+	resp->capabilities = pp->capabilities;
+
+	/*
+	 * This ioctl is called twice, once to determine how much space is
+	 * needed, and the 2nd time to fill it.
+	 */
+	if (pp->src_format_count &&
+	    (resp->src_format_count >= pp->src_format_count)) {
+		format_ptr = (uint32_t __user *)
+				(unsigned long)resp->src_format_type_ptr;
+		if (copy_to_user(format_ptr, pp->src_format_types,
+				 sizeof(uint32_t) * pp->src_format_count))
+			return -EFAULT;
+	}
+	if (pp->dst_format_count &&
+	    (resp->dst_format_count >= pp->dst_format_count)) {
+		format_ptr = (uint32_t __user *)
+				(unsigned long)resp->dst_format_type_ptr;
+		if (copy_to_user(format_ptr, pp->dst_format_types,
+				 sizeof(uint32_t) * pp->dst_format_count))
+			return -EFAULT;
+	}
+	if (pp->parameters_count &&
+	    (resp->params_count >= pp->parameters_count)) {
+		format_ptr = (uint32_t __user *)
+				(unsigned long)resp->params_ptr;
+		if (copy_to_user(format_ptr, pp->parameters,
+				 sizeof(uint32_t) * pp->parameters_count))
+			return -EFAULT;
+	}
+	resp->src_format_count = pp->src_format_count;
+	resp->dst_format_count = pp->dst_format_count;
+	resp->params_count = pp->parameters_count;
+
+	return 0;
+}
+
+static inline struct exynos_drm_pp_task *
+	exynos_drm_pp_task_alloc(struct exynos_drm_pp *pp)
+{
+	struct exynos_drm_pp_task *task;
+
+	task = kzalloc(sizeof(*task), GFP_KERNEL);
+	if (!task)
+		return NULL;
+
+	task->dev = pp->dev;
+	task->pp = pp;
+	task->src_w = task->dst_w = UINT_MAX;
+	task->src_h = task->dst_h = UINT_MAX;
+	task->rotation = DRM_ROTATE_0;
+
+	DRM_DEBUG_DRIVER("Allocated task %pK\n", task);
+
+	return task;
+}
+
+static void exynos_drm_pp_task_free(struct exynos_drm_pp *pp,
+				 struct exynos_drm_pp_task *task)
+{
+	DRM_DEBUG_DRIVER("Freeing task %pK\n", task);
+
+	task->pp = NULL;
+
+	if (task->src_fb) {
+		drm_framebuffer_unreference(task->src_fb);
+		task->src_fb = NULL;
+	}
+	if (task->dst_fb) {
+		drm_framebuffer_unreference(task->dst_fb);
+		task->dst_fb = NULL;
+	}
+	if (task->event) {
+		drm_event_cancel_free(pp->dev, &task->event->base);
+		task->event = NULL;
+	}
+	kfree(task);
+}
+
+static int exynos_drm_pp_task_set_param(struct exynos_drm_pp_task *task,
+		unsigned int param, unsigned int value)
+{
+	struct drm_device *dev = task->dev;
+	struct drm_framebuffer *fb;
+	int ret = 0;
+
+	if (param == DRM_EXYNOS_PP_PARAM_SRC_FB) {
+		fb = drm_framebuffer_lookup(dev, value);
+		if (task->src_fb)
+			drm_framebuffer_unreference(task->src_fb);
+		task->src_fb = fb;
+	} else if (param == DRM_EXYNOS_PP_PARAM_SRC_X) {
+		task->src_x = value;
+	} else if (param == DRM_EXYNOS_PP_PARAM_SRC_Y) {
+		task->src_y = value;
+	} else if (param == DRM_EXYNOS_PP_PARAM_SRC_WIDTH) {
+		task->src_w = value;
+	} else if (param == DRM_EXYNOS_PP_PARAM_SRC_HEIGHT) {
+		task->src_h = value;
+	} else if (param == DRM_EXYNOS_PP_PARAM_DST_FB) {
+		fb = drm_framebuffer_lookup(dev, value);
+		if (task->dst_fb)
+			drm_framebuffer_unreference(task->dst_fb);
+		task->dst_fb = fb;
+	} else if (param == DRM_EXYNOS_PP_PARAM_DST_X) {
+		task->dst_x = value;
+	} else if (param == DRM_EXYNOS_PP_PARAM_DST_Y) {
+		task->dst_y = value;
+	} else if (param == DRM_EXYNOS_PP_PARAM_DST_WIDTH) {
+		task->dst_w = value;
+	} else if (param == DRM_EXYNOS_PP_PARAM_DST_HEIGHT) {
+		task->dst_h = value;
+	} else if (param == DRM_EXYNOS_PP_PARAM_ROTATION) {
+		task->rotation = value;
+	} else {
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static struct drm_pending_exynos_pp_event *exynos_drm_pp_event_create(
+			struct drm_device *dev, struct drm_file *file_priv,
+			uint64_t user_data)
+{
+	struct drm_pending_exynos_pp_event *e = NULL;
+	int ret;
+
+	e = kzalloc(sizeof(*e), GFP_KERNEL);
+	if (!e)
+		return NULL;
+
+	e->event.base.type = DRM_EXYNOS_PP_EVENT;
+	e->event.base.length = sizeof(e->event);
+	e->event.user_data = user_data;
+
+	if (file_priv) {
+		ret = drm_event_reserve_init(dev, file_priv, &e->base,
+					     &e->event.base);
+		if (ret) {
+			kfree(e);
+			return NULL;
+		}
+	}
+
+	return e;
+}
+
+static void exynos_drm_pp_event_send(struct drm_device *dev,
+				  struct exynos_drm_pp *pp,
+				  struct drm_pending_exynos_pp_event *e)
+{
+	struct timeval now = ktime_to_timeval(ktime_get());
+
+	e->event.tv_sec = now.tv_sec;
+	e->event.tv_usec = now.tv_usec;
+	e->event.sequence = atomic_inc_return(&pp->sequence);
+
+	drm_send_event(dev, &e->base);
+}
+
+static inline bool drm_fb_check_format(struct drm_framebuffer *fb,
+				const uint32_t *formats, int format_counts)
+{
+	while (format_counts--)
+		if (*formats++ == fb->format->format)
+			return true;
+	return false;
+}
+
+static int exynos_drm_pp_task_check(struct exynos_drm_pp_task *task)
+{
+	struct exynos_drm_pp *pp = task->pp;
+	int ret = 0;
+
+	DRM_DEBUG_DRIVER("checking %pK\n", task);
+
+	if (!task->src_fb || !task->dst_fb)
+		return -EINVAL;
+
+	if (!drm_fb_check_format(task->src_fb, pp->src_format_types,
+				 pp->src_format_count))
+		return -EINVAL;
+
+	if (!drm_fb_check_format(task->dst_fb, pp->dst_format_types,
+				 pp->dst_format_count))
+		return -EINVAL;
+
+	if (task->src_w == UINT_MAX)
+		task->src_w = task->src_fb->width << 16;
+	if (task->src_h == UINT_MAX)
+		task->src_h = task->src_fb->height << 16;
+	if (task->dst_w == UINT_MAX)
+		task->dst_w = task->dst_fb->width << 16;
+	if (task->dst_h == UINT_MAX)
+		task->dst_h = task->dst_fb->height << 16;
+
+	if (task->src_x + task->src_w > (task->src_fb->width << 16) ||
+	    task->src_y + task->src_h > (task->src_fb->height << 16) ||
+	    task->dst_x + task->dst_w > (task->dst_fb->width << 16) ||
+	    task->dst_y + task->dst_h > (task->dst_fb->height << 16))
+		return -EINVAL;
+
+	if (!(pp->capabilities & DRM_EXYNOS_PP_CAP_CROP) &&
+	    (task->src_x || task->src_y || task->dst_x || task->dst_y))
+		return -EINVAL;
+
+	if (!(pp->capabilities & DRM_EXYNOS_PP_CAP_ROTATE) &&
+	    task->rotation != DRM_ROTATE_0)
+		return -EINVAL;
+
+	if (!(pp->capabilities & DRM_EXYNOS_PP_CAP_SCALE) &&
+	    (!drm_rotation_90_or_270(task->rotation) &&
+	     (task->src_w != task->dst_w || task->src_h != task->dst_h)) &&
+	    (drm_rotation_90_or_270(task->rotation) &&
+	     (task->src_w != task->dst_h || task->src_h != task->dst_w)))
+		return -EINVAL;
+
+	if (!(pp->capabilities & DRM_EXYNOS_PP_CAP_CONVERT) &&
+	    task->src_fb->format->format != task->dst_fb->format->format)
+		return -EINVAL;
+
+	if (pp->funcs->check)
+		ret = pp->funcs->check(pp, task);
+
+	return ret;
+}
+
+static int exynos_drm_pp_task_cleanup(struct exynos_drm_pp_task *task)
+{
+	int ret = task->ret;
+
+	if (ret == 0 && task->event) {
+		exynos_drm_pp_event_send(task->dev, task->pp, task->event);
+		/* ensure event won't be canceled on task free */
+		task->event = NULL;
+	}
+
+	exynos_drm_pp_task_free(task->pp, task);
+	return ret;
+}
+
+static void exynos_drm_pp_cleanup_work(struct work_struct *work)
+{
+	struct exynos_drm_pp_task *task = container_of(work,
+				struct exynos_drm_pp_task, cleanup_work);
+
+	exynos_drm_pp_task_cleanup(task);
+}
+
+static void exynos_drm_pp_next_task(struct exynos_drm_pp *pp);
+
+void exynos_drm_pp_task_done(struct exynos_drm_pp_task *task, int ret)
+{
+	struct exynos_drm_pp *pp = task->pp;
+	unsigned long flags;
+
+	DRM_DEBUG_DRIVER("pp: %d, task %pK done\n", pp->id, task);
+
+	spin_lock_irqsave(&pp->lock, flags);
+	if (pp->task == task)
+		pp->task = NULL;
+	task->flags |= DRM_EXYNOS_PP_TASK_DONE;
+	task->ret = ret;
+	spin_unlock_irqrestore(&pp->lock, flags);
+
+	exynos_drm_pp_next_task(pp);
+	wake_up(&pp->done_wq);
+
+	if (task->flags & DRM_EXYNOS_PP_TASK_ASYNC) {
+		INIT_WORK(&task->cleanup_work, exynos_drm_pp_cleanup_work);
+		schedule_work(&task->cleanup_work);
+	}
+}
+
+static void exynos_drm_pp_next_task(struct exynos_drm_pp *pp)
+{
+	struct exynos_drm_pp_task *task;
+	unsigned long flags;
+	int ret;
+
+	DRM_DEBUG_DRIVER("pp: %d, try to run new task\n", pp->id);
+
+	spin_lock_irqsave(&pp->lock, flags);
+
+	if (pp->task || list_empty(&pp->todo_list)) {
+		spin_unlock_irqrestore(&pp->lock, flags);
+		return;
+	}
+
+	task = list_first_entry(&pp->todo_list, struct exynos_drm_pp_task,
+				head);
+	list_del_init(&task->head);
+	pp->task = task;
+
+	spin_unlock_irqrestore(&pp->lock, flags);
+
+	DRM_DEBUG_DRIVER("pp: %d, selected task %pK to run\n", pp->id, task);
+
+	ret = pp->funcs->commit(pp, task);
+	if (ret)
+		exynos_drm_pp_task_done(task, ret);
+}
+
+static void exynos_drm_pp_schedule_task(struct exynos_drm_pp *pp,
+				     struct exynos_drm_pp_task *task)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&pp->lock, flags);
+	list_add(&task->head, &pp->todo_list);
+	spin_unlock_irqrestore(&pp->lock, flags);
+
+	exynos_drm_pp_next_task(pp);
+}
+
+static void exynos_drm_pp_task_abort(struct exynos_drm_pp *pp,
+				  struct exynos_drm_pp_task *task)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&pp->lock, flags);
+	if (task->flags & DRM_EXYNOS_PP_TASK_DONE) {
+		/* already completed task */
+		exynos_drm_pp_task_cleanup(task);
+	} else if (pp->task != task) {
+		/* task has not been scheduled for execution yet */
+		list_del_init(&task->head);
+		exynos_drm_pp_task_cleanup(task);
+	} else {
+		/*
+		 * currently processed task, call abort() and perform
+		 * cleanup with async worker
+		 */
+		task->flags |= DRM_EXYNOS_PP_TASK_ASYNC;
+		if (pp->funcs->abort)
+			pp->funcs->abort(pp, task);
+	}
+	spin_unlock_irqrestore(&pp->lock, flags);
+}
+
+/**
+ * exynos_drm_pp_ioctl - perform operation on framebuffer processor object
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Construct a pp 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_pp_commit(struct drm_device *dev, void *data,
+			  struct drm_file *file_priv)
+{
+	struct drm_exynos_pp_commit *arg = data;
+	uint32_t __user *param_ids_ptr =
+		(uint32_t __user *)(unsigned long)(arg->param_ids_ptr);
+	uint64_t __user *param_values_ptr =
+		(uint64_t __user *)(unsigned long)(arg->param_values_ptr);
+	struct exynos_drm_pp *pp;
+	struct exynos_drm_pp_task *task;
+	int ret = 0;
+	unsigned int i;
+
+	if (arg->flags & ~DRM_EXYNOS_PP_FLAGS)
+		return -EINVAL;
+
+	if (arg->reserved)
+		return -EINVAL;
+
+	/* can't test and expect an event at the same time */
+	if ((arg->flags & DRM_EXYNOS_PP_FLAG_TEST_ONLY) &&
+			(arg->flags & DRM_EXYNOS_PP_FLAG_EVENT))
+		return -EINVAL;
+
+	pp = exynos_drm_pp_find(dev, arg->id);
+	if (!pp)
+		return -ENOENT;
+
+	task = exynos_drm_pp_task_alloc(pp);
+	if (!task) {
+		ret = -ENOMEM;
+		goto free;
+	}
+
+	for (i = 0; i < arg->params_count; i++) {
+		uint32_t param_id;
+		uint64_t param_value;
+
+		if (get_user(param_id, param_ids_ptr + i)) {
+			ret = -EFAULT;
+			goto free;
+		}
+
+		if (copy_from_user(&param_value, param_values_ptr + i,
+				   sizeof(param_value))) {
+			ret = -EFAULT;
+			goto free;
+		}
+
+		ret = exynos_drm_pp_task_set_param(task, param_id, param_value);
+		if (ret)
+			goto free;
+	}
+
+	if (arg->flags & DRM_EXYNOS_PP_FLAG_EVENT) {
+		struct drm_pending_exynos_pp_event *e;
+
+		e = exynos_drm_pp_event_create(dev, file_priv, arg->user_data);
+		if (!e) {
+			ret = -ENOMEM;
+			goto free;
+		}
+		task->event = e;
+	}
+
+	ret = exynos_drm_pp_task_check(task);
+	if (ret || arg->flags & DRM_EXYNOS_PP_FLAG_TEST_ONLY)
+		goto free;
+
+	/*
+	 * Queue task for processing on the hardware. task object will be
+	 * then freed after exynos_drm_pp_task_done()
+	 */
+	if (arg->flags & DRM_EXYNOS_PP_FLAG_NONBLOCK) {
+		DRM_DEBUG_DRIVER("pp: %d, nonblocking processing task %pK\n",
+				 task->pp->id, task);
+
+		task->flags |= DRM_EXYNOS_PP_TASK_ASYNC;
+		exynos_drm_pp_schedule_task(task->pp, task);
+		ret = 0;
+	} else {
+		DRM_DEBUG_DRIVER("pp: %d, processing task %pK\n", pp->id,
+				 task);
+		exynos_drm_pp_schedule_task(pp, task);
+		ret = wait_event_interruptible(pp->done_wq,
+					task->flags & DRM_EXYNOS_PP_TASK_DONE);
+		if (ret)
+			exynos_drm_pp_task_abort(pp, task);
+		else
+			ret = exynos_drm_pp_task_cleanup(task);
+	}
+	return ret;
+free:
+	exynos_drm_pp_task_free(pp, task);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_pp.h b/drivers/gpu/drm/exynos/exynos_drm_pp.h
new file mode 100644
index 000000000000..d0422abb2de4
--- /dev/null
+++ b/drivers/gpu/drm/exynos/exynos_drm_pp.h
@@ -0,0 +1,153 @@
+/*
+ * 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_FBPROC_H_
+#define _EXYNOS_DRM_FBPORC_H_
+
+#include <drm/drmP.h>
+
+struct exynos_drm_pp;
+struct exynos_drm_pp_task;
+
+/**
+ * struct exynos_drm_pp_funcs - exynos_drm_pp control functions
+ */
+struct exynos_drm_pp_funcs {
+	/**
+	 * @check:
+	 *
+	 * This is the optional hook to validate an pp 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 framebuffers, scaling and placement
+	 *    requirements and so on are within the limits of the hardware.
+	 *
+	 *  - The driver does not need to repeat basic input validation like
+	 *    done in the exynos_drm_pp_check_only() 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_pp *pp,
+		     struct exynos_drm_pp_task *task);
+
+	/**
+	 * @commit:
+	 *
+	 * This is the main entry point to start framebuffer processing
+	 * in the hardware. The exynos_drm_pp_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_pp_task_done() function.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or negative error codes in case of failure.
+	 */
+	int (*commit)(struct exynos_drm_pp *pp,
+		      struct exynos_drm_pp_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_pp_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_pp *pp,
+		      struct exynos_drm_pp_task *task);
+};
+
+/**
+ * struct exynos_drm_pp - central picture processor module structure
+ */
+struct exynos_drm_pp {
+	struct drm_device *dev;
+	struct list_head head;
+	unsigned int id;
+
+	const char *name;
+	const struct exynos_drm_pp_funcs *funcs;
+	unsigned int capabilities;
+	unsigned int *params;
+	unsigned int rotation;
+	atomic_t sequence;
+
+	spinlock_t lock;
+	struct exynos_drm_pp_task *task;
+	struct list_head todo_list;
+	wait_queue_head_t done_wq;
+
+	const uint32_t *src_format_types;
+	unsigned int src_format_count;
+	const uint32_t *dst_format_types;
+	unsigned int dst_format_count;
+	const uint32_t *parameters;
+	unsigned int parameters_count;
+};
+
+/**
+ * struct exynos_drm_pp_task - a structure describing transformation that
+ * has to be performed by the picture processor hardware module
+ */
+struct exynos_drm_pp_task {
+	struct drm_device *dev;
+	struct exynos_drm_pp *pp;
+	struct list_head head;
+
+	struct drm_framebuffer *src_fb;
+
+	/* Source values are 16.16 fixed point */
+	uint32_t src_x, src_y;
+	uint32_t src_h, src_w;
+
+	struct drm_framebuffer *dst_fb;
+
+	/* Destination values are 16.16 fixed point */
+	uint32_t dst_x, dst_y;
+	uint32_t dst_h, dst_w;
+
+	uint32_t rotation;
+
+	struct work_struct cleanup_work;
+	unsigned int flags;
+	int ret;
+
+	struct drm_pending_exynos_pp_event *event;
+};
+
+#define DRM_EXYNOS_PP_TASK_DONE		(1 << 0)
+#define DRM_EXYNOS_PP_TASK_ASYNC	(1 << 1)
+
+int exynos_drm_pp_register(struct drm_device *dev, struct exynos_drm_pp *pp,
+		    const struct exynos_drm_pp_funcs *funcs, unsigned int caps,
+		    const uint32_t *src_fmts, unsigned int src_fmt_count,
+		    const uint32_t *dst_fmts, unsigned int dst_fmt_count,
+		    unsigned int rotation, const char *name);
+void exynos_drm_pp_unregister(struct drm_device *dev, struct exynos_drm_pp *pp);
+
+void exynos_drm_pp_task_done(struct exynos_drm_pp_task *task, int ret);
+
+int exynos_drm_pp_get_res(struct drm_device *dev, void *data,
+			  struct drm_file *file_priv);
+int exynos_drm_pp_get(struct drm_device *dev, void *data,
+		      struct drm_file *file_priv);
+int exynos_drm_pp_commit(struct drm_device *dev,
+			 void *data, struct drm_file *file_priv);
+
+#endif
diff --git a/include/uapi/drm/exynos_drm.h b/include/uapi/drm/exynos_drm.h
index cb3e9f9d029f..a4e980395ddd 100644
--- a/include/uapi/drm/exynos_drm.h
+++ b/include/uapi/drm/exynos_drm.h
@@ -300,6 +300,60 @@ struct drm_exynos_ipp_cmd_ctrl {
 	__u32	ctrl;
 };
 
+struct drm_exynos_pp_get_res {
+	__u64 pp_id_ptr;
+	__u32 count_pps;
+};
+
+#define DRM_EXYNOS_PP_PARAM_SRC_FB	0x0001
+#define DRM_EXYNOS_PP_PARAM_SRC_X	0x0002
+#define DRM_EXYNOS_PP_PARAM_SRC_Y	0x0004
+#define DRM_EXYNOS_PP_PARAM_SRC_WIDTH	0x0008
+#define DRM_EXYNOS_PP_PARAM_SRC_HEIGHT	0x0010
+#define DRM_EXYNOS_PP_PARAM_DST_FB	0x0020
+#define DRM_EXYNOS_PP_PARAM_DST_X	0x0040
+#define DRM_EXYNOS_PP_PARAM_DST_Y	0x0080
+#define DRM_EXYNOS_PP_PARAM_DST_WIDTH	0x0100
+#define DRM_EXYNOS_PP_PARAM_DST_HEIGHT	0x0200
+#define DRM_EXYNOS_PP_PARAM_ROTATION	0x0400
+
+struct drm_exynos_pp_get {
+	__u32 pp_id;
+	__u32 capabilities;
+
+	__u32 src_format_count;
+	__u32 dst_format_count;
+	__u32 params_count;
+	__u32 reserved1;
+
+	__u64 src_format_type_ptr;
+	__u64 dst_format_type_ptr;
+	__u64 params_ptr;
+	__u64 reserved2;
+};
+
+#define DRM_EXYNOS_PP_CAP_CROP		0x01
+#define DRM_EXYNOS_PP_CAP_ROTATE	0x02
+#define DRM_EXYNOS_PP_CAP_SCALE		0x04
+#define DRM_EXYNOS_PP_CAP_CONVERT	0x08
+
+#define DRM_EXYNOS_PP_FLAG_EVENT	0x01
+#define DRM_EXYNOS_PP_FLAG_TEST_ONLY	0x02
+#define DRM_EXYNOS_PP_FLAG_NONBLOCK	0x04
+
+#define DRM_EXYNOS_PP_FLAGS (DRM_EXYNOS_PP_FLAG_EVENT |\
+		DRM_EXYNOS_PP_FLAG_TEST_ONLY | DRM_EXYNOS_PP_FLAG_NONBLOCK)
+
+struct drm_exynos_pp_commit {
+	__u32 id;
+	__u32 flags;
+	__u32 params_count;
+	__u32 reserved;
+	__u64 param_ids_ptr;
+	__u64 param_values_ptr;
+	__u64 user_data;
+};
+
 #define DRM_EXYNOS_GEM_CREATE		0x00
 #define DRM_EXYNOS_GEM_MAP		0x01
 /* Reserved 0x03 ~ 0x05 for exynos specific gem ioctl */
@@ -317,6 +371,10 @@ struct drm_exynos_ipp_cmd_ctrl {
 #define DRM_EXYNOS_IPP_QUEUE_BUF	0x32
 #define DRM_EXYNOS_IPP_CMD_CTRL	0x33
 
+#define DRM_EXYNOS_PP_GET_RESOURCES	0x40
+#define DRM_EXYNOS_PP_GET		0x41
+#define DRM_EXYNOS_PP_COMMIT		0x42
+
 #define DRM_IOCTL_EXYNOS_GEM_CREATE		DRM_IOWR(DRM_COMMAND_BASE + \
 		DRM_EXYNOS_GEM_CREATE, struct drm_exynos_gem_create)
 #define DRM_IOCTL_EXYNOS_GEM_MAP		DRM_IOWR(DRM_COMMAND_BASE + \
@@ -343,9 +401,17 @@ struct drm_exynos_ipp_cmd_ctrl {
 #define DRM_IOCTL_EXYNOS_IPP_CMD_CTRL		DRM_IOWR(DRM_COMMAND_BASE + \
 		DRM_EXYNOS_IPP_CMD_CTRL, struct drm_exynos_ipp_cmd_ctrl)
 
+#define DRM_IOCTL_EXYNOS_PP_GET_RESOURCES	DRM_IOWR(DRM_COMMAND_BASE + \
+		DRM_EXYNOS_PP_GET_RESOURCES, struct drm_exynos_pp_get_res)
+#define DRM_IOCTL_EXYNOS_PP_GET			DRM_IOWR(DRM_COMMAND_BASE + \
+		DRM_EXYNOS_PP_GET, struct drm_exynos_pp_get)
+#define DRM_IOCTL_EXYNOS_PP_COMMIT		DRM_IOWR(DRM_COMMAND_BASE + \
+		DRM_EXYNOS_PP_COMMIT, struct drm_exynos_pp_commit)
+
 /* EXYNOS specific events */
 #define DRM_EXYNOS_G2D_EVENT		0x80000000
 #define DRM_EXYNOS_IPP_EVENT		0x80000001
+#define DRM_EXYNOS_PP_EVENT		0x80000002
 
 struct drm_exynos_g2d_event {
 	struct drm_event	base;
@@ -366,6 +432,16 @@ struct drm_exynos_ipp_event {
 	__u32			buf_id[EXYNOS_DRM_OPS_MAX];
 };
 
+struct drm_exynos_pp_event {
+	struct drm_event	base;
+	__u64			user_data;
+	__u32			tv_sec;
+	__u32			tv_usec;
+	__u32			pp_id;
+	__u32			sequence;
+	__u64			reserved;
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
1.9.1

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

[RFC v2 2/2] drm/exynos: Convert Exynos Rotator driver to Picture Processor interface

[Marek Szyprowski]

This patch converts Exynos Rotator driver from Exynos IPP API to
Exynos DRM Picture Processor API.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/Kconfig              |   1 -
 drivers/gpu/drm/exynos/exynos_drm_drv.c     |   1 +
 drivers/gpu/drm/exynos/exynos_drm_rotator.c | 493 +++++++---------------------
 drivers/gpu/drm/exynos/exynos_drm_rotator.h |  19 --
 4 files changed, 118 insertions(+), 396 deletions(-)
 delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_rotator.h

diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index 1d185347c64c..84c8cc2aa28d 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -106,7 +106,6 @@ config DRM_EXYNOS_FIMC
 
 config DRM_EXYNOS_ROTATOR
 	bool "Rotator"
-	depends on DRM_EXYNOS_IPP
 	help
 	  Choose this option if you want to use Exynos Rotator for DRM.
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 386a9d325c5a..b3b2d0628842 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -401,6 +401,7 @@ struct exynos_drm_driver_info {
 		DRV_PTR(fimc_driver, CONFIG_DRM_EXYNOS_FIMC),
 	}, {
 		DRV_PTR(rotator_driver, CONFIG_DRM_EXYNOS_ROTATOR),
+		DRM_COMPONENT_DRIVER
 	}, {
 		DRV_PTR(gsc_driver, CONFIG_DRM_EXYNOS_GSC),
 	}, {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
index 79282a820ecc..a23a780bfa26 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/component.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -21,29 +22,18 @@
 #include <drm/drmP.h>
 #include <drm/exynos_drm.h>
 #include "regs-rotator.h"
+#include "exynos_drm_fb.h"
 #include "exynos_drm_drv.h"
-#include "exynos_drm_ipp.h"
+#include "exynos_drm_iommu.h"
+#include "exynos_drm_pp.h"
 
 /*
  * Rotator supports image crop/rotator and input/output DMA operations.
  * input DMA reads image data from the memory.
  * output DMA writes image data to memory.
- *
- * M2M operation : supports crop/scale/rotation/csc so on.
- * Memory ----> Rotator H/W ----> Memory.
- */
-
-/*
- * TODO
- * 1. check suspend/resume api if needed.
- * 2. need to check use case platform_device_id.
- * 3. check src/dst size with, height.
- * 4. need to add supported list in prop_list.
  */
 
 #define get_rot_context(dev)	platform_get_drvdata(to_platform_device(dev))
-#define get_ctx_from_ippdrv(ippdrv)	container_of(ippdrv,\
-					struct rot_context, ippdrv);
 #define rot_read(offset)		readl(rot->regs + (offset))
 #define rot_write(cfg, offset)	writel(cfg, rot->regs + (offset))
 
@@ -88,18 +78,19 @@ struct rot_limit_table {
  * @clock: rotator gate clock.
  * @limit_tbl: limitation of rotator.
  * @irq: irq number.
- * @cur_buf_id: current operation buffer id.
  * @suspended: suspended state.
  */
 struct rot_context {
-	struct exynos_drm_ippdrv	ippdrv;
+	struct exynos_drm_pp pp;
+	struct drm_device *drm_dev;
+	struct device	*dev;
 	struct resource	*regs_res;
 	void __iomem	*regs;
 	struct clk	*clock;
 	struct rot_limit_table	*limit_tbl;
 	int	irq;
-	int	cur_buf_id[EXYNOS_DRM_OPS_MAX];
 	bool	suspended;
+	struct exynos_drm_pp_task	*task;
 };
 
 static void rotator_reg_set_irq(struct rot_context *rot, bool enable)
@@ -138,9 +129,6 @@ static enum rot_irq_status rotator_reg_get_irq_status(struct rot_context *rot)
 static irqreturn_t rotator_irq_handler(int irq, void *arg)
 {
 	struct rot_context *rot = arg;
-	struct exynos_drm_ippdrv *ippdrv = &rot->ippdrv;
-	struct drm_exynos_ipp_cmd_node *c_node = ippdrv->c_node;
-	struct drm_exynos_ipp_event_work *event_work = c_node->event_work;
 	enum rot_irq_status irq_status;
 	u32 val;
 
@@ -152,13 +140,13 @@ static irqreturn_t rotator_irq_handler(int irq, void *arg)
 	val |= ROT_STATUS_IRQ_PENDING((u32)irq_status);
 	rot_write(val, ROT_STATUS);
 
-	if (irq_status == ROT_IRQ_STATUS_COMPLETE) {
-		event_work->ippdrv = ippdrv;
-		event_work->buf_id[EXYNOS_DRM_OPS_DST] =
-			rot->cur_buf_id[EXYNOS_DRM_OPS_DST];
-		queue_work(ippdrv->event_workq, &event_work->work);
-	} else {
-		DRM_ERROR("the SFR is set illegally\n");
+	if (rot->task) {
+		struct exynos_drm_pp_task *task = rot->task;
+
+		rot->task = NULL;
+		pm_runtime_put(rot->dev);
+		exynos_drm_pp_task_done(task,
+			irq_status == ROT_IRQ_STATUS_COMPLETE ? 0 : -EINVAL);
 	}
 
 	return IRQ_HANDLED;
@@ -214,9 +202,6 @@ static int rotator_src_set_fmt(struct device *dev, u32 fmt)
 	case DRM_FORMAT_XRGB8888:
 		val |= ROT_CONTROL_FMT_RGB888;
 		break;
-	default:
-		DRM_ERROR("invalid image format\n");
-		return -EINVAL;
 	}
 
 	rot_write(val, ROT_CONTROL);
@@ -224,33 +209,18 @@ static int rotator_src_set_fmt(struct device *dev, u32 fmt)
 	return 0;
 }
 
-static inline bool rotator_check_reg_fmt(u32 fmt)
-{
-	if ((fmt == ROT_CONTROL_FMT_YCBCR420_2P) ||
-	    (fmt == ROT_CONTROL_FMT_RGB888))
-		return true;
-
-	return false;
-}
-
-static int rotator_src_set_size(struct device *dev, int swap,
-		struct drm_exynos_pos *pos,
-		struct drm_exynos_sz *sz)
+static int rotator_src_set_buf(struct device *dev, struct drm_exynos_pos *pos,
+			       struct drm_framebuffer *fb)
 {
 	struct rot_context *rot = dev_get_drvdata(dev);
 	u32 fmt, hsize, vsize;
 	u32 val;
 
-	/* Get format */
 	fmt = rotator_reg_get_fmt(rot);
-	if (!rotator_check_reg_fmt(fmt)) {
-		DRM_ERROR("invalid format.\n");
-		return -EINVAL;
-	}
 
 	/* Align buffer size */
-	hsize = sz->hsize;
-	vsize = sz->vsize;
+	hsize = fb->width;
+	vsize = fb->height;
 	rotator_align_size(rot, fmt, &hsize, &vsize);
 
 	/* Set buffer size configuration */
@@ -263,131 +233,54 @@ static int rotator_src_set_size(struct device *dev, int swap,
 	val = ROT_SRC_CROP_SIZE_H(pos->h) | ROT_SRC_CROP_SIZE_W(pos->w);
 	rot_write(val, ROT_SRC_CROP_SIZE);
 
-	return 0;
-}
-
-static int rotator_src_set_addr(struct device *dev,
-		struct drm_exynos_ipp_buf_info *buf_info,
-		u32 buf_id, enum drm_exynos_ipp_buf_type buf_type)
-{
-	struct rot_context *rot = dev_get_drvdata(dev);
-	dma_addr_t addr[EXYNOS_DRM_PLANAR_MAX];
-	u32 val, fmt, hsize, vsize;
-	int i;
-
-	/* Set current buf_id */
-	rot->cur_buf_id[EXYNOS_DRM_OPS_SRC] = buf_id;
-
-	switch (buf_type) {
-	case IPP_BUF_ENQUEUE:
-		/* Set address configuration */
-		for_each_ipp_planar(i)
-			addr[i] = buf_info->base[i];
-
-		/* Get format */
-		fmt = rotator_reg_get_fmt(rot);
-		if (!rotator_check_reg_fmt(fmt)) {
-			DRM_ERROR("invalid format.\n");
-			return -EINVAL;
-		}
-
-		/* Re-set cb planar for NV12 format */
-		if ((fmt == ROT_CONTROL_FMT_YCBCR420_2P) &&
-		    !addr[EXYNOS_DRM_PLANAR_CB]) {
-
-			val = rot_read(ROT_SRC_BUF_SIZE);
-			hsize = ROT_GET_BUF_SIZE_W(val);
-			vsize = ROT_GET_BUF_SIZE_H(val);
-
-			/* Set cb planar */
-			addr[EXYNOS_DRM_PLANAR_CB] =
-				addr[EXYNOS_DRM_PLANAR_Y] + hsize * vsize;
-		}
-
-		for_each_ipp_planar(i)
-			rot_write(addr[i], ROT_SRC_BUF_ADDR(i));
-		break;
-	case IPP_BUF_DEQUEUE:
-		for_each_ipp_planar(i)
-			rot_write(0x0, ROT_SRC_BUF_ADDR(i));
-		break;
-	default:
-		/* Nothing to do */
-		break;
-	}
+	/* Set buffer DMA address */
+	rot_write(exynos_drm_fb_dma_addr(fb, 0), ROT_SRC_BUF_ADDR(0));
+	rot_write(exynos_drm_fb_dma_addr(fb, 1), ROT_SRC_BUF_ADDR(1));
 
 	return 0;
 }
 
-static int rotator_dst_set_transf(struct device *dev,
-		enum drm_exynos_degree degree,
-		enum drm_exynos_flip flip, bool *swap)
+static int rotator_dst_set_transf(struct device *dev, unsigned int rotation)
 {
 	struct rot_context *rot = dev_get_drvdata(dev);
 	u32 val;
 
 	/* Set transform configuration */
 	val = rot_read(ROT_CONTROL);
+
 	val &= ~ROT_CONTROL_FLIP_MASK;
 
-	switch (flip) {
-	case EXYNOS_DRM_FLIP_VERTICAL:
+	if (rotation & DRM_REFLECT_Y)
 		val |= ROT_CONTROL_FLIP_VERTICAL;
-		break;
-	case EXYNOS_DRM_FLIP_HORIZONTAL:
+	if (rotation & DRM_REFLECT_X)
 		val |= ROT_CONTROL_FLIP_HORIZONTAL;
-		break;
-	default:
-		/* Flip None */
-		break;
-	}
 
 	val &= ~ROT_CONTROL_ROT_MASK;
 
-	switch (degree) {
-	case EXYNOS_DRM_DEGREE_90:
+	if (rotation & DRM_ROTATE_90)
 		val |= ROT_CONTROL_ROT_90;
-		break;
-	case EXYNOS_DRM_DEGREE_180:
+	else if (rotation & DRM_ROTATE_180)
 		val |= ROT_CONTROL_ROT_180;
-		break;
-	case EXYNOS_DRM_DEGREE_270:
+	else if (rotation & DRM_ROTATE_270)
 		val |= ROT_CONTROL_ROT_270;
-		break;
-	default:
-		/* Rotation 0 Degree */
-		break;
-	}
 
 	rot_write(val, ROT_CONTROL);
 
-	/* Check degree for setting buffer size swap */
-	if ((degree == EXYNOS_DRM_DEGREE_90) ||
-	    (degree == EXYNOS_DRM_DEGREE_270))
-		*swap = true;
-	else
-		*swap = false;
-
 	return 0;
 }
 
-static int rotator_dst_set_size(struct device *dev, int swap,
-		struct drm_exynos_pos *pos,
-		struct drm_exynos_sz *sz)
+static int rotator_dst_set_buf(struct device *dev, struct drm_exynos_pos *pos,
+			       struct drm_framebuffer *fb)
 {
 	struct rot_context *rot = dev_get_drvdata(dev);
-	u32 val, fmt, hsize, vsize;
+	u32 fmt, hsize, vsize;
+	u32 val;
 
-	/* Get format */
 	fmt = rotator_reg_get_fmt(rot);
-	if (!rotator_check_reg_fmt(fmt)) {
-		DRM_ERROR("invalid format.\n");
-		return -EINVAL;
-	}
 
 	/* Align buffer size */
-	hsize = sz->hsize;
-	vsize = sz->vsize;
+	hsize = fb->width;
+	vsize = fb->height;
 	rotator_align_size(rot, fmt, &hsize, &vsize);
 
 	/* Set buffer size configuration */
@@ -398,227 +291,23 @@ static int rotator_dst_set_size(struct device *dev, int swap,
 	val = ROT_CROP_POS_Y(pos->y) | ROT_CROP_POS_X(pos->x);
 	rot_write(val, ROT_DST_CROP_POS);
 
-	return 0;
-}
-
-static int rotator_dst_set_addr(struct device *dev,
-		struct drm_exynos_ipp_buf_info *buf_info,
-		u32 buf_id, enum drm_exynos_ipp_buf_type buf_type)
-{
-	struct rot_context *rot = dev_get_drvdata(dev);
-	dma_addr_t addr[EXYNOS_DRM_PLANAR_MAX];
-	u32 val, fmt, hsize, vsize;
-	int i;
-
-	/* Set current buf_id */
-	rot->cur_buf_id[EXYNOS_DRM_OPS_DST] = buf_id;
-
-	switch (buf_type) {
-	case IPP_BUF_ENQUEUE:
-		/* Set address configuration */
-		for_each_ipp_planar(i)
-			addr[i] = buf_info->base[i];
-
-		/* Get format */
-		fmt = rotator_reg_get_fmt(rot);
-		if (!rotator_check_reg_fmt(fmt)) {
-			DRM_ERROR("invalid format.\n");
-			return -EINVAL;
-		}
-
-		/* Re-set cb planar for NV12 format */
-		if ((fmt == ROT_CONTROL_FMT_YCBCR420_2P) &&
-		    !addr[EXYNOS_DRM_PLANAR_CB]) {
-			/* Get buf size */
-			val = rot_read(ROT_DST_BUF_SIZE);
-
-			hsize = ROT_GET_BUF_SIZE_W(val);
-			vsize = ROT_GET_BUF_SIZE_H(val);
-
-			/* Set cb planar */
-			addr[EXYNOS_DRM_PLANAR_CB] =
-				addr[EXYNOS_DRM_PLANAR_Y] + hsize * vsize;
-		}
-
-		for_each_ipp_planar(i)
-			rot_write(addr[i], ROT_DST_BUF_ADDR(i));
-		break;
-	case IPP_BUF_DEQUEUE:
-		for_each_ipp_planar(i)
-			rot_write(0x0, ROT_DST_BUF_ADDR(i));
-		break;
-	default:
-		/* Nothing to do */
-		break;
-	}
-
-	return 0;
-}
-
-static struct exynos_drm_ipp_ops rot_src_ops = {
-	.set_fmt	=	rotator_src_set_fmt,
-	.set_size	=	rotator_src_set_size,
-	.set_addr	=	rotator_src_set_addr,
-};
-
-static struct exynos_drm_ipp_ops rot_dst_ops = {
-	.set_transf	=	rotator_dst_set_transf,
-	.set_size	=	rotator_dst_set_size,
-	.set_addr	=	rotator_dst_set_addr,
-};
-
-static int rotator_init_prop_list(struct exynos_drm_ippdrv *ippdrv)
-{
-	struct drm_exynos_ipp_prop_list *prop_list = &ippdrv->prop_list;
-
-	prop_list->version = 1;
-	prop_list->flip = (1 << EXYNOS_DRM_FLIP_VERTICAL) |
-				(1 << EXYNOS_DRM_FLIP_HORIZONTAL);
-	prop_list->degree = (1 << EXYNOS_DRM_DEGREE_0) |
-				(1 << EXYNOS_DRM_DEGREE_90) |
-				(1 << EXYNOS_DRM_DEGREE_180) |
-				(1 << EXYNOS_DRM_DEGREE_270);
-	prop_list->csc = 0;
-	prop_list->crop = 0;
-	prop_list->scale = 0;
+	/* Set buffer DMA address */
+	rot_write(exynos_drm_fb_dma_addr(fb, 0), ROT_DST_BUF_ADDR(0));
+	rot_write(exynos_drm_fb_dma_addr(fb, 1), ROT_DST_BUF_ADDR(1));
 
 	return 0;
 }
 
-static inline bool rotator_check_drm_fmt(u32 fmt)
-{
-	switch (fmt) {
-	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_NV12:
-		return true;
-	default:
-		DRM_DEBUG_KMS("not support format\n");
-		return false;
-	}
-}
-
-static inline bool rotator_check_drm_flip(enum drm_exynos_flip flip)
-{
-	switch (flip) {
-	case EXYNOS_DRM_FLIP_NONE:
-	case EXYNOS_DRM_FLIP_VERTICAL:
-	case EXYNOS_DRM_FLIP_HORIZONTAL:
-	case EXYNOS_DRM_FLIP_BOTH:
-		return true;
-	default:
-		DRM_DEBUG_KMS("invalid flip\n");
-		return false;
-	}
-}
-
-static int rotator_ippdrv_check_property(struct device *dev,
-		struct drm_exynos_ipp_property *property)
-{
-	struct drm_exynos_ipp_config *src_config =
-					&property->config[EXYNOS_DRM_OPS_SRC];
-	struct drm_exynos_ipp_config *dst_config =
-					&property->config[EXYNOS_DRM_OPS_DST];
-	struct drm_exynos_pos *src_pos = &src_config->pos;
-	struct drm_exynos_pos *dst_pos = &dst_config->pos;
-	struct drm_exynos_sz *src_sz = &src_config->sz;
-	struct drm_exynos_sz *dst_sz = &dst_config->sz;
-	bool swap = false;
-
-	/* Check format configuration */
-	if (src_config->fmt != dst_config->fmt) {
-		DRM_DEBUG_KMS("not support csc feature\n");
-		return -EINVAL;
-	}
-
-	if (!rotator_check_drm_fmt(dst_config->fmt)) {
-		DRM_DEBUG_KMS("invalid format\n");
-		return -EINVAL;
-	}
-
-	/* Check transform configuration */
-	if (src_config->degree != EXYNOS_DRM_DEGREE_0) {
-		DRM_DEBUG_KMS("not support source-side rotation\n");
-		return -EINVAL;
-	}
-
-	switch (dst_config->degree) {
-	case EXYNOS_DRM_DEGREE_90:
-	case EXYNOS_DRM_DEGREE_270:
-		swap = true;
-	case EXYNOS_DRM_DEGREE_0:
-	case EXYNOS_DRM_DEGREE_180:
-		/* No problem */
-		break;
-	default:
-		DRM_DEBUG_KMS("invalid degree\n");
-		return -EINVAL;
-	}
-
-	if (src_config->flip != EXYNOS_DRM_FLIP_NONE) {
-		DRM_DEBUG_KMS("not support source-side flip\n");
-		return -EINVAL;
-	}
-
-	if (!rotator_check_drm_flip(dst_config->flip)) {
-		DRM_DEBUG_KMS("invalid flip\n");
-		return -EINVAL;
-	}
-
-	/* Check size configuration */
-	if ((src_pos->x + src_pos->w > src_sz->hsize) ||
-		(src_pos->y + src_pos->h > src_sz->vsize)) {
-		DRM_DEBUG_KMS("out of source buffer bound\n");
-		return -EINVAL;
-	}
-
-	if (swap) {
-		if ((dst_pos->x + dst_pos->h > dst_sz->vsize) ||
-			(dst_pos->y + dst_pos->w > dst_sz->hsize)) {
-			DRM_DEBUG_KMS("out of destination buffer bound\n");
-			return -EINVAL;
-		}
-
-		if ((src_pos->w != dst_pos->h) || (src_pos->h != dst_pos->w)) {
-			DRM_DEBUG_KMS("not support scale feature\n");
-			return -EINVAL;
-		}
-	} else {
-		if ((dst_pos->x + dst_pos->w > dst_sz->hsize) ||
-			(dst_pos->y + dst_pos->h > dst_sz->vsize)) {
-			DRM_DEBUG_KMS("out of destination buffer bound\n");
-			return -EINVAL;
-		}
-
-		if ((src_pos->w != dst_pos->w) || (src_pos->h != dst_pos->h)) {
-			DRM_DEBUG_KMS("not support scale feature\n");
-			return -EINVAL;
-		}
-	}
-
-	return 0;
-}
-
-static int rotator_ippdrv_start(struct device *dev, enum drm_exynos_ipp_cmd cmd)
+static int rotator_start(struct device *dev)
 {
 	struct rot_context *rot = dev_get_drvdata(dev);
 	u32 val;
 
-	if (rot->suspended) {
-		DRM_ERROR("suspended state\n");
-		return -EPERM;
-	}
-
-	if (cmd != IPP_CMD_M2M) {
-		DRM_ERROR("not support cmd: %d\n", cmd);
-		return -EINVAL;
-	}
-
 	/* Set interrupt enable */
 	rotator_reg_set_irq(rot, true);
 
 	val = rot_read(ROT_CONTROL);
 	val |= ROT_CONTROL_START;
-
 	rot_write(val, ROT_CONTROL);
 
 	return 0;
@@ -692,11 +381,84 @@ static int rotator_ippdrv_start(struct device *dev, enum drm_exynos_ipp_cmd cmd)
 };
 MODULE_DEVICE_TABLE(of, exynos_rotator_match);
 
+static int rotator_commit(struct exynos_drm_pp *pp,
+			  struct exynos_drm_pp_task *task)
+{
+	struct rot_context *rot =
+			container_of(pp, struct rot_context, pp);
+	struct device *dev = rot->dev;
+	struct drm_exynos_pos src_pos = {
+		task->src_x >> 16, task->src_y >> 16,
+		task->src_w >> 16, task->src_h >> 16,
+	};
+	struct drm_exynos_pos dst_pos = {
+		task->dst_x >> 16, task->dst_y >> 16,
+		task->dst_w >> 16, task->dst_h >> 16,
+	};
+
+	pm_runtime_get_sync(dev);
+	rot->task = task;
+
+	rotator_src_set_fmt(dev, task->src_fb->format->format);
+	rotator_src_set_buf(dev, &src_pos, task->src_fb);
+	rotator_dst_set_transf(dev, task->rotation);
+	rotator_dst_set_buf(dev, &dst_pos, task->dst_fb);
+	rotator_start(dev);
+
+	return 0;
+}
+
+struct exynos_drm_pp_funcs pp_funcs = {
+	.commit = rotator_commit,
+};
+
+static const uint32_t rotator_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_NV12,
+};
+
+static int rotator_bind(struct device *dev, struct device *master, void *data)
+{
+	struct rot_context *rot = dev_get_drvdata(dev);
+	struct drm_device *drm_dev = data;
+	struct exynos_drm_pp *pp = &rot->pp;
+
+	rot->drm_dev = drm_dev;
+	drm_iommu_attach_device(drm_dev, dev);
+
+	exynos_drm_pp_register(drm_dev, pp, &pp_funcs,
+			   DRM_EXYNOS_PP_CAP_CROP | DRM_EXYNOS_PP_CAP_ROTATE,
+			   rotator_formats, ARRAY_SIZE(rotator_formats),
+			   rotator_formats, ARRAY_SIZE(rotator_formats),
+			   DRM_ROTATE_0 | DRM_ROTATE_90 | DRM_ROTATE_180 |
+			   DRM_ROTATE_270 | DRM_REFLECT_X | DRM_REFLECT_Y,
+			   "rotator");
+
+	dev_info(dev, "The exynos rotator has been probed successfully\n");
+
+	return 0;
+}
+
+static void rotator_unbind(struct device *dev, struct device *master,
+			void *data)
+{
+	struct rot_context *rot = dev_get_drvdata(dev);
+	struct drm_device *drm_dev = data;
+	struct exynos_drm_pp *pp = &rot->pp;
+
+	exynos_drm_pp_unregister(drm_dev, pp);
+	drm_iommu_detach_device(rot->drm_dev, rot->dev);
+}
+
+static const struct component_ops rotator_component_ops = {
+	.bind	= rotator_bind,
+	.unbind = rotator_unbind,
+};
+
 static int rotator_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct rot_context *rot;
-	struct exynos_drm_ippdrv *ippdrv;
 	int ret;
 
 	if (!dev->of_node) {
@@ -710,6 +472,7 @@ static int rotator_probe(struct platform_device *pdev)
 
 	rot->limit_tbl = (struct rot_limit_table *)
 				of_device_get_match_data(dev);
+	rot->dev = dev;
 	rot->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	rot->regs = devm_ioremap_resource(dev, rot->regs_res);
 	if (IS_ERR(rot->regs))
@@ -735,30 +498,11 @@ static int rotator_probe(struct platform_device *pdev)
 	}
 
 	pm_runtime_enable(dev);
-
-	ippdrv = &rot->ippdrv;
-	ippdrv->dev = dev;
-	ippdrv->ops[EXYNOS_DRM_OPS_SRC] = &rot_src_ops;
-	ippdrv->ops[EXYNOS_DRM_OPS_DST] = &rot_dst_ops;
-	ippdrv->check_property = rotator_ippdrv_check_property;
-	ippdrv->start = rotator_ippdrv_start;
-	ret = rotator_init_prop_list(ippdrv);
-	if (ret < 0) {
-		dev_err(dev, "failed to init property list.\n");
-		goto err_ippdrv_register;
-	}
-
-	DRM_DEBUG_KMS("ippdrv[%pK]\n", ippdrv);
-
 	platform_set_drvdata(pdev, rot);
 
-	ret = exynos_drm_ippdrv_register(ippdrv);
-	if (ret < 0) {
-		dev_err(dev, "failed to register drm rotator device\n");
+	ret = component_add(dev, &rotator_component_ops);
+	if (ret)
 		goto err_ippdrv_register;
-	}
-
-	dev_info(dev, "The exynos rotator is probed successfully\n");
 
 	return 0;
 
@@ -770,11 +514,8 @@ static int rotator_probe(struct platform_device *pdev)
 static int rotator_remove(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct rot_context *rot = dev_get_drvdata(dev);
-	struct exynos_drm_ippdrv *ippdrv = &rot->ippdrv;
-
-	exynos_drm_ippdrv_unregister(ippdrv);
 
+	component_del(dev, &rotator_component_ops);
 	pm_runtime_disable(dev);
 
 	return 0;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.h b/drivers/gpu/drm/exynos/exynos_drm_rotator.h
deleted file mode 100644
index 71a0b4c0c1e8..000000000000
--- a/drivers/gpu/drm/exynos/exynos_drm_rotator.h
+++ /dev/null
@@ -1,19 +0,0 @@
-/*
- * Copyright (c) 2012 Samsung Electronics Co., Ltd.
- *
- * Authors:
- *	YoungJun Cho <yj44.cho@samsung.com>
- *	Eunchul Kim <chulspro.kim@samsung.com>
- *
- * 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_ROTATOR_H_
-#define	_EXYNOS_DRM_ROTATOR_H_
-
-/* TODO */
-
-#endif
-- 
1.9.1

Re: [RFC v2 1/2] drm/exynos: Add Picture Processor framework

[Emil Velikov]

Hi Marek,

A couple of small nitpicks from UAPI POV.

On 8 May 2017 at 10:11, Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> --- a/include/uapi/drm/exynos_drm.h
> +++ b/include/uapi/drm/exynos_drm.h

> +struct drm_exynos_pp_get_res {
> +       __u64 pp_id_ptr;
> +       __u32 count_pps;
Add __u32 pad - sizeof(struct ...) should be multiple of sizeof(__u64).


> +struct drm_exynos_pp_get {
> +       __u32 pp_id;
> +       __u32 capabilities;
> +
> +       __u32 src_format_count;
> +       __u32 dst_format_count;
> +       __u32 params_count;
> +       __u32 reserved1;
> +
Replace with __u32 flags; so that you can extend the struct as applicable.

> +       __u64 src_format_type_ptr;
> +       __u64 dst_format_type_ptr;
> +       __u64 params_ptr;
> +       __u64 reserved2;
And now you can drop this piece.


> +struct drm_exynos_pp_commit {
> +       __u32 id;
> +       __u32 flags;
> +       __u32 params_count;
> +       __u32 reserved;
Why the reserved here - flags should help you extend as needed.

> +       __u64 param_ids_ptr;
> +       __u64 param_values_ptr;
> +       __u64 user_data;
Unused user_data?


> +struct drm_exynos_pp_event {
> +       struct drm_event        base;
> +       __u64                   user_data;
Unused user_data?

> +       __u32                   tv_sec;
> +       __u32                   tv_usec;
> +       __u32                   pp_id;
> +       __u32                   sequence;
> +       __u64                   reserved;
Drop in favour of flags?

Regards,
Emil

Re: [RFC v2 1/2] drm/exynos: Add Picture Processor framework

[Marek Szyprowski]

Hi Emil,

On 2017-05-08 15:43, Emil Velikov wrote:
> Hi Marek,
>
> A couple of small nitpicks from UAPI POV.

Thanks for your comments!

> On 8 May 2017 at 10:11, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
>> --- a/include/uapi/drm/exynos_drm.h
>> +++ b/include/uapi/drm/exynos_drm.h
>> +struct drm_exynos_pp_get_res {
>> +       __u64 pp_id_ptr;
>> +       __u32 count_pps;
> Add __u32 pad - sizeof(struct ...) should be multiple of sizeof(__u64).

ok

>> +struct drm_exynos_pp_get {
>> +       __u32 pp_id;
>> +       __u32 capabilities;
>> +
>> +       __u32 src_format_count;
>> +       __u32 dst_format_count;
>> +       __u32 params_count;
>> +       __u32 reserved1;
>> +
> Replace with __u32 flags; so that you can extend the struct as applicable.

ok

>> +       __u64 src_format_type_ptr;
>> +       __u64 dst_format_type_ptr;
>> +       __u64 params_ptr;
>> +       __u64 reserved2;
> And now you can drop this piece.
>
>
>> +struct drm_exynos_pp_commit {
>> +       __u32 id;
>> +       __u32 flags;
>> +       __u32 params_count;
>> +       __u32 reserved;
> Why the reserved here - flags should help you extend as needed.

To ensure that the structure will be a multiple of sizeof(u64).

>> +       __u64 param_ids_ptr;
>> +       __u64 param_values_ptr;
>> +       __u64 user_data;
> Unused user_data?

Nope. User_data provided here will be reported back to userspace in the 
struct
drm_exynos_pp_event, so we cannot drop it.

>> +struct drm_exynos_pp_event {
>> +       struct drm_event        base;
>> +       __u64                   user_data;
> Unused user_data?

See comment above.

>> +       __u32                   tv_sec;
>> +       __u32                   tv_usec;
>> +       __u32                   pp_id;
>> +       __u32                   sequence;
>> +       __u64                   reserved;
> Drop in favour of flags?

u64 flags? Probably u32 flags + u32 reserved would make a bit more sense to
keep structure a multiple of sizeof(u64).

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland