[PATCH v2] drm/amd/display: add Coverage blend mode for overlay plane
Kim, Sung joon
Sungjoon.Kim at amd.com
Fri May 20 17:50:52 UTC 2022
[AMD Official Use Only - General]
-----Original Message-----
From: Wentland, Harry <Harry.Wentland at amd.com>
Sent: Thursday, May 19, 2022 4:45 PM
To: Kim, Sung joon <Sungjoon.Kim at amd.com>; Li, Sun peng (Leo) <Sunpeng.Li at amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>
Cc: mwen at igalia.com; contact at emersion.fr; markyacoub at chromium.org; amd-gfx list <amd-gfx at lists.freedesktop.org>
Subject: Re: [PATCH v2] drm/amd/display: add Coverage blend mode for overlay plane
Fixing amd-gfx address.
On 2022-05-19 16:42, Harry Wentland wrote:
>
>
> On 2022-05-13 16:22, 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.
>>
>> Reference:
>> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-compositi
>> on-properties
>>
>> Adding Coverage support also enables IGT kms_plane_alpha_blend
>> Coverage subtests:
>> 1. coverage-7efc
>> 2. coverage-vs-premult-vs-constant
>>
>> Signed-off-by: Sung Joon Kim <Sungjoon.Kim at 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;
>> -
>
> Could this break color management use-cases on DCN1.x for Windows?
> Dmytro Laktyushkin added this code 5 years ago. Maybe check with him.
>
> Overall I like this patch but let's see if we can confirm that this
> won't break DCN1.x on Windows.
>
> See commit ad32734699da4dd185405637459bf915a4f4cff6.
>
> Harry
I spoke with Dmytro and he is okay with the change
Joon
>
>> -
>> /*
>> * 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;
>> +
>> if (pipe_ctx->plane_state->format
>> == SURFACE_PIXEL_FORMAT_GRPH_RGBE_ALPHA)
>> blnd_cfg.pre_multiplied_alpha = false;
>
More information about the amd-gfx
mailing list