[PATCH 3/4] drm/exynos/decon5433: fix vblank event handling

[Inki Dae]

Thanks for fixing it.

Andrzej,
DECON_CRFMID register is new to me. Where did you refer this register description from? I couldn't find this register in datasheet I have for Exynos5433.

Below are a little bit trivial comments.

2017년 02월 23일 01:05에 Andrzej Hajda 이(가) 쓴 글:
> Current implementation of event handling assumes that vblank interrupt is
> always called at the right time. It is not true, it can be delayed due to
> various reasons. As a result different races can happen. The patch fixes
> the issue by using hardware frame counter present in DECON to serialize
> vblank and commit completion events.
> 
> Signed-off-by: Andrzej Hajda <a.hajda at samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 62 +++++++++++++++++++++++++--
>  include/video/exynos5433_decon.h              |  9 ++++
>  2 files changed, 67 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> index 147911e..bfa9396 100644
> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> @@ -68,6 +68,8 @@ struct decon_context {
>  	unsigned long			flags;
>  	unsigned long			out_type;
>  	int				first_win;
> +	spinlock_t			vblank_lock;
> +	u32				frame_id;
>  };
>  
>  static const uint32_t decon_formats[] = {
> @@ -365,25 +367,32 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
>  static void decon_atomic_flush(struct exynos_drm_crtc *crtc)
>  {
>  	struct decon_context *ctx = crtc->ctx;
> +	unsigned long flags;
>  	int i;
>  
>  	if (test_bit(BIT_SUSPENDED, &ctx->flags))
>  		return;
>  
> +	spin_lock_irqsave(&ctx->vblank_lock, flags);
> +
>  	for (i = ctx->first_win; i < WINDOWS_NR; i++)
>  		decon_shadow_protect_win(ctx, i, false);
>  
> +	if (ctx->out_type & IFTYPE_I80)
> +		set_bit(BIT_WIN_UPDATED, &ctx->flags);
> +
>  	if (test_and_clear_bit(BIT_REQUEST_UPDATE, &ctx->flags))
>  		decon_set_bits(ctx, DECON_UPDATE, STANDALONE_UPDATE_F, ~0);
>  
> -	if (ctx->out_type & IFTYPE_I80)
> -		set_bit(BIT_WIN_UPDATED, &ctx->flags);
> -	exynos_crtc_handle_event(crtc);
> +	exynos_crtc_handle_event(ctx->crtc);

You don't have to change 'crtc' to 'ctx->crtc'. Keep 'crtc'.

> +
> +	spin_unlock_irqrestore(&ctx->vblank_lock, flags);
>  }
>  
>  static void decon_swreset(struct decon_context *ctx)
>  {
>  	unsigned int tries;
> +	unsigned long flags;
>  
>  	writel(0, ctx->addr + DECON_VIDCON0);
>  	for (tries = 2000; tries; --tries) {
> @@ -401,6 +410,10 @@ static void decon_swreset(struct decon_context *ctx)
>  
>  	WARN(tries == 0, "failed to software reset DECON\n");
>  
> +	spin_lock_irqsave(&ctx->vblank_lock, flags);
> +	ctx->frame_id = 0;
> +	spin_unlock_irqrestore(&ctx->vblank_lock, flags);
> +
>  	if (!(ctx->out_type & IFTYPE_HDMI))
>  		return;
>  
> @@ -579,6 +592,46 @@ static const struct component_ops decon_component_ops = {
>  	.unbind = decon_unbind,
>  };
>  
> +static void decon_handle_vblank(struct decon_context *ctx)
> +{
> +	u32 frm, pfrm, status, cnt;
> +
> +	spin_lock(&ctx->vblank_lock);
> +
> +	/* To get consistent result repeat read until frame id is stable. */
> +	frm = readl(ctx->addr + DECON_CRFMID);
> +	cnt = 3;

Is there some guide that initial value of cnt should be 3?

> +	do {
> +		status = readl(ctx->addr + DECON_VIDCON1);
> +		pfrm = frm;
> +		frm = readl(ctx->addr + DECON_CRFMID);
> +	} while (frm != pfrm && --cnt);
> +
> +	status &= VIDCON1_VSTATUS_MASK | VIDCON1_I80_ACTIVE;

I couldn't find I80_ACTIVE field on DECON_VIDCON1 register descrption. Do you have other datasheet, new one?

> +
> +	/* In case of delayed vblank CRFMID could be already incremented,
> +	 * it should be taken into account.
> +	 */
> +	if (frm > 0)
> +		switch (status) {
> +		case VIDCON1_VSTATUS_VS:
> +			if (ctx->out_type & IFTYPE_I80)
> +				break;
> +		case VIDCON1_I80_ACTIVE:
> +		case VIDCON1_VSTATUS_BP:
> +		case VIDCON1_VSTATUS_AC:
> +			--frm;

Let's add 'break;' and 'default: break;' explicitly.

> +		}
> +
> +	if (frm != ctx->frame_id) {
> +		if (frm > ctx->frame_id)
> +			drm_crtc_handle_vblank(&ctx->crtc->base);
> +		ctx->frame_id = frm;
> +	}
> +
> +	spin_unlock(&ctx->vblank_lock);
> +}
> +
>  static irqreturn_t decon_irq_handler(int irq, void *dev_id)
>  {
>  	struct decon_context *ctx = dev_id;
> @@ -599,7 +652,7 @@ static irqreturn_t decon_irq_handler(int irq, void *dev_id)
>  			    (VIDOUT_INTERLACE_EN_F | VIDOUT_INTERLACE_FIELD_F))
>  				return IRQ_HANDLED;
>  		}
> -		drm_crtc_handle_vblank(&ctx->crtc->base);
> +		decon_handle_vblank(ctx);
>  	}
>  
>  out:
> @@ -672,6 +725,7 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
>  	__set_bit(BIT_SUSPENDED, &ctx->flags);
>  	ctx->dev = dev;
>  	ctx->out_type = (unsigned long)of_device_get_match_data(dev);
> +	spin_lock_init(&ctx->vblank_lock);
>  
>  	if (ctx->out_type & IFTYPE_HDMI) {
>  		ctx->first_win = 1;
> diff --git a/include/video/exynos5433_decon.h b/include/video/exynos5433_decon.h
> index ef8e2a8..beefc62 100644
> --- a/include/video/exynos5433_decon.h
> +++ b/include/video/exynos5433_decon.h
> @@ -46,6 +46,8 @@
>  #define DECON_FRAMEFIFO_STATUS		0x0524
>  #define DECON_CMU			0x1404
>  #define DECON_UPDATE			0x1410
> +#define DECON_CRFMID			0x1414
> +#define DECON_RRFRMID			0x1418

Above definition is not used in this patch.

Thanks.

>  #define DECON_UPDATE_SCHEME		0x1438
>  #define DECON_VIDCON1			0x2000
>  #define DECON_VIDCON2			0x2004
> @@ -142,6 +144,13 @@
>  #define STANDALONE_UPDATE_F		(1 << 0)
>  
>  /* DECON_VIDCON1 */
> +#define VIDCON1_LINECNT_MASK		(0x0fff << 16)
> +#define VIDCON1_I80_ACTIVE		(1 << 15)
> +#define VIDCON1_VSTATUS_MASK		(0x3 << 13)
> +#define VIDCON1_VSTATUS_VS		(0 << 13)
> +#define VIDCON1_VSTATUS_BP		(1 << 13)
> +#define VIDCON1_VSTATUS_AC		(2 << 13)
> +#define VIDCON1_VSTATUS_FP		(3 << 13)
>  #define VIDCON1_VCLK_MASK		(0x3 << 9)
>  #define VIDCON1_VCLK_RUN_VDEN_DISABLE	(0x3 << 9)
>  #define VIDCON1_VCLK_HOLD		(0x0 << 9)
>