[RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush()
Tobias Jakobi
tjakobi at math.uni-bielefeld.de
Tue Sep 20 14:09:32 UTC 2016
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.
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
>
>
More information about the dri-devel
mailing list