Re: [Freedreno] [PATCH] drm/msm/dpu: add support for alpha blending properties

[abhinavk@codeaurora.org]

On 2021-06-28 12:19, Dmitry Baryshkov wrote:
> Add support for alpha blending properties. Setup the plane blend state
> according to those properties.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I think this has already been picked up by Rob but just had a couple of 
comments
below.

Also, how has this been validated? On RB boards i dont think all the 
paths get
executed.

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 43 ++++++++++++++++-------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 ++++--
>  2 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 9a5c70c87cc8..768012243b44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -30,12 +30,6 @@
>  #include "dpu_core_perf.h"
>  #include "dpu_trace.h"
> 
> -#define DPU_DRM_BLEND_OP_NOT_DEFINED    0
> -#define DPU_DRM_BLEND_OP_OPAQUE         1
> -#define DPU_DRM_BLEND_OP_PREMULTIPLIED  2
> -#define DPU_DRM_BLEND_OP_COVERAGE       3
> -#define DPU_DRM_BLEND_OP_MAX            4
> -
>  /* layer mixer index on dpu_crtc */
>  #define LEFT_MIXER 0
>  #define RIGHT_MIXER 1
> @@ -146,20 +140,43 @@ static void _dpu_crtc_setup_blend_cfg(struct
> dpu_crtc_mixer *mixer,
>  {
>  	struct dpu_hw_mixer *lm = mixer->hw_lm;
>  	uint32_t blend_op;
> +	uint32_t fg_alpha, bg_alpha;
> 
> -	/* default to opaque blending */
> -	blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
> -		DPU_BLEND_BG_ALPHA_BG_CONST;
> +	fg_alpha = pstate->base.alpha >> 8;
> +	bg_alpha = 0xff - fg_alpha;
> 
> -	if (format->alpha_enable) {
> +	/* default to opaque blending */
> +	if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE ||
> +	    !format->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) 
> {
> +		blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
> +			DPU_BLEND_BG_ALPHA_FG_PIXEL;
> +		if (fg_alpha != 0xff) {
> +			bg_alpha = fg_alpha;
> +			blend_op |= DPU_BLEND_BG_MOD_ALPHA |
> +				    DPU_BLEND_BG_INV_MOD_ALPHA;
> +		} else {
> +			blend_op |= DPU_BLEND_BG_INV_ALPHA;
> +		}
> +	} else {
>  		/* coverage blending */
>  		blend_op = DPU_BLEND_FG_ALPHA_FG_PIXEL |
> -			DPU_BLEND_BG_ALPHA_FG_PIXEL |
> -			DPU_BLEND_BG_INV_ALPHA;
> +			DPU_BLEND_BG_ALPHA_FG_PIXEL;
> +		if (fg_alpha != 0xff) {
> +			bg_alpha = fg_alpha;
> +			blend_op |= DPU_BLEND_FG_MOD_ALPHA |
> +				    DPU_BLEND_FG_INV_MOD_ALPHA |
comparing this with the blend rule downstream, is this inversion 
necessary?
I only see below rule downstream:

628 			if (fg_alpha != 0xff) {
629 				bg_alpha = fg_alpha;
630 				blend_op |= SDE_BLEND_FG_MOD_ALPHA |
631 					SDE_BLEND_BG_MOD_ALPHA |
632 					SDE_BLEND_BG_INV_MOD_ALPHA;

> +				    DPU_BLEND_BG_MOD_ALPHA |
> +				    DPU_BLEND_BG_INV_MOD_ALPHA;
> +		} else {
> +			blend_op |= DPU_BLEND_BG_INV_ALPHA;
> +		}
>  	}
> 
>  	lm->ops.setup_blend_config(lm, pstate->stage,
> -				0xFF, 0, blend_op);
> +				fg_alpha, bg_alpha, blend_op);
> 
>  	DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%u blend_op:0x%x\n",
>  		  &format->base.pixel_format, format->alpha_enable, blend_op);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index ec4a6f04394a..c989621209aa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1339,9 +1339,7 @@ static void dpu_plane_reset(struct drm_plane 
> *plane)
>  		return;
>  	}
> 
> -	pstate->base.plane = plane;
> -
> -	plane->state = &pstate->base;
> +	__drm_atomic_helper_plane_reset(plane, &pstate->base);
>  }
> 
>  #ifdef CONFIG_DEBUG_FS
> @@ -1647,6 +1645,12 @@ struct drm_plane *dpu_plane_init(struct 
> drm_device *dev,
>  	if (ret)
>  		DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
> 
> +	drm_plane_create_alpha_property(plane);
> +	drm_plane_create_blend_mode_property(plane,
> +			BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> +			BIT(DRM_MODE_BLEND_PREMULTI) |
> +			BIT(DRM_MODE_BLEND_COVERAGE));
> +
>  	drm_plane_create_rotation_property(plane,
>  			DRM_MODE_ROTATE_0,
>  			DRM_MODE_ROTATE_0 |