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

[YoungJun Cho <yj44.cho@samsung.com>]

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);
>>
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel