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 at 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 */
>>
>
>
