Re: [PATCH v2 4/5] drm/exynos: mixer: do blending setup in mixer_cfg_layer()

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

Hey Inki,


Inki Dae wrote:
> 
> 
> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>> This updates the blending setup when the layer configuration
>> changes (triggered by mixer_win_{commit,disable}).
>>
>> To avoid unnecesary reconfigurations we cache the layer
>> state in the mixer context.
>>
>> Extra care has to be taken for the layer that is currently
>> being enabled/disabled.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 41 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index ec9659e..1c24fb5 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 exynos_drm_plane	planes[MIXER_WIN_NR];
>>  	const struct layer_cfg	*layer_cfg;
>>  	unsigned int		num_layer;
>> +	u32			layer_state;
>>  	int			pipe;
>>  	unsigned long		flags;
>>  	bool			interlace;
>> @@ -189,6 +190,27 @@ static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int
>>  	}
>>  }
>>  
>> +static inline u32 get_layer_state(const struct mixer_context *ctx,
>> +	unsigned int win, bool enable)
>> +{
>> +	u32 enable_state, alpha_state;
>> +
>> +	enable_state = ctx->layer_state & 0xffff;
>> +	alpha_state = ctx->layer_state >> 16;
>> +
>> +	if (enable)
>> +		enable_state |= (1 << win);
>> +	else
>> +		enable_state &= ~(1 << win);
>> +
>> +	if (enable && is_alpha_format(ctx, win))
>> +		alpha_state |= (1 << win);
>> +	else
>> +		alpha_state &= ~(1 << win);
>> +
>> +	return ((alpha_state << 16) | enable_state);
>> +}
>> +
>>  static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>>  {
>>  	return readl(res->vp_regs + reg_id);
>> @@ -370,8 +392,9 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>  {
>>  	u32 val;
>>  	struct mixer_resources *res = &ctx->mixer_res;
>> +	const u32 alpha_state = ctx->layer_state >> 16;
>>  
>> -	if (is_alpha_format(ctx, cfg->index)) {
>> +	if (alpha_state & (1 << cfg->index)) {
>>  		val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>  		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
>>  		val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
>> @@ -397,10 +420,11 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>  	}
>>  }
>>  
>> -static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
>> +static void mixer_layer_blending(struct mixer_context *ctx)
>>  {
>>  	unsigned int i, index;
>>  	bool bottom_layer = false;
>> +	const u32 enable_state = ctx->layer_state & 0xffff;
>>  
>>  	for (i = 0; i < ctx->num_layer; ++i) {
>>  		index = ctx->layer_cfg[i].index;
>> @@ -503,8 +527,19 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>  				bool enable)
>>  {
>>  	struct mixer_resources *res = &ctx->mixer_res;
>> +	u32 new_layer_state;
>>  	u32 val = enable ? ~0 : 0;
>>  
>> +	new_layer_state = get_layer_state(ctx, win, enable);
>> +	if (new_layer_state == ctx->layer_state)
>> +		return;
>> +
>> +	/*
>> +	 * Update the layer state so that mixer_layer_blending()
>> +	 * below can use it.
>> +	 */
>> +	ctx->layer_state = new_layer_state;
> 
> It may be trivial but I think it'd be better to move above line to most bottom of this function.
Sure, I can do that, but I would rather know what's the rationale here.


With best wishes,
Tobias

>> +
>>  	switch (win) {
>>  	case 0:
>>  		mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>> @@ -520,6 +555,8 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>  		}
>>  		break;
>>  	}
>> +
>> +	mixer_layer_blending(ctx);
> 
> Here.
> 
> Thanks,
> Inki Dae
> 
>>  }
>>  
>>  static void mixer_run(struct mixer_context *ctx)
>>