[AMD Official Use Only - General] -----Original Message----- From: Wentland, Harry <Harry.Wentland@xxxxxxx> Sent: Thursday, May 19, 2022 4:45 PM To: Kim, Sung joon <Sungjoon.Kim@xxxxxxx>; Li, Sun peng (Leo) <Sunpeng.Li@xxxxxxx>; Siqueira, Rodrigo <Rodrigo.Siqueira@xxxxxxx>; Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx> Cc: mwen@xxxxxxxxxx; contact@xxxxxxxxxxx; markyacoub@xxxxxxxxxxxx; amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxx> >> --- >> .../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; >
