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 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?
(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
