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

[Tobias Jakobi <tjakobi@math.uni-bielefeld.de>]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel