[RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush()

[Andrzej Hajda]

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