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