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@xxxxxxx> > --- > .../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:
signature.asc
Description: PGP signature