Re: [PATCH 3/7] drm/exynos: Fix G2D core mulfunctioning issue

[Joonyoung Shim <jy0922.shim@samsung.com>]

On 03/13/2013 07:53 PM, Inki Dae wrote:
>> -----Original Message-----
>> From: Joonyoung Shim [mailto:jy0922.shim@samsung.com]
>> Sent: Wednesday, March 13, 2013 7:28 PM
>> To: Inki Dae
>> Cc:airlied@linux.ie;dri-devel@lists.freedesktop.org;
>> kyungmin.park@samsung.com;sw0312.kim@samsung.com; 'YoungJun Cho'
>> Subject: Re: [PATCH 3/7] drm/exynos: Fix G2D core mulfunctioning issue
>>
>> On 03/13/2013 07:14 PM, Inki Dae wrote:
>>>> -----Original Message-----
>>>> From: Joonyoung Shim [mailto:jy0922.shim@samsung.com]
>>>> Sent: Wednesday, March 13, 2013 6:53 PM
>>>> To: Inki Dae
>>>> Cc:airlied@linux.ie;dri-devel@lists.freedesktop.org;
>>>> kyungmin.park@samsung.com;sw0312.kim@samsung.com; YoungJun Cho
>>>> Subject: Re: [PATCH 3/7] drm/exynos: Fix G2D core mulfunctioning issue
>>>>
>>>> On 03/13/2013 06:04 PM, Inki Dae wrote:
>>>>> From: YoungJun Cho<yj44.cho@samsung.com>
>>>>>
>>>>> This patch fixes G2D core mulfunctioning issue once g2d dma is
> started.
>>>>> Without 'DMA_HOLD_CMD_REG' register setting, there is only one
>> interrupt
>>>>> after the execution to all command lists have been completed. And that
>>>>> induces watchdog. So this patch sets 'LIST_HOLD' command to the
>> register
>>>>> so that command execution interrupt can be occured whenever each
>> command
>>>>> list execution is finished.
>>>> No, this problem occurs as GCF bit of INTEN_REG register is enabled
>>>> always. If wants to raise interrupt immediately after a command list
>>>> finished, GCF bit should be enabled, and it also needs to enable of
>> LIST
>>>> Hold bit of DMA_HOLD_CMD_REG register. If one of the two is missed out,
>>>> g2d hardware will not work normally sometimes.
>>> Right, these two things(LIST HOLD command and GCF interrupt enabling)
>> should
>>> be pair.
>>>
>>>> This patch is just workaround and it can happen performance issue
>>>> because g2d hardware stops a moment whenever a command list finished.
>>>>
>>>> So, we need the way which enable GCF bit only when a command list
>>>> completion interrupt needs.
>>>>
>>> Agree. How about this? If node->event isn't NULL then set GCF to INTEN
>>> register in g2d_dma_start(). For this way, I already mentioned through
>>> internal email thread.
>> No, Once set GCF, it is set on end. Who can clear it?
>>
> Maybe you say that g2d_dma_start() is called by exec ioctl so the GCF bit
> could be set by only last node.  For this, We need to look into dma-driven
> command processing. Assume that two more command lists exist and they are
> executed at once. Then we CAN NOT GET each node while dma operation because
> the command lists of each node are executed by dma at once.
>
> On other words, there is no way to enable GCF bit only in case that a
> command list completion interrupt is needed because the need is from user
> side.

I requested to Youngjun Cho whether it is possible to work and to insert
a command to set GCF bit of INTEN in command list and he said it is
possiable and works because clears GCF bit in irq handler if command
list completion interrupt occurs.

> So I think we need some policy for g2d driver. For example, if user wants to
> get event to each node, all nodes from the user should be operated with GCF
> bit otherwise without GCF bit.

Actually now there is no way to inform completion of a set of command
lists to user on async mode. So, i think completion event of a set of
command lists needs also.

Thanks.

>   
> Any other idea?
>
>>>> Thanks.
>>>>
>>>>> Signed-off-by: YoungJun Cho<yj44.cho@samsung.com>
>>>>> Signed-off-by: Inki Dae<inki.dae@samsung.com>
>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>>> ---
>>>>>     drivers/gpu/drm/exynos/exynos_drm_g2d.c |   13 ++++++++-----
>>>>>     1 files changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
>>>> b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
>>>>> index 095520f..91bc4cc 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
>>>>> @@ -82,7 +82,7 @@
>>>>>     #define G2D_DMA_LIST_DONE_COUNT_OFFSET	17
>>>>>
>>>>>     /* G2D_DMA_HOLD_CMD */
>>>>> -#define G2D_USET_HOLD			(1 << 2)
>>>>> +#define G2D_USER_HOLD			(1 << 2)
>>>>>     #define G2D_LIST_HOLD			(1 << 1)
>>>>>     #define G2D_BITBLT_HOLD			(1 << 0)
>>>>>
>>>>> @@ -863,10 +863,13 @@ int exynos_g2d_set_cmdlist_ioctl(struct
>> drm_device
>>>> *drm_dev, void *data,
>>>>>     	cmdlist->data[cmdlist->last++] = G2D_SRC_BASE_ADDR;
>>>>>     	cmdlist->data[cmdlist->last++] = 0;
>>>>>
>>>>> -	if (node->event) {
>>>>> -		cmdlist->data[cmdlist->last++] = G2D_DMA_HOLD_CMD;
>>>>> -		cmdlist->data[cmdlist->last++] = G2D_LIST_HOLD;
>>>>> -	}
>>>>> +	/*
>>>>> +	 * 'LIST_HOLD' command should be set to the DMA_HOLD_CMD_REG
>>>>> +	 * if user wants G2D interrupt event once each command list or
>>>>> +	 * BitBLT command execution is finished.
>>>>> +	 */
>>>>> +	cmdlist->data[cmdlist->last++] = G2D_DMA_HOLD_CMD;
>>>>> +	cmdlist->data[cmdlist->last++] = G2D_LIST_HOLD;
>>>>>
>>>>>     	/* Check size of cmdlist: last 2 is about G2D_BITBLT_START
> */
>>>>>     	size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2
> + 2;