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

[Inki Dae <inki.dae@samsung.com>]

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

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