Re: [PATCH] drm/exynos/mixer: fix MIXER shadow registry synchronisation code
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 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 1465.sysmmu: 1445.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
>> Signed-off-by: Andrzej Hajda
>> ---
>> 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
>> +#include
>> #include
>> #include
>> #include
>> @@ -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, >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(), 10);
>> +
>> + 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, >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
Re: [PATCH] drm/exynos/mixer: fix MIXER shadow registry synchronisation code
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 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 1465.sysmmu: 1445.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
> Signed-off-by: Andrzej Hajda
> ---
> 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
> +#include
> #include
> #include
> #include
> @@ -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, >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(), 10);
> +
> + 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, >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(>reg_slock, flags);
>
> - vp_reg_write(ctx, VP_SHADOW_UPDATE, 1);
> /* interlace or progressive scan mode */
> val = (test_bit(MXR_BIT_INTERLACE, >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
[PATCH] drm/exynos/mixer: fix MIXER shadow registry synchronisation code
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 1465.sysmmu: 1445.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
Signed-off-by: Andrzej Hajda
---
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
+#include
#include
#include
#include
@@ -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, >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(), 10);
+
+ 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, >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(>reg_slock, flags);
- vp_reg_write(ctx, VP_SHADOW_UPDATE, 1);
/* interlace or progressive scan mode */
val = (test_bit(MXR_BIT_INTERLACE, >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 */
-
