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

[Andrzej Hajda]

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