[PATCH 00/15] drm/exynos/ipp: image post processing fixes and improvements, part four

[PATCH 00/15] drm/exynos/ipp: image post processing fixes and improvements, part four

[Andrzej Hajda]

This set of patches contains various improvement and fixes
for exynos_drm ipp framework.
The patchset is based on exynos-drm-next branch.

IPP framework was tested for regressions on exynos4210-trats target.

Regards
Andrzej


Andrzej Hajda (15):
  drm/exynos/ipp: remove fake pm callbacks
  drm/exynos/ipp: cancel works before command node clean
  drm/exynos/ipp: move file reference from memory to command node
  drm/exynos/ipp: remove only related commands on file close
  drm/exynos/ipp: remove unused field in command node
  drm/exynos/ipp: free partially allocated resources on error
  drm/exynos/ipp: move nodes cleaning to separate function
  drm/exynos/ipp: clean memory nodes on command node cleaning
  drm/exynos/ipp: replace work_struct casting with better constructs
  drm/exynos/ipp: stop hardware before freeing memory
  drm/exynos/ipp: remove events during command cleaning
  drm/exynos/fimc: avoid clearing overflow bits
  drm/exynos/fimc: do not enable fimc twice
  drm/exynos/fimc: simplify buffer queuing
  drm/exynos/fimc: fix source buffer registers

 drivers/gpu/drm/exynos/exynos_drm_fimc.c    |  90 ++-----
 drivers/gpu/drm/exynos/exynos_drm_gsc.c     |   3 +-
 drivers/gpu/drm/exynos/exynos_drm_ipp.c     | 369 ++++++++++++----------------
 drivers/gpu/drm/exynos/exynos_drm_ipp.h     |   4 +-
 drivers/gpu/drm/exynos/exynos_drm_rotator.c |   3 +-
 5 files changed, 180 insertions(+), 289 deletions(-)

-- 
1.9.1

[PATCH 01/15] drm/exynos/ipp: remove fake pm callbacks

[Andrzej Hajda]

PM callbacks in ipp core do nothing, so the patch removes it.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 51 ---------------------------------
 1 file changed, 51 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index c411399..da917ca 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -1808,63 +1808,12 @@ static int ipp_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int ipp_power_ctrl(struct ipp_context *ctx, bool enable)
-{
-	DRM_DEBUG_KMS("enable[%d]\n", enable);
-
-	return 0;
-}
-
-#ifdef CONFIG_PM_SLEEP
-static int ipp_suspend(struct device *dev)
-{
-	struct ipp_context *ctx = get_ipp_context(dev);
-
-	if (pm_runtime_suspended(dev))
-		return 0;
-
-	return ipp_power_ctrl(ctx, false);
-}
-
-static int ipp_resume(struct device *dev)
-{
-	struct ipp_context *ctx = get_ipp_context(dev);
-
-	if (!pm_runtime_suspended(dev))
-		return ipp_power_ctrl(ctx, true);
-
-	return 0;
-}
-#endif
-
-#ifdef CONFIG_PM_RUNTIME
-static int ipp_runtime_suspend(struct device *dev)
-{
-	struct ipp_context *ctx = get_ipp_context(dev);
-
-	return ipp_power_ctrl(ctx, false);
-}
-
-static int ipp_runtime_resume(struct device *dev)
-{
-	struct ipp_context *ctx = get_ipp_context(dev);
-
-	return ipp_power_ctrl(ctx, true);
-}
-#endif
-
-static const struct dev_pm_ops ipp_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(ipp_suspend, ipp_resume)
-	SET_RUNTIME_PM_OPS(ipp_runtime_suspend, ipp_runtime_resume, NULL)
-};
-
 struct platform_driver ipp_driver = {
 	.probe		= ipp_probe,
 	.remove		= ipp_remove,
 	.driver		= {
 		.name	= "exynos-drm-ipp",
 		.owner	= THIS_MODULE,
-		.pm	= &ipp_pm_ops,
 	},
 };
 
-- 
1.9.1

[PATCH 02/15] drm/exynos/ipp: cancel works before command node clean

[Andrzej Hajda]

All pending works should be canceled prior to its removal.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index da917ca..9770966 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -502,6 +502,11 @@ err_clear:
 static void ipp_clean_cmd_node(struct ipp_context *ctx,
 				struct drm_exynos_ipp_cmd_node *c_node)
 {
+	/* cancel works */
+	cancel_work_sync(&c_node->start_work->work);
+	cancel_work_sync(&c_node->stop_work->work);
+	cancel_work_sync(&c_node->event_work->work);
+
 	/* delete list */
 	list_del(&c_node->list);
 
-- 
1.9.1

[PATCH 03/15] drm/exynos/ipp: move file reference from memory to command node

[Andrzej Hajda]

Command node should contain file reference to distinguish commands
created by different processes.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 5 ++---
 drivers/gpu/drm/exynos/exynos_drm_ipp.h | 2 ++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 9770966..bbe9968 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -75,7 +75,6 @@ struct drm_exynos_ipp_mem_node {
 	u32	prop_id;
 	u32	buf_id;
 	struct drm_exynos_ipp_buf_info	buf_info;
-	struct drm_file		*filp;
 };
 
 /*
@@ -448,6 +447,7 @@ int exynos_drm_ipp_set_property(struct drm_device *drm_dev, void *data,
 	c_node->dev = dev;
 	c_node->property = *property;
 	c_node->state = IPP_STATE_IDLE;
+	c_node->filp = file;
 
 	c_node->start_work = ipp_create_cmd_work();
 	if (IS_ERR(c_node->start_work)) {
@@ -645,7 +645,6 @@ static struct drm_exynos_ipp_mem_node
 		}
 	}
 
-	m_node->filp = file;
 	mutex_lock(&c_node->mem_lock);
 	list_add_tail(&m_node->list, &c_node->mem_list[qbuf->ops_id]);
 	mutex_unlock(&c_node->mem_lock);
@@ -677,7 +676,7 @@ static int ipp_put_mem_node(struct drm_device *drm_dev,
 		unsigned long handle = m_node->buf_info.handles[i];
 		if (handle)
 			exynos_drm_gem_put_dma_addr(drm_dev, handle,
-							m_node->filp);
+							c_node->filp);
 	}
 
 	/* delete list in queue */
diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.h b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
index 6f48d62..0311035 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
@@ -62,6 +62,7 @@ struct drm_exynos_ipp_cmd_work {
  * @stop_work: stop command work structure.
  * @event_work: event work structure.
  * @state: state of command node.
+ * @filp: associated file pointer.
  */
 struct drm_exynos_ipp_cmd_node {
 	struct device		*dev;
@@ -78,6 +79,7 @@ struct drm_exynos_ipp_cmd_node {
 	struct drm_exynos_ipp_cmd_work *stop_work;
 	struct drm_exynos_ipp_event_work *event_work;
 	enum drm_exynos_ipp_state	state;
+	struct drm_file	*filp;
 };
 
 /*
-- 
1.9.1

Re: [PATCH 03/15] drm/exynos/ipp: move file reference from memory to command node

[Joonyoung Shim]

Hi Andrzej,

On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
> Command node should contain file reference to distinguish commands
> created by different processes.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 5 ++---
>  drivers/gpu/drm/exynos/exynos_drm_ipp.h | 2 ++
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> index 9770966..bbe9968 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> @@ -75,7 +75,6 @@ struct drm_exynos_ipp_mem_node {
>  	u32	prop_id;
>  	u32	buf_id;
>  	struct drm_exynos_ipp_buf_info	buf_info;
> -	struct drm_file		*filp;
>  };
>  
>  /*
> @@ -448,6 +447,7 @@ int exynos_drm_ipp_set_property(struct drm_device *drm_dev, void *data,
>  	c_node->dev = dev;
>  	c_node->property = *property;
>  	c_node->state = IPP_STATE_IDLE;
> +	c_node->filp = file;
>  
>  	c_node->start_work = ipp_create_cmd_work();
>  	if (IS_ERR(c_node->start_work)) {
> @@ -645,7 +645,6 @@ static struct drm_exynos_ipp_mem_node
>  		}
>  	}
>  
> -	m_node->filp = file;
>  	mutex_lock(&c_node->mem_lock);
>  	list_add_tail(&m_node->list, &c_node->mem_list[qbuf->ops_id]);
>  	mutex_unlock(&c_node->mem_lock);
> @@ -677,7 +676,7 @@ static int ipp_put_mem_node(struct drm_device *drm_dev,

Then, could you remove file argument from exynos_drm_ipp_queue_buf() and
ipp_get_event()?

>  		unsigned long handle = m_node->buf_info.handles[i];
>  		if (handle)
>  			exynos_drm_gem_put_dma_addr(drm_dev, handle,
> -							m_node->filp);
> +							c_node->filp);
>  	}
>  
>  	/* delete list in queue */
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.h b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
> index 6f48d62..0311035 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
> @@ -62,6 +62,7 @@ struct drm_exynos_ipp_cmd_work {
>   * @stop_work: stop command work structure.
>   * @event_work: event work structure.
>   * @state: state of command node.
> + * @filp: associated file pointer.
>   */
>  struct drm_exynos_ipp_cmd_node {
>  	struct device		*dev;
> @@ -78,6 +79,7 @@ struct drm_exynos_ipp_cmd_node {
>  	struct drm_exynos_ipp_cmd_work *stop_work;
>  	struct drm_exynos_ipp_event_work *event_work;
>  	enum drm_exynos_ipp_state	state;
> +	struct drm_file	*filp;
>  };
>  
>  /*
> 

Re: [PATCH 03/15] drm/exynos/ipp: move file reference from memory to command node

[Joonyoung Shim]

On 08/26/2014 11:55 AM, Joonyoung Shim wrote:
> Hi Andrzej,
> 
> On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
>> Command node should contain file reference to distinguish commands
>> created by different processes.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 5 ++---
>>  drivers/gpu/drm/exynos/exynos_drm_ipp.h | 2 ++
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> index 9770966..bbe9968 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> @@ -75,7 +75,6 @@ struct drm_exynos_ipp_mem_node {
>>  	u32	prop_id;
>>  	u32	buf_id;
>>  	struct drm_exynos_ipp_buf_info	buf_info;
>> -	struct drm_file		*filp;
>>  };
>>  
>>  /*
>> @@ -448,6 +447,7 @@ int exynos_drm_ipp_set_property(struct drm_device *drm_dev, void *data,
>>  	c_node->dev = dev;
>>  	c_node->property = *property;
>>  	c_node->state = IPP_STATE_IDLE;
>> +	c_node->filp = file;
>>  
>>  	c_node->start_work = ipp_create_cmd_work();
>>  	if (IS_ERR(c_node->start_work)) {
>> @@ -645,7 +645,6 @@ static struct drm_exynos_ipp_mem_node
>>  		}
>>  	}
>>  
>> -	m_node->filp = file;
>>  	mutex_lock(&c_node->mem_lock);
>>  	list_add_tail(&m_node->list, &c_node->mem_list[qbuf->ops_id]);
>>  	mutex_unlock(&c_node->mem_lock);
>> @@ -677,7 +676,7 @@ static int ipp_put_mem_node(struct drm_device *drm_dev,
> 
> Then, could you remove file argument from exynos_drm_ipp_queue_buf() and
> ipp_get_event()?

sorry, i mean ipp_put_mem_node() instead of exynos_drm_ipp_queue_buf().

> 
>>  		unsigned long handle = m_node->buf_info.handles[i];
>>  		if (handle)
>>  			exynos_drm_gem_put_dma_addr(drm_dev, handle,
>> -							m_node->filp);
>> +							c_node->filp);
>>  	}
>>  
>>  	/* delete list in queue */
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.h b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
>> index 6f48d62..0311035 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
>> @@ -62,6 +62,7 @@ struct drm_exynos_ipp_cmd_work {
>>   * @stop_work: stop command work structure.
>>   * @event_work: event work structure.
>>   * @state: state of command node.
>> + * @filp: associated file pointer.
>>   */
>>  struct drm_exynos_ipp_cmd_node {
>>  	struct device		*dev;
>> @@ -78,6 +79,7 @@ struct drm_exynos_ipp_cmd_node {
>>  	struct drm_exynos_ipp_cmd_work *stop_work;
>>  	struct drm_exynos_ipp_event_work *event_work;
>>  	enum drm_exynos_ipp_state	state;
>> +	struct drm_file	*filp;
>>  };
>>  
>>  /*
>>
> 
> 

Re: [PATCH 03/15] drm/exynos/ipp: move file reference from memory to command node

[Andrzej Hajda]

Hi Joonyoung,

Thanks for review.

On 08/26/2014 04:59 AM, Joonyoung Shim wrote:
> On 08/26/2014 11:55 AM, Joonyoung Shim wrote:
>> Hi Andrzej,
>>
>> On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
>>> Command node should contain file reference to distinguish commands
>>> created by different processes.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 5 ++---
>>>  drivers/gpu/drm/exynos/exynos_drm_ipp.h | 2 ++
>>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>>> index 9770966..bbe9968 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>>> @@ -75,7 +75,6 @@ struct drm_exynos_ipp_mem_node {
>>>  	u32	prop_id;
>>>  	u32	buf_id;
>>>  	struct drm_exynos_ipp_buf_info	buf_info;
>>> -	struct drm_file		*filp;
>>>  };
>>>  
>>>  /*
>>> @@ -448,6 +447,7 @@ int exynos_drm_ipp_set_property(struct drm_device *drm_dev, void *data,
>>>  	c_node->dev = dev;
>>>  	c_node->property = *property;
>>>  	c_node->state = IPP_STATE_IDLE;
>>> +	c_node->filp = file;
>>>  
>>>  	c_node->start_work = ipp_create_cmd_work();
>>>  	if (IS_ERR(c_node->start_work)) {
>>> @@ -645,7 +645,6 @@ static struct drm_exynos_ipp_mem_node
>>>  		}
>>>  	}
>>>  
>>> -	m_node->filp = file;
>>>  	mutex_lock(&c_node->mem_lock);
>>>  	list_add_tail(&m_node->list, &c_node->mem_list[qbuf->ops_id]);
>>>  	mutex_unlock(&c_node->mem_lock);
>>> @@ -677,7 +676,7 @@ static int ipp_put_mem_node(struct drm_device *drm_dev,
>> Then, could you remove file argument from exynos_drm_ipp_queue_buf() and
>> ipp_get_event()?
> sorry, i mean ipp_put_mem_node() instead of exynos_drm_ipp_queue_buf().

I guess you mean ipp_get_mem_node() and ipp_get_event().
You are right, it should be removed. Additionally file check should be added
to exynos_drm_ipp_queue_buf.

Regards
Andrzej

>
>>>  		unsigned long handle = m_node->buf_info.handles[i];
>>>  		if (handle)
>>>  			exynos_drm_gem_put_dma_addr(drm_dev, handle,
>>> -							m_node->filp);
>>> +							c_node->filp);
>>>  	}
>>>  
>>>  	/* delete list in queue */
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.h b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
>>> index 6f48d62..0311035 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.h
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
>>> @@ -62,6 +62,7 @@ struct drm_exynos_ipp_cmd_work {
>>>   * @stop_work: stop command work structure.
>>>   * @event_work: event work structure.
>>>   * @state: state of command node.
>>> + * @filp: associated file pointer.
>>>   */
>>>  struct drm_exynos_ipp_cmd_node {
>>>  	struct device		*dev;
>>> @@ -78,6 +79,7 @@ struct drm_exynos_ipp_cmd_node {
>>>  	struct drm_exynos_ipp_cmd_work *stop_work;
>>>  	struct drm_exynos_ipp_event_work *event_work;
>>>  	enum drm_exynos_ipp_state	state;
>>> +	struct drm_file	*filp;
>>>  };
>>>  
>>>  /*
>>>
>>
>

[PATCH 04/15] drm/exynos/ipp: remove only related commands on file close

[Andrzej Hajda]

On file close driver should remove only command nodes created
via this file.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index bbe9968..81f780e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -1681,14 +1681,11 @@ static int ipp_subdrv_open(struct drm_device *drm_dev, struct device *dev,
 static void ipp_subdrv_close(struct drm_device *drm_dev, struct device *dev,
 		struct drm_file *file)
 {
-	struct drm_exynos_file_private *file_priv = file->driver_priv;
 	struct exynos_drm_ippdrv *ippdrv = NULL;
 	struct ipp_context *ctx = get_ipp_context(dev);
 	struct drm_exynos_ipp_cmd_node *c_node, *tc_node;
 	int count = 0;
 
-	DRM_DEBUG_KMS("for priv[0x%x]\n", (int)file_priv->ipp_dev);
-
 	list_for_each_entry(ippdrv, &exynos_drm_ippdrv_list, drv_list) {
 		mutex_lock(&ippdrv->cmd_lock);
 		list_for_each_entry_safe(c_node, tc_node,
@@ -1696,7 +1693,7 @@ static void ipp_subdrv_close(struct drm_device *drm_dev, struct device *dev,
 			DRM_DEBUG_KMS("count[%d]ippdrv[0x%x]\n",
 				count++, (int)ippdrv);
 
-			if (c_node->dev == file_priv->ipp_dev) {
+			if (c_node->filp == file) {
 				/*
 				 * userland goto unnormal state. process killed.
 				 * and close the file.
-- 
1.9.1

[PATCH 05/15] drm/exynos/ipp: remove unused field in command node

[Andrzej Hajda]

Since command node have file pointer dev field became useless.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 1 -
 drivers/gpu/drm/exynos/exynos_drm_ipp.h | 2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 81f780e..060a198 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -444,7 +444,6 @@ int exynos_drm_ipp_set_property(struct drm_device *drm_dev, void *data,
 		property->prop_id, property->cmd, (int)ippdrv);
 
 	/* stored property information and ippdrv in private data */
-	c_node->dev = dev;
 	c_node->property = *property;
 	c_node->state = IPP_STATE_IDLE;
 	c_node->filp = file;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.h b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
index 0311035..2a61547 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.h
@@ -48,7 +48,6 @@ struct drm_exynos_ipp_cmd_work {
 /*
  * A structure of command node.
  *
- * @dev: IPP device.
  * @list: list head to command queue information.
  * @event_list: list head of event.
  * @mem_list: list head to source,destination memory queue information.
@@ -65,7 +64,6 @@ struct drm_exynos_ipp_cmd_work {
  * @filp: associated file pointer.
  */
 struct drm_exynos_ipp_cmd_node {
-	struct device		*dev;
 	struct list_head	list;
 	struct list_head	event_list;
 	struct list_head	mem_list[EXYNOS_DRM_OPS_MAX];
-- 
1.9.1

[PATCH 06/15] drm/exynos/ipp: free partially allocated resources on error

[Andrzej Hajda]

In case of allocation errors some already allocated buffers
were not freed. The patch fixes it.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 68 ++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 060a198..728f3b9 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -599,6 +599,37 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv,
 	return ret;
 }
 
+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;
+
+	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
+
+	if (!m_node) {
+		DRM_ERROR("invalid dequeue node.\n");
+		return -EFAULT;
+	}
+
+	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
+
+	/* put gem buffer */
+	for_each_ipp_planar(i) {
+		unsigned long handle = m_node->buf_info.handles[i];
+		if (handle)
+			exynos_drm_gem_put_dma_addr(drm_dev, handle,
+							c_node->filp);
+	}
+
+	/* conditionally remove from queue */
+	if (m_node->list.next)
+		list_del(&m_node->list);
+	kfree(m_node);
+
+	return 0;
+}
+
 static struct drm_exynos_ipp_mem_node
 		*ipp_get_mem_node(struct drm_device *drm_dev,
 		struct drm_file *file,
@@ -634,7 +665,8 @@ static struct drm_exynos_ipp_mem_node
 					qbuf->handle[i], file);
 			if (IS_ERR(addr)) {
 				DRM_ERROR("failed to get addr.\n");
-				goto err_clear;
+				ipp_put_mem_node(drm_dev, c_node, m_node);
+				return ERR_PTR(-EFAULT);
 			}
 
 			buf_info->handles[i] = qbuf->handle[i];
@@ -649,40 +681,6 @@ static struct drm_exynos_ipp_mem_node
 	mutex_unlock(&c_node->mem_lock);
 
 	return m_node;
-
-err_clear:
-	kfree(m_node);
-	return ERR_PTR(-EFAULT);
-}
-
-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;
-
-	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
-
-	if (!m_node) {
-		DRM_ERROR("invalid dequeue node.\n");
-		return -EFAULT;
-	}
-
-	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
-
-	/* put gem buffer */
-	for_each_ipp_planar(i) {
-		unsigned long handle = m_node->buf_info.handles[i];
-		if (handle)
-			exynos_drm_gem_put_dma_addr(drm_dev, handle,
-							c_node->filp);
-	}
-
-	/* delete list in queue */
-	list_del(&m_node->list);
-	kfree(m_node);
-
-	return 0;
 }
 
 static void ipp_free_event(struct drm_pending_event *event)
-- 
1.9.1

Re: [PATCH 06/15] drm/exynos/ipp: free partially allocated resources on error

[Joonyoung Shim]

Hi Andrzej,

On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
> In case of allocation errors some already allocated buffers
> were not freed. The patch fixes it.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 68 ++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> index 060a198..728f3b9 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
> @@ -599,6 +599,37 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv,
>  	return ret;
>  }
>  
> +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;
> +
> +	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
> +
> +	if (!m_node) {
> +		DRM_ERROR("invalid dequeue node.\n");
> +		return -EFAULT;
> +	}
> +
> +	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
> +
> +	/* put gem buffer */
> +	for_each_ipp_planar(i) {
> +		unsigned long handle = m_node->buf_info.handles[i];
> +		if (handle)
> +			exynos_drm_gem_put_dma_addr(drm_dev, handle,
> +							c_node->filp);
> +	}
> +
> +	/* conditionally remove from queue */
> +	if (m_node->list.next)

How about do INIT_LIST_HEAD for list in ipp_get_mem_node()?

> +		list_del(&m_node->list);
> +	kfree(m_node);
> +
> +	return 0;
> +}
> +
>  static struct drm_exynos_ipp_mem_node
>  		*ipp_get_mem_node(struct drm_device *drm_dev,
>  		struct drm_file *file,
> @@ -634,7 +665,8 @@ static struct drm_exynos_ipp_mem_node
>  					qbuf->handle[i], file);
>  			if (IS_ERR(addr)) {
>  				DRM_ERROR("failed to get addr.\n");
> -				goto err_clear;
> +				ipp_put_mem_node(drm_dev, c_node, m_node);
> +				return ERR_PTR(-EFAULT);
>  			}
>  
>  			buf_info->handles[i] = qbuf->handle[i];
> @@ -649,40 +681,6 @@ static struct drm_exynos_ipp_mem_node
>  	mutex_unlock(&c_node->mem_lock);
>  
>  	return m_node;
> -
> -err_clear:
> -	kfree(m_node);
> -	return ERR_PTR(-EFAULT);
> -}
> -
> -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;
> -
> -	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
> -
> -	if (!m_node) {
> -		DRM_ERROR("invalid dequeue node.\n");
> -		return -EFAULT;
> -	}
> -
> -	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
> -
> -	/* put gem buffer */
> -	for_each_ipp_planar(i) {
> -		unsigned long handle = m_node->buf_info.handles[i];
> -		if (handle)
> -			exynos_drm_gem_put_dma_addr(drm_dev, handle,
> -							c_node->filp);
> -	}
> -
> -	/* delete list in queue */
> -	list_del(&m_node->list);
> -	kfree(m_node);
> -
> -	return 0;
>  }
>  
>  static void ipp_free_event(struct drm_pending_event *event)
> 

Re: [PATCH 06/15] drm/exynos/ipp: free partially allocated resources on error

[Andrzej Hajda]

On 08/26/2014 07:00 AM, Joonyoung Shim wrote:
> Hi Andrzej,
>
> On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
>> In case of allocation errors some already allocated buffers
>> were not freed. The patch fixes it.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 68 ++++++++++++++++-----------------
>>  1 file changed, 33 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> index 060a198..728f3b9 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> @@ -599,6 +599,37 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv,
>>  	return ret;
>>  }
>>  
>> +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;
>> +
>> +	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
>> +
>> +	if (!m_node) {
>> +		DRM_ERROR("invalid dequeue node.\n");
>> +		return -EFAULT;
>> +	}
>> +
>> +	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
>> +
>> +	/* put gem buffer */
>> +	for_each_ipp_planar(i) {
>> +		unsigned long handle = m_node->buf_info.handles[i];
>> +		if (handle)
>> +			exynos_drm_gem_put_dma_addr(drm_dev, handle,
>> +							c_node->filp);
>> +	}
>> +
>> +	/* conditionally remove from queue */
>> +	if (m_node->list.next)
> How about do INIT_LIST_HEAD for list in ipp_get_mem_node()?

I am not sure if it is better. For sure it adds unnecessary
initialization sequence.
Maybe adding list_is_initialized inline function to list.h would be the
best solution.

Regards
Andrzej

>
>> +		list_del(&m_node->list);
>> +	kfree(m_node);
>> +
>> +	return 0;
>> +}
>> +
>>  static struct drm_exynos_ipp_mem_node
>>  		*ipp_get_mem_node(struct drm_device *drm_dev,
>>  		struct drm_file *file,
>> @@ -634,7 +665,8 @@ static struct drm_exynos_ipp_mem_node
>>  					qbuf->handle[i], file);
>>  			if (IS_ERR(addr)) {
>>  				DRM_ERROR("failed to get addr.\n");
>> -				goto err_clear;
>> +				ipp_put_mem_node(drm_dev, c_node, m_node);
>> +				return ERR_PTR(-EFAULT);
>>  			}
>>  
>>  			buf_info->handles[i] = qbuf->handle[i];
>> @@ -649,40 +681,6 @@ static struct drm_exynos_ipp_mem_node
>>  	mutex_unlock(&c_node->mem_lock);
>>  
>>  	return m_node;
>> -
>> -err_clear:
>> -	kfree(m_node);
>> -	return ERR_PTR(-EFAULT);
>> -}
>> -
>> -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;
>> -
>> -	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
>> -
>> -	if (!m_node) {
>> -		DRM_ERROR("invalid dequeue node.\n");
>> -		return -EFAULT;
>> -	}
>> -
>> -	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
>> -
>> -	/* put gem buffer */
>> -	for_each_ipp_planar(i) {
>> -		unsigned long handle = m_node->buf_info.handles[i];
>> -		if (handle)
>> -			exynos_drm_gem_put_dma_addr(drm_dev, handle,
>> -							c_node->filp);
>> -	}
>> -
>> -	/* delete list in queue */
>> -	list_del(&m_node->list);
>> -	kfree(m_node);
>> -
>> -	return 0;
>>  }
>>  
>>  static void ipp_free_event(struct drm_pending_event *event)
>>
>

Re: [PATCH 06/15] drm/exynos/ipp: free partially allocated resources on error

[Joonyoung Shim]

Hi,

On 08/27/2014 07:27 PM, Andrzej Hajda wrote:
> On 08/26/2014 07:00 AM, Joonyoung Shim wrote:
>> Hi Andrzej,
>>
>> On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
>>> In case of allocation errors some already allocated buffers
>>> were not freed. The patch fixes it.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 68 ++++++++++++++++-----------------
>>>  1 file changed, 33 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>>> index 060a198..728f3b9 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>>> @@ -599,6 +599,37 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv,
>>>  	return ret;
>>>  }
>>>  
>>> +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;
>>> +
>>> +	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
>>> +
>>> +	if (!m_node) {
>>> +		DRM_ERROR("invalid dequeue node.\n");
>>> +		return -EFAULT;
>>> +	}
>>> +
>>> +	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
>>> +
>>> +	/* put gem buffer */
>>> +	for_each_ipp_planar(i) {
>>> +		unsigned long handle = m_node->buf_info.handles[i];
>>> +		if (handle)
>>> +			exynos_drm_gem_put_dma_addr(drm_dev, handle,
>>> +							c_node->filp);
>>> +	}
>>> +
>>> +	/* conditionally remove from queue */
>>> +	if (m_node->list.next)
>> How about do INIT_LIST_HEAD for list in ipp_get_mem_node()?
> 
> I am not sure if it is better. For sure it adds unnecessary
> initialization sequence.

In other words, this NULL checking is unnecessary if you initialize the
list_head by INIT_LIST_HEAD.

> Maybe adding list_is_initialized inline function to list.h would be the
> best solution.

There is just list_empty and we can't know whether list is initialized
or not. I recommend to use initialized list_head.

Thanks.

> 
> Regards
> Andrzej
> 
>>
>>> +		list_del(&m_node->list);
>>> +	kfree(m_node);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static struct drm_exynos_ipp_mem_node
>>>  		*ipp_get_mem_node(struct drm_device *drm_dev,
>>>  		struct drm_file *file,
>>> @@ -634,7 +665,8 @@ static struct drm_exynos_ipp_mem_node
>>>  					qbuf->handle[i], file);
>>>  			if (IS_ERR(addr)) {
>>>  				DRM_ERROR("failed to get addr.\n");
>>> -				goto err_clear;
>>> +				ipp_put_mem_node(drm_dev, c_node, m_node);
>>> +				return ERR_PTR(-EFAULT);
>>>  			}
>>>  
>>>  			buf_info->handles[i] = qbuf->handle[i];
>>> @@ -649,40 +681,6 @@ static struct drm_exynos_ipp_mem_node
>>>  	mutex_unlock(&c_node->mem_lock);
>>>  
>>>  	return m_node;
>>> -
>>> -err_clear:
>>> -	kfree(m_node);
>>> -	return ERR_PTR(-EFAULT);
>>> -}
>>> -
>>> -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;
>>> -
>>> -	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
>>> -
>>> -	if (!m_node) {
>>> -		DRM_ERROR("invalid dequeue node.\n");
>>> -		return -EFAULT;
>>> -	}
>>> -
>>> -	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
>>> -
>>> -	/* put gem buffer */
>>> -	for_each_ipp_planar(i) {
>>> -		unsigned long handle = m_node->buf_info.handles[i];
>>> -		if (handle)
>>> -			exynos_drm_gem_put_dma_addr(drm_dev, handle,
>>> -							c_node->filp);
>>> -	}
>>> -
>>> -	/* delete list in queue */
>>> -	list_del(&m_node->list);
>>> -	kfree(m_node);
>>> -
>>> -	return 0;
>>>  }
>>>  
>>>  static void ipp_free_event(struct drm_pending_event *event)
>>>
>>
> 
> 

[PATCH 07/15] drm/exynos/ipp: move nodes cleaning to separate function

[Andrzej Hajda]

The patch introduces ipp_clean_mem_nodes function which replaces
redundant code. Additionally memory node function definitions
are moved up to increase its visibility.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 231 +++++++++++++++-----------------
 1 file changed, 107 insertions(+), 124 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 728f3b9..22bd551 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -498,6 +498,109 @@ err_clear:
 	return ret;
 }
 
+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;
+
+	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
+
+	if (!m_node) {
+		DRM_ERROR("invalid dequeue node.\n");
+		return -EFAULT;
+	}
+
+	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
+
+	/* put gem buffer */
+	for_each_ipp_planar(i) {
+		unsigned long handle = m_node->buf_info.handles[i];
+		if (handle)
+			exynos_drm_gem_put_dma_addr(drm_dev, handle,
+							c_node->filp);
+	}
+
+	/* conditionally remove from queue */
+	if (m_node->list.next)
+		list_del(&m_node->list);
+	kfree(m_node);
+
+	return 0;
+}
+
+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;
+	int i;
+
+	m_node = kzalloc(sizeof(*m_node), GFP_KERNEL);
+	if (!m_node)
+		return ERR_PTR(-ENOMEM);
+
+	buf_info = &m_node->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("m_node[0x%x]ops_id[%d]\n", (int)m_node, qbuf->ops_id);
+	DRM_DEBUG_KMS("prop_id[%d]buf_id[%d]\n", qbuf->prop_id, m_node->buf_id);
+
+	for_each_ipp_planar(i) {
+		DRM_DEBUG_KMS("i[%d]handle[0x%x]\n", i, qbuf->handle[i]);
+
+		/* get dma address by handle */
+		if (qbuf->handle[i]) {
+			dma_addr_t *addr;
+
+			addr = exynos_drm_gem_get_dma_addr(drm_dev,
+					qbuf->handle[i], file);
+			if (IS_ERR(addr)) {
+				DRM_ERROR("failed to get addr.\n");
+				ipp_put_mem_node(drm_dev, c_node, m_node);
+				return ERR_PTR(-EFAULT);
+			}
+
+			buf_info->handles[i] = qbuf->handle[i];
+			buf_info->base[i] = *addr;
+			DRM_DEBUG_KMS("i[%d]base[0x%x]hd[0x%lx]\n", i,
+				      buf_info->base[i], buf_info->handles[i]);
+		}
+	}
+
+	mutex_lock(&c_node->mem_lock);
+	list_add_tail(&m_node->list, &c_node->mem_list[qbuf->ops_id]);
+	mutex_unlock(&c_node->mem_lock);
+
+	return m_node;
+}
+
+static void ipp_clean_mem_nodes(struct drm_device *drm_dev,
+			       struct drm_exynos_ipp_cmd_node *c_node, int ops)
+{
+	struct drm_exynos_ipp_mem_node *m_node, *tm_node;
+	struct list_head *head = &c_node->mem_list[ops];
+
+	mutex_lock(&c_node->mem_lock);
+
+	list_for_each_entry_safe(m_node, tm_node, head, list) {
+		int ret;
+
+		ret = ipp_put_mem_node(drm_dev, c_node, m_node);
+		if (ret)
+			DRM_ERROR("failed to put m_node.\n");
+	}
+
+	mutex_unlock(&c_node->mem_lock);
+}
+
 static void ipp_clean_cmd_node(struct ipp_context *ctx,
 				struct drm_exynos_ipp_cmd_node *c_node)
 {
@@ -599,90 +702,6 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv,
 	return ret;
 }
 
-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;
-
-	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
-
-	if (!m_node) {
-		DRM_ERROR("invalid dequeue node.\n");
-		return -EFAULT;
-	}
-
-	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
-
-	/* put gem buffer */
-	for_each_ipp_planar(i) {
-		unsigned long handle = m_node->buf_info.handles[i];
-		if (handle)
-			exynos_drm_gem_put_dma_addr(drm_dev, handle,
-							c_node->filp);
-	}
-
-	/* conditionally remove from queue */
-	if (m_node->list.next)
-		list_del(&m_node->list);
-	kfree(m_node);
-
-	return 0;
-}
-
-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;
-	int i;
-
-	m_node = kzalloc(sizeof(*m_node), GFP_KERNEL);
-	if (!m_node)
-		return ERR_PTR(-ENOMEM);
-
-	buf_info = &m_node->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("m_node[0x%x]ops_id[%d]\n", (int)m_node, qbuf->ops_id);
-	DRM_DEBUG_KMS("prop_id[%d]buf_id[%d]\n", qbuf->prop_id, m_node->buf_id);
-
-	for_each_ipp_planar(i) {
-		DRM_DEBUG_KMS("i[%d]handle[0x%x]\n", i, qbuf->handle[i]);
-
-		/* get dma address by handle */
-		if (qbuf->handle[i]) {
-			dma_addr_t *addr;
-
-			addr = exynos_drm_gem_get_dma_addr(drm_dev,
-					qbuf->handle[i], file);
-			if (IS_ERR(addr)) {
-				DRM_ERROR("failed to get addr.\n");
-				ipp_put_mem_node(drm_dev, c_node, m_node);
-				return ERR_PTR(-EFAULT);
-			}
-
-			buf_info->handles[i] = qbuf->handle[i];
-			buf_info->base[i] = *addr;
-			DRM_DEBUG_KMS("i[%d]base[0x%x]hd[0x%lx]\n", i,
-				      buf_info->base[i], buf_info->handles[i]);
-		}
-	}
-
-	mutex_lock(&c_node->mem_lock);
-	list_add_tail(&m_node->list, &c_node->mem_list[qbuf->ops_id]);
-	mutex_unlock(&c_node->mem_lock);
-
-	return m_node;
-}
-
 static void ipp_free_event(struct drm_pending_event *event)
 {
 	kfree(event);
@@ -1258,9 +1277,7 @@ static int ipp_stop_property(struct drm_device *drm_dev,
 		struct exynos_drm_ippdrv *ippdrv,
 		struct drm_exynos_ipp_cmd_node *c_node)
 {
-	struct drm_exynos_ipp_mem_node *m_node, *tm_node;
 	struct drm_exynos_ipp_property *property = &c_node->property;
-	struct list_head *head;
 	int ret = 0, i;
 
 	DRM_DEBUG_KMS("prop_id[%d]\n", property->prop_id);
@@ -1268,49 +1285,17 @@ static int ipp_stop_property(struct drm_device *drm_dev,
 	/* put event */
 	ipp_put_event(c_node, NULL);
 
-	mutex_lock(&c_node->mem_lock);
-
 	/* check command */
 	switch (property->cmd) {
 	case IPP_CMD_M2M:
-		for_each_ipp_ops(i) {
-			/* source/destination memory list */
-			head = &c_node->mem_list[i];
-
-			list_for_each_entry_safe(m_node, tm_node,
-				head, list) {
-				ret = ipp_put_mem_node(drm_dev, c_node,
-					m_node);
-				if (ret) {
-					DRM_ERROR("failed to put m_node.\n");
-					goto err_clear;
-				}
-			}
-		}
+		for_each_ipp_ops(i)
+			ipp_clean_mem_nodes(drm_dev, c_node, i);
 		break;
 	case IPP_CMD_WB:
-		/* destination memory list */
-		head = &c_node->mem_list[EXYNOS_DRM_OPS_DST];
-
-		list_for_each_entry_safe(m_node, tm_node, head, list) {
-			ret = ipp_put_mem_node(drm_dev, c_node, m_node);
-			if (ret) {
-				DRM_ERROR("failed to put m_node.\n");
-				goto err_clear;
-			}
-		}
+		ipp_clean_mem_nodes(drm_dev, c_node, EXYNOS_DRM_OPS_DST);
 		break;
 	case IPP_CMD_OUTPUT:
-		/* source memory list */
-		head = &c_node->mem_list[EXYNOS_DRM_OPS_SRC];
-
-		list_for_each_entry_safe(m_node, tm_node, head, list) {
-			ret = ipp_put_mem_node(drm_dev, c_node, m_node);
-			if (ret) {
-				DRM_ERROR("failed to put m_node.\n");
-				goto err_clear;
-			}
-		}
+		ipp_clean_mem_nodes(drm_dev, c_node, EXYNOS_DRM_OPS_SRC);
 		break;
 	default:
 		DRM_ERROR("invalid operations.\n");
@@ -1319,8 +1304,6 @@ static int ipp_stop_property(struct drm_device *drm_dev,
 	}
 
 err_clear:
-	mutex_unlock(&c_node->mem_lock);
-
 	/* stop operations */
 	if (ippdrv->stop)
 		ippdrv->stop(ippdrv->dev, property->cmd);
-- 
1.9.1

[PATCH 08/15] drm/exynos/ipp: clean memory nodes on command node cleaning

[Andrzej Hajda]

The nodes should be removed before removing command node.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 22bd551..6ab6190 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -604,11 +604,16 @@ static void ipp_clean_mem_nodes(struct drm_device *drm_dev,
 static void ipp_clean_cmd_node(struct ipp_context *ctx,
 				struct drm_exynos_ipp_cmd_node *c_node)
 {
+	int i;
+
 	/* cancel works */
 	cancel_work_sync(&c_node->start_work->work);
 	cancel_work_sync(&c_node->stop_work->work);
 	cancel_work_sync(&c_node->event_work->work);
 
+	for_each_ipp_ops(i)
+		ipp_clean_mem_nodes(ctx->subdrv.drm_dev, c_node, i);
+
 	/* delete list */
 	list_del(&c_node->list);
 
-- 
1.9.1

[PATCH 09/15] drm/exynos/ipp: replace work_struct casting with better constructs

[Andrzej Hajda]

Type casting should be avoided if possible. In case of
work_struct it can be simply replaced by reference to member field.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimc.c    | 2 +-
 drivers/gpu/drm/exynos/exynos_drm_gsc.c     | 3 +--
 drivers/gpu/drm/exynos/exynos_drm_ipp.c     | 6 +++---
 drivers/gpu/drm/exynos/exynos_drm_rotator.c | 3 +--
 4 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index 35db665..3264ed3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1307,7 +1307,7 @@ static irqreturn_t fimc_irq_handler(int irq, void *dev_id)
 
 	event_work->ippdrv = ippdrv;
 	event_work->buf_id[EXYNOS_DRM_OPS_DST] = buf_id;
-	queue_work(ippdrv->event_workq, (struct work_struct *)event_work);
+	queue_work(ippdrv->event_workq, &event_work->work);
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
index 9e3ff16..c6a013f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
@@ -1326,8 +1326,7 @@ static irqreturn_t gsc_irq_handler(int irq, void *dev_id)
 			buf_id[EXYNOS_DRM_OPS_SRC];
 		event_work->buf_id[EXYNOS_DRM_OPS_DST] =
 			buf_id[EXYNOS_DRM_OPS_DST];
-		queue_work(ippdrv->event_workq,
-			(struct work_struct *)event_work);
+		queue_work(ippdrv->event_workq, &event_work->work);
 	}
 
 	return IRQ_HANDLED;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 6ab6190..c72d8d1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -791,7 +791,7 @@ static void ipp_handle_cmd_work(struct device *dev,
 
 	cmd_work->ippdrv = ippdrv;
 	cmd_work->c_node = c_node;
-	queue_work(ctx->cmd_workq, (struct work_struct *)cmd_work);
+	queue_work(ctx->cmd_workq, &cmd_work->work);
 }
 
 static int ipp_queue_buf_with_run(struct device *dev,
@@ -1319,7 +1319,7 @@ err_clear:
 void ipp_sched_cmd(struct work_struct *work)
 {
 	struct drm_exynos_ipp_cmd_work *cmd_work =
-		(struct drm_exynos_ipp_cmd_work *)work;
+		container_of(work, struct drm_exynos_ipp_cmd_work, work);
 	struct exynos_drm_ippdrv *ippdrv;
 	struct drm_exynos_ipp_cmd_node *c_node;
 	struct drm_exynos_ipp_property *property;
@@ -1532,7 +1532,7 @@ err_event_unlock:
 void ipp_sched_event(struct work_struct *work)
 {
 	struct drm_exynos_ipp_event_work *event_work =
-		(struct drm_exynos_ipp_event_work *)work;
+		container_of(work, struct drm_exynos_ipp_event_work, work);
 	struct exynos_drm_ippdrv *ippdrv;
 	struct drm_exynos_ipp_cmd_node *c_node;
 	int ret;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
index 55af6b4..b6a37d4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
@@ -156,8 +156,7 @@ static irqreturn_t rotator_irq_handler(int irq, void *arg)
 		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,
-			(struct work_struct *)event_work);
+		queue_work(ippdrv->event_workq, &event_work->work);
 	} else {
 		DRM_ERROR("the SFR is set illegally\n");
 	}
-- 
1.9.1

[PATCH 10/15] drm/exynos/ipp: stop hardware before freeing memory

[Andrzej Hajda]

Memory shouldn't be freed when hardware is still running.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index c72d8d1..6de75aa 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -1283,12 +1283,15 @@ static int ipp_stop_property(struct drm_device *drm_dev,
 		struct drm_exynos_ipp_cmd_node *c_node)
 {
 	struct drm_exynos_ipp_property *property = &c_node->property;
-	int ret = 0, i;
+	int i;
 
 	DRM_DEBUG_KMS("prop_id[%d]\n", property->prop_id);
 
 	/* put event */
 	ipp_put_event(c_node, NULL);
+	/* stop operations */
+	if (ippdrv->stop)
+		ippdrv->stop(ippdrv->dev, property->cmd);
 
 	/* check command */
 	switch (property->cmd) {
@@ -1304,16 +1307,10 @@ static int ipp_stop_property(struct drm_device *drm_dev,
 		break;
 	default:
 		DRM_ERROR("invalid operations.\n");
-		ret = -EINVAL;
-		goto err_clear;
+		return -EINVAL;
 	}
 
-err_clear:
-	/* stop operations */
-	if (ippdrv->stop)
-		ippdrv->stop(ippdrv->dev, property->cmd);
-
-	return ret;
+	return 0;
 }
 
 void ipp_sched_cmd(struct work_struct *work)
-- 
1.9.1

[PATCH 11/15] drm/exynos/ipp: remove events during command cleaning

[Andrzej Hajda]

Events were removed only during stop command, as a result
there were memory leaks if program prematurely exited.
This patch fixes it.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 155 ++++++++++++++++----------------
 1 file changed, 78 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 6de75aa..ec88393 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -601,6 +601,81 @@ static void ipp_clean_mem_nodes(struct drm_device *drm_dev,
 	mutex_unlock(&c_node->mem_lock);
 }
 
+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("ops_id[%d]buf_id[%d]\n", qbuf->ops_id, qbuf->buf_id);
+
+	e = kzalloc(sizeof(*e), GFP_KERNEL);
+	if (!e) {
+		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;
+	mutex_lock(&c_node->event_lock);
+	list_add_tail(&e->base.link, &c_node->event_list);
+	mutex_unlock(&c_node->event_lock);
+
+	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;
+
+	mutex_lock(&c_node->event_lock);
+	list_for_each_entry_safe(e, te, &c_node->event_list, base.link) {
+		DRM_DEBUG_KMS("count[%d]e[0x%x]\n", count++, (int)e);
+
+		/*
+		 * qbuf == NULL condition means all event deletion.
+		 * stop operations want to delete all event list.
+		 * another case delete only same buf id.
+		 */
+		if (!qbuf) {
+			/* delete list */
+			list_del(&e->base.link);
+			kfree(e);
+		}
+
+		/* compare buffer id */
+		if (qbuf && (qbuf->buf_id ==
+		    e->event.buf_id[EXYNOS_DRM_OPS_DST])) {
+			/* delete list */
+			list_del(&e->base.link);
+			kfree(e);
+			goto out_unlock;
+		}
+	}
+
+out_unlock:
+	mutex_unlock(&c_node->event_lock);
+	return;
+}
+
 static void ipp_clean_cmd_node(struct ipp_context *ctx,
 				struct drm_exynos_ipp_cmd_node *c_node)
 {
@@ -611,6 +686,9 @@ static void ipp_clean_cmd_node(struct ipp_context *ctx,
 	cancel_work_sync(&c_node->stop_work->work);
 	cancel_work_sync(&c_node->event_work->work);
 
+	/* put event */
+	ipp_put_event(c_node, NULL);
+
 	for_each_ipp_ops(i)
 		ipp_clean_mem_nodes(ctx->subdrv.drm_dev, c_node, i);
 
@@ -707,81 +785,6 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv,
 	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("ops_id[%d]buf_id[%d]\n", qbuf->ops_id, qbuf->buf_id);
-
-	e = kzalloc(sizeof(*e), GFP_KERNEL);
-	if (!e) {
-		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;
-	mutex_lock(&c_node->event_lock);
-	list_add_tail(&e->base.link, &c_node->event_list);
-	mutex_unlock(&c_node->event_lock);
-
-	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;
-
-	mutex_lock(&c_node->event_lock);
-	list_for_each_entry_safe(e, te, &c_node->event_list, base.link) {
-		DRM_DEBUG_KMS("count[%d]e[0x%x]\n", count++, (int)e);
-
-		/*
-		 * qbuf == NULL condition means all event deletion.
-		 * stop operations want to delete all event list.
-		 * another case delete only same buf id.
-		 */
-		if (!qbuf) {
-			/* delete list */
-			list_del(&e->base.link);
-			kfree(e);
-		}
-
-		/* compare buffer id */
-		if (qbuf && (qbuf->buf_id ==
-		    e->event.buf_id[EXYNOS_DRM_OPS_DST])) {
-			/* delete list */
-			list_del(&e->base.link);
-			kfree(e);
-			goto out_unlock;
-		}
-	}
-
-out_unlock:
-	mutex_unlock(&c_node->event_lock);
-	return;
-}
-
 static void ipp_handle_cmd_work(struct device *dev,
 		struct exynos_drm_ippdrv *ippdrv,
 		struct drm_exynos_ipp_cmd_work *cmd_work,
@@ -1287,8 +1290,6 @@ static int ipp_stop_property(struct drm_device *drm_dev,
 
 	DRM_DEBUG_KMS("prop_id[%d]\n", property->prop_id);
 
-	/* put event */
-	ipp_put_event(c_node, NULL);
 	/* stop operations */
 	if (ippdrv->stop)
 		ippdrv->stop(ippdrv->dev, property->cmd);
-- 
1.9.1

[PATCH 12/15] drm/exynos/fimc: avoid clearing overflow bits

[Andrzej Hajda]

Overflow bits shall be cleared by H/W.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index 3264ed3..bbaf4f9 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -341,9 +341,6 @@ static bool fimc_check_ovf(struct fimc_context *ctx)
 		fimc_set_bits(ctx, EXYNOS_CIWDOFST,
 			EXYNOS_CIWDOFST_CLROVFIY | EXYNOS_CIWDOFST_CLROVFICB |
 			EXYNOS_CIWDOFST_CLROVFICR);
-		fimc_clear_bits(ctx, EXYNOS_CIWDOFST,
-			EXYNOS_CIWDOFST_CLROVFIY | EXYNOS_CIWDOFST_CLROVFICB |
-			EXYNOS_CIWDOFST_CLROVFICR);
 
 		dev_err(ippdrv->dev, "occurred overflow at %d, status 0x%x.\n",
 			ctx->id, status);
-- 
1.9.1

[PATCH 13/15] drm/exynos/fimc: do not enable fimc twice

[Andrzej Hajda]

The patch removes redundant H/W activation.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index bbaf4f9..bd6628d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1596,12 +1596,9 @@ static int fimc_ippdrv_start(struct device *dev, enum drm_exynos_ipp_cmd cmd)
 
 	fimc_clear_bits(ctx, EXYNOS_CIOCTRL, EXYNOS_CIOCTRL_WEAVE_MASK);
 
-	if (cmd == IPP_CMD_M2M) {
+	if (cmd == IPP_CMD_M2M)
 		fimc_set_bits(ctx, EXYNOS_MSCTRL, EXYNOS_MSCTRL_ENVID);
 
-		fimc_set_bits(ctx, EXYNOS_MSCTRL, EXYNOS_MSCTRL_ENVID);
-	}
-
 	return 0;
 }
 
-- 
1.9.1

[PATCH 14/15] drm/exynos/fimc: simplify buffer queuing

[Andrzej Hajda]

The patch removes redundant checks, redundant HW reads
and simplifies code.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimc.c | 64 ++++++++------------------------
 1 file changed, 15 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index bd6628d..b20078e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1124,67 +1124,34 @@ static int fimc_dst_set_size(struct device *dev, int swap,
 	return 0;
 }
 
-static int fimc_dst_get_buf_count(struct fimc_context *ctx)
-{
-	u32 cfg, buf_num;
-
-	cfg = fimc_read(ctx, EXYNOS_CIFCNTSEQ);
-
-	buf_num = hweight32(cfg);
-
-	DRM_DEBUG_KMS("buf_num[%d]\n", buf_num);
-
-	return buf_num;
-}
-
-static int fimc_dst_set_buf_seq(struct fimc_context *ctx, u32 buf_id,
+static void fimc_dst_set_buf_seq(struct fimc_context *ctx, u32 buf_id,
 		enum drm_exynos_ipp_buf_type buf_type)
 {
-	struct exynos_drm_ippdrv *ippdrv = &ctx->ippdrv;
-	bool enable;
-	u32 cfg;
-	u32 mask = 0x00000001 << buf_id;
-	int ret = 0;
 	unsigned long flags;
+	u32 buf_num;
+	u32 cfg;
 
 	DRM_DEBUG_KMS("buf_id[%d]buf_type[%d]\n", buf_id, buf_type);
 
 	spin_lock_irqsave(&ctx->lock, flags);
 
-	/* mask register set */
 	cfg = fimc_read(ctx, EXYNOS_CIFCNTSEQ);
 
-	switch (buf_type) {
-	case IPP_BUF_ENQUEUE:
-		enable = true;
-		break;
-	case IPP_BUF_DEQUEUE:
-		enable = false;
-		break;
-	default:
-		dev_err(ippdrv->dev, "invalid buf ctrl parameter.\n");
-		ret =  -EINVAL;
-		goto err_unlock;
-	}
+	if (buf_type == IPP_BUF_ENQUEUE)
+		cfg |= (1 << buf_id);
+	else
+		cfg &= (1 << buf_id);
 
-	/* sequence id */
-	cfg &= ~mask;
-	cfg |= (enable << buf_id);
 	fimc_write(ctx, cfg, EXYNOS_CIFCNTSEQ);
 
-	/* interrupt enable */
-	if (buf_type == IPP_BUF_ENQUEUE &&
-	    fimc_dst_get_buf_count(ctx) >= FIMC_BUF_START)
-		fimc_mask_irq(ctx, true);
+	buf_num = hweight32(cfg);
 
-	/* interrupt disable */
-	if (buf_type == IPP_BUF_DEQUEUE &&
-	    fimc_dst_get_buf_count(ctx) <= FIMC_BUF_STOP)
+	if (buf_type == IPP_BUF_ENQUEUE && buf_num >= FIMC_BUF_START)
+		fimc_mask_irq(ctx, true);
+	else if (buf_type == IPP_BUF_DEQUEUE && buf_num <= FIMC_BUF_STOP)
 		fimc_mask_irq(ctx, false);
 
-err_unlock:
 	spin_unlock_irqrestore(&ctx->lock, flags);
-	return ret;
 }
 
 static int fimc_dst_set_addr(struct device *dev,
@@ -1242,7 +1209,9 @@ static int fimc_dst_set_addr(struct device *dev,
 		break;
 	}
 
-	return fimc_dst_set_buf_seq(ctx, buf_id, buf_type);
+	fimc_dst_set_buf_seq(ctx, buf_id, buf_type);
+
+	return 0;
 }
 
 static struct exynos_drm_ipp_ops fimc_dst_ops = {
@@ -1297,10 +1266,7 @@ static irqreturn_t fimc_irq_handler(int irq, void *dev_id)
 
 	DRM_DEBUG_KMS("buf_id[%d]\n", buf_id);
 
-	if (fimc_dst_set_buf_seq(ctx, buf_id, IPP_BUF_DEQUEUE) < 0) {
-		DRM_ERROR("failed to dequeue.\n");
-		return IRQ_HANDLED;
-	}
+	fimc_dst_set_buf_seq(ctx, buf_id, IPP_BUF_DEQUEUE);
 
 	event_work->ippdrv = ippdrv;
 	event_work->buf_id[EXYNOS_DRM_OPS_DST] = buf_id;
-- 
1.9.1

Re: [PATCH 14/15] drm/exynos/fimc: simplify buffer queuing

[Joonyoung Shim]

Hi Andrzej,

On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
> The patch removes redundant checks, redundant HW reads
> and simplifies code.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimc.c | 64 ++++++++------------------------
>  1 file changed, 15 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> index bd6628d..b20078e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> @@ -1124,67 +1124,34 @@ static int fimc_dst_set_size(struct device *dev, int swap,
>  	return 0;
>  }
>  
> -static int fimc_dst_get_buf_count(struct fimc_context *ctx)
> -{
> -	u32 cfg, buf_num;
> -
> -	cfg = fimc_read(ctx, EXYNOS_CIFCNTSEQ);
> -
> -	buf_num = hweight32(cfg);
> -
> -	DRM_DEBUG_KMS("buf_num[%d]\n", buf_num);
> -
> -	return buf_num;
> -}
> -
> -static int fimc_dst_set_buf_seq(struct fimc_context *ctx, u32 buf_id,
> +static void fimc_dst_set_buf_seq(struct fimc_context *ctx, u32 buf_id,
>  		enum drm_exynos_ipp_buf_type buf_type)
>  {
> -	struct exynos_drm_ippdrv *ippdrv = &ctx->ippdrv;
> -	bool enable;
> -	u32 cfg;
> -	u32 mask = 0x00000001 << buf_id;
> -	int ret = 0;
>  	unsigned long flags;
> +	u32 buf_num;
> +	u32 cfg;
>  
>  	DRM_DEBUG_KMS("buf_id[%d]buf_type[%d]\n", buf_id, buf_type);
>  
>  	spin_lock_irqsave(&ctx->lock, flags);
>  
> -	/* mask register set */
>  	cfg = fimc_read(ctx, EXYNOS_CIFCNTSEQ);
>  
> -	switch (buf_type) {
> -	case IPP_BUF_ENQUEUE:
> -		enable = true;
> -		break;
> -	case IPP_BUF_DEQUEUE:
> -		enable = false;
> -		break;
> -	default:
> -		dev_err(ippdrv->dev, "invalid buf ctrl parameter.\n");
> -		ret =  -EINVAL;
> -		goto err_unlock;
> -	}
> +	if (buf_type == IPP_BUF_ENQUEUE)
> +		cfg |= (1 << buf_id);
> +	else
> +		cfg &= (1 << buf_id);

~ Missing?

>  
> -	/* sequence id */
> -	cfg &= ~mask;
> -	cfg |= (enable << buf_id);
>  	fimc_write(ctx, cfg, EXYNOS_CIFCNTSEQ);
>  
> -	/* interrupt enable */
> -	if (buf_type == IPP_BUF_ENQUEUE &&
> -	    fimc_dst_get_buf_count(ctx) >= FIMC_BUF_START)
> -		fimc_mask_irq(ctx, true);
> +	buf_num = hweight32(cfg);
>  
> -	/* interrupt disable */
> -	if (buf_type == IPP_BUF_DEQUEUE &&
> -	    fimc_dst_get_buf_count(ctx) <= FIMC_BUF_STOP)
> +	if (buf_type == IPP_BUF_ENQUEUE && buf_num >= FIMC_BUF_START)
> +		fimc_mask_irq(ctx, true);
> +	else if (buf_type == IPP_BUF_DEQUEUE && buf_num <= FIMC_BUF_STOP)
>  		fimc_mask_irq(ctx, false);
>  
> -err_unlock:
>  	spin_unlock_irqrestore(&ctx->lock, flags);
> -	return ret;
>  }
>  
>  static int fimc_dst_set_addr(struct device *dev,
> @@ -1242,7 +1209,9 @@ static int fimc_dst_set_addr(struct device *dev,
>  		break;
>  	}
>  
> -	return fimc_dst_set_buf_seq(ctx, buf_id, buf_type);
> +	fimc_dst_set_buf_seq(ctx, buf_id, buf_type);
> +
> +	return 0;
>  }
>  
>  static struct exynos_drm_ipp_ops fimc_dst_ops = {
> @@ -1297,10 +1266,7 @@ static irqreturn_t fimc_irq_handler(int irq, void *dev_id)
>  
>  	DRM_DEBUG_KMS("buf_id[%d]\n", buf_id);
>  
> -	if (fimc_dst_set_buf_seq(ctx, buf_id, IPP_BUF_DEQUEUE) < 0) {
> -		DRM_ERROR("failed to dequeue.\n");
> -		return IRQ_HANDLED;
> -	}
> +	fimc_dst_set_buf_seq(ctx, buf_id, IPP_BUF_DEQUEUE);
>  
>  	event_work->ippdrv = ippdrv;
>  	event_work->buf_id[EXYNOS_DRM_OPS_DST] = buf_id;
> 

Re: [PATCH 14/15] drm/exynos/fimc: simplify buffer queuing

[Andrzej Hajda]

On 08/26/2014 07:53 AM, Joonyoung Shim wrote:
> Hi Andrzej,
>
> On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
>> The patch removes redundant checks, redundant HW reads
>> and simplifies code.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_fimc.c | 64 ++++++++------------------------
>>  1 file changed, 15 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> index bd6628d..b20078e 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> @@ -1124,67 +1124,34 @@ static int fimc_dst_set_size(struct device *dev, int swap,
>>  	return 0;
>>  }
>>  
>> -static int fimc_dst_get_buf_count(struct fimc_context *ctx)
>> -{
>> -	u32 cfg, buf_num;
>> -
>> -	cfg = fimc_read(ctx, EXYNOS_CIFCNTSEQ);
>> -
>> -	buf_num = hweight32(cfg);
>> -
>> -	DRM_DEBUG_KMS("buf_num[%d]\n", buf_num);
>> -
>> -	return buf_num;
>> -}
>> -
>> -static int fimc_dst_set_buf_seq(struct fimc_context *ctx, u32 buf_id,
>> +static void fimc_dst_set_buf_seq(struct fimc_context *ctx, u32 buf_id,
>>  		enum drm_exynos_ipp_buf_type buf_type)
>>  {
>> -	struct exynos_drm_ippdrv *ippdrv = &ctx->ippdrv;
>> -	bool enable;
>> -	u32 cfg;
>> -	u32 mask = 0x00000001 << buf_id;
>> -	int ret = 0;
>>  	unsigned long flags;
>> +	u32 buf_num;
>> +	u32 cfg;
>>  
>>  	DRM_DEBUG_KMS("buf_id[%d]buf_type[%d]\n", buf_id, buf_type);
>>  
>>  	spin_lock_irqsave(&ctx->lock, flags);
>>  
>> -	/* mask register set */
>>  	cfg = fimc_read(ctx, EXYNOS_CIFCNTSEQ);
>>  
>> -	switch (buf_type) {
>> -	case IPP_BUF_ENQUEUE:
>> -		enable = true;
>> -		break;
>> -	case IPP_BUF_DEQUEUE:
>> -		enable = false;
>> -		break;
>> -	default:
>> -		dev_err(ippdrv->dev, "invalid buf ctrl parameter.\n");
>> -		ret =  -EINVAL;
>> -		goto err_unlock;
>> -	}
>> +	if (buf_type == IPP_BUF_ENQUEUE)
>> +		cfg |= (1 << buf_id);
>> +	else
>> +		cfg &= (1 << buf_id);
> ~ Missing?

Yes, thanks for pointing it out.

Regards
Andrzej

>
>>  
>> -	/* sequence id */
>> -	cfg &= ~mask;
>> -	cfg |= (enable << buf_id);
>>  	fimc_write(ctx, cfg, EXYNOS_CIFCNTSEQ);
>>  
>> -	/* interrupt enable */
>> -	if (buf_type == IPP_BUF_ENQUEUE &&
>> -	    fimc_dst_get_buf_count(ctx) >= FIMC_BUF_START)
>> -		fimc_mask_irq(ctx, true);
>> +	buf_num = hweight32(cfg);
>>  
>> -	/* interrupt disable */
>> -	if (buf_type == IPP_BUF_DEQUEUE &&
>> -	    fimc_dst_get_buf_count(ctx) <= FIMC_BUF_STOP)
>> +	if (buf_type == IPP_BUF_ENQUEUE && buf_num >= FIMC_BUF_START)
>> +		fimc_mask_irq(ctx, true);
>> +	else if (buf_type == IPP_BUF_DEQUEUE && buf_num <= FIMC_BUF_STOP)
>>  		fimc_mask_irq(ctx, false);
>>  
>> -err_unlock:
>>  	spin_unlock_irqrestore(&ctx->lock, flags);
>> -	return ret;
>>  }
>>  
>>  static int fimc_dst_set_addr(struct device *dev,
>> @@ -1242,7 +1209,9 @@ static int fimc_dst_set_addr(struct device *dev,
>>  		break;
>>  	}
>>  
>> -	return fimc_dst_set_buf_seq(ctx, buf_id, buf_type);
>> +	fimc_dst_set_buf_seq(ctx, buf_id, buf_type);
>> +
>> +	return 0;
>>  }
>>  
>>  static struct exynos_drm_ipp_ops fimc_dst_ops = {
>> @@ -1297,10 +1266,7 @@ static irqreturn_t fimc_irq_handler(int irq, void *dev_id)
>>  
>>  	DRM_DEBUG_KMS("buf_id[%d]\n", buf_id);
>>  
>> -	if (fimc_dst_set_buf_seq(ctx, buf_id, IPP_BUF_DEQUEUE) < 0) {
>> -		DRM_ERROR("failed to dequeue.\n");
>> -		return IRQ_HANDLED;
>> -	}
>> +	fimc_dst_set_buf_seq(ctx, buf_id, IPP_BUF_DEQUEUE);
>>  
>>  	event_work->ippdrv = ippdrv;
>>  	event_work->buf_id[EXYNOS_DRM_OPS_DST] = buf_id;
>>
>

[PATCH v2 14/15] drm/exynos/fimc: simplify buffer queuing

[Andrzej Hajda]

The patch removes redundant checks, redundant HW reads
and simplifies code.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
v2:
- fixed bit cleaning operation
---
 drivers/gpu/drm/exynos/exynos_drm_fimc.c | 64 ++++++++------------------------
 1 file changed, 15 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index 62ba033..59ba8bb 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1126,67 +1126,34 @@ static int fimc_dst_set_size(struct device *dev, int swap,
 	return 0;
 }
 
-static int fimc_dst_get_buf_count(struct fimc_context *ctx)
-{
-	u32 cfg, buf_num;
-
-	cfg = fimc_read(ctx, EXYNOS_CIFCNTSEQ);
-
-	buf_num = hweight32(cfg);
-
-	DRM_DEBUG_KMS("buf_num[%d]\n", buf_num);
-
-	return buf_num;
-}
-
-static int fimc_dst_set_buf_seq(struct fimc_context *ctx, u32 buf_id,
+static void fimc_dst_set_buf_seq(struct fimc_context *ctx, u32 buf_id,
 		enum drm_exynos_ipp_buf_type buf_type)
 {
-	struct exynos_drm_ippdrv *ippdrv = &ctx->ippdrv;
-	bool enable;
-	u32 cfg;
-	u32 mask = 0x00000001 << buf_id;
-	int ret = 0;
 	unsigned long flags;
+	u32 buf_num;
+	u32 cfg;
 
 	DRM_DEBUG_KMS("buf_id[%d]buf_type[%d]\n", buf_id, buf_type);
 
 	spin_lock_irqsave(&ctx->lock, flags);
 
-	/* mask register set */
 	cfg = fimc_read(ctx, EXYNOS_CIFCNTSEQ);
 
-	switch (buf_type) {
-	case IPP_BUF_ENQUEUE:
-		enable = true;
-		break;
-	case IPP_BUF_DEQUEUE:
-		enable = false;
-		break;
-	default:
-		dev_err(ippdrv->dev, "invalid buf ctrl parameter.\n");
-		ret =  -EINVAL;
-		goto err_unlock;
-	}
+	if (buf_type == IPP_BUF_ENQUEUE)
+		cfg |= (1 << buf_id);
+	else
+		cfg &= ~(1 << buf_id);
 
-	/* sequence id */
-	cfg &= ~mask;
-	cfg |= (enable << buf_id);
 	fimc_write(ctx, cfg, EXYNOS_CIFCNTSEQ);
 
-	/* interrupt enable */
-	if (buf_type == IPP_BUF_ENQUEUE &&
-	    fimc_dst_get_buf_count(ctx) >= FIMC_BUF_START)
-		fimc_mask_irq(ctx, true);
+	buf_num = hweight32(cfg);
 
-	/* interrupt disable */
-	if (buf_type == IPP_BUF_DEQUEUE &&
-	    fimc_dst_get_buf_count(ctx) <= FIMC_BUF_STOP)
+	if (buf_type == IPP_BUF_ENQUEUE && buf_num >= FIMC_BUF_START)
+		fimc_mask_irq(ctx, true);
+	else if (buf_type == IPP_BUF_DEQUEUE && buf_num <= FIMC_BUF_STOP)
 		fimc_mask_irq(ctx, false);
 
-err_unlock:
 	spin_unlock_irqrestore(&ctx->lock, flags);
-	return ret;
 }
 
 static int fimc_dst_set_addr(struct device *dev,
@@ -1244,7 +1211,9 @@ static int fimc_dst_set_addr(struct device *dev,
 		break;
 	}
 
-	return fimc_dst_set_buf_seq(ctx, buf_id, buf_type);
+	fimc_dst_set_buf_seq(ctx, buf_id, buf_type);
+
+	return 0;
 }
 
 static struct exynos_drm_ipp_ops fimc_dst_ops = {
@@ -1299,10 +1268,7 @@ static irqreturn_t fimc_irq_handler(int irq, void *dev_id)
 
 	DRM_DEBUG_KMS("buf_id[%d]\n", buf_id);
 
-	if (fimc_dst_set_buf_seq(ctx, buf_id, IPP_BUF_DEQUEUE) < 0) {
-		DRM_ERROR("failed to dequeue.\n");
-		return IRQ_HANDLED;
-	}
+	fimc_dst_set_buf_seq(ctx, buf_id, IPP_BUF_DEQUEUE);
 
 	event_work->ippdrv = ippdrv;
 	event_work->buf_id[EXYNOS_DRM_OPS_DST] = buf_id;
-- 
1.9.1

[PATCH 15/15] drm/exynos/fimc: fix source buffer registers

[Andrzej Hajda]

FIMC in default mode of operation uses only one input buffer,
but the driver used also second buffer, as a result only the
first frame was processed correctly. The patch fixes it.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index b20078e..e985253 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -720,24 +720,24 @@ static int fimc_src_set_addr(struct device *dev,
 	case IPP_BUF_ENQUEUE:
 		config = &property->config[EXYNOS_DRM_OPS_SRC];
 		fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_Y],
-			EXYNOS_CIIYSA(buf_id));
+			EXYNOS_CIIYSA0);
 
 		if (config->fmt == DRM_FORMAT_YVU420) {
 			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CR],
-				EXYNOS_CIICBSA(buf_id));
+				EXYNOS_CIICBSA0);
 			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CB],
-				EXYNOS_CIICRSA(buf_id));
+				EXYNOS_CIICRSA0);
 		} else {
 			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CB],
-				EXYNOS_CIICBSA(buf_id));
+				EXYNOS_CIICBSA0);
 			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CR],
-				EXYNOS_CIICRSA(buf_id));
+				EXYNOS_CIICRSA0);
 		}
 		break;
 	case IPP_BUF_DEQUEUE:
-		fimc_write(ctx, 0x0, EXYNOS_CIIYSA(buf_id));
-		fimc_write(ctx, 0x0, EXYNOS_CIICBSA(buf_id));
-		fimc_write(ctx, 0x0, EXYNOS_CIICRSA(buf_id));
+		fimc_write(ctx, 0x0, EXYNOS_CIIYSA0);
+		fimc_write(ctx, 0x0, EXYNOS_CIICBSA0);
+		fimc_write(ctx, 0x0, EXYNOS_CIICRSA0);
 		break;
 	default:
 		/* bypass */
-- 
1.9.1

Re: [PATCH 15/15] drm/exynos/fimc: fix source buffer registers

[Joonyoung Shim]

Hi Andrzej,

On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
> FIMC in default mode of operation uses only one input buffer,
> but the driver used also second buffer, as a result only the
> first frame was processed correctly. The patch fixes it.

I can't understand well, then we don't need to distinguish buf_id in
fimc_src_set_addr()?

> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimc.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> index b20078e..e985253 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> @@ -720,24 +720,24 @@ static int fimc_src_set_addr(struct device *dev,
>  	case IPP_BUF_ENQUEUE:
>  		config = &property->config[EXYNOS_DRM_OPS_SRC];
>  		fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_Y],
> -			EXYNOS_CIIYSA(buf_id));
> +			EXYNOS_CIIYSA0);
>  
>  		if (config->fmt == DRM_FORMAT_YVU420) {
>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CR],
> -				EXYNOS_CIICBSA(buf_id));
> +				EXYNOS_CIICBSA0);
>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CB],
> -				EXYNOS_CIICRSA(buf_id));
> +				EXYNOS_CIICRSA0);
>  		} else {
>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CB],
> -				EXYNOS_CIICBSA(buf_id));
> +				EXYNOS_CIICBSA0);
>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CR],
> -				EXYNOS_CIICRSA(buf_id));
> +				EXYNOS_CIICRSA0);
>  		}
>  		break;
>  	case IPP_BUF_DEQUEUE:
> -		fimc_write(ctx, 0x0, EXYNOS_CIIYSA(buf_id));
> -		fimc_write(ctx, 0x0, EXYNOS_CIICBSA(buf_id));
> -		fimc_write(ctx, 0x0, EXYNOS_CIICRSA(buf_id));
> +		fimc_write(ctx, 0x0, EXYNOS_CIIYSA0);
> +		fimc_write(ctx, 0x0, EXYNOS_CIICBSA0);
> +		fimc_write(ctx, 0x0, EXYNOS_CIICRSA0);
>  		break;
>  	default:
>  		/* bypass */
> 

Re: [PATCH 15/15] drm/exynos/fimc: fix source buffer registers

[Andrzej Hajda]

On 08/26/2014 07:57 AM, Joonyoung Shim wrote:
> Hi Andrzej,
>
> On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
>> FIMC in default mode of operation uses only one input buffer,
>> but the driver used also second buffer, as a result only the
>> first frame was processed correctly. The patch fixes it.
> I can't understand well, then we don't need to distinguish buf_id in
> fimc_src_set_addr()?
Yes. FIMC in default operation mode uses only one input buffer pointer
which should
be updated when processing of the previous buffer has been finished.

There exists also ping-pong mode which uses two buffer pointers, as I
have spotted in
specs. However I have not seen it was implemented neither in drm,
neither in camera drivers.
I will try to implement it later.

Regards
Andrzej

>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_fimc.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> index b20078e..e985253 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> @@ -720,24 +720,24 @@ static int fimc_src_set_addr(struct device *dev,
>>  	case IPP_BUF_ENQUEUE:
>>  		config = &property->config[EXYNOS_DRM_OPS_SRC];
>>  		fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_Y],
>> -			EXYNOS_CIIYSA(buf_id));
>> +			EXYNOS_CIIYSA0);
>>  
>>  		if (config->fmt == DRM_FORMAT_YVU420) {
>>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CR],
>> -				EXYNOS_CIICBSA(buf_id));
>> +				EXYNOS_CIICBSA0);
>>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CB],
>> -				EXYNOS_CIICRSA(buf_id));
>> +				EXYNOS_CIICRSA0);
>>  		} else {
>>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CB],
>> -				EXYNOS_CIICBSA(buf_id));
>> +				EXYNOS_CIICBSA0);
>>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CR],
>> -				EXYNOS_CIICRSA(buf_id));
>> +				EXYNOS_CIICRSA0);
>>  		}
>>  		break;
>>  	case IPP_BUF_DEQUEUE:
>> -		fimc_write(ctx, 0x0, EXYNOS_CIIYSA(buf_id));
>> -		fimc_write(ctx, 0x0, EXYNOS_CIICBSA(buf_id));
>> -		fimc_write(ctx, 0x0, EXYNOS_CIICRSA(buf_id));
>> +		fimc_write(ctx, 0x0, EXYNOS_CIIYSA0);
>> +		fimc_write(ctx, 0x0, EXYNOS_CIICBSA0);
>> +		fimc_write(ctx, 0x0, EXYNOS_CIICRSA0);
>>  		break;
>>  	default:
>>  		/* bypass */
>>
>

Re: [PATCH 15/15] drm/exynos/fimc: fix source buffer registers

[Joonyoung Shim]

On 08/26/2014 03:35 PM, Andrzej Hajda wrote:
> On 08/26/2014 07:57 AM, Joonyoung Shim wrote:
>> Hi Andrzej,
>>
>> On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
>>> FIMC in default mode of operation uses only one input buffer,
>>> but the driver used also second buffer, as a result only the
>>> first frame was processed correctly. The patch fixes it.
>> I can't understand well, then we don't need to distinguish buf_id in
>> fimc_src_set_addr()?
> Yes. FIMC in default operation mode uses only one input buffer pointer
> which should
> be updated when processing of the previous buffer has been finished.
> 
> There exists also ping-pong mode which uses two buffer pointers, as I
> have spotted in
> specs. However I have not seen it was implemented neither in drm,
> neither in camera drivers.
> I will try to implement it later.

OK if operation is no problem, and i think it's better to add comments
about this.

> 
> Regards
> Andrzej
> 
>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_fimc.c | 16 ++++++++--------
>>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>>> index b20078e..e985253 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>>> @@ -720,24 +720,24 @@ static int fimc_src_set_addr(struct device *dev,
>>>  	case IPP_BUF_ENQUEUE:
>>>  		config = &property->config[EXYNOS_DRM_OPS_SRC];
>>>  		fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_Y],
>>> -			EXYNOS_CIIYSA(buf_id));
>>> +			EXYNOS_CIIYSA0);
>>>  
>>>  		if (config->fmt == DRM_FORMAT_YVU420) {
>>>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CR],
>>> -				EXYNOS_CIICBSA(buf_id));
>>> +				EXYNOS_CIICBSA0);
>>>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CB],
>>> -				EXYNOS_CIICRSA(buf_id));
>>> +				EXYNOS_CIICRSA0);
>>>  		} else {
>>>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CB],
>>> -				EXYNOS_CIICBSA(buf_id));
>>> +				EXYNOS_CIICBSA0);
>>>  			fimc_write(ctx, buf_info->base[EXYNOS_DRM_PLANAR_CR],
>>> -				EXYNOS_CIICRSA(buf_id));
>>> +				EXYNOS_CIICRSA0);
>>>  		}
>>>  		break;
>>>  	case IPP_BUF_DEQUEUE:
>>> -		fimc_write(ctx, 0x0, EXYNOS_CIIYSA(buf_id));
>>> -		fimc_write(ctx, 0x0, EXYNOS_CIICBSA(buf_id));
>>> -		fimc_write(ctx, 0x0, EXYNOS_CIICRSA(buf_id));
>>> +		fimc_write(ctx, 0x0, EXYNOS_CIIYSA0);
>>> +		fimc_write(ctx, 0x0, EXYNOS_CIICBSA0);
>>> +		fimc_write(ctx, 0x0, EXYNOS_CIICRSA0);
>>>  		break;
>>>  	default:
>>>  		/* bypass */
>>>
>>
> 
> 

Re: [PATCH 00/15] drm/exynos/ipp: image post processing fixes and improvements, part four

[Joonyoung Shim]

On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
> This set of patches contains various improvement and fixes
> for exynos_drm ipp framework.
> The patchset is based on exynos-drm-next branch.
> 
> IPP framework was tested for regressions on exynos4210-trats target.
> 
> Regards
> Andrzej
> 
> 
> Andrzej Hajda (15):
>   drm/exynos/ipp: remove fake pm callbacks
>   drm/exynos/ipp: cancel works before command node clean
>   drm/exynos/ipp: move file reference from memory to command node
>   drm/exynos/ipp: remove only related commands on file close
>   drm/exynos/ipp: remove unused field in command node
>   drm/exynos/ipp: free partially allocated resources on error
>   drm/exynos/ipp: move nodes cleaning to separate function
>   drm/exynos/ipp: clean memory nodes on command node cleaning
>   drm/exynos/ipp: replace work_struct casting with better constructs
>   drm/exynos/ipp: stop hardware before freeing memory
>   drm/exynos/ipp: remove events during command cleaning
>   drm/exynos/fimc: avoid clearing overflow bits
>   drm/exynos/fimc: do not enable fimc twice
>   drm/exynos/fimc: simplify buffer queuing
>   drm/exynos/fimc: fix source buffer registers

With some minor comments,

Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>

Thanks.

> 
>  drivers/gpu/drm/exynos/exynos_drm_fimc.c    |  90 ++-----
>  drivers/gpu/drm/exynos/exynos_drm_gsc.c     |   3 +-
>  drivers/gpu/drm/exynos/exynos_drm_ipp.c     | 369 ++++++++++++----------------
>  drivers/gpu/drm/exynos/exynos_drm_ipp.h     |   4 +-
>  drivers/gpu/drm/exynos/exynos_drm_rotator.c |   3 +-
>  5 files changed, 180 insertions(+), 289 deletions(-)
>