diff mbox

[5/7] drm/exynos: fimd: modify I80 i/f interrupt relevant routine

Message ID 1412144353-13114-6-git-send-email-yj44.cho@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

YoungJun Cho Oct. 1, 2014, 6:19 a.m. UTC 
For the I80 interface, the video interrupt pending register(VIDINTCON1)
should be handled in fimd_irq_handler() and the video interrupt control
register(VIDINTCON0) should be handled in fimd_enable_vblank() and
fimd_disable_vblank() like RGB interface.
So this patch moves each set / unset routines into proper positions.
And adds triggering unset routine in fimd_trigger() to exit from it
because there is a case like set config which requires triggering
but vblank is not enabled.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 60 ++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 26 deletions(-)

Comments

Inki Dae Nov. 14, 2014, 1:36 a.m. UTC | #1 
On 2014? 10? 01? 15:19, YoungJun Cho wrote:
> For the I80 interface, the video interrupt pending register(VIDINTCON1)
> should be handled in fimd_irq_handler() and the video interrupt control
> register(VIDINTCON0) should be handled in fimd_enable_vblank() and
> fimd_disable_vblank() like RGB interface.
> So this patch moves each set / unset routines into proper positions.
> And adds triggering unset routine in fimd_trigger() to exit from it
> because there is a case like set config which requires triggering
> but vblank is not enabled.

Reasonable but how about separating this patch into two patches. One is
set/unset properly the registers relevant to interrupt, and other?

It seems that your patch includes some codes not relevant to above
description. And below is my comment.

Thanks,
Inki Dae

> 
> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
> Acked-by: Inki Dae <inki.dae@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 60 ++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index f062335..c949099 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -463,12 +463,19 @@ static int fimd_enable_vblank(struct exynos_drm_manager *mgr)
>  		val = readl(ctx->regs + VIDINTCON0);
>  
>  		val |= VIDINTCON0_INT_ENABLE;
> -		val |= VIDINTCON0_INT_FRAME;
>  
> -		val &= ~VIDINTCON0_FRAMESEL0_MASK;
> -		val |= VIDINTCON0_FRAMESEL0_VSYNC;
> -		val &= ~VIDINTCON0_FRAMESEL1_MASK;
> -		val |= VIDINTCON0_FRAMESEL1_NONE;
> +		if (ctx->i80_if) {
> +			val |= VIDINTCON0_INT_I80IFDONE;
> +			val |= VIDINTCON0_INT_SYSMAINCON;
> +			val &= ~VIDINTCON0_INT_SYSSUBCON;
> +		} else {
> +			val |= VIDINTCON0_INT_FRAME;
> +
> +			val &= ~VIDINTCON0_FRAMESEL0_MASK;
> +			val |= VIDINTCON0_FRAMESEL0_VSYNC;
> +			val &= ~VIDINTCON0_FRAMESEL1_MASK;
> +			val |= VIDINTCON0_FRAMESEL1_NONE;
> +		}
>  
>  		writel(val, ctx->regs + VIDINTCON0);
>  	}
> @@ -487,9 +494,15 @@ static void fimd_disable_vblank(struct exynos_drm_manager *mgr)
>  	if (test_and_clear_bit(0, &ctx->irq_flags)) {
>  		val = readl(ctx->regs + VIDINTCON0);
>  
> -		val &= ~VIDINTCON0_INT_FRAME;
>  		val &= ~VIDINTCON0_INT_ENABLE;
>  
> +		if (ctx->i80_if) {
> +			val &= ~VIDINTCON0_INT_I80IFDONE;
> +			val &= ~VIDINTCON0_INT_SYSMAINCON;
> +			val &= ~VIDINTCON0_INT_SYSSUBCON;
> +		} else
> +			val &= ~VIDINTCON0_INT_FRAME;
> +
>  		writel(val, ctx->regs + VIDINTCON0);
>  	}
>  }
> @@ -945,16 +958,19 @@ static void fimd_trigger(struct device *dev)
>  	void *timing_base = ctx->regs + driver_data->timing_base;
>  	u32 reg;
>  
> +	/* Enters triggering mode */
>  	atomic_set(&ctx->triggering, 1);
>  
> -	reg = readl(ctx->regs + VIDINTCON0);
> -	reg |= (VIDINTCON0_INT_ENABLE | VIDINTCON0_INT_I80IFDONE |
> -						VIDINTCON0_INT_SYSMAINCON);
> -	writel(reg, ctx->regs + VIDINTCON0);
> -
>  	reg = readl(timing_base + TRIGCON);
>  	reg |= (TRGMODE_I80_RGB_ENABLE_I80 | SWTRGCMD_I80_RGB_ENABLE);
>  	writel(reg, timing_base + TRIGCON);
> +
> +	/*
> +	 * Exits triggering mode if vblank is not enabled yet, because when the
> +	 * VIDINTCON0 register is not set, it can not exit from triggering mode.
> +	 */
> +	if (!test_bit(0, &ctx->irq_flags))
> +		atomic_set(&ctx->triggering, 0);

I think this code would make for other trigger requested while triggering.

>  }
>  
>  static void fimd_te_handler(struct exynos_drm_manager *mgr)
> @@ -966,9 +982,9 @@ static void fimd_te_handler(struct exynos_drm_manager *mgr)
>  		return;
>  
>  	 /*
> -	 * Skips to trigger if in triggering state, because multiple triggering
> -	 * requests can cause panel reset.
> -	 */
> +	  * Skips triggering if in triggering mode, because multiple triggering
> +	  * requests can cause panel reset.
> +	  */
>  	if (atomic_read(&ctx->triggering))
>  		return;
>  
> @@ -1023,21 +1039,13 @@ static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
>  	if (ctx->pipe < 0 || !ctx->drm_dev)
>  		goto out;
>  
> -	if (ctx->i80_if) {
> -		/* unset I80 frame done interrupt */
> -		val = readl(ctx->regs + VIDINTCON0);
> -		val &= ~(VIDINTCON0_INT_I80IFDONE | VIDINTCON0_INT_SYSMAINCON);
> -		writel(val, ctx->regs + VIDINTCON0);
> +	drm_handle_vblank(ctx->drm_dev, ctx->pipe);
> +	exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
>  
> -		/* exit triggering mode */
> +	if (ctx->i80_if) {
> +		/* Exits triggering mode */
>  		atomic_set(&ctx->triggering, 0);
> -
> -		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
> -		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
>  	} else {
> -		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
> -		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
> -
>  		/* set wait vsync event to zero and wake up queue. */
>  		if (atomic_read(&ctx->wait_vsync_event)) {
>  			atomic_set(&ctx->wait_vsync_event, 0);
>
YoungJun Cho Nov. 16, 2014, 12:53 a.m. UTC | #2 
Hi Inki,

On 11/14/2014 10:36 AM, Inki Dae wrote:
> On 2014? 10? 01? 15:19, YoungJun Cho wrote:
>> For the I80 interface, the video interrupt pending register(VIDINTCON1)
>> should be handled in fimd_irq_handler() and the video interrupt control
>> register(VIDINTCON0) should be handled in fimd_enable_vblank() and
>> fimd_disable_vblank() like RGB interface.
>> So this patch moves each set / unset routines into proper positions.
>> And adds triggering unset routine in fimd_trigger() to exit from it
>> because there is a case like set config which requires triggering
>> but vblank is not enabled.
>
> Reasonable but how about separating this patch into two patches. One is
> set/unset properly the registers relevant to interrupt, and other?
>
> It seems that your patch includes some codes not relevant to above
> description. And below is my comment.
>
> Thanks,
> Inki Dae
>
>>
>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>> Acked-by: Inki Dae <inki.dae@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_fimd.c | 60 ++++++++++++++++++--------------
>>   1 file changed, 34 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index f062335..c949099 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -463,12 +463,19 @@ static int fimd_enable_vblank(struct exynos_drm_manager *mgr)
>>   		val = readl(ctx->regs + VIDINTCON0);
>>
>>   		val |= VIDINTCON0_INT_ENABLE;
>> -		val |= VIDINTCON0_INT_FRAME;
>>
>> -		val &= ~VIDINTCON0_FRAMESEL0_MASK;
>> -		val |= VIDINTCON0_FRAMESEL0_VSYNC;
>> -		val &= ~VIDINTCON0_FRAMESEL1_MASK;
>> -		val |= VIDINTCON0_FRAMESEL1_NONE;
>> +		if (ctx->i80_if) {
>> +			val |= VIDINTCON0_INT_I80IFDONE;
>> +			val |= VIDINTCON0_INT_SYSMAINCON;
>> +			val &= ~VIDINTCON0_INT_SYSSUBCON;
>> +		} else {
>> +			val |= VIDINTCON0_INT_FRAME;
>> +
>> +			val &= ~VIDINTCON0_FRAMESEL0_MASK;
>> +			val |= VIDINTCON0_FRAMESEL0_VSYNC;
>> +			val &= ~VIDINTCON0_FRAMESEL1_MASK;
>> +			val |= VIDINTCON0_FRAMESEL1_NONE;
>> +		}
>>
>>   		writel(val, ctx->regs + VIDINTCON0);
>>   	}
>> @@ -487,9 +494,15 @@ static void fimd_disable_vblank(struct exynos_drm_manager *mgr)
>>   	if (test_and_clear_bit(0, &ctx->irq_flags)) {
>>   		val = readl(ctx->regs + VIDINTCON0);
>>
>> -		val &= ~VIDINTCON0_INT_FRAME;
>>   		val &= ~VIDINTCON0_INT_ENABLE;
>>
>> +		if (ctx->i80_if) {
>> +			val &= ~VIDINTCON0_INT_I80IFDONE;
>> +			val &= ~VIDINTCON0_INT_SYSMAINCON;
>> +			val &= ~VIDINTCON0_INT_SYSSUBCON;
>> +		} else
>> +			val &= ~VIDINTCON0_INT_FRAME;
>> +
>>   		writel(val, ctx->regs + VIDINTCON0);
>>   	}
>>   }
>> @@ -945,16 +958,19 @@ static void fimd_trigger(struct device *dev)
>>   	void *timing_base = ctx->regs + driver_data->timing_base;
>>   	u32 reg;
>>
>> +	/* Enters triggering mode */
>>   	atomic_set(&ctx->triggering, 1);
>>
>> -	reg = readl(ctx->regs + VIDINTCON0);
>> -	reg |= (VIDINTCON0_INT_ENABLE | VIDINTCON0_INT_I80IFDONE |
>> -						VIDINTCON0_INT_SYSMAINCON);
>> -	writel(reg, ctx->regs + VIDINTCON0);
>> -
>>   	reg = readl(timing_base + TRIGCON);
>>   	reg |= (TRGMODE_I80_RGB_ENABLE_I80 | SWTRGCMD_I80_RGB_ENABLE);
>>   	writel(reg, timing_base + TRIGCON);
>> +
>> +	/*
>> +	 * Exits triggering mode if vblank is not enabled yet, because when the
>> +	 * VIDINTCON0 register is not set, it can not exit from triggering mode.
>> +	 */
>> +	if (!test_bit(0, &ctx->irq_flags))
>> +		atomic_set(&ctx->triggering, 0);
>
> I think this code would make for other trigger requested while triggering.
>

I missed this comment.

After modifying this patch, the I80 interface works with vblank enable / 
disable.
And there is a case like set config which requires triggering to update 
frame buffer but vblank is not enabled yet. So this exception routine is 
required to escape from triggering mode.

The connector DPMS is off earlier than CRTC DPMS. So I think the TE 
interrupt is stopped earlier than vblank is disabled.

Thank you.
Best regards YJ

>>   }
>>
>>   static void fimd_te_handler(struct exynos_drm_manager *mgr)
>> @@ -966,9 +982,9 @@ static void fimd_te_handler(struct exynos_drm_manager *mgr)
>>   		return;
>>
>>   	 /*
>> -	 * Skips to trigger if in triggering state, because multiple triggering
>> -	 * requests can cause panel reset.
>> -	 */
>> +	  * Skips triggering if in triggering mode, because multiple triggering
>> +	  * requests can cause panel reset.
>> +	  */
>>   	if (atomic_read(&ctx->triggering))
>>   		return;
>>
>> @@ -1023,21 +1039,13 @@ static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
>>   	if (ctx->pipe < 0 || !ctx->drm_dev)
>>   		goto out;
>>
>> -	if (ctx->i80_if) {
>> -		/* unset I80 frame done interrupt */
>> -		val = readl(ctx->regs + VIDINTCON0);
>> -		val &= ~(VIDINTCON0_INT_I80IFDONE | VIDINTCON0_INT_SYSMAINCON);
>> -		writel(val, ctx->regs + VIDINTCON0);
>> +	drm_handle_vblank(ctx->drm_dev, ctx->pipe);
>> +	exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
>>
>> -		/* exit triggering mode */
>> +	if (ctx->i80_if) {
>> +		/* Exits triggering mode */
>>   		atomic_set(&ctx->triggering, 0);
>> -
>> -		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
>> -		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
>>   	} else {
>> -		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
>> -		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
>> -
>>   		/* set wait vsync event to zero and wake up queue. */
>>   		if (atomic_read(&ctx->wait_vsync_event)) {
>>   			atomic_set(&ctx->wait_vsync_event, 0);
>>
>
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index f062335..c949099 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -463,12 +463,19 @@  static int fimd_enable_vblank(struct exynos_drm_manager *mgr)
 		val = readl(ctx->regs + VIDINTCON0);
 
 		val |= VIDINTCON0_INT_ENABLE;
-		val |= VIDINTCON0_INT_FRAME;
 
-		val &= ~VIDINTCON0_FRAMESEL0_MASK;
-		val |= VIDINTCON0_FRAMESEL0_VSYNC;
-		val &= ~VIDINTCON0_FRAMESEL1_MASK;
-		val |= VIDINTCON0_FRAMESEL1_NONE;
+		if (ctx->i80_if) {
+			val |= VIDINTCON0_INT_I80IFDONE;
+			val |= VIDINTCON0_INT_SYSMAINCON;
+			val &= ~VIDINTCON0_INT_SYSSUBCON;
+		} else {
+			val |= VIDINTCON0_INT_FRAME;
+
+			val &= ~VIDINTCON0_FRAMESEL0_MASK;
+			val |= VIDINTCON0_FRAMESEL0_VSYNC;
+			val &= ~VIDINTCON0_FRAMESEL1_MASK;
+			val |= VIDINTCON0_FRAMESEL1_NONE;
+		}
 
 		writel(val, ctx->regs + VIDINTCON0);
 	}
@@ -487,9 +494,15 @@  static void fimd_disable_vblank(struct exynos_drm_manager *mgr)
 	if (test_and_clear_bit(0, &ctx->irq_flags)) {
 		val = readl(ctx->regs + VIDINTCON0);
 
-		val &= ~VIDINTCON0_INT_FRAME;
 		val &= ~VIDINTCON0_INT_ENABLE;
 
+		if (ctx->i80_if) {
+			val &= ~VIDINTCON0_INT_I80IFDONE;
+			val &= ~VIDINTCON0_INT_SYSMAINCON;
+			val &= ~VIDINTCON0_INT_SYSSUBCON;
+		} else
+			val &= ~VIDINTCON0_INT_FRAME;
+
 		writel(val, ctx->regs + VIDINTCON0);
 	}
 }
@@ -945,16 +958,19 @@  static void fimd_trigger(struct device *dev)
 	void *timing_base = ctx->regs + driver_data->timing_base;
 	u32 reg;
 
+	/* Enters triggering mode */
 	atomic_set(&ctx->triggering, 1);
 
-	reg = readl(ctx->regs + VIDINTCON0);
-	reg |= (VIDINTCON0_INT_ENABLE | VIDINTCON0_INT_I80IFDONE |
-						VIDINTCON0_INT_SYSMAINCON);
-	writel(reg, ctx->regs + VIDINTCON0);
-
 	reg = readl(timing_base + TRIGCON);
 	reg |= (TRGMODE_I80_RGB_ENABLE_I80 | SWTRGCMD_I80_RGB_ENABLE);
 	writel(reg, timing_base + TRIGCON);
+
+	/*
+	 * Exits triggering mode if vblank is not enabled yet, because when the
+	 * VIDINTCON0 register is not set, it can not exit from triggering mode.
+	 */
+	if (!test_bit(0, &ctx->irq_flags))
+		atomic_set(&ctx->triggering, 0);
 }
 
 static void fimd_te_handler(struct exynos_drm_manager *mgr)
@@ -966,9 +982,9 @@  static void fimd_te_handler(struct exynos_drm_manager *mgr)
 		return;
 
 	 /*
-	 * Skips to trigger if in triggering state, because multiple triggering
-	 * requests can cause panel reset.
-	 */
+	  * Skips triggering if in triggering mode, because multiple triggering
+	  * requests can cause panel reset.
+	  */
 	if (atomic_read(&ctx->triggering))
 		return;
 
@@ -1023,21 +1039,13 @@  static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
 	if (ctx->pipe < 0 || !ctx->drm_dev)
 		goto out;
 
-	if (ctx->i80_if) {
-		/* unset I80 frame done interrupt */
-		val = readl(ctx->regs + VIDINTCON0);
-		val &= ~(VIDINTCON0_INT_I80IFDONE | VIDINTCON0_INT_SYSMAINCON);
-		writel(val, ctx->regs + VIDINTCON0);
+	drm_handle_vblank(ctx->drm_dev, ctx->pipe);
+	exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
 
-		/* exit triggering mode */
+	if (ctx->i80_if) {
+		/* Exits triggering mode */
 		atomic_set(&ctx->triggering, 0);
-
-		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
-		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
 	} else {
-		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
-		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
-
 		/* set wait vsync event to zero and wake up queue. */
 		if (atomic_read(&ctx->wait_vsync_event)) {
 			atomic_set(&ctx->wait_vsync_event, 0);