| Message ID | 1474294604-18876-1-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 */
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(-)