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 at samsung.com>:
>> 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 at samsung.com>
>> Signed-off-by: Jinyoung Jeon <jy0.jeon at samsung.com>
>> ---
>> 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 at samsung.com>
>> + * Jinyoung Jeon <jy0.jeon at samsung.com>
>> + * Sangmin Lee <lsmin.lee at 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.
>> + *
>> + */
>> +#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
