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

[Andrzej Hajda]

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

Regards
Andrzej