diff mbox

[3/3] drm/exynos/decon5433: fix trigger configuration

Message ID 1461937369-22994-3-git-send-email-a.hajda@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrzej Hajda April 29, 2016, 1:42 p.m. UTC
It seems trigger cannot be configured too early, otherwise it does not work in
case of panel. The patch fixes also trigger flag logic, previously HW-TRIGGER
flag was cleared in case of panel - as a result panel used always software
trigger.

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

Comments

Inki Dae May 10, 2016, 5:31 a.m. UTC | #1
Hi Andrzej,

2016? 04? 29? 22:42? Andrzej Hajda ?(?) ? ?:
> It seems trigger cannot be configured too early, otherwise it does not work in
> case of panel. The patch fixes also trigger flag logic, previously HW-TRIGGER
> flag was cleared in case of panel - as a result panel used always software

Andrzej, sorry but I can't understand what above two lines mean. Can you give me more details?

> trigger.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> index 7b4f699..9ae913b 100644
> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> @@ -151,11 +151,13 @@ static void decon_commit(struct exynos_drm_crtc *crtc)
>  	val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;
>  	writel(val, ctx->addr + DECON_CMU);
>  
> +	if (ctx->out_type & (IFTYPE_I80 | I80_HW_TRG))
> +		decon_setup_trigger(ctx);

Shouldn't it be configured in case of using SW trigger mode also?

Thanks,
Inki Dae

> +
>  	/* lcd on and use command if */
>  	val = VIDOUT_LCD_ON;
>  	if (ctx->out_type & IFTYPE_I80) {
>  		val |= VIDOUT_COMMAND_IF;
> -		decon_setup_trigger(ctx);
>  	} else {
>  		val |= VIDOUT_RGB_IF;
>  	}
> @@ -380,9 +382,6 @@ static void decon_swreset(struct decon_context *ctx)
>  	writel(VIDCON1_VCLK_RUN_VDEN_DISABLE, ctx->addr + DECON_VIDCON1);
>  	writel(CRCCTRL_CRCEN | CRCCTRL_CRCSTART_F | CRCCTRL_CRCCLKEN,
>  	       ctx->addr + DECON_CRCCTRL);
> -
> -	if (ctx->out_type & IFTYPE_I80)
> -		decon_setup_trigger(ctx);
>  }
>  
>  static void decon_enable(struct exynos_drm_crtc *crtc)
> @@ -652,9 +651,8 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
>  
>  	if (ctx->out_type & IFTYPE_HDMI) {
>  		ctx->first_win = 1;
> -		ctx->out_type = IFTYPE_I80;
>  	} else if (of_get_child_by_name(dev->of_node, "i80-if-timings")) {
> -		ctx->out_type = IFTYPE_I80;
> +		ctx->out_type |= IFTYPE_I80;
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
>
Andrzej Hajda May 10, 2016, 6:08 a.m. UTC | #2
Hi Inki,


On 05/10/2016 07:31 AM, Inki Dae wrote:
> Hi Andrzej,
>
> 2016? 04? 29? 22:42? Andrzej Hajda ?(?) ? ?:
>> It seems trigger cannot be configured too early, otherwise it does not work in
>> case of panel. The patch fixes also trigger flag logic, previously HW-TRIGGER
>> flag was cleared in case of panel - as a result panel used always software
> Andrzej, sorry but I can't understand what above two lines mean. Can you give me more details?

Details below.

>
>> trigger.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>> index 7b4f699..9ae913b 100644
>> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>> @@ -151,11 +151,13 @@ static void decon_commit(struct exynos_drm_crtc *crtc)
>>  	val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;
>>  	writel(val, ctx->addr + DECON_CMU);
>>  
>> +	if (ctx->out_type & (IFTYPE_I80 | I80_HW_TRG))
>> +		decon_setup_trigger(ctx);
> Shouldn't it be configured in case of using SW trigger mode also?
>
> Thanks,
> Inki Dae
>
>> +
>>  	/* lcd on and use command if */
>>  	val = VIDOUT_LCD_ON;
>>  	if (ctx->out_type & IFTYPE_I80) {
>>  		val |= VIDOUT_COMMAND_IF;
>> -		decon_setup_trigger(ctx);
>>  	} else {
>>  		val |= VIDOUT_RGB_IF;
>>  	}
>> @@ -380,9 +382,6 @@ static void decon_swreset(struct decon_context *ctx)
>>  	writel(VIDCON1_VCLK_RUN_VDEN_DISABLE, ctx->addr + DECON_VIDCON1);
>>  	writel(CRCCTRL_CRCEN | CRCCTRL_CRCSTART_F | CRCCTRL_CRCCLKEN,
>>  	       ctx->addr + DECON_CRCCTRL);
>> -
>> -	if (ctx->out_type & IFTYPE_I80)
>> -		decon_setup_trigger(ctx);
>>  }
>>  
>>  static void decon_enable(struct exynos_drm_crtc *crtc)
>> @@ -652,9 +651,8 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
>>  
>>  	if (ctx->out_type & IFTYPE_HDMI) {
>>  		ctx->first_win = 1;
>> -		ctx->out_type = IFTYPE_I80;
>>  	} else if (of_get_child_by_name(dev->of_node, "i80-if-timings")) {
>> -		ctx->out_type = IFTYPE_I80;
>> +		ctx->out_type |= IFTYPE_I80;

ctx->out_type was overwritten here with IFTYPE_I80. So when
decon_setup_trigger
were called I80_HW_TRG bit was always clear and DECON_TRIGCON was configured
to use soft trigger.

Regards
Andrzej

>>  	}
>>  
>>  	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
>>
Inki Dae May 10, 2016, 6:24 a.m. UTC | #3
Hi Andrzej,

2016? 05? 10? 15:08? Andrzej Hajda ?(?) ? ?:
> Hi Inki,
> 
> 
> On 05/10/2016 07:31 AM, Inki Dae wrote:
>> Hi Andrzej,
>>
>> 2016? 04? 29? 22:42? Andrzej Hajda ?(?) ? ?:
>>> It seems trigger cannot be configured too early, otherwise it does not work in
>>> case of panel. The patch fixes also trigger flag logic, previously HW-TRIGGER
>>> flag was cleared in case of panel - as a result panel used always software
>> Andrzej, sorry but I can't understand what above two lines mean. Can you give me more details?
> 
> Details below.
> 
>>
>>> trigger.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 ++++------
>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>> index 7b4f699..9ae913b 100644
>>> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>> @@ -151,11 +151,13 @@ static void decon_commit(struct exynos_drm_crtc *crtc)
>>>  	val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;
>>>  	writel(val, ctx->addr + DECON_CMU);
>>>  
>>> +	if (ctx->out_type & (IFTYPE_I80 | I80_HW_TRG))
>>> +		decon_setup_trigger(ctx);
>> Shouldn't it be configured in case of using SW trigger mode also?
>>
>> Thanks,
>> Inki Dae
>>
>>> +
>>>  	/* lcd on and use command if */
>>>  	val = VIDOUT_LCD_ON;
>>>  	if (ctx->out_type & IFTYPE_I80) {
>>>  		val |= VIDOUT_COMMAND_IF;
>>> -		decon_setup_trigger(ctx);
>>>  	} else {
>>>  		val |= VIDOUT_RGB_IF;
>>>  	}
>>> @@ -380,9 +382,6 @@ static void decon_swreset(struct decon_context *ctx)
>>>  	writel(VIDCON1_VCLK_RUN_VDEN_DISABLE, ctx->addr + DECON_VIDCON1);
>>>  	writel(CRCCTRL_CRCEN | CRCCTRL_CRCSTART_F | CRCCTRL_CRCCLKEN,
>>>  	       ctx->addr + DECON_CRCCTRL);
>>> -
>>> -	if (ctx->out_type & IFTYPE_I80)
>>> -		decon_setup_trigger(ctx);
>>>  }
>>>  
>>>  static void decon_enable(struct exynos_drm_crtc *crtc)
>>> @@ -652,9 +651,8 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
>>>  
>>>  	if (ctx->out_type & IFTYPE_HDMI) {
>>>  		ctx->first_win = 1;
>>> -		ctx->out_type = IFTYPE_I80;
>>>  	} else if (of_get_child_by_name(dev->of_node, "i80-if-timings")) {
>>> -		ctx->out_type = IFTYPE_I80;
>>> +		ctx->out_type |= IFTYPE_I80;
> 
> ctx->out_type was overwritten here with IFTYPE_I80. So when
> decon_setup_trigger
> were called I80_HW_TRG bit was always clear and DECON_TRIGCON was configured
> to use soft trigger.

Indeed. Then shouldn't decon_setup_trigger function be called in both cases - SW and HW trigger modes?
Is there any reason to call the function only in case of HW trigger mode?

Thanks,
Inki Dae

> 
> Regards
> Andrzej
> 
>>>  	}
>>>  
>>>  	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
>>>
> 
>
Andrzej Hajda May 10, 2016, 7:08 a.m. UTC | #4
On 05/10/2016 08:24 AM, Inki Dae wrote:
> Hi Andrzej,
>
> 2016? 05? 10? 15:08? Andrzej Hajda ?(?) ? ?:
>> Hi Inki,
>>
>>
>> On 05/10/2016 07:31 AM, Inki Dae wrote:
>>> Hi Andrzej,
>>>
>>> 2016? 04? 29? 22:42? Andrzej Hajda ?(?) ? ?:
>>>> It seems trigger cannot be configured too early, otherwise it does not work in
>>>> case of panel. The patch fixes also trigger flag logic, previously HW-TRIGGER
>>>> flag was cleared in case of panel - as a result panel used always software
>>> Andrzej, sorry but I can't understand what above two lines mean. Can you give me more details?
>> Details below.
>>
>>>> trigger.
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 ++++------
>>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>>> index 7b4f699..9ae913b 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>>> @@ -151,11 +151,13 @@ static void decon_commit(struct exynos_drm_crtc *crtc)
>>>>  	val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;
>>>>  	writel(val, ctx->addr + DECON_CMU);
>>>>  
>>>> +	if (ctx->out_type & (IFTYPE_I80 | I80_HW_TRG))
>>>> +		decon_setup_trigger(ctx);
>>> Shouldn't it be configured in case of using SW trigger mode also?
>>>
>>> Thanks,
>>> Inki Dae
>>>
>>>> +
>>>>  	/* lcd on and use command if */
>>>>  	val = VIDOUT_LCD_ON;
>>>>  	if (ctx->out_type & IFTYPE_I80) {
>>>>  		val |= VIDOUT_COMMAND_IF;
>>>> -		decon_setup_trigger(ctx);
>>>>  	} else {
>>>>  		val |= VIDOUT_RGB_IF;
>>>>  	}
>>>> @@ -380,9 +382,6 @@ static void decon_swreset(struct decon_context *ctx)
>>>>  	writel(VIDCON1_VCLK_RUN_VDEN_DISABLE, ctx->addr + DECON_VIDCON1);
>>>>  	writel(CRCCTRL_CRCEN | CRCCTRL_CRCSTART_F | CRCCTRL_CRCCLKEN,
>>>>  	       ctx->addr + DECON_CRCCTRL);
>>>> -
>>>> -	if (ctx->out_type & IFTYPE_I80)
>>>> -		decon_setup_trigger(ctx);
>>>>  }
>>>>  
>>>>  static void decon_enable(struct exynos_drm_crtc *crtc)
>>>> @@ -652,9 +651,8 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
>>>>  
>>>>  	if (ctx->out_type & IFTYPE_HDMI) {
>>>>  		ctx->first_win = 1;
>>>> -		ctx->out_type = IFTYPE_I80;
>>>>  	} else if (of_get_child_by_name(dev->of_node, "i80-if-timings")) {
>>>> -		ctx->out_type = IFTYPE_I80;
>>>> +		ctx->out_type |= IFTYPE_I80;
>> ctx->out_type was overwritten here with IFTYPE_I80. So when
>> decon_setup_trigger
>> were called I80_HW_TRG bit was always clear and DECON_TRIGCON was configured
>> to use soft trigger.
> Indeed. Then shouldn't decon_setup_trigger function be called in both cases - SW and HW trigger modes?
> Is there any reason to call the function only in case of HW trigger mode?

With this patch function is called under following condition:
> +	if (ctx->out_type & (IFTYPE_I80 | I80_HW_TRG))
> +		decon_setup_trigger(ctx);

So it is called always in case of I80 mode, in such case value of I80_HW_TRG
determines if it should use sw or hw trigger.

Regards
Andrzej


>
> Thanks,
> Inki Dae
>
>> Regards
>> Andrzej
>>
>>>>  	}
>>>>  
>>>>  	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
>>>>
>>
Inki Dae May 10, 2016, 7:38 a.m. UTC | #5
2016? 05? 10? 16:08? Andrzej Hajda ?(?) ? ?:
> On 05/10/2016 08:24 AM, Inki Dae wrote:
>> Hi Andrzej,
>>
>> 2016? 05? 10? 15:08? Andrzej Hajda ?(?) ? ?:
>>> Hi Inki,
>>>
>>>
>>> On 05/10/2016 07:31 AM, Inki Dae wrote:
>>>> Hi Andrzej,
>>>>
>>>> 2016? 04? 29? 22:42? Andrzej Hajda ?(?) ? ?:
>>>>> It seems trigger cannot be configured too early, otherwise it does not work in
>>>>> case of panel. The patch fixes also trigger flag logic, previously HW-TRIGGER
>>>>> flag was cleared in case of panel - as a result panel used always software
>>>> Andrzej, sorry but I can't understand what above two lines mean. Can you give me more details?
>>> Details below.
>>>
>>>>> trigger.
>>>>>
>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>> ---
>>>>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 ++++------
>>>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>>>> index 7b4f699..9ae913b 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>>>> @@ -151,11 +151,13 @@ static void decon_commit(struct exynos_drm_crtc *crtc)
>>>>>  	val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;
>>>>>  	writel(val, ctx->addr + DECON_CMU);
>>>>>  
>>>>> +	if (ctx->out_type & (IFTYPE_I80 | I80_HW_TRG))
>>>>> +		decon_setup_trigger(ctx);
>>>> Shouldn't it be configured in case of using SW trigger mode also?
>>>>
>>>> Thanks,
>>>> Inki Dae
>>>>
>>>>> +
>>>>>  	/* lcd on and use command if */
>>>>>  	val = VIDOUT_LCD_ON;
>>>>>  	if (ctx->out_type & IFTYPE_I80) {
>>>>>  		val |= VIDOUT_COMMAND_IF;
>>>>> -		decon_setup_trigger(ctx);
>>>>>  	} else {
>>>>>  		val |= VIDOUT_RGB_IF;
>>>>>  	}
>>>>> @@ -380,9 +382,6 @@ static void decon_swreset(struct decon_context *ctx)
>>>>>  	writel(VIDCON1_VCLK_RUN_VDEN_DISABLE, ctx->addr + DECON_VIDCON1);
>>>>>  	writel(CRCCTRL_CRCEN | CRCCTRL_CRCSTART_F | CRCCTRL_CRCCLKEN,
>>>>>  	       ctx->addr + DECON_CRCCTRL);
>>>>> -
>>>>> -	if (ctx->out_type & IFTYPE_I80)
>>>>> -		decon_setup_trigger(ctx);
>>>>>  }
>>>>>  
>>>>>  static void decon_enable(struct exynos_drm_crtc *crtc)
>>>>> @@ -652,9 +651,8 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
>>>>>  
>>>>>  	if (ctx->out_type & IFTYPE_HDMI) {
>>>>>  		ctx->first_win = 1;
>>>>> -		ctx->out_type = IFTYPE_I80;
>>>>>  	} else if (of_get_child_by_name(dev->of_node, "i80-if-timings")) {
>>>>> -		ctx->out_type = IFTYPE_I80;
>>>>> +		ctx->out_type |= IFTYPE_I80;
>>> ctx->out_type was overwritten here with IFTYPE_I80. So when
>>> decon_setup_trigger
>>> were called I80_HW_TRG bit was always clear and DECON_TRIGCON was configured
>>> to use soft trigger.
>> Indeed. Then shouldn't decon_setup_trigger function be called in both cases - SW and HW trigger modes?
>> Is there any reason to call the function only in case of HW trigger mode?
> 
> With this patch function is called under following condition:
>> +	if (ctx->out_type & (IFTYPE_I80 | I80_HW_TRG))
>> +		decon_setup_trigger(ctx);
> 
> So it is called always in case of I80 mode, in such case value of I80_HW_TRG
> determines if it should use sw or hw trigger.

Ah, sorry. I misleaded above condition.

Thanks,
Inki Dae

> 
> Regards
> Andrzej
> 
> 
>>
>> Thanks,
>> Inki Dae
>>
>>> Regards
>>> Andrzej
>>>
>>>>>  	}
>>>>>  
>>>>>  	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
>>>>>
>>>
> 
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index 7b4f699..9ae913b 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -151,11 +151,13 @@  static void decon_commit(struct exynos_drm_crtc *crtc)
 	val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;
 	writel(val, ctx->addr + DECON_CMU);
 
+	if (ctx->out_type & (IFTYPE_I80 | I80_HW_TRG))
+		decon_setup_trigger(ctx);
+
 	/* lcd on and use command if */
 	val = VIDOUT_LCD_ON;
 	if (ctx->out_type & IFTYPE_I80) {
 		val |= VIDOUT_COMMAND_IF;
-		decon_setup_trigger(ctx);
 	} else {
 		val |= VIDOUT_RGB_IF;
 	}
@@ -380,9 +382,6 @@  static void decon_swreset(struct decon_context *ctx)
 	writel(VIDCON1_VCLK_RUN_VDEN_DISABLE, ctx->addr + DECON_VIDCON1);
 	writel(CRCCTRL_CRCEN | CRCCTRL_CRCSTART_F | CRCCTRL_CRCCLKEN,
 	       ctx->addr + DECON_CRCCTRL);
-
-	if (ctx->out_type & IFTYPE_I80)
-		decon_setup_trigger(ctx);
 }
 
 static void decon_enable(struct exynos_drm_crtc *crtc)
@@ -652,9 +651,8 @@  static int exynos5433_decon_probe(struct platform_device *pdev)
 
 	if (ctx->out_type & IFTYPE_HDMI) {
 		ctx->first_win = 1;
-		ctx->out_type = IFTYPE_I80;
 	} else if (of_get_child_by_name(dev->of_node, "i80-if-timings")) {
-		ctx->out_type = IFTYPE_I80;
+		ctx->out_type |= IFTYPE_I80;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {