| Message ID | 20190319130511.20729-1-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] drm/exynos/mixer: fix MIXER shadow registry synchronisation code | expand |
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); > } >
Hi Inki, On 2019-03-21 09:20, Inki Dae wrote: > 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. :( Andrzej's patch has been tested on Exynos4412 too. HDMI display works fine on Odroid U3. ... Best regards
Hi Marek, 19. 3. 21. 오후 5:32에 Marek Szyprowski 이(가) 쓴 글: > Hi Inki, > > On 2019-03-21 09:20, Inki Dae wrote: >> 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. :( > > Andrzej's patch has been tested on Exynos4412 too. HDMI display works > fine on Odroid U3. Applied. Thanks, Inki Dae > > ... > > Best regards >
This patch works on my odroid xu4 too, on 5.0.3 kernel. Thanks! -Marian > On 19 Mar 2019, at 11:35 pm, 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 > > 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 ;) > > 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); > } > -- > 2.17.1 >
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); }
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 ;) Regards Andrzej --- drivers/gpu/drm/exynos/exynos_mixer.c | 110 +++++++++++++++----------- 1 file changed, 66 insertions(+), 44 deletions(-)