| Message ID | 20190306150559.20987-1-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/exynos/mixer: fix MIXER shadow registry synchronisation code | expand |
Tested on my Exynos5422 Odroid XU4, vsync seems to not happen at all, screen remains black since kernel loads. -- Either I've been missing something or nothing has been going on. (K. E. Gordon) On Thu, Mar 7, 2019 at 1:36 AM Andrzej Hajda <a.hajda@samsung.com> wrote: > > 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 > > Reported-by: Marian Mihailescu <mihailescu2m@gmail.com> > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 107 +++++++++++++++----------- > 1 file changed, 63 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > index 0573eab0e190..42ce01c226ef 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,59 @@ 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) > +{ > + 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 +543,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 +597,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 +679,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 +743,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 +757,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 +934,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 +983,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 +998,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 +1011,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); > } > -- > 2.17.1 >
19. 3. 8. 오전 9:12에 Marian Mihailescu 이(가) 쓴 글: > Tested on my Exynos5422 Odroid XU4, vsync seems to not happen at all,> screen remains black since kernel loads. I faced with same issue. Andrzej, Did you test this patch and worked correctly? I had tested this patch on mainline kernel - linux-5.1.0-rc1 - and even on Tizen kernel (tizen-next branch) but screen remained blank after running modetest app. Without this patch, modetest app worked well on both kernels. Thanks, Inki Dae > > -- > Either I've been missing something or nothing has been going on. (K. E. Gordon) > > On Thu, Mar 7, 2019 at 1:36 AM Andrzej Hajda <a.hajda@samsung.com> wrote: >> >> 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 >> >> Reported-by: Marian Mihailescu <mihailescu2m@gmail.com> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_mixer.c | 107 +++++++++++++++----------- >> 1 file changed, 63 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >> index 0573eab0e190..42ce01c226ef 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,59 @@ 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) >> +{ >> + 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 +543,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 +597,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 +679,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 +743,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 +757,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 +934,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 +983,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 +998,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 +1011,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); >> } >> -- >> 2.17.1 >> > >
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 0573eab0e190..42ce01c226ef 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,59 @@ 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) +{ + 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 +543,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 +597,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 +679,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 +743,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 +757,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 +934,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 +983,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 +998,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 +1011,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); }
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 Reported-by: Marian Mihailescu <mihailescu2m@gmail.com> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> --- drivers/gpu/drm/exynos/exynos_mixer.c | 107 +++++++++++++++----------- 1 file changed, 63 insertions(+), 44 deletions(-)