Re: [PATCH v2] drm/amd/display: add Coverage blend mode for overlay plane

[Melissa Wen <mwen@igalia.com>]

[-- Attachment #1: Type: text/plain, Size: 9931 bytes --]

On 05/13, Sung Joon Kim wrote:
> Issue fixed: Overlay plane alpha channel blending is incorrect
> 
> Issue tracker: https://gitlab.freedesktop.org/drm/amd/-/issues/1769
> 
> According to the KMS man page, there is a
> "Coverage" alpha blend mode that assumes the
> pixel color values have NOT been pre-multiplied
> and will be done when the actual blending to
> the background color values happens.
> 
> Previously, this mode hasn't been enabled
> in our driver and it was assumed that all
> normal overlay planes are pre-multiplied
> by default.
> 
> When a 3rd party app is used to input a image
> in a specific format, e.g. PNG, as a source
> of a overlay plane to blend with the background
> primary plane, the pixel color values are not
> pre-multiplied. So by adding "Coverage" blend
> mode, our driver will support those cases.
> 
Hi,

overall lgtm. I would move those "Issue" points to here (after change
description).

> Reference:
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties
> 
> Adding Coverage support also enables IGT
> kms_plane_alpha_blend Coverage subtests:
> 1. coverage-7efc
> 2. coverage-vs-premult-vs-constant

.. and include a summary of changes from v1 here.
> 
> Signed-off-by: Sung Joon Kim <Sungjoon.Kim@amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 ++++++++----
>  .../gpu/drm/amd/display/dc/core/dc_surface.c  |  2 ++
>  drivers/gpu/drm/amd/display/dc/dc.h           |  2 ++
>  .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 27 ++++++++++---------
>  .../drm/amd/display/dc/dcn20/dcn20_hwseq.c    | 16 ++++++-----
>  5 files changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 2ea20dd7fccf..af2b5d232742 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5380,17 +5380,19 @@ fill_plane_buffer_attributes(struct amdgpu_device *adev,
>  
>  static void
>  fill_blending_from_plane_state(const struct drm_plane_state *plane_state,
> -			       bool *per_pixel_alpha, bool *global_alpha,
> -			       int *global_alpha_value)
> +			       bool *per_pixel_alpha, bool *pre_multiplied_alpha,
> +			       bool *global_alpha, int *global_alpha_value)
>  {
>  	*per_pixel_alpha = false;
> +	*pre_multiplied_alpha = true;
>  	*global_alpha = false;
>  	*global_alpha_value = 0xff;
>  
>  	if (plane_state->plane->type != DRM_PLANE_TYPE_OVERLAY)
>  		return;
>  
> -	if (plane_state->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) {
> +	if (plane_state->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI ||
> +		plane_state->pixel_blend_mode == DRM_MODE_BLEND_COVERAGE) {
>  		static const uint32_t alpha_formats[] = {
>  			DRM_FORMAT_ARGB8888,
>  			DRM_FORMAT_RGBA8888,
> @@ -5405,6 +5407,9 @@ fill_blending_from_plane_state(const struct drm_plane_state *plane_state,
>  				break;
>  			}
>  		}
> +
> +		if (per_pixel_alpha && plane_state->pixel_blend_mode == DRM_MODE_BLEND_COVERAGE)
> +			*pre_multiplied_alpha = false;
>  	}
>  
>  	if (plane_state->alpha < 0xffff) {
> @@ -5567,7 +5572,7 @@ fill_dc_plane_info_and_addr(struct amdgpu_device *adev,
>  		return ret;
>  
>  	fill_blending_from_plane_state(
> -		plane_state, &plane_info->per_pixel_alpha,
> +		plane_state, &plane_info->per_pixel_alpha, &plane_info->pre_multiplied_alpha,
>  		&plane_info->global_alpha, &plane_info->global_alpha_value);
>  
>  	return 0;
> @@ -5614,6 +5619,7 @@ static int fill_dc_plane_attributes(struct amdgpu_device *adev,
>  	dc_plane_state->tiling_info = plane_info.tiling_info;
>  	dc_plane_state->visible = plane_info.visible;
>  	dc_plane_state->per_pixel_alpha = plane_info.per_pixel_alpha;
> +	dc_plane_state->pre_multiplied_alpha = plane_info.pre_multiplied_alpha;
>  	dc_plane_state->global_alpha = plane_info.global_alpha;
>  	dc_plane_state->global_alpha_value = plane_info.global_alpha_value;
>  	dc_plane_state->dcc = plane_info.dcc;
> @@ -7905,7 +7911,8 @@ static int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
>  	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
>  	    plane_cap && plane_cap->per_pixel_alpha) {
>  		unsigned int blend_caps = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> -					  BIT(DRM_MODE_BLEND_PREMULTI);
> +					  BIT(DRM_MODE_BLEND_PREMULTI) |
> +					  BIT(DRM_MODE_BLEND_COVERAGE);
>  
>  		drm_plane_create_alpha_property(plane);
>  		drm_plane_create_blend_mode_property(plane, blend_caps);
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_surface.c b/drivers/gpu/drm/amd/display/dc/core/dc_surface.c
> index e6b9c6a71841..5bc6ff2fa73e 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_surface.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_surface.c
> @@ -61,6 +61,8 @@ static void dc_plane_construct(struct dc_context *ctx, struct dc_plane_state *pl
>  		plane_state->blend_tf->type = TF_TYPE_BYPASS;
>  	}
>  
> +	plane_state->pre_multiplied_alpha = true;
> +
>  }
>  
>  static void dc_plane_destruct(struct dc_plane_state *plane_state)
> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
> index 26c24db8f1da..c353842d5c40 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc.h
> @@ -1011,6 +1011,7 @@ struct dc_plane_state {
>  
>  	bool is_tiling_rotated;
>  	bool per_pixel_alpha;
> +	bool pre_multiplied_alpha;
>  	bool global_alpha;
>  	int  global_alpha_value;
>  	bool visible;
> @@ -1045,6 +1046,7 @@ struct dc_plane_info {
>  	bool horizontal_mirror;
>  	bool visible;
>  	bool per_pixel_alpha;
> +	bool pre_multiplied_alpha;
>  	bool global_alpha;
>  	int  global_alpha_value;
>  	bool input_csc_enabled;
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> index e02ac75afbf7..e3a62873c0e7 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> @@ -2550,12 +2550,21 @@ void dcn10_update_mpcc(struct dc *dc, struct pipe_ctx *pipe_ctx)
>  	blnd_cfg.overlap_only = false;
>  	blnd_cfg.global_gain = 0xff;
>  
> -	if (per_pixel_alpha && pipe_ctx->plane_state->global_alpha) {
> -		blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN;
> -		blnd_cfg.global_gain = pipe_ctx->plane_state->global_alpha_value;
> -	} else if (per_pixel_alpha) {
> -		blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
> +	if (per_pixel_alpha) {
> +		/* DCN1.0 has output CM before MPC which seems to screw with
> +		 * pre-multiplied alpha.
> +		 */
> +		blnd_cfg.pre_multiplied_alpha = (is_rgb_cspace(
> +				pipe_ctx->stream->output_color_space)
> +						&& pipe_ctx->plane_state->pre_multiplied_alpha);
> +		if (pipe_ctx->plane_state->global_alpha) {
> +			blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN;
> +			blnd_cfg.global_gain = pipe_ctx->plane_state->global_alpha_value;
> +		} else {
> +			blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
> +		}
>  	} else {
> +		blnd_cfg.pre_multiplied_alpha = false;
>  		blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
>  	}
>  
> @@ -2564,14 +2573,6 @@ void dcn10_update_mpcc(struct dc *dc, struct pipe_ctx *pipe_ctx)
>  	else
>  		blnd_cfg.global_alpha = 0xff;
>  
> -	/* DCN1.0 has output CM before MPC which seems to screw with
> -	 * pre-multiplied alpha.
> -	 */
> -	blnd_cfg.pre_multiplied_alpha = is_rgb_cspace(
> -			pipe_ctx->stream->output_color_space)
> -					&& per_pixel_alpha;
> -
> -
>  	/*
>  	 * TODO: remove hack
>  	 * Note: currently there is a bug in init_hw such that
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
> index e1f87bd72e4a..c117830b8b9d 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
> @@ -2346,12 +2346,16 @@ void dcn20_update_mpcc(struct dc *dc, struct pipe_ctx *pipe_ctx)
>  	blnd_cfg.overlap_only = false;
>  	blnd_cfg.global_gain = 0xff;
>  
> -	if (per_pixel_alpha && pipe_ctx->plane_state->global_alpha) {
> -		blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN;
> -		blnd_cfg.global_gain = pipe_ctx->plane_state->global_alpha_value;
> -	} else if (per_pixel_alpha) {
> -		blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
> +	if (per_pixel_alpha) {
> +		blnd_cfg.pre_multiplied_alpha = pipe_ctx->plane_state->pre_multiplied_alpha;
> +		if (pipe_ctx->plane_state->global_alpha) {
> +			blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN;
> +			blnd_cfg.global_gain = pipe_ctx->plane_state->global_alpha_value;
> +		} else {
> +			blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
> +		}
>  	} else {
> +		blnd_cfg.pre_multiplied_alpha = false;
>  		blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
>  	}
>  
> @@ -2365,7 +2369,7 @@ void dcn20_update_mpcc(struct dc *dc, struct pipe_ctx *pipe_ctx)
>  	blnd_cfg.top_gain = 0x1f000;
>  	blnd_cfg.bottom_inside_gain = 0x1f000;
>  	blnd_cfg.bottom_outside_gain = 0x1f000;
> -	blnd_cfg.pre_multiplied_alpha = per_pixel_alpha;
> +	
^ checkpatch complains about `trailing whitespace` here.

Anyway, after addressing these minor comments, the patch is

Reviewed-by: Melissa Wen <mwen@igalia.com>

Thanks a lot!

Next, I'm going to refactor the IGT coverage-7efc test to match the
changes done to alpha-7efc (pre-multiplied).

>  	if (pipe_ctx->plane_state->format
>  			== SURFACE_PIXEL_FORMAT_GRPH_RGBE_ALPHA)
>  		blnd_cfg.pre_multiplied_alpha = false;
> -- 
> 2.20.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]