Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once in mixer_cfg_layer(). Trigger this via atomic flush.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------ drivers/gpu/drm/exynos/regs-mixer.h | 2 + 2 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 1e78d57..d4efd9c 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -99,6 +99,7 @@ struct mixer_context { struct drm_device *drm_dev; struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR]; + unsigned long state_cache; int pipe; unsigned long flags;
@@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); }
-static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win, - unsigned int priority, bool enable) +static void mixer_cfg_layer(struct mixer_context *ctx) { struct mixer_resources *res = &ctx->mixer_res; - u32 val = enable ? ~0 : 0; - - switch (win) { - case 0: - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE); - mixer_reg_writemask(res, MXR_LAYER_CFG, - MXR_LAYER_CFG_GRP0_VAL(priority), - MXR_LAYER_CFG_GRP0_MASK); - break; - case 1: - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE); - mixer_reg_writemask(res, MXR_LAYER_CFG, - MXR_LAYER_CFG_GRP1_VAL(priority), - MXR_LAYER_CFG_GRP1_MASK); + unsigned int win;
- break; - case VP_DEFAULT_WIN: - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) { - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON); - mixer_reg_writemask(res, MXR_CFG, val, - MXR_CFG_VP_ENABLE); - mixer_reg_writemask(res, MXR_LAYER_CFG, - MXR_LAYER_CFG_VP_VAL(priority), - MXR_LAYER_CFG_VP_MASK); + struct exynos_drm_plane_state *state; + struct drm_framebuffer *fb; + unsigned int priority; + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0; + bool enable; + + for (win = 0; win < MIXER_WIN_NR; ++win) { + state = to_exynos_plane_state(ctx->planes[win].base.state); + fb = state->fb; + + priority = state->base.normalized_zpos + 1; + enable = test_bit(win, &ctx->state_cache); + + if (!enable) + continue; + + switch (win) { + case 0: + mxr_cfg |= MXR_CFG_GRP0_ENABLE; + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority); + break; + + case 1: + mxr_cfg |= MXR_CFG_GRP1_ENABLE; + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority); + break; + + case VP_DEFAULT_WIN: + vp_enable = VP_ENABLE_ON; + mxr_cfg |= MXR_CFG_VP_ENABLE; + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority); + break; + } + + if (!fb) + continue; + + /* + * TODO: Don't enable alpha blending for the bottom window. + */ + switch (win) { + case 0: + case 1: + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format)); + break; + + case VP_DEFAULT_WIN: + mixer_cfg_vp_blend(ctx); + break; } - break; } + + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON); + + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK); + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK); }
static void mixer_run(struct mixer_context *ctx) @@ -478,7 +510,6 @@ static void vp_video_buffer(struct mixer_context *ctx, struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; struct mixer_resources *res = &ctx->mixer_res; struct drm_framebuffer *fb = state->fb; - unsigned int priority = state->base.normalized_zpos + 1; unsigned long flags; dma_addr_t luma_addr[2], chroma_addr[2]; bool tiled_mode = false; @@ -563,8 +594,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
mixer_cfg_scan(ctx, mode->vdisplay); mixer_cfg_rgb_fmt(ctx, mode->vdisplay); - mixer_cfg_layer(ctx, plane->index, priority, true); - mixer_cfg_vp_blend(ctx); mixer_run(ctx);
spin_unlock_irqrestore(&res->reg_slock, flags); @@ -588,7 +617,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; struct mixer_resources *res = &ctx->mixer_res; struct drm_framebuffer *fb = state->fb; - unsigned int priority = state->base.normalized_zpos + 1; unsigned long flags; unsigned int win = plane->index; unsigned int x_ratio = 0, y_ratio = 0; @@ -680,8 +708,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
mixer_cfg_scan(ctx, mode->vdisplay); mixer_cfg_rgb_fmt(ctx, mode->vdisplay); - mixer_cfg_layer(ctx, win, priority, true); - mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
/* layer update mandatory for mixer 16.0.33.0 */ if (ctx->mxr_ver == MXR_VER_16_0_33_0 || @@ -994,32 +1020,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc, vp_video_buffer(mixer_ctx, plane); else mixer_graph_buffer(mixer_ctx, plane); + + __set_bit(plane->index, &mixer_ctx->state_cache); }
static void mixer_disable_plane(struct exynos_drm_crtc *crtc, struct exynos_drm_plane *plane) { struct mixer_context *mixer_ctx = crtc->ctx; - struct mixer_resources *res = &mixer_ctx->mixer_res; - unsigned long flags;
DRM_DEBUG_KMS("win: %d\n", plane->index);
if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) return;
- spin_lock_irqsave(&res->reg_slock, flags); - mixer_cfg_layer(mixer_ctx, plane->index, 0, false); - spin_unlock_irqrestore(&res->reg_slock, flags); + __clear_bit(plane->index, &mixer_ctx->state_cache); }
static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) { struct mixer_context *mixer_ctx = crtc->ctx; + struct mixer_resources *res = &mixer_ctx->mixer_res; + unsigned long flags;
if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) return;
+ spin_lock_irqsave(&res->reg_slock, flags); + mixer_cfg_layer(mixer_ctx); + spin_unlock_irqrestore(&res->reg_slock, flags); + mixer_vsync_set_update(mixer_ctx, true); }
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h index 7f22df5..728a18e 100644 --- a/drivers/gpu/drm/exynos/regs-mixer.h +++ b/drivers/gpu/drm/exynos/regs-mixer.h @@ -100,6 +100,7 @@ #define MXR_CFG_GRP1_ENABLE (1 << 5) #define MXR_CFG_GRP0_ENABLE (1 << 4) #define MXR_CFG_VP_ENABLE (1 << 3) +#define MXR_CFG_ENABLE_MASK (0x7 << 3) #define MXR_CFG_SCAN_INTERLACE (0 << 2) #define MXR_CFG_SCAN_PROGRESSIVE (1 << 2) #define MXR_CFG_SCAN_NTSC (0 << 1) @@ -151,6 +152,7 @@ #define MXR_LAYER_CFG_GRP0_MASK MXR_LAYER_CFG_GRP0_VAL(~0) #define MXR_LAYER_CFG_VP_VAL(x) MXR_MASK_VAL(x, 3, 0) #define MXR_LAYER_CFG_VP_MASK MXR_LAYER_CFG_VP_VAL(~0) +#define MXR_LAYER_CFG_MASK 0xFFF
#endif /* SAMSUNG_REGS_MIXER_H */
On 19.09.2016 16:16, Tobias Jakobi wrote:
Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once in mixer_cfg_layer(). Trigger this via atomic flush.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------ drivers/gpu/drm/exynos/regs-mixer.h | 2 + 2 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 1e78d57..d4efd9c 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -99,6 +99,7 @@ struct mixer_context { struct drm_device *drm_dev; struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR];
- unsigned long state_cache;
The name of the variable is cryptic.
int pipe; unsigned long flags;
@@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); }
-static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
unsigned int priority, bool enable)+static void mixer_cfg_layer(struct mixer_context *ctx) { struct mixer_resources *res = &ctx->mixer_res;
- u32 val = enable ? ~0 : 0;
- switch (win) {
- case 0:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP0_VAL(priority),MXR_LAYER_CFG_GRP0_MASK);break;- case 1:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP1_VAL(priority),MXR_LAYER_CFG_GRP1_MASK);
- unsigned int win;
break;- case VP_DEFAULT_WIN:
if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);mixer_reg_writemask(res, MXR_CFG, val,MXR_CFG_VP_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_VP_VAL(priority),MXR_LAYER_CFG_VP_MASK);
- struct exynos_drm_plane_state *state;
- struct drm_framebuffer *fb;
- unsigned int priority;
- u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
- bool enable;
- for (win = 0; win < MIXER_WIN_NR; ++win) {
state = to_exynos_plane_state(ctx->planes[win].base.state);fb = state->fb;priority = state->base.normalized_zpos + 1;enable = test_bit(win, &ctx->state_cache);if (!enable)continue;switch (win) {case 0:mxr_cfg |= MXR_CFG_GRP0_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);break;case 1:mxr_cfg |= MXR_CFG_GRP1_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);break;case VP_DEFAULT_WIN:vp_enable = VP_ENABLE_ON;mxr_cfg |= MXR_CFG_VP_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);break;}if (!fb)continue;/** TODO: Don't enable alpha blending for the bottom window.*/switch (win) {case 0:case 1:mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));break;case VP_DEFAULT_WIN:mixer_cfg_vp_blend(ctx);break;
break;
- if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);- mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
- mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
What about enabled planes which are not updated? Corresponding bit in ctx->state_cache will be 0. It looks like this routine will disable them in hardware, am I right?
Regards Andrzej
}
static void mixer_run(struct mixer_context *ctx) @@ -478,7 +510,6 @@ static void vp_video_buffer(struct mixer_context *ctx, struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; struct mixer_resources *res = &ctx->mixer_res; struct drm_framebuffer *fb = state->fb;
- unsigned int priority = state->base.normalized_zpos + 1; unsigned long flags; dma_addr_t luma_addr[2], chroma_addr[2]; bool tiled_mode = false;
@@ -563,8 +594,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
mixer_cfg_scan(ctx, mode->vdisplay); mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
mixer_cfg_layer(ctx, plane->index, priority, true);
mixer_cfg_vp_blend(ctx); mixer_run(ctx);
spin_unlock_irqrestore(&res->reg_slock, flags);
@@ -588,7 +617,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; struct mixer_resources *res = &ctx->mixer_res; struct drm_framebuffer *fb = state->fb;
- unsigned int priority = state->base.normalized_zpos + 1; unsigned long flags; unsigned int win = plane->index; unsigned int x_ratio = 0, y_ratio = 0;
@@ -680,8 +708,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
mixer_cfg_scan(ctx, mode->vdisplay); mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
mixer_cfg_layer(ctx, win, priority, true);
mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
/* layer update mandatory for mixer 16.0.33.0 */ if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
@@ -994,32 +1020,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc, vp_video_buffer(mixer_ctx, plane); else mixer_graph_buffer(mixer_ctx, plane);
- __set_bit(plane->index, &mixer_ctx->state_cache);
}
static void mixer_disable_plane(struct exynos_drm_crtc *crtc, struct exynos_drm_plane *plane) { struct mixer_context *mixer_ctx = crtc->ctx;
struct mixer_resources *res = &mixer_ctx->mixer_res;
unsigned long flags;
DRM_DEBUG_KMS("win: %d\n", plane->index);
if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) return;
spin_lock_irqsave(&res->reg_slock, flags);
mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
spin_unlock_irqrestore(&res->reg_slock, flags);
- __clear_bit(plane->index, &mixer_ctx->state_cache);
}
static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) { struct mixer_context *mixer_ctx = crtc->ctx;
struct mixer_resources *res = &mixer_ctx->mixer_res;
unsigned long flags;
if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) return;
spin_lock_irqsave(&res->reg_slock, flags);
mixer_cfg_layer(mixer_ctx);
spin_unlock_irqrestore(&res->reg_slock, flags);
mixer_vsync_set_update(mixer_ctx, true);
}
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h index 7f22df5..728a18e 100644 --- a/drivers/gpu/drm/exynos/regs-mixer.h +++ b/drivers/gpu/drm/exynos/regs-mixer.h @@ -100,6 +100,7 @@ #define MXR_CFG_GRP1_ENABLE (1 << 5) #define MXR_CFG_GRP0_ENABLE (1 << 4) #define MXR_CFG_VP_ENABLE (1 << 3) +#define MXR_CFG_ENABLE_MASK (0x7 << 3) #define MXR_CFG_SCAN_INTERLACE (0 << 2) #define MXR_CFG_SCAN_PROGRESSIVE (1 << 2) #define MXR_CFG_SCAN_NTSC (0 << 1) @@ -151,6 +152,7 @@ #define MXR_LAYER_CFG_GRP0_MASK MXR_LAYER_CFG_GRP0_VAL(~0) #define MXR_LAYER_CFG_VP_VAL(x) MXR_MASK_VAL(x, 3, 0) #define MXR_LAYER_CFG_VP_MASK MXR_LAYER_CFG_VP_VAL(~0) +#define MXR_LAYER_CFG_MASK 0xFFF
#endif /* SAMSUNG_REGS_MIXER_H */
Hello Andrzej,
first of all, I've noticed an error myself. mixer_disable() calls mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
Now to your points...
Andrzej Hajda wrote:
On 19.09.2016 16:16, Tobias Jakobi wrote:
Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once in mixer_cfg_layer(). Trigger this via atomic flush.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------ drivers/gpu/drm/exynos/regs-mixer.h | 2 + 2 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 1e78d57..d4efd9c 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -99,6 +99,7 @@ struct mixer_context { struct drm_device *drm_dev; struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR];
- unsigned long state_cache;
The name of the variable is cryptic.
Yes, I'll try to come up with something better. It would probably be easier if struct mixer_context had a documentation for its fields.
Anyway, any suggestions?
int pipe; unsigned long flags;
@@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); }
-static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
unsigned int priority, bool enable)+static void mixer_cfg_layer(struct mixer_context *ctx) { struct mixer_resources *res = &ctx->mixer_res;
- u32 val = enable ? ~0 : 0;
- switch (win) {
- case 0:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP0_VAL(priority),MXR_LAYER_CFG_GRP0_MASK);break;- case 1:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP1_VAL(priority),MXR_LAYER_CFG_GRP1_MASK);
- unsigned int win;
break;- case VP_DEFAULT_WIN:
if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);mixer_reg_writemask(res, MXR_CFG, val,MXR_CFG_VP_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_VP_VAL(priority),MXR_LAYER_CFG_VP_MASK);
- struct exynos_drm_plane_state *state;
- struct drm_framebuffer *fb;
- unsigned int priority;
- u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
- bool enable;
- for (win = 0; win < MIXER_WIN_NR; ++win) {
state = to_exynos_plane_state(ctx->planes[win].base.state);fb = state->fb;priority = state->base.normalized_zpos + 1;enable = test_bit(win, &ctx->state_cache);if (!enable)continue;switch (win) {case 0:mxr_cfg |= MXR_CFG_GRP0_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);break;case 1:mxr_cfg |= MXR_CFG_GRP1_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);break;case VP_DEFAULT_WIN:vp_enable = VP_ENABLE_ON;mxr_cfg |= MXR_CFG_VP_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);break;}if (!fb)continue;/** TODO: Don't enable alpha blending for the bottom window.*/switch (win) {case 0:case 1:mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));break;case VP_DEFAULT_WIN:mixer_cfg_vp_blend(ctx);break;
break;
- if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);- mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
- mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
What about enabled planes which are not updated? Corresponding bit in ctx->state_cache will be 0.
Hmm, it shouldn't. state_cache is initialized once when the mixer context struct is calloced. If the plane is not updated, the corresponding bit in state_cache doesn't change, hence it stays 'on' in this case (enabled plane).
But I guess one could make this more explicit, by putting state_cache initialisation in mixer_win_reset() and replace the manipulation of the MXR_LAYER_CFG and MXR_CFG registers with a call to mixer_cfg_layer().
With best wishes, Tobias
It looks like this routine will disable them in hardware, am I right?
Regards Andrzej
}
static void mixer_run(struct mixer_context *ctx) @@ -478,7 +510,6 @@ static void vp_video_buffer(struct mixer_context *ctx, struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; struct mixer_resources *res = &ctx->mixer_res; struct drm_framebuffer *fb = state->fb;
- unsigned int priority = state->base.normalized_zpos + 1; unsigned long flags; dma_addr_t luma_addr[2], chroma_addr[2]; bool tiled_mode = false;
@@ -563,8 +594,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
mixer_cfg_scan(ctx, mode->vdisplay); mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
mixer_cfg_layer(ctx, plane->index, priority, true);
mixer_cfg_vp_blend(ctx); mixer_run(ctx);
spin_unlock_irqrestore(&res->reg_slock, flags);
@@ -588,7 +617,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx, struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode; struct mixer_resources *res = &ctx->mixer_res; struct drm_framebuffer *fb = state->fb;
- unsigned int priority = state->base.normalized_zpos + 1; unsigned long flags; unsigned int win = plane->index; unsigned int x_ratio = 0, y_ratio = 0;
@@ -680,8 +708,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
mixer_cfg_scan(ctx, mode->vdisplay); mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
mixer_cfg_layer(ctx, win, priority, true);
mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
/* layer update mandatory for mixer 16.0.33.0 */ if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
@@ -994,32 +1020,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc, vp_video_buffer(mixer_ctx, plane); else mixer_graph_buffer(mixer_ctx, plane);
- __set_bit(plane->index, &mixer_ctx->state_cache);
}
static void mixer_disable_plane(struct exynos_drm_crtc *crtc, struct exynos_drm_plane *plane) { struct mixer_context *mixer_ctx = crtc->ctx;
struct mixer_resources *res = &mixer_ctx->mixer_res;
unsigned long flags;
DRM_DEBUG_KMS("win: %d\n", plane->index);
if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) return;
spin_lock_irqsave(&res->reg_slock, flags);
mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
spin_unlock_irqrestore(&res->reg_slock, flags);
- __clear_bit(plane->index, &mixer_ctx->state_cache);
}
static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) { struct mixer_context *mixer_ctx = crtc->ctx;
struct mixer_resources *res = &mixer_ctx->mixer_res;
unsigned long flags;
if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) return;
spin_lock_irqsave(&res->reg_slock, flags);
mixer_cfg_layer(mixer_ctx);
spin_unlock_irqrestore(&res->reg_slock, flags);
mixer_vsync_set_update(mixer_ctx, true);
}
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h index 7f22df5..728a18e 100644 --- a/drivers/gpu/drm/exynos/regs-mixer.h +++ b/drivers/gpu/drm/exynos/regs-mixer.h @@ -100,6 +100,7 @@ #define MXR_CFG_GRP1_ENABLE (1 << 5) #define MXR_CFG_GRP0_ENABLE (1 << 4) #define MXR_CFG_VP_ENABLE (1 << 3) +#define MXR_CFG_ENABLE_MASK (0x7 << 3) #define MXR_CFG_SCAN_INTERLACE (0 << 2) #define MXR_CFG_SCAN_PROGRESSIVE (1 << 2) #define MXR_CFG_SCAN_NTSC (0 << 1) @@ -151,6 +152,7 @@ #define MXR_LAYER_CFG_GRP0_MASK MXR_LAYER_CFG_GRP0_VAL(~0) #define MXR_LAYER_CFG_VP_VAL(x) MXR_MASK_VAL(x, 3, 0) #define MXR_LAYER_CFG_VP_MASK MXR_LAYER_CFG_VP_VAL(~0) +#define MXR_LAYER_CFG_MASK 0xFFF
#endif /* SAMSUNG_REGS_MIXER_H */
On 20.09.2016 13:23, Tobias Jakobi wrote:
Hello Andrzej,
first of all, I've noticed an error myself. mixer_disable() calls mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
Now to your points...
Andrzej Hajda wrote:
On 19.09.2016 16:16, Tobias Jakobi wrote:
Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once in mixer_cfg_layer(). Trigger this via atomic flush.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------ drivers/gpu/drm/exynos/regs-mixer.h | 2 + 2 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 1e78d57..d4efd9c 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -99,6 +99,7 @@ struct mixer_context { struct drm_device *drm_dev; struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR];
- unsigned long state_cache;
The name of the variable is cryptic.
Yes, I'll try to come up with something better. It would probably be easier if struct mixer_context had a documentation for its fields.
Anyway, any suggestions?
(active|enabled)_(planes|windows), or sth similar?
int pipe; unsigned long flags;
@@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); }
-static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
unsigned int priority, bool enable)+static void mixer_cfg_layer(struct mixer_context *ctx) { struct mixer_resources *res = &ctx->mixer_res;
- u32 val = enable ? ~0 : 0;
- switch (win) {
- case 0:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP0_VAL(priority),MXR_LAYER_CFG_GRP0_MASK);break;- case 1:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP1_VAL(priority),MXR_LAYER_CFG_GRP1_MASK);
- unsigned int win;
break;- case VP_DEFAULT_WIN:
if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);mixer_reg_writemask(res, MXR_CFG, val,MXR_CFG_VP_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_VP_VAL(priority),MXR_LAYER_CFG_VP_MASK);
- struct exynos_drm_plane_state *state;
- struct drm_framebuffer *fb;
- unsigned int priority;
- u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
- bool enable;
- for (win = 0; win < MIXER_WIN_NR; ++win) {
state = to_exynos_plane_state(ctx->planes[win].base.state);fb = state->fb;priority = state->base.normalized_zpos + 1;enable = test_bit(win, &ctx->state_cache);if (!enable)continue;switch (win) {case 0:mxr_cfg |= MXR_CFG_GRP0_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);break;case 1:mxr_cfg |= MXR_CFG_GRP1_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);break;case VP_DEFAULT_WIN:vp_enable = VP_ENABLE_ON;mxr_cfg |= MXR_CFG_VP_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);break;}if (!fb)continue;/** TODO: Don't enable alpha blending for the bottom window.*/switch (win) {case 0:case 1:mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));break;case VP_DEFAULT_WIN:mixer_cfg_vp_blend(ctx);break;
break;
- if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);- mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
- mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
What about enabled planes which are not updated? Corresponding bit in ctx->state_cache will be 0.
Hmm, it shouldn't. state_cache is initialized once when the mixer context struct is calloced. If the plane is not updated, the corresponding bit in state_cache doesn't change, hence it stays 'on' in this case (enabled plane).
Thanks for the explanation, I have though (incorrectly) that state_cache is reset before every atomic transaction. By the way I wonder, if we cannot get information if window is enabled by checking 'plane_state->crtc && plane_state->crtc->state->active'.
But I guess one could make this more explicit, by putting state_cache initialisation in mixer_win_reset() and replace the manipulation of the MXR_LAYER_CFG and MXR_CFG registers with a call to mixer_cfg_layer().
I think now it is OK, just good name for the variable should be enough to make it more readable.
Regards Andrzej
On 20.09.2016 14:34, Andrzej Hajda wrote:
On 20.09.2016 13:23, Tobias Jakobi wrote:
Hello Andrzej,
first of all, I've noticed an error myself. mixer_disable() calls mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
Now to your points...
Andrzej Hajda wrote:
On 19.09.2016 16:16, Tobias Jakobi wrote:
Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once in mixer_cfg_layer(). Trigger this via atomic flush.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------ drivers/gpu/drm/exynos/regs-mixer.h | 2 + 2 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 1e78d57..d4efd9c 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -99,6 +99,7 @@ struct mixer_context { struct drm_device *drm_dev; struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR];
- unsigned long state_cache;
The name of the variable is cryptic.
Yes, I'll try to come up with something better. It would probably be easier if struct mixer_context had a documentation for its fields.
Anyway, any suggestions?
(active|enabled)_(planes|windows), or sth similar?
int pipe; unsigned long flags;
@@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); }
-static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
unsigned int priority, bool enable)+static void mixer_cfg_layer(struct mixer_context *ctx) { struct mixer_resources *res = &ctx->mixer_res;
- u32 val = enable ? ~0 : 0;
- switch (win) {
- case 0:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP0_VAL(priority),MXR_LAYER_CFG_GRP0_MASK);break;- case 1:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP1_VAL(priority),MXR_LAYER_CFG_GRP1_MASK);
- unsigned int win;
break;- case VP_DEFAULT_WIN:
if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);mixer_reg_writemask(res, MXR_CFG, val,MXR_CFG_VP_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_VP_VAL(priority),MXR_LAYER_CFG_VP_MASK);
- struct exynos_drm_plane_state *state;
- struct drm_framebuffer *fb;
- unsigned int priority;
- u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
- bool enable;
- for (win = 0; win < MIXER_WIN_NR; ++win) {
state = to_exynos_plane_state(ctx->planes[win].base.state);fb = state->fb;priority = state->base.normalized_zpos + 1;enable = test_bit(win, &ctx->state_cache);if (!enable)continue;switch (win) {case 0:mxr_cfg |= MXR_CFG_GRP0_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);break;case 1:mxr_cfg |= MXR_CFG_GRP1_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);break;case VP_DEFAULT_WIN:vp_enable = VP_ENABLE_ON;mxr_cfg |= MXR_CFG_VP_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);break;}if (!fb)continue;/** TODO: Don't enable alpha blending for the bottom window.*/switch (win) {case 0:case 1:mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));break;case VP_DEFAULT_WIN:mixer_cfg_vp_blend(ctx);break;
break;
- if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);- mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
- mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
What about enabled planes which are not updated? Corresponding bit in ctx->state_cache will be 0.
Hmm, it shouldn't. state_cache is initialized once when the mixer context struct is calloced. If the plane is not updated, the corresponding bit in state_cache doesn't change, hence it stays 'on' in this case (enabled plane).
Thanks for the explanation, I have though (incorrectly) that state_cache is reset before every atomic transaction. By the way I wonder, if we cannot get information if window is enabled by checking 'plane_state->crtc && plane_state->crtc->state->active'.
drm_atomic_crtc_for_each_plane seems to be a better choice, msm and vc4 uses it in flush function.
Regards Andrzej
Hi Andrzej,
Andrzej Hajda wrote:
On 20.09.2016 14:34, Andrzej Hajda wrote:
On 20.09.2016 13:23, Tobias Jakobi wrote:
Hello Andrzej,
first of all, I've noticed an error myself. mixer_disable() calls mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
Now to your points...
Andrzej Hajda wrote:
On 19.09.2016 16:16, Tobias Jakobi wrote:
Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once in mixer_cfg_layer(). Trigger this via atomic flush.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------ drivers/gpu/drm/exynos/regs-mixer.h | 2 + 2 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 1e78d57..d4efd9c 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -99,6 +99,7 @@ struct mixer_context { struct drm_device *drm_dev; struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR];
- unsigned long state_cache;
The name of the variable is cryptic.
Yes, I'll try to come up with something better. It would probably be easier if struct mixer_context had a documentation for its fields.
Anyway, any suggestions?
(active|enabled)_(planes|windows), or sth similar?
Thanks, I think I'll go with the 'window' terminology then.
int pipe; unsigned long flags;
@@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); }
-static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
unsigned int priority, bool enable)+static void mixer_cfg_layer(struct mixer_context *ctx) { struct mixer_resources *res = &ctx->mixer_res;
- u32 val = enable ? ~0 : 0;
- switch (win) {
- case 0:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP0_VAL(priority),MXR_LAYER_CFG_GRP0_MASK);break;- case 1:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP1_VAL(priority),MXR_LAYER_CFG_GRP1_MASK);
- unsigned int win;
break;- case VP_DEFAULT_WIN:
if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);mixer_reg_writemask(res, MXR_CFG, val,MXR_CFG_VP_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_VP_VAL(priority),MXR_LAYER_CFG_VP_MASK);
- struct exynos_drm_plane_state *state;
- struct drm_framebuffer *fb;
- unsigned int priority;
- u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
- bool enable;
- for (win = 0; win < MIXER_WIN_NR; ++win) {
state = to_exynos_plane_state(ctx->planes[win].base.state);fb = state->fb;priority = state->base.normalized_zpos + 1;enable = test_bit(win, &ctx->state_cache);if (!enable)continue;switch (win) {case 0:mxr_cfg |= MXR_CFG_GRP0_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);break;case 1:mxr_cfg |= MXR_CFG_GRP1_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);break;case VP_DEFAULT_WIN:vp_enable = VP_ENABLE_ON;mxr_cfg |= MXR_CFG_VP_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);break;}if (!fb)continue;/** TODO: Don't enable alpha blending for the bottom window.*/switch (win) {case 0:case 1:mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));break;case VP_DEFAULT_WIN:mixer_cfg_vp_blend(ctx);break;
break;
- if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);- mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
- mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
What about enabled planes which are not updated? Corresponding bit in ctx->state_cache will be 0.
Hmm, it shouldn't. state_cache is initialized once when the mixer context struct is calloced. If the plane is not updated, the corresponding bit in state_cache doesn't change, hence it stays 'on' in this case (enabled plane).
Thanks for the explanation, I have though (incorrectly) that state_cache is reset before every atomic transaction. By the way I wonder, if we cannot get information if window is enabled by checking 'plane_state->crtc && plane_state->crtc->state->active'.
drm_atomic_crtc_for_each_plane seems to be a better choice, msm and vc4 uses it in flush function.
I have looked into this, but this seems to be quite a detour.
If I understand atomic sequencing correctly, then we always have: (1) atomic_begin() (2) calls to update_plane() and disable_plane() (3) atomic_flush()
We already get all the necessary information through the calls in step (2), so I don't see the need to query this information again from DRM core in step (3). Especially since this means iterating over lists again.
In particular drm_atomic_crtc_for_each_plane() does list_for_each_entry calling drm_plane_index() in each step, which in turn iterates over the list of planes again.
I rather put another unsigned long into the mixer context and walk a normal array in mixer_cfg_layer(). :-)
With best wishes, Tobias
Regards Andrzej
On 20.09.2016 16:09, Tobias Jakobi wrote:
Hi Andrzej,
Andrzej Hajda wrote:
On 20.09.2016 14:34, Andrzej Hajda wrote:
On 20.09.2016 13:23, Tobias Jakobi wrote:
Hello Andrzej,
first of all, I've noticed an error myself. mixer_disable() calls mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
Now to your points...
Andrzej Hajda wrote:
On 19.09.2016 16:16, Tobias Jakobi wrote:
Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once in mixer_cfg_layer(). Trigger this via atomic flush.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------ drivers/gpu/drm/exynos/regs-mixer.h | 2 + 2 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 1e78d57..d4efd9c 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -99,6 +99,7 @@ struct mixer_context { struct drm_device *drm_dev; struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR];
- unsigned long state_cache;
The name of the variable is cryptic.
Yes, I'll try to come up with something better. It would probably be easier if struct mixer_context had a documentation for its fields.
Anyway, any suggestions?
(active|enabled)_(planes|windows), or sth similar?
Thanks, I think I'll go with the 'window' terminology then.
int pipe; unsigned long flags;
@@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height) mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK); }
-static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
unsigned int priority, bool enable)+static void mixer_cfg_layer(struct mixer_context *ctx) { struct mixer_resources *res = &ctx->mixer_res;
- u32 val = enable ? ~0 : 0;
- switch (win) {
- case 0:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP0_VAL(priority),MXR_LAYER_CFG_GRP0_MASK);break;- case 1:
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_GRP1_VAL(priority),MXR_LAYER_CFG_GRP1_MASK);
- unsigned int win;
break;- case VP_DEFAULT_WIN:
if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);mixer_reg_writemask(res, MXR_CFG, val,MXR_CFG_VP_ENABLE);mixer_reg_writemask(res, MXR_LAYER_CFG,MXR_LAYER_CFG_VP_VAL(priority),MXR_LAYER_CFG_VP_MASK);
- struct exynos_drm_plane_state *state;
- struct drm_framebuffer *fb;
- unsigned int priority;
- u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
- bool enable;
- for (win = 0; win < MIXER_WIN_NR; ++win) {
state = to_exynos_plane_state(ctx->planes[win].base.state);fb = state->fb;priority = state->base.normalized_zpos + 1;enable = test_bit(win, &ctx->state_cache);if (!enable)continue;switch (win) {case 0:mxr_cfg |= MXR_CFG_GRP0_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);break;case 1:mxr_cfg |= MXR_CFG_GRP1_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);break;case VP_DEFAULT_WIN:vp_enable = VP_ENABLE_ON;mxr_cfg |= MXR_CFG_VP_ENABLE;mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);break;}if (!fb)continue;/** TODO: Don't enable alpha blending for the bottom window.*/switch (win) {case 0:case 1:mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));break;case VP_DEFAULT_WIN:mixer_cfg_vp_blend(ctx);break;
break;
- if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);- mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
- mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
What about enabled planes which are not updated? Corresponding bit in ctx->state_cache will be 0.
Hmm, it shouldn't. state_cache is initialized once when the mixer context struct is calloced. If the plane is not updated, the corresponding bit in state_cache doesn't change, hence it stays 'on' in this case (enabled plane).
Thanks for the explanation, I have though (incorrectly) that state_cache is reset before every atomic transaction. By the way I wonder, if we cannot get information if window is enabled by checking 'plane_state->crtc && plane_state->crtc->state->active'.
drm_atomic_crtc_for_each_plane seems to be a better choice, msm and vc4 uses it in flush function.
I have looked into this, but this seems to be quite a detour.
If I understand atomic sequencing correctly, then we always have: (1) atomic_begin() (2) calls to update_plane() and disable_plane() (3) atomic_flush()
We already get all the necessary information through the calls in step (2), so I don't see the need to query this information again from DRM core in step (3). Especially since this means iterating over lists again.
In particular drm_atomic_crtc_for_each_plane() does list_for_each_entry calling drm_plane_index() in each step, which in turn iterates over the list of planes again.
Fortunately drm_plane_index() is saner since 'drm: Store the plane's index' patch, so the loop have similar complexity, but is more generic. Moreover state_cache duplicates drm_crtc_state::plane_mask.
Anyway I have no strong feeling about it, do as you wish :)
Regards Andrzej
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Tobias Jakobi