Re: [PATCH 1/4] drm/exynos: add ipp subsystem

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

 



Thank's quick review

I have some question about your some comment's
please check my comment and send one more time.

On 10/27/2012 12:47 AM, Inki Dae wrote:
below is quick review.

2012/10/18 Eunchul Kim <chulspro.kim@xxxxxxxxxxx>:
IPP stand for Image Post Processing and supports image scaler/rotator
/crop/flip/csc(color space conversion) and input/output DMA operations using ipp drivers.
also supports writeback and display output operations.
ipp driver include FIMC, Rotator, GSC, SC, so on.
and ipp is integration device driver for each hardware.

Signed-off-by: Eunchul Kim <chulspro.kim@xxxxxxxxxxx>
Signed-off-by: Jinyoung Jeon <jy0.jeon@xxxxxxxxxxx>
---
  drivers/gpu/drm/exynos/Kconfig          |    6 +
  drivers/gpu/drm/exynos/Makefile         |    1 +
  drivers/gpu/drm/exynos/exynos_drm_drv.c |   24 +
  drivers/gpu/drm/exynos/exynos_drm_drv.h |    7 +
  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 1937 +++++++++++++++++++++++++++++++
  drivers/gpu/drm/exynos/exynos_drm_ipp.h |  268 +++++
  include/uapi/drm/exynos_drm.h           |  189 +++
  7 files changed, 2432 insertions(+), 0 deletions(-)
  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 59a26e5..4c83d0b 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -39,3 +39,9 @@ config DRM_EXYNOS_G2D
         depends on DRM_EXYNOS && !VIDEO_SAMSUNG_S5P_G2D
         help
           Choose this option if you want to use Exynos G2D for DRM.
+
+config DRM_EXYNOS_IPP
+       bool "Exynos DRM IPP"
+       depends on DRM_EXYNOS
+       help
+         Choose this option if you want to use IPP feature for DRM.
diff --git a/drivers/gpu/drm/exynos/Makefile b/drivers/gpu/drm/exynos/Makefile
index eb651ca..e748cc7 100644
--- a/drivers/gpu/drm/exynos/Makefile
+++ b/drivers/gpu/drm/exynos/Makefile
@@ -15,5 +15,6 @@ exynosdrm-$(CONFIG_DRM_EXYNOS_HDMI)   += exynos_hdmi.o exynos_mixer.o \
                                            exynos_drm_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

  obj-$(CONFIG_DRM_EXYNOS)               += exynosdrm.o
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index c4c719b..9e14d61 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -40,6 +40,7 @@
  #include "exynos_drm_vidi.h"
  #include "exynos_drm_dmabuf.h"
  #include "exynos_drm_g2d.h"
+#include "exynos_drm_ipp.h"

  #define DRIVER_NAME    "exynos"
  #define DRIVER_DESC    "Samsung SoC DRM"
@@ -232,6 +233,14 @@ static struct drm_ioctl_desc exynos_ioctls[] = {
                         exynos_g2d_set_cmdlist_ioctl, DRM_UNLOCKED | DRM_AUTH),
         DRM_IOCTL_DEF_DRV(EXYNOS_G2D_EXEC,
                         exynos_g2d_exec_ioctl, DRM_UNLOCKED | DRM_AUTH),
+       DRM_IOCTL_DEF_DRV(EXYNOS_IPP_GET_PROPERTY,
+                       exynos_drm_ipp_get_property, DRM_UNLOCKED | DRM_AUTH),
+       DRM_IOCTL_DEF_DRV(EXYNOS_IPP_SET_PROPERTY,
+                       exynos_drm_ipp_set_property, DRM_UNLOCKED | DRM_AUTH),
+       DRM_IOCTL_DEF_DRV(EXYNOS_IPP_QUEUE_BUF,
+                       exynos_drm_ipp_queue_buf, DRM_UNLOCKED | DRM_AUTH),
+       DRM_IOCTL_DEF_DRV(EXYNOS_IPP_CMD_CTRL,
+                       exynos_drm_ipp_cmd_ctrl, DRM_UNLOCKED | DRM_AUTH),
  };

  static const struct file_operations exynos_drm_driver_fops = {
@@ -346,6 +355,12 @@ static int __init exynos_drm_init(void)
                 goto out_g2d;
  #endif

+#ifdef CONFIG_DRM_EXYNOS_IPP
+       ret = platform_driver_register(&ipp_driver);
+       if (ret < 0)
+               goto out_ipp;
+#endif
+
         ret = platform_driver_register(&exynos_drm_platform_driver);
         if (ret < 0)
                 goto out_drm;
@@ -363,6 +378,11 @@ out:
         platform_driver_unregister(&exynos_drm_platform_driver);

  out_drm:
+#ifdef CONFIG_DRM_EXYNOS_IPP
+       platform_driver_unregister(&ipp_driver);
+out_ipp:
+#endif
+
  #ifdef CONFIG_DRM_EXYNOS_G2D
         platform_driver_unregister(&g2d_driver);
  out_g2d:
@@ -399,6 +419,10 @@ static void __exit exynos_drm_exit(void)

         platform_driver_unregister(&exynos_drm_platform_driver);

+#ifdef CONFIG_DRM_EXYNOS_IPP
+       platform_driver_unregister(&ipp_driver);
+#endif
+
  #ifdef CONFIG_DRM_EXYNOS_G2D
         platform_driver_unregister(&g2d_driver);
  #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 531c991..4033d82 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -235,8 +235,14 @@ struct exynos_drm_g2d_private {
         unsigned int            gem_nr;
  };

+struct exynos_drm_ipp_private {
+       struct device   *dev;
+       struct list_head        event_list;
+};
+
  struct drm_exynos_file_private {
         struct exynos_drm_g2d_private   *g2d_priv;
+       struct exynos_drm_ipp_private   *ipp_priv;
  };

  /*
@@ -335,4 +341,5 @@ extern struct platform_driver mixer_driver;
  extern struct platform_driver exynos_drm_common_hdmi_driver;
  extern struct platform_driver vidi_driver;
  extern struct platform_driver g2d_driver;
+extern struct platform_driver ipp_driver;
  #endif
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 0000000..cee18e5
--- /dev/null
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -0,0 +1,1937 @@
+/*
+ * Copyright (C) 2012 Samsung Electronics Co.Ltd
+ * Authors:
+ *     Eunchul Kim <chulspro.kim@xxxxxxxxxxx>
+ *     Jinyoung Jeon <jy0.jeon@xxxxxxxxxxx>
+ *     Sangmin Lee <lsmin.lee@xxxxxxxxxxx>
+ *
+ * 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.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/pm_runtime.h>
+#include <plat/map-base.h>
+
+#include <drm/drmP.h>
+#include <drm/exynos_drm.h>
+#include "exynos_drm_drv.h"
+#include "exynos_drm_gem.h"
+#include "exynos_drm_ipp.h"
+
+/*
+ * IPP is stand for Image Post Processing and
+ * supports image scaler/rotator and input/output DMA operations.
+ * using FIMC, GSC, Rotator, so on.
+ * IPP is integration device driver of same attribute h/w
+ */
+
+#define get_ipp_context(dev)   platform_get_drvdata(to_platform_device(dev))
+
+/*
+ * A structure of event.
+ *
+ * @base: base of event.
+ * @event: ipp event.
+ */
+struct drm_exynos_ipp_send_event {
+       struct drm_pending_event        base;
+       struct drm_exynos_ipp_event     event;
+};
+
+/*
+ * A structure of memory node.
+ *
+ * @list: list head to memory queue information.
+ * @ops_id: id of operations.
+ * @prop_id: id of property.
+ * @buf_id: id of buffer.
+ * @buf_info: gem objects and dma address, size.
+ */
+struct drm_exynos_ipp_mem_node {
+       struct list_head        list;
+       enum drm_exynos_ops_id  ops_id;
+       u32     prop_id;
+       u32     buf_id;
+       struct drm_exynos_ipp_buf_info  buf_info;
+};
+
+/*
+ * A structure of ipp context.
+ *
+ * @subdrv: prepare initialization using subdrv.
+ * @ipp_lock: lock for synchronization of access to ipp_idr.
+ * @prop_lock: lock for synchronization of access to prop_idr.
+ * @ipp_idr: ipp driver idr.
+ * @prop_idr: property idr.
+ * @event_workq: event work queue.
+ * @cmd_workq: command work queue.
+ */
+struct ipp_context {
+       struct exynos_drm_subdrv        subdrv;
+       spinlock_t      ipp_lock;
+       spinlock_t      prop_lock;
+       struct idr      ipp_idr;
+       struct idr      prop_idr;
+       struct workqueue_struct *event_workq;
+       struct workqueue_struct *cmd_workq;
+};
+
+static LIST_HEAD(exynos_drm_ippdrv_list);
+static BLOCKING_NOTIFIER_HEAD(exynos_drm_ippnb_list);
+
+int exynos_drm_ippdrv_register(struct exynos_drm_ippdrv *ippdrv)
+{
+       DRM_DEBUG_KMS("%s\n", __func__);
+
+       if (!ippdrv)
+               return -EINVAL;

ippdrv can't be null.

just error handling of ipp driver.
I will remove that.


+
+       list_add_tail(&ippdrv->drv_list, &exynos_drm_ippdrv_list);

Isn't exynos_drm_ippdrv_list global? add mutex_lock/unlock.

It's global. but this api called by ipp driver. so, this platform driver already serialized in exynos_drm_init(). please one more comment's


+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(exynos_drm_ippdrv_register);
+
+int exynos_drm_ippdrv_unregister(struct exynos_drm_ippdrv *ippdrv)
+{
+       DRM_DEBUG_KMS("%s\n", __func__);
+
+       if (!ippdrv)
+               return -EINVAL;
+

ditto.

remove it.


+       list_del(&ippdrv->drv_list);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(exynos_drm_ippdrv_unregister);
+
+static int ipp_create_id(struct idr *id_idr, spinlock_t *lock, void *obj,
+       u32 *idp)
+{
+       int ret = -EINVAL;
+
+       DRM_DEBUG_KMS("%s\n", __func__);
+
+again:
+       /* ensure there is space available to allocate a handle */
+       if (idr_pre_get(id_idr, GFP_KERNEL) == 0)
+               return -ENOMEM;
+
+       /* do the allocation under our spinlock */
+       spin_lock(lock);
+       ret = idr_get_new_above(id_idr, obj, 1, (int *)idp);
+       spin_unlock(lock);

Is there any reason to use spin_lock? Please, refrain from using spin lock.

I will change from spin lock to mutex lock. mutex lock was enough


+       if (ret == -EAGAIN)
+               goto again;
+
+       return ret;
+}
+
+static void *ipp_find_id(struct idr *id_idr, spinlock_t *lock, u32 id)
+{
+       void *obj;
+
+       DRM_DEBUG_KMS("%s:id[%d]\n", __func__, id);
+
+       spin_lock(lock);
+
+       /* find object using handle */
+       obj = idr_find(id_idr, id);
+       if (obj == NULL) {
+               spin_unlock(lock);
+               return NULL;
+       }
+
+       spin_unlock(lock);

ditto.

change to mutex


+
+       return obj;
+}
+
+static struct exynos_drm_ippdrv *ipp_find_driver(struct ipp_context *ctx,
+       struct drm_exynos_ipp_property *property)
+{
+       struct exynos_drm_ippdrv *ippdrv;
+       u32 ipp_id = property->ipp_id;
+
+       DRM_DEBUG_KMS("%s:ipp_id[%d]\n", __func__, ipp_id);
+
+       if (ipp_id) {
+               /* find ipp driver */
+               ippdrv = ipp_find_id(&ctx->ipp_idr, &ctx->ipp_lock,
+                       ipp_id);
+               if (!ippdrv) {
+                       DRM_ERROR("not found ipp%d driver.\n", ipp_id);
+                       goto err_null;
+               }
+
+               /* check dedicated state */
+               if (ippdrv->dedicated) {
+                       DRM_ERROR("used choose device.\n");
+                       goto err_null;
+               }
+
+               if (property->cmd != IPP_CMD_M2M
+                       && !pm_runtime_suspended(ippdrv->dev)) {
+                       DRM_ERROR("can't run dedicatedly.\n");
+                       goto err_null;
+               }
+
+               /* check property */
+               if (ippdrv->check_property &&
+                   ippdrv->check_property(ippdrv->dev, property)) {
+                       DRM_ERROR("not support property.\n");
+                       goto err_null;
+               }
+
+               return ippdrv;
+       } else {
+               /* get ipp driver entry */
+               list_for_each_entry(ippdrv, &exynos_drm_ippdrv_list, drv_list) {
+                       /* check dedicated state */
+                       if (ippdrv->dedicated)
+                               continue;
+
+                       if (property->cmd != IPP_CMD_M2M
+                               && !pm_runtime_suspended(ippdrv->dev)) {
+                               DRM_DEBUG_KMS("%s:can't run dedicatedly.\n",
+                                       __func__);
+                               continue;
+                       }
+
+                       /* check property */
+                       if (ippdrv->check_property &&
+                           ippdrv->check_property(ippdrv->dev, property)) {
+                               DRM_DEBUG_KMS("%s:not support property.\n",
+                                       __func__);
+                               continue;
+                       }
+
+                       return ippdrv;
+               }
+
+               DRM_ERROR("not support ipp driver operations.\n");
+       }
+

return ERR_PTR(proper error type) and share duplicated codes.

we support two kind of method in ipp_find_driver.
first, If user library set ipp_id. we find this one.
second, If user library not set ipp_id(default), we search idle state driver.
so, I think we can't share this code.

and I will add return ERR_PTR(error type)


+err_null:
+       return NULL;
+}
+
+static struct exynos_drm_ippdrv *ipp_find_drv_node(u32 prop_id)
+{
+       struct exynos_drm_ippdrv *ippdrv;
+       struct drm_exynos_ipp_cmd_node *c_node;
+       int count = 0;
+
+       DRM_DEBUG_KMS("%s:prop_id[%d]\n", __func__, prop_id);
+

add mutex lock.

OK, I will add lock.


+       if (list_empty(&exynos_drm_ippdrv_list)) {
+               DRM_DEBUG_KMS("%s:ippdrv_list is empty.\n",
+                       __func__);
+               return NULL;
+       }
+
+       list_for_each_entry(ippdrv, &exynos_drm_ippdrv_list, drv_list) {
+               DRM_DEBUG_KMS("%s:count[%d]ippdrv[0x%x]\n",
+                       __func__, count++, (int)ippdrv);
+
+               if (!list_empty(&ippdrv->cmd_list)) {
+                       list_for_each_entry(c_node, &ippdrv->cmd_list, list) {
+                               if (c_node->property.prop_id == prop_id)
+                                       return ippdrv;
+                       }
+               }
+       }
+
+       return NULL;
+}
+
+int exynos_drm_ipp_get_property(struct drm_device *drm_dev, void *data,
+       struct drm_file *file)
+{
+       struct drm_exynos_file_private *file_priv = file->driver_priv;
+       struct exynos_drm_ipp_private *priv = file_priv->ipp_priv;
+       struct device *dev = priv->dev;
+       struct ipp_context *ctx = get_ipp_context(dev);
+       struct drm_exynos_ipp_prop_list *prop_list = data;
+       struct exynos_drm_ippdrv *ippdrv;
+       int count = 0;
+
+       DRM_DEBUG_KMS("%s\n", __func__);
+
+       if (!ctx) {
+               DRM_ERROR("invalid context.\n");
+               return -EINVAL;
+       }
+
+       if (!prop_list) {
+               DRM_ERROR("invalid property parameter.\n");
+               return -EINVAL;
+       }
+
+       DRM_DEBUG_KMS("%s:ipp_id[%d]\n", __func__, prop_list->ipp_id);
+
+       if (prop_list->ipp_id == 0) {
+               list_for_each_entry(ippdrv, &exynos_drm_ippdrv_list, drv_list)
+                       count++;
+               prop_list->count = count;
+       } else {
+               ippdrv = ipp_find_id(&ctx->ipp_idr, &ctx->ipp_lock,
+                                                  prop_list->ipp_id);
+
+               if (!ippdrv) {
+                       DRM_ERROR("not found ipp%d driver.\n",
+                                       prop_list->ipp_id);
+                       return -EINVAL;
+               }
+
+               prop_list = ippdrv->prop_list;
+       }
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(exynos_drm_ipp_get_property);
+

no need symbol export.

our all exynos specific ioctl is EXPORT symbol.
so, I added EXPORT symbol. is it some probem ?


+int exynos_drm_ipp_set_property(struct drm_device *drm_dev, void *data,
+       struct drm_file *file)
+{
+       struct drm_exynos_file_private *file_priv = file->driver_priv;
+       struct exynos_drm_ipp_private *priv = file_priv->ipp_priv;
+       struct device *dev = priv->dev;
+       struct ipp_context *ctx = get_ipp_context(dev);
+       struct drm_exynos_ipp_property *property = data;
+       struct exynos_drm_ippdrv *ippdrv;
+       struct drm_exynos_ipp_cmd_node *c_node;
+       struct drm_exynos_ipp_config *config;
+       struct drm_exynos_pos *pos;
+       struct drm_exynos_sz *sz;
+       int ret, i;
+
+       DRM_DEBUG_KMS("%s\n", __func__);
+
+       if (!ctx) {
+               DRM_ERROR("invalid context.\n");
+               return -EINVAL;
+       }
+
+       if (!property) {
+               DRM_ERROR("invalid property parameter.\n");
+               return -EINVAL;
+       }
+
+       for_each_ipp_ops(i) {
+               config = &property->config[i];
+               pos = &config->pos;
+               sz = &config->sz;
+
+               DRM_DEBUG_KMS("%s:prop_id[%d]ops[%s]fmt[0x%x]\n",
+                       __func__, property->prop_id,
+                       i ? "dst" : "src", config->fmt);
+
+               DRM_DEBUG_KMS("%s:pos[%d %d %d %d]sz[%d %d]f[%d]r[%d]\n",
+                       __func__, pos->x, pos->y, pos->w, pos->h,
+                       sz->hsize, sz->vsize, config->flip, config->degree);
+       }
+
+       if (property->prop_id) {
+               ippdrv = ipp_find_drv_node(property->prop_id);
+               if (!ippdrv) {
+                       DRM_ERROR("failed to get ipp driver.\n");
+                       return -EINVAL;
+               }
+
+               list_for_each_entry(c_node, &ippdrv->cmd_list, list) {
+                       if ((c_node->property.prop_id ==
+                               property->prop_id) &&
+                               (c_node->state == IPP_STATE_STOP)) {
+                               DRM_DEBUG_KMS("%s:found cmd[%d]ippdrv[0x%x]\n",
+                                       __func__, property->cmd, (int)ippdrv);
+
+                               c_node->property = *property;
+                               return 0;
+                       }
+               }
+
+               DRM_ERROR("failed to search property.\n");
+               return -EINVAL;
+       }
+
+       /* find ipp driver using ipp id */
+       ippdrv = ipp_find_driver(ctx, property);
+       if (!ippdrv) {
+               DRM_ERROR("failed to get ipp driver.\n");
+               return -EINVAL;
+       }
+
+       /* allocate command node */
+       c_node = kzalloc(sizeof(*c_node), GFP_KERNEL);
+       if (!c_node) {
+               DRM_ERROR("failed to allocate map node.\n");
+               return -ENOMEM;
+       }
+
+       /* create property id */
+       ret = ipp_create_id(&ctx->prop_idr, &ctx->prop_lock, c_node,
+               &property->prop_id);
+       if (ret) {
+               DRM_ERROR("failed to create id.\n");
+               goto err_clear;
+       }
+
+       DRM_DEBUG_KMS("%s:created prop_id[%d]cmd[%d]ippdrv[0x%x]\n",
+               __func__, property->prop_id, property->cmd, (int)ippdrv);
+
+       /* stored property information and ippdrv in private data */
+       c_node->priv = priv;
+       c_node->property = *property;
+       c_node->state = IPP_STATE_IDLE;
+
+       c_node->start_work = kzalloc(sizeof(*c_node->start_work),
+               GFP_KERNEL);
+       if (!c_node->start_work) {
+               DRM_ERROR("failed to alloc start_work.\n");
+               ret = -ENOMEM;
+               goto err_clear;
+       }
+
+       INIT_WORK((struct work_struct *)c_node->start_work,
+               ipp_sched_cmd);
+
+       c_node->stop_work = kzalloc(sizeof(*c_node->stop_work),
+               GFP_KERNEL);
+       if (!c_node->stop_work) {
+               DRM_ERROR("failed to alloc stop_work.\n");
+               ret = -ENOMEM;
+               goto err_free_start;
+       }
+
+       INIT_WORK((struct work_struct *)c_node->stop_work,
+               ipp_sched_cmd);
+
+       c_node->event_work = kzalloc(sizeof(*c_node->event_work),
+               GFP_KERNEL);
+       if (!c_node->event_work) {
+               DRM_ERROR("failed to alloc event_work.\n");
+               ret = -ENOMEM;
+               goto err_free_stop;
+       }
+
+       INIT_WORK((struct work_struct *)c_node->event_work,
+               ipp_sched_event);
+
+       /* init ioctl lock */
+       mutex_init(&c_node->cmd_lock);
+       mutex_init(&c_node->mem_lock);
+       mutex_init(&c_node->event_lock);
+       init_completion(&c_node->start_complete);
+       init_completion(&c_node->stop_complete);
+
+       for_each_ipp_ops(i)
+               INIT_LIST_HEAD(&c_node->mem_list[i]);
+
+       INIT_LIST_HEAD(&c_node->event_list);
+       list_splice_init(&priv->event_list, &c_node->event_list);
+       list_add_tail(&c_node->list, &ippdrv->cmd_list);
+
+       /* make dedicated state without m2m */
+       if (property->cmd != IPP_CMD_M2M)
+               ippdrv->dedicated = true;
+
+       return 0;
+
+err_free_stop:
+       kfree(c_node->stop_work);
+err_free_start:
+       kfree(c_node->start_work);
+err_clear:
+       kfree(c_node);
+       return ret;
+}
+EXPORT_SYMBOL_GPL(exynos_drm_ipp_set_property);
+

no need symbol export.

ditto.


+static struct drm_exynos_ipp_mem_node
+       *ipp_find_mem_node(struct drm_exynos_ipp_cmd_node *c_node,
+       struct drm_exynos_ipp_queue_buf *qbuf)
+{
+       struct drm_exynos_ipp_mem_node *m_node;
+       struct list_head *head;
+       int count = 0;
+
+       DRM_DEBUG_KMS("%s:buf_id[%d]\n", __func__, qbuf->buf_id);
+

no need mutex lock? I think c_node is a member of ctx->ippdrv and ctx
is unique to driver.

you are right. but this api just find memory node. no delete and add.
we aleady have locking in get/put.
so, I think we don't need locking in this case.
please one more comment.


+       /* source/destination memory list */
+       head = &c_node->mem_list[qbuf->ops_id];
+
+       /* find memory node entry */
+       list_for_each_entry(m_node, head, list) {
+               DRM_DEBUG_KMS("%s:count[%d]m_node[0x%x]\n",
+                       __func__, count++, (int)m_node);
+
+               /* compare buffer id */
+               if (m_node->buf_id == qbuf->buf_id)
+                       return m_node;
+       }
+
+       return NULL;
+}
+
+static int ipp_check_mem_list(struct drm_exynos_ipp_cmd_node *c_node)
+{
+       struct drm_exynos_ipp_property *property = &c_node->property;
+       struct drm_exynos_ipp_mem_node *m_node;
+       struct list_head *head;
+       int ret, i, count[EXYNOS_DRM_OPS_MAX] = { 0, };
+
+       DRM_DEBUG_KMS("%s\n", __func__);
+
+       mutex_lock(&c_node->mem_lock);
+
+       for_each_ipp_ops(i) {
+               /* source/destination memory list */
+               head = &c_node->mem_list[i];
+
+               if (list_empty(head)) {
+                       DRM_DEBUG_KMS("%s:%s memory empty.\n", __func__,
+                               i ? "dst" : "src");
+                       continue;
+               }
+
+               /* find memory node entry */
+               list_for_each_entry(m_node, head, list) {
+                       DRM_DEBUG_KMS("%s:%s,count[%d]m_node[0x%x]\n", __func__,
+                               i ? "dst" : "src", count[i], (int)m_node);
+                       count[i]++;
+               }
+       }
+
+       DRM_DEBUG_KMS("%s:min[%d]max[%d]\n", __func__,
+               min(count[EXYNOS_DRM_OPS_SRC], count[EXYNOS_DRM_OPS_DST]),
+               max(count[EXYNOS_DRM_OPS_SRC], count[EXYNOS_DRM_OPS_DST]));
+
+
+       if (property->cmd == IPP_CMD_M2M)
+               ret = min(count[EXYNOS_DRM_OPS_SRC],
+                       count[EXYNOS_DRM_OPS_DST]);
+       else
+               ret = max(count[EXYNOS_DRM_OPS_SRC],
+                       count[EXYNOS_DRM_OPS_DST]);
+
+       mutex_unlock(&c_node->mem_lock);
+
+       return ret;
+}
+
+static void ipp_clean_cmd_node(struct drm_exynos_ipp_cmd_node *c_node)
+{
+       DRM_DEBUG_KMS("%s\n", __func__);
+
+       /* delete list */
+       list_del(&c_node->list);
+
+       /* destroy mutex */
+       mutex_destroy(&c_node->cmd_lock);
+       mutex_destroy(&c_node->mem_lock);
+       mutex_destroy(&c_node->event_lock);
+
+       /* free command node */
+       kfree(c_node->start_work);
+       kfree(c_node->stop_work);
+       kfree(c_node->event_work);
+       kfree(c_node);
+}
+
+static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv,
+       struct drm_exynos_ipp_cmd_node *c_node,
+       struct drm_exynos_ipp_mem_node *m_node)
+{
+       struct exynos_drm_ipp_ops *ops = NULL;
+       int ret = 0;
+
+       DRM_DEBUG_KMS("%s:node[0x%x]\n", __func__, (int)m_node);
+
+       if (!m_node) {
+               DRM_ERROR("invalid queue node.\n");
+               return -EFAULT;
+       }
+
+       mutex_lock(&c_node->mem_lock);
+
+       DRM_DEBUG_KMS("%s:ops_id[%d]\n", __func__, m_node->ops_id);
+
+       /* get operations callback */
+       ops = ippdrv->ops[m_node->ops_id];
+       if (!ops) {
+               DRM_ERROR("not support ops.\n");
+               ret = -EIO;

-EFAULT;

OK, I will change it.


+               goto err_unlock;
+       }
+
+       /* set address and enable irq */
+       if (ops->set_addr) {
+               ret = ops->set_addr(ippdrv->dev, &m_node->buf_info,
+                       m_node->buf_id, IPP_BUF_ENQUEUE);
+               if (ret) {
+                       DRM_ERROR("failed to set addr.\n");
+                       goto err_unlock;
+               }
+       }
+
+err_unlock:
+       mutex_unlock(&c_node->mem_lock);
+       return ret;
+}
+
+static struct drm_exynos_ipp_mem_node
+       *ipp_get_mem_node(struct drm_device *drm_dev,
+       struct drm_file *file,
+       struct drm_exynos_ipp_cmd_node *c_node,
+       struct drm_exynos_ipp_queue_buf *qbuf)
+{
+       struct drm_exynos_ipp_mem_node *m_node;
+       struct drm_exynos_ipp_buf_info buf_info;
+       void *addr;
+       int i;
+
+       mutex_lock(&c_node->mem_lock);
+
+       m_node = kzalloc(sizeof(*m_node), GFP_KERNEL);
+       if (!m_node) {
+               DRM_ERROR("failed to allocate queue node.\n");
+               goto err_unlock;
+       }
+
+       /* clear base address for error handling */
+       memset(&buf_info, 0x0, sizeof(buf_info));
+
+       /* operations, buffer id */
+       m_node->ops_id = qbuf->ops_id;
+       m_node->prop_id = qbuf->prop_id;
+       m_node->buf_id = qbuf->buf_id;
+
+       DRM_DEBUG_KMS("%s:m_node[0x%x]ops_id[%d]\n", __func__,
+               (int)m_node, qbuf->ops_id);
+       DRM_DEBUG_KMS("%s:prop_id[%d]buf_id[%d]\n", __func__,
+               qbuf->prop_id, m_node->buf_id);
+
+       for_each_ipp_planar(i) {
+               DRM_DEBUG_KMS("%s:i[%d]handle[0x%x]\n", __func__,
+                       i, qbuf->handle[i]);
+
+               /* get dma address by handle */
+               if (qbuf->handle[i] != 0) {
+                       addr = exynos_drm_gem_get_dma_addr(drm_dev,
+                                       qbuf->handle[i], file);
+                       if (!addr) {
+                               DRM_ERROR("failed to get addr.\n");
+                               goto err_clear;
+                       }
+
+                       buf_info.gem_handle[i] = qbuf->handle[i];
+                       buf_info.base[i] = *(dma_addr_t *) addr;
+                       buf_info.file = file;
+                       DRM_DEBUG_KMS("%s:i[%d]base[0x%x]gem_handle[0x%x]\n",
+                               __func__, i, buf_info.base[i],
+                               buf_info.gem_handle[i]);
+               }
+       }
+
+       m_node->buf_info = buf_info;
+       list_add_tail(&m_node->list, &c_node->mem_list[qbuf->ops_id]);
+
+       mutex_unlock(&c_node->mem_lock);
+       return m_node;
+
+err_clear:
+       kfree(m_node);
+
+err_unlock:
+       mutex_unlock(&c_node->mem_lock);
+
+       return NULL;
+}
+
+static int ipp_put_mem_node(struct drm_device *drm_dev,
+       struct drm_exynos_ipp_cmd_node *c_node,
+       struct drm_exynos_ipp_mem_node *m_node)
+{
+       int i, ret = 0;
+
+       DRM_DEBUG_KMS("%s:node[0x%x]\n", __func__, (int)m_node);
+
+       if (!m_node) {
+               DRM_ERROR("invalid dequeue node.\n");
+               return -EFAULT;
+       }
+
+       if (list_empty(&m_node->list)) {
+               DRM_ERROR("empty memory node.\n");
+               return -ENOMEM;
+       }
+
+       mutex_lock(&c_node->mem_lock);
+
+       DRM_DEBUG_KMS("%s:ops_id[%d]\n", __func__, m_node->ops_id);
+
+       /* put gem buffer */
+       for_each_ipp_planar(i) {
+               struct drm_file *file = m_node->buf_info.file;
+               unsigned int gem_handle = m_node->buf_info.gem_handle[i];
+
+               if (gem_handle)
+                       exynos_drm_gem_put_dma_addr(drm_dev, gem_handle, file);
+       }
+
+       /* delete list in queue */
+       list_del(&m_node->list);
+       kfree(m_node);
+
+       mutex_unlock(&c_node->mem_lock);
+       return ret;
+}
+
+static void ipp_free_event(struct drm_pending_event *event)
+{
+       kfree(event);
+}
+
+static int ipp_get_event(struct drm_device *drm_dev,
+       struct drm_file *file,
+       struct drm_exynos_ipp_cmd_node *c_node,
+       struct drm_exynos_ipp_queue_buf *qbuf)
+{
+       struct drm_exynos_ipp_send_event *e;
+       unsigned long flags;
+
+       DRM_DEBUG_KMS("%s:ops_id[%d]buf_id[%d]\n", __func__,
+               qbuf->ops_id, qbuf->buf_id);
+
+       e = kzalloc(sizeof(*e), GFP_KERNEL);
+
+       if (!e) {
+               DRM_ERROR("failed to allocate event.\n");
+               spin_lock_irqsave(&drm_dev->event_lock, flags);
+               file->event_space += sizeof(e->event);
+               spin_unlock_irqrestore(&drm_dev->event_lock, flags);
+               return -ENOMEM;
+       }
+
+       /* make event */
+       e->event.base.type = DRM_EXYNOS_IPP_EVENT;
+       e->event.base.length = sizeof(e->event);
+       e->event.user_data = qbuf->user_data;
+       e->event.prop_id = qbuf->prop_id;
+       e->event.buf_id[EXYNOS_DRM_OPS_DST] = qbuf->buf_id;
+       e->base.event = &e->event.base;
+       e->base.file_priv = file;
+       e->base.destroy = ipp_free_event;
+       list_add_tail(&e->base.link, &c_node->event_list);
+
+       return 0;
+}
+
+static void ipp_put_event(struct drm_exynos_ipp_cmd_node *c_node,
+       struct drm_exynos_ipp_queue_buf *qbuf)
+{
+       struct drm_exynos_ipp_send_event *e, *te;
+       int count = 0;
+
+       if (list_empty(&c_node->event_list)) {
+               DRM_DEBUG_KMS("%s:event_list is empty.\n", __func__);
+               return;
+       }
+
+       list_for_each_entry_safe(e, te, &c_node->event_list, base.link) {
+               DRM_DEBUG_KMS("%s:count[%d]e[0x%x]\n",
+                       __func__, count++, (int)e);
+
+               if (!qbuf) {
+                       /* delete list */
+                       list_del(&e->base.link);
+                       kfree(e);
+               } else if (e->event.buf_id[EXYNOS_DRM_OPS_DST]
+                       == qbuf->buf_id) {
+                       /* delete list */
+                       list_del(&e->base.link);
+                       kfree(e);
+                       return;
+               }
+       }
+
+       return;
+}
+
+void ipp_handle_cmd_work(struct device *dev,
+       struct exynos_drm_ippdrv *ippdrv,
+       struct drm_exynos_ipp_cmd_work *cmd_work,
+       struct drm_exynos_ipp_cmd_node *c_node)
+{
+       struct ipp_context *ctx = get_ipp_context(dev);
+
+       cmd_work->ippdrv = ippdrv;
+       cmd_work->c_node = c_node;
+       queue_work(ctx->cmd_workq, (struct work_struct *)cmd_work);
+}
+
+int exynos_drm_ipp_queue_buf(struct drm_device *drm_dev, void *data,
+       struct drm_file *file)
+{
+       struct drm_exynos_file_private *file_priv = file->driver_priv;
+       struct exynos_drm_ipp_private *priv = file_priv->ipp_priv;
+       struct device *dev = priv->dev;
+       struct ipp_context *ctx = get_ipp_context(dev);
+       struct drm_exynos_ipp_queue_buf *qbuf = data;
+       struct exynos_drm_ippdrv *ippdrv;
+       struct drm_exynos_ipp_property *property;
+       struct exynos_drm_ipp_ops *ops;
+       struct drm_exynos_ipp_cmd_node *c_node;
+       struct drm_exynos_ipp_mem_node *m_node, *tm_node;
+       int ret;
+
+       DRM_DEBUG_KMS("%s\n", __func__);
+
+       if (!qbuf) {
+               DRM_ERROR("invalid buf parameter.\n");
+               return -EINVAL;
+       }
+
+       ippdrv = ipp_find_drv_node(qbuf->prop_id);
+
+       if (!ippdrv) {
+               DRM_ERROR("failed to get ipp driver.\n");
+               return -EINVAL;

return -EFAULT;

OK, I will change it.


+       }
+
+       if (qbuf->ops_id >= EXYNOS_DRM_OPS_MAX) {
+               DRM_ERROR("invalid ops parameter.\n");
+               return -EINVAL;
+       }
+
+       ops = ippdrv->ops[qbuf->ops_id];
+       if (!ops) {
+               DRM_ERROR("failed to get ops.\n");
+               return -EINVAL;

return -EFAULT;

OK, I will change it.


+       }
+
+       DRM_DEBUG_KMS("%s:prop_id[%d]ops_id[%s]buf_id[%d]buf_type[%d]\n",
+               __func__, qbuf->prop_id, qbuf->ops_id ? "dst" : "src",
+               qbuf->buf_id, qbuf->buf_type);
+
+       /* find command node */
+       c_node = ipp_find_id(&ctx->prop_idr, &ctx->prop_lock,
+               qbuf->prop_id);
+       if (!c_node) {
+               DRM_ERROR("failed to get command node.\n");
+               return -EINVAL;

return -EFAULT;

OK, I will change it.



Eunchul, it seems like that your patch set should be more cleared and
simply. Please re-post them again as RFC after more clean.

OK. I wil send one more time using RFC.


Thanks,
Inki Dae


Thank's
Eunchul Kim

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


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux