Re: [PATCH 10/12] drm/msm: convert msm_format::alpha_enable to the flag

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
> Instead of having a bool field alpha_enable, convert it to the
> flag, this save space in the tables and allows us to handle all booleans
> in the same way.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 12 ++++++-----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 24 ++++++++++-----------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c |  7 +++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c |  3 ++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c   |  4 ++--
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c   |  2 +-
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c  |  3 ++-
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c   |  9 ++++----
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c  |  3 ++-
>   drivers/gpu/drm/msm/disp/mdp_format.c       |  2 +-
>   drivers/gpu/drm/msm/msm_drv.h               |  4 ++--
>   11 files changed, 40 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 9041b0d71b25..201010038660 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -342,7 +342,7 @@ static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer,
>   
>   	/* default to opaque blending */
>   	if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE ||
> -	    !format->alpha_enable) {
> +	    !(format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE)) {
>   		blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
>   			DPU_BLEND_BG_ALPHA_BG_CONST;
>   	} else if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) {
> @@ -373,8 +373,8 @@ static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer,
>   	lm->ops.setup_blend_config(lm, pstate->stage,
>   				fg_alpha, bg_alpha, blend_op);
>   
> -	DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%u blend_op:0x%x\n",
> -		  &format->pixel_format, format->alpha_enable, blend_op);
> +	DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%lu blend_op:0x%x\n",
> +		  &format->pixel_format, format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE, blend_op);
>   }
>   
>   static void _dpu_crtc_program_lm_output_roi(struct drm_crtc *crtc)
> @@ -472,7 +472,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>   
>   		format = msm_framebuffer_format(pstate->base.fb);
>   
> -		if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
> +		if (pstate->stage == DPU_STAGE_BASE &&
> +		    format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE)
>   			bg_alpha_enable = true;
>   
>   		set_bit(pstate->pipe.sspp->idx, fetch_active);
> @@ -495,7 +496,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>   		for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
>   			_dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format);
>   
> -			if (bg_alpha_enable && !format->alpha_enable)
> +			if (bg_alpha_enable &&
> +			    !(format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE))
>   				mixer[lm_idx].mixer_op_mode = 0;
>   			else
>   				mixer[lm_idx].mixer_op_mode |=
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> index baf0fd67bf42..de9e93cb42c4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> @@ -36,7 +36,6 @@ bp, flg, fm, np)                                                          \
>   {                                                                         \
>   	.pixel_format = DRM_FORMAT_ ## fmt,                               \
>   	.fetch_type = MDP_PLANE_INTERLEAVED,                              \
> -	.alpha_enable = alpha,                                            \
>   	.element = { (e0), (e1), (e2), (e3) },                            \
>   	.bpc_g_y = g,                                                     \
>   	.bpc_b_cb = b,                                                    \
> @@ -46,7 +45,9 @@ bp, flg, fm, np)                                                          \
>   	.unpack_count = uc,                                               \
>   	.bpp = bp,                                                        \
>   	.fetch_mode = fm,                                                 \
> -	.flags = MSM_FORMAT_FLAG_UNPACK_TIGHT | flg,                      \
> +	.flags = MSM_FORMAT_FLAG_UNPACK_TIGHT |                           \
> +		(alpha ? MSM_FORMAT_FLAG_ALPHA_ENABLE : 0) |              \
> +		flg,                                                      \

In the previous two patches where the same thing was done for 
unpack_tight and unpack_align_msb, it was different in the sense that 
just on the basis of which macro we were choosing we knew the value of 
those flags so you could just unconditionally OR those flags.

But for alpha, you are performing a conditional before ORing this so I 
think for this leaving it as a bool is cleaner.