amd-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* RE: [PATCH] drm/amd/display: add Coverage blend mode for overlay plane
       [not found] <20220512204746.4533-1-Sungjoon.Kim@amd.com>
@ 2022-05-12 20:53 ` Kim, Sung joon
  2022-05-13 11:25 ` Melissa Wen
  1 sibling, 0 replies; 2+ messages in thread
From: Kim, Sung joon @ 2022-05-12 20:53 UTC (permalink / raw)
  To: Wentland, Harry, Li, Sun peng (Leo),
	Siqueira, Rodrigo, Deucher, Alexander, Koenig, Christian
  Cc: mwen, contact, amd-gfx, markyacoub

[AMD Official Use Only - General]

Made a spelling mistake in the amd-gfx email, sending again.

-----Original Message-----
From: Kim, Sung joon <Sungjoon.Kim@amd.com>
Sent: Thursday, May 12, 2022 4:48 PM
To: Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Cc: amd-gfx@lists.freekdesktop.org; mwen@igalia.com; contact@emersion.fr; markyacoub@chromium.org; Kim, Sung joon <Sungjoon.Kim@amd.com>
Subject: [PATCH] drm/amd/display: add Coverage blend mode for overlay plane

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.

Reference:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

Signed-off-by: Sung Joon Kim <Sungjoon.Kim@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 17 ++++++++++++-----
 drivers/gpu/drm/amd/display/dc/dc.h             |  2 ++
 .../gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c  | 15 +++++++++------
 3 files changed, 23 insertions(+), 11 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..48a179182d22 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 *alpha_not_pre_multiplied,
+                              bool *global_alpha, int *global_alpha_value)
 {
        *per_pixel_alpha = false;
+       *alpha_not_pre_multiplied = false;
        *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)
+                       *alpha_not_pre_multiplied = true;
        }

        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->alpha_not_pre_multiplied,
                &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->alpha_not_pre_multiplied =
+plane_info.alpha_not_pre_multiplied;
        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/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
index 26c24db8f1da..714047d0c4f9 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 alpha_not_pre_multiplied;
        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 alpha_not_pre_multiplied;
        bool global_alpha;
        int  global_alpha_value;
        bool input_csc_enabled;
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..e541fe85c455 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,15 @@ 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->alpha_not_pre_multiplied ? false : true;
+               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 +2368,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;
+
        if (pipe_ctx->plane_state->format
                        == SURFACE_PIXEL_FORMAT_GRPH_RGBE_ALPHA)
                blnd_cfg.pre_multiplied_alpha = false;
--
2.20.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] drm/amd/display: add Coverage blend mode for overlay plane
       [not found] <20220512204746.4533-1-Sungjoon.Kim@amd.com>
  2022-05-12 20:53 ` [PATCH] drm/amd/display: add Coverage blend mode for overlay plane Kim, Sung joon
@ 2022-05-13 11:25 ` Melissa Wen
  1 sibling, 0 replies; 2+ messages in thread
From: Melissa Wen @ 2022-05-13 11:25 UTC (permalink / raw)
  To: Sung Joon Kim
  Cc: sunpeng.li, contact, rodrigo.siqueira, amd-gfx, markyacoub,
	alexander.deucher, harry.wentland, christian.koenig

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

On 05/12, Sung Joon Kim wrote:
> 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,

Thanks, enabling coverage mode is great.

I also verify that adding coverage support enables IGT
kms_plane_alpha_blend subtests for coverage blend mode (coverage-7efc
and coverage-vs-premult-vs-constant). Can you include it in the commit
description?

Regarding results, coverage-vs-premult-vs-constant is succesful and I
think coverage-7efc fails for the same issues that led me to refactor
the alpha-7efc in the past.

It'd also be nice to say here the issue on AMD issue tracker this patch
addresses.
> 
> Reference:
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties
> 
> Signed-off-by: Sung Joon Kim <Sungjoon.Kim@amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 17 ++++++++++++-----
>  drivers/gpu/drm/amd/display/dc/dc.h             |  2 ++
>  .../gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c  | 15 +++++++++------
>  3 files changed, 23 insertions(+), 11 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..48a179182d22 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 *alpha_not_pre_multiplied,
> +			       bool *global_alpha, int *global_alpha_value)
>  {
>  	*per_pixel_alpha = false;
> +	*alpha_not_pre_multiplied = false;
Why not use `pre_multiplied_alpha` to follow the same name in mpcc blnd_cfg.pre_multiplied_alpha?
>  	*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) {
code style: aligment should match open parenthesis ^
>  		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)
> +			*alpha_not_pre_multiplied = true;
>  	}
>  
>  	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->alpha_not_pre_multiplied,
>  		&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->alpha_not_pre_multiplied = plane_info.alpha_not_pre_multiplied;
>  	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/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
> index 26c24db8f1da..714047d0c4f9 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 alpha_not_pre_multiplied;
>  	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 alpha_not_pre_multiplied;
>  	bool global_alpha;
>  	int  global_alpha_value;
>  	bool input_csc_enabled;
> 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..e541fe85c455 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,15 @@ 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->alpha_not_pre_multiplied ? false : true;
so, `pre_multiplied_alpha` is easier to follow/handle than
`alpha_not_pre_multiplied` here.
> +		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;
code style: else statement needs braces to be balanced to if clause
>  	} else {
> +		blnd_cfg.pre_multiplied_alpha = false;
>  		blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
>  	}
>  
> @@ -2365,7 +2368,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;
> +	
>  	if (pipe_ctx->plane_state->format
>  			== SURFACE_PIXEL_FORMAT_GRPH_RGBE_ALPHA)
>  		blnd_cfg.pre_multiplied_alpha = false;
I'm not an AMD expert, but should coverage mode also apply to dcn10 and
therefore should this change be expanded to 1.0 family too? I just
remember this recomendation from a previous related patch.

Thanks,

Melissa
> -- 
> 2.20.1
> 

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

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-05-13 11:25 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220512204746.4533-1-Sungjoon.Kim@amd.com>
2022-05-12 20:53 ` [PATCH] drm/amd/display: add Coverage blend mode for overlay plane Kim, Sung joon
2022-05-13 11:25 ` Melissa Wen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).