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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>>  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 */
>>  
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux