[PATCH] drm/amd/display: add Coverage blend mode for overlay plane
Melissa Wen
mwen at igalia.com
Fri May 13 11:25:18 UTC 2022
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 at 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20220513/05c5236b/attachment.sig>
More information about the amd-gfx
mailing list