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

Message ID 1474294604-18876-1-git-send-email-tjakobi@math.uni-bielefeld.de
State New
Headers show

Commit Message

Tobias Jakobi Sept. 19, 2016, 2:16 p.m. UTC
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(-)

Comments

Andrzej Hajda Sept. 20, 2016, 10 a.m. UTC | #1
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.

>  	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.
It looks like this routine will disable them in hardware, am I right?

Regards
Andrzej

>  }
>  
>  static void mixer_run(struct mixer_context *ctx)
> @@ -478,7 +510,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
>  	struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
>  	struct mixer_resources *res = &ctx->mixer_res;
>  	struct drm_framebuffer *fb = state->fb;
> -	unsigned int priority = state->base.normalized_zpos + 1;
>  	unsigned long flags;
>  	dma_addr_t luma_addr[2], chroma_addr[2];
>  	bool tiled_mode = false;
> @@ -563,8 +594,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
>  
>  	mixer_cfg_scan(ctx, mode->vdisplay);
>  	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
> -	mixer_cfg_layer(ctx, plane->index, priority, true);
> -	mixer_cfg_vp_blend(ctx);
>  	mixer_run(ctx);
>  
>  	spin_unlock_irqrestore(&res->reg_slock, flags);
> @@ -588,7 +617,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>  	struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
>  	struct mixer_resources *res = &ctx->mixer_res;
>  	struct drm_framebuffer *fb = state->fb;
> -	unsigned int priority = state->base.normalized_zpos + 1;
>  	unsigned long flags;
>  	unsigned int win = plane->index;
>  	unsigned int x_ratio = 0, y_ratio = 0;
> @@ -680,8 +708,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>  
>  	mixer_cfg_scan(ctx, mode->vdisplay);
>  	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
> -	mixer_cfg_layer(ctx, win, priority, true);
> -	mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>  
>  	/* layer update mandatory for mixer 16.0.33.0 */
>  	if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
> @@ -994,32 +1020,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
>  		vp_video_buffer(mixer_ctx, plane);
>  	else
>  		mixer_graph_buffer(mixer_ctx, plane);
> +
> +	__set_bit(plane->index, &mixer_ctx->state_cache);
>  }
>  
>  static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
>  				struct exynos_drm_plane *plane)
>  {
>  	struct mixer_context *mixer_ctx = crtc->ctx;
> -	struct mixer_resources *res = &mixer_ctx->mixer_res;
> -	unsigned long flags;
>  
>  	DRM_DEBUG_KMS("win: %d\n", plane->index);
>  
>  	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
>  		return;
>  
> -	spin_lock_irqsave(&res->reg_slock, flags);
> -	mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
> -	spin_unlock_irqrestore(&res->reg_slock, flags);
> +	__clear_bit(plane->index, &mixer_ctx->state_cache);
>  }
>  
>  static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
>  {
>  	struct mixer_context *mixer_ctx = crtc->ctx;
> +	struct mixer_resources *res = &mixer_ctx->mixer_res;
> +	unsigned long flags;
>  
>  	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
>  		return;
>  
> +	spin_lock_irqsave(&res->reg_slock, flags);
> +	mixer_cfg_layer(mixer_ctx);
> +	spin_unlock_irqrestore(&res->reg_slock, flags);
> +
>  	mixer_vsync_set_update(mixer_ctx, true);
>  }
>  
> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
> index 7f22df5..728a18e 100644
> --- a/drivers/gpu/drm/exynos/regs-mixer.h
> +++ b/drivers/gpu/drm/exynos/regs-mixer.h
> @@ -100,6 +100,7 @@
>  #define MXR_CFG_GRP1_ENABLE		(1 << 5)
>  #define MXR_CFG_GRP0_ENABLE		(1 << 4)
>  #define MXR_CFG_VP_ENABLE		(1 << 3)
> +#define MXR_CFG_ENABLE_MASK		(0x7 << 3)
>  #define MXR_CFG_SCAN_INTERLACE		(0 << 2)
>  #define MXR_CFG_SCAN_PROGRESSIVE	(1 << 2)
>  #define MXR_CFG_SCAN_NTSC		(0 << 1)
> @@ -151,6 +152,7 @@
>  #define MXR_LAYER_CFG_GRP0_MASK		MXR_LAYER_CFG_GRP0_VAL(~0)
>  #define MXR_LAYER_CFG_VP_VAL(x)		MXR_MASK_VAL(x, 3, 0)
>  #define MXR_LAYER_CFG_VP_MASK		MXR_LAYER_CFG_VP_VAL(~0)
> +#define MXR_LAYER_CFG_MASK		0xFFF
>  
>  #endif /* SAMSUNG_REGS_MIXER_H */
>
Tobias Jakobi Sept. 20, 2016, 11:23 a.m. UTC | #2
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?



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


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().



With best wishes,
Tobias


> It looks like this routine will disable them in hardware, am I right?
> 
> Regards
> Andrzej
> 
>>  }
>>  
>>  static void mixer_run(struct mixer_context *ctx)
>> @@ -478,7 +510,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
>>  	struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
>>  	struct mixer_resources *res = &ctx->mixer_res;
>>  	struct drm_framebuffer *fb = state->fb;
>> -	unsigned int priority = state->base.normalized_zpos + 1;
>>  	unsigned long flags;
>>  	dma_addr_t luma_addr[2], chroma_addr[2];
>>  	bool tiled_mode = false;
>> @@ -563,8 +594,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
>>  
>>  	mixer_cfg_scan(ctx, mode->vdisplay);
>>  	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
>> -	mixer_cfg_layer(ctx, plane->index, priority, true);
>> -	mixer_cfg_vp_blend(ctx);
>>  	mixer_run(ctx);
>>  
>>  	spin_unlock_irqrestore(&res->reg_slock, flags);
>> @@ -588,7 +617,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>>  	struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
>>  	struct mixer_resources *res = &ctx->mixer_res;
>>  	struct drm_framebuffer *fb = state->fb;
>> -	unsigned int priority = state->base.normalized_zpos + 1;
>>  	unsigned long flags;
>>  	unsigned int win = plane->index;
>>  	unsigned int x_ratio = 0, y_ratio = 0;
>> @@ -680,8 +708,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>>  
>>  	mixer_cfg_scan(ctx, mode->vdisplay);
>>  	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
>> -	mixer_cfg_layer(ctx, win, priority, true);
>> -	mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>>  
>>  	/* layer update mandatory for mixer 16.0.33.0 */
>>  	if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
>> @@ -994,32 +1020,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
>>  		vp_video_buffer(mixer_ctx, plane);
>>  	else
>>  		mixer_graph_buffer(mixer_ctx, plane);
>> +
>> +	__set_bit(plane->index, &mixer_ctx->state_cache);
>>  }
>>  
>>  static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
>>  				struct exynos_drm_plane *plane)
>>  {
>>  	struct mixer_context *mixer_ctx = crtc->ctx;
>> -	struct mixer_resources *res = &mixer_ctx->mixer_res;
>> -	unsigned long flags;
>>  
>>  	DRM_DEBUG_KMS("win: %d\n", plane->index);
>>  
>>  	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
>>  		return;
>>  
>> -	spin_lock_irqsave(&res->reg_slock, flags);
>> -	mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
>> -	spin_unlock_irqrestore(&res->reg_slock, flags);
>> +	__clear_bit(plane->index, &mixer_ctx->state_cache);
>>  }
>>  
>>  static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
>>  {
>>  	struct mixer_context *mixer_ctx = crtc->ctx;
>> +	struct mixer_resources *res = &mixer_ctx->mixer_res;
>> +	unsigned long flags;
>>  
>>  	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
>>  		return;
>>  
>> +	spin_lock_irqsave(&res->reg_slock, flags);
>> +	mixer_cfg_layer(mixer_ctx);
>> +	spin_unlock_irqrestore(&res->reg_slock, flags);
>> +
>>  	mixer_vsync_set_update(mixer_ctx, true);
>>  }
>>  
>> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
>> index 7f22df5..728a18e 100644
>> --- a/drivers/gpu/drm/exynos/regs-mixer.h
>> +++ b/drivers/gpu/drm/exynos/regs-mixer.h
>> @@ -100,6 +100,7 @@
>>  #define MXR_CFG_GRP1_ENABLE		(1 << 5)
>>  #define MXR_CFG_GRP0_ENABLE		(1 << 4)
>>  #define MXR_CFG_VP_ENABLE		(1 << 3)
>> +#define MXR_CFG_ENABLE_MASK		(0x7 << 3)
>>  #define MXR_CFG_SCAN_INTERLACE		(0 << 2)
>>  #define MXR_CFG_SCAN_PROGRESSIVE	(1 << 2)
>>  #define MXR_CFG_SCAN_NTSC		(0 << 1)
>> @@ -151,6 +152,7 @@
>>  #define MXR_LAYER_CFG_GRP0_MASK		MXR_LAYER_CFG_GRP0_VAL(~0)
>>  #define MXR_LAYER_CFG_VP_VAL(x)		MXR_MASK_VAL(x, 3, 0)
>>  #define MXR_LAYER_CFG_VP_MASK		MXR_LAYER_CFG_VP_VAL(~0)
>> +#define MXR_LAYER_CFG_MASK		0xFFF
>>  
>>  #endif /* SAMSUNG_REGS_MIXER_H */
>>  
> 
>
Andrzej Hajda Sept. 20, 2016, 12:34 p.m. UTC | #3
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?

>
>
>>>  	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
Andrzej Hajda Sept. 20, 2016, 12:50 p.m. UTC | #4
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?
>
>>
>>>>  	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
Tobias Jakobi Sept. 20, 2016, 2:09 p.m. UTC | #5
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
> 
>
Andrzej Hajda Sept. 20, 2016, 2:30 p.m. UTC | #6
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@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

Patch
diff mbox

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;
 	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);
 }
 
 static void mixer_run(struct mixer_context *ctx)
@@ -478,7 +510,6 @@  static void vp_video_buffer(struct mixer_context *ctx,
 	struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
 	struct mixer_resources *res = &ctx->mixer_res;
 	struct drm_framebuffer *fb = state->fb;
-	unsigned int priority = state->base.normalized_zpos + 1;
 	unsigned long flags;
 	dma_addr_t luma_addr[2], chroma_addr[2];
 	bool tiled_mode = false;
@@ -563,8 +594,6 @@  static void vp_video_buffer(struct mixer_context *ctx,
 
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
-	mixer_cfg_layer(ctx, plane->index, priority, true);
-	mixer_cfg_vp_blend(ctx);
 	mixer_run(ctx);
 
 	spin_unlock_irqrestore(&res->reg_slock, flags);
@@ -588,7 +617,6 @@  static void mixer_graph_buffer(struct mixer_context *ctx,
 	struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
 	struct mixer_resources *res = &ctx->mixer_res;
 	struct drm_framebuffer *fb = state->fb;
-	unsigned int priority = state->base.normalized_zpos + 1;
 	unsigned long flags;
 	unsigned int win = plane->index;
 	unsigned int x_ratio = 0, y_ratio = 0;
@@ -680,8 +708,6 @@  static void mixer_graph_buffer(struct mixer_context *ctx,
 
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
-	mixer_cfg_layer(ctx, win, priority, true);
-	mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
 
 	/* layer update mandatory for mixer 16.0.33.0 */
 	if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
@@ -994,32 +1020,36 @@  static void mixer_update_plane(struct exynos_drm_crtc *crtc,
 		vp_video_buffer(mixer_ctx, plane);
 	else
 		mixer_graph_buffer(mixer_ctx, plane);
+
+	__set_bit(plane->index, &mixer_ctx->state_cache);
 }
 
 static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
 				struct exynos_drm_plane *plane)
 {
 	struct mixer_context *mixer_ctx = crtc->ctx;
-	struct mixer_resources *res = &mixer_ctx->mixer_res;
-	unsigned long flags;
 
 	DRM_DEBUG_KMS("win: %d\n", plane->index);
 
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
 
-	spin_lock_irqsave(&res->reg_slock, flags);
-	mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
-	spin_unlock_irqrestore(&res->reg_slock, flags);
+	__clear_bit(plane->index, &mixer_ctx->state_cache);
 }
 
 static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
 {
 	struct mixer_context *mixer_ctx = crtc->ctx;
+	struct mixer_resources *res = &mixer_ctx->mixer_res;
+	unsigned long flags;
 
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
 
+	spin_lock_irqsave(&res->reg_slock, flags);
+	mixer_cfg_layer(mixer_ctx);
+	spin_unlock_irqrestore(&res->reg_slock, flags);
+
 	mixer_vsync_set_update(mixer_ctx, true);
 }
 
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
index 7f22df5..728a18e 100644
--- a/drivers/gpu/drm/exynos/regs-mixer.h
+++ b/drivers/gpu/drm/exynos/regs-mixer.h
@@ -100,6 +100,7 @@ 
 #define MXR_CFG_GRP1_ENABLE		(1 << 5)
 #define MXR_CFG_GRP0_ENABLE		(1 << 4)
 #define MXR_CFG_VP_ENABLE		(1 << 3)
+#define MXR_CFG_ENABLE_MASK		(0x7 << 3)
 #define MXR_CFG_SCAN_INTERLACE		(0 << 2)
 #define MXR_CFG_SCAN_PROGRESSIVE	(1 << 2)
 #define MXR_CFG_SCAN_NTSC		(0 << 1)
@@ -151,6 +152,7 @@ 
 #define MXR_LAYER_CFG_GRP0_MASK		MXR_LAYER_CFG_GRP0_VAL(~0)
 #define MXR_LAYER_CFG_VP_VAL(x)		MXR_MASK_VAL(x, 3, 0)
 #define MXR_LAYER_CFG_VP_MASK		MXR_LAYER_CFG_VP_VAL(~0)
+#define MXR_LAYER_CFG_MASK		0xFFF
 
 #endif /* SAMSUNG_REGS_MIXER_H */