Re: [PATCH v2] drm/exynos/mixer: fix MIXER shadow registry synchronisation code

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

19. 3. 19. 오후 10:05에 Andrzej Hajda 이(가) 쓴 글:
> MIXER on Exynos5 SoCs uses different synchronisation method than Exynos4
> to update internal state (shadow registers).
> Apparently the driver implements it incorrectly. The rule should be
> as follows:
> - do not request updating registers until previous request was finished,
>   ie. MXR_CFG_LAYER_UPDATE_COUNT must be 0.
> - before setting registers synchronisation on VSYNC should be turned off,
>   ie. MXR_STATUS_SYNC_ENABLE should be reset,
> - after finishing MXR_STATUS_SYNC_ENABLE should be set again.
> The patch hopefully implements it correctly.
> Below sample kernel log from page fault caused by the bug:
> 
> [   25.670038] exynos-sysmmu 14650000.sysmmu: 14450000.mixer: PAGE FAULT occurred at 0x2247b800
> [   25.677888] ------------[ cut here ]------------
> [   25.682164] kernel BUG at ../drivers/iommu/exynos-iommu.c:450!
> [   25.687971] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> [   25.693778] Modules linked in:
> [   25.696816] CPU: 5 PID: 1553 Comm: fb-release_test Not tainted 5.0.0-rc7-01157-g5f86b1566bdd #136
> [   25.705646] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   25.711710] PC is at exynos_sysmmu_irq+0x1c0/0x264
> [   25.716470] LR is at lock_is_held_type+0x44/0x64
> 
> v2: added missing MXR_CFG_LAYER_UPDATE bit setting in mixer_enable_sync
> 
> Reported-by: Marian Mihailescu <mihailescu2m@gmail.com>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> Hi Inki and Marian,
> 
> This is fixed version of my previous patch. The only difference is
> added missing MXR_CFG_LAYER_UPDATE setting in mixer_enable_sync.
> I hope this time it is correct. It should solve one page fault issue
> in MIXER, Marek is preparing fix for another issue (to low clock set by
> devfreq). I hope with both patches page faults will not happen anymore ;)

With this patch modetest worked well.
BTW, this change may affect Exynos4 series - which have different synchronization way to update shadow registers - so could you or someone else who has Exynos4xxx based board check it on Odroid-u3 board? I have no board. :(

Thanks,
Inki Dae

> 
> Regards
> Andrzej
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 110 +++++++++++++++-----------
>  1 file changed, 66 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 0573eab0e190..f35e4ab55b27 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -20,6 +20,7 @@
>  #include "regs-vp.h"
>  
>  #include <linux/kernel.h>
> +#include <linux/ktime.h>
>  #include <linux/spinlock.h>
>  #include <linux/wait.h>
>  #include <linux/i2c.h>
> @@ -352,15 +353,62 @@ static void mixer_cfg_vp_blend(struct mixer_context *ctx, unsigned int alpha)
>  	mixer_reg_write(ctx, MXR_VIDEO_CFG, val);
>  }
>  
> -static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
> +static bool mixer_is_synced(struct mixer_context *ctx)
>  {
> -	/* block update on vsync */
> -	mixer_reg_writemask(ctx, MXR_STATUS, enable ?
> -			MXR_STATUS_SYNC_ENABLE : 0, MXR_STATUS_SYNC_ENABLE);
> +	u32 base, shadow;
>  
> +	if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
> +	    ctx->mxr_ver == MXR_VER_128_0_0_184)
> +		return !(mixer_reg_read(ctx, MXR_CFG) &
> +			 MXR_CFG_LAYER_UPDATE_COUNT_MASK);
> +
> +	if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags) &&
> +	    vp_reg_read(ctx, VP_SHADOW_UPDATE))
> +		return false;
> +
> +	base = mixer_reg_read(ctx, MXR_CFG);
> +	shadow = mixer_reg_read(ctx, MXR_CFG_S);
> +	if (base != shadow)
> +		return false;
> +
> +	base = mixer_reg_read(ctx, MXR_GRAPHIC_BASE(0));
> +	shadow = mixer_reg_read(ctx, MXR_GRAPHIC_BASE_S(0));
> +	if (base != shadow)
> +		return false;
> +
> +	base = mixer_reg_read(ctx, MXR_GRAPHIC_BASE(1));
> +	shadow = mixer_reg_read(ctx, MXR_GRAPHIC_BASE_S(1));
> +	if (base != shadow)
> +		return false;
> +
> +	return true;
> +}
> +
> +static int mixer_wait_for_sync(struct mixer_context *ctx)
> +{
> +	ktime_t timeout = ktime_add_us(ktime_get(), 100000);
> +
> +	while (!mixer_is_synced(ctx)) {
> +		usleep_range(1000, 2000);
> +		if (ktime_compare(ktime_get(), timeout) > 0)
> +			return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
> +static void mixer_disable_sync(struct mixer_context *ctx)
> +{
> +	mixer_reg_writemask(ctx, MXR_STATUS, 0, MXR_STATUS_SYNC_ENABLE);
> +}
> +
> +static void mixer_enable_sync(struct mixer_context *ctx)
> +{
> +	if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
> +	    ctx->mxr_ver == MXR_VER_128_0_0_184)
> +		mixer_reg_writemask(ctx, MXR_CFG, ~0, MXR_CFG_LAYER_UPDATE);
> +	mixer_reg_writemask(ctx, MXR_STATUS, ~0, MXR_STATUS_SYNC_ENABLE);
>  	if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
> -		vp_reg_write(ctx, VP_SHADOW_UPDATE, enable ?
> -			VP_SHADOW_UPDATE_ENABLE : 0);
> +		vp_reg_write(ctx, VP_SHADOW_UPDATE, VP_SHADOW_UPDATE_ENABLE);
>  }
>  
>  static void mixer_cfg_scan(struct mixer_context *ctx, int width, int height)
> @@ -498,7 +546,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
>  
>  	spin_lock_irqsave(&ctx->reg_slock, flags);
>  
> -	vp_reg_write(ctx, VP_SHADOW_UPDATE, 1);
>  	/* interlace or progressive scan mode */
>  	val = (test_bit(MXR_BIT_INTERLACE, &ctx->flags) ? ~0 : 0);
>  	vp_reg_writemask(ctx, VP_MODE, val, VP_MODE_LINE_SKIP);
> @@ -553,11 +600,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
>  	vp_regs_dump(ctx);
>  }
>  
> -static void mixer_layer_update(struct mixer_context *ctx)
> -{
> -	mixer_reg_writemask(ctx, MXR_CFG, ~0, MXR_CFG_LAYER_UPDATE);
> -}
> -
>  static void mixer_graph_buffer(struct mixer_context *ctx,
>  			       struct exynos_drm_plane *plane)
>  {
> @@ -640,11 +682,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>  	mixer_cfg_layer(ctx, win, priority, true);
>  	mixer_cfg_gfx_blend(ctx, win, pixel_alpha, state->base.alpha);
>  
> -	/* layer update mandatory for mixer 16.0.33.0 */
> -	if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
> -		ctx->mxr_ver == MXR_VER_128_0_0_184)
> -		mixer_layer_update(ctx);
> -
>  	spin_unlock_irqrestore(&ctx->reg_slock, flags);
>  
>  	mixer_regs_dump(ctx);
> @@ -709,7 +746,7 @@ static void mixer_win_reset(struct mixer_context *ctx)
>  static irqreturn_t mixer_irq_handler(int irq, void *arg)
>  {
>  	struct mixer_context *ctx = arg;
> -	u32 val, base, shadow;
> +	u32 val;
>  
>  	spin_lock(&ctx->reg_slock);
>  
> @@ -723,26 +760,9 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
>  		val &= ~MXR_INT_STATUS_VSYNC;
>  
>  		/* interlace scan need to check shadow register */
> -		if (test_bit(MXR_BIT_INTERLACE, &ctx->flags)) {
> -			if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags) &&
> -			    vp_reg_read(ctx, VP_SHADOW_UPDATE))
> -				goto out;
> -
> -			base = mixer_reg_read(ctx, MXR_CFG);
> -			shadow = mixer_reg_read(ctx, MXR_CFG_S);
> -			if (base != shadow)
> -				goto out;
> -
> -			base = mixer_reg_read(ctx, MXR_GRAPHIC_BASE(0));
> -			shadow = mixer_reg_read(ctx, MXR_GRAPHIC_BASE_S(0));
> -			if (base != shadow)
> -				goto out;
> -
> -			base = mixer_reg_read(ctx, MXR_GRAPHIC_BASE(1));
> -			shadow = mixer_reg_read(ctx, MXR_GRAPHIC_BASE_S(1));
> -			if (base != shadow)
> -				goto out;
> -		}
> +		if (test_bit(MXR_BIT_INTERLACE, &ctx->flags)
> +		    && !mixer_is_synced(ctx))
> +			goto out;
>  
>  		drm_crtc_handle_vblank(&ctx->crtc->base);
>  	}
> @@ -917,12 +937,14 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
>  
>  static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
>  {
> -	struct mixer_context *mixer_ctx = crtc->ctx;
> +	struct mixer_context *ctx = crtc->ctx;
>  
> -	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
> +	if (!test_bit(MXR_BIT_POWERED, &ctx->flags))
>  		return;
>  
> -	mixer_vsync_set_update(mixer_ctx, false);
> +	if (mixer_wait_for_sync(ctx))
> +		dev_err(ctx->dev, "timeout waiting for VSYNC\n");
> +	mixer_disable_sync(ctx);
>  }
>  
>  static void mixer_update_plane(struct exynos_drm_crtc *crtc,
> @@ -964,7 +986,7 @@ static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
>  	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
>  		return;
>  
> -	mixer_vsync_set_update(mixer_ctx, true);
> +	mixer_enable_sync(mixer_ctx);
>  	exynos_crtc_handle_event(crtc);
>  }
>  
> @@ -979,7 +1001,7 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
>  
>  	exynos_drm_pipe_clk_enable(crtc, true);
>  
> -	mixer_vsync_set_update(ctx, false);
> +	mixer_disable_sync(ctx);
>  
>  	mixer_reg_writemask(ctx, MXR_STATUS, ~0, MXR_STATUS_SOFT_RESET);
>  
> @@ -992,7 +1014,7 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
>  
>  	mixer_commit(ctx);
>  
> -	mixer_vsync_set_update(ctx, true);
> +	mixer_enable_sync(ctx);
>  
>  	set_bit(MXR_BIT_POWERED, &ctx->flags);
>  }
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel