[AMD Official Use Only] > -----Original Message----- > From: Melissa Wen <mwen@xxxxxxxxxx> > Sent: Friday, March 25, 2022 4:45 PM > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Wentland, Harry > <Harry.Wentland@xxxxxxx>; Deucher, Alexander > <Alexander.Deucher@xxxxxxx>; Siqueira, Rodrigo > <Rodrigo.Siqueira@xxxxxxx>; Kazlauskas, Nicholas > <Nicholas.Kazlauskas@xxxxxxx>; Gutierrez, Agustin > <Agustin.Gutierrez@xxxxxxx>; Liu, Zhan <Zhan.Liu@xxxxxxx> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Simon Ser <contact@xxxxxxxxxxx> > Subject: [RFC PATCH] drm/amd/display: dont ignore alpha property > Importance: High > > Hi all, > > I'm examining the IGT kms_plane_alpha_blend test, specifically the > alpha-7efc. It fails on AMD and Intel gen8 hw, but passes on Intel > gen11. At first, I thought it was a rounding issue. In fact, it may be > the problem for different results between intel hw generations. > > However, I changed the test locally to compare CRCs for all alpha values > in the range before the test fails. Interestingly, it fails for all > values when running on AMD, even when comparing planes with zero alpha > (fully transparent). Moreover, I see the same CRC values regardless of > the value set in the alpha property. > > To ensure that the blending mode is as expected, I explicitly set the > Pre-multiplied blending mode in the test. Then I tried using different > framebuffer data and alpha values. I've tried obvious comparisons too, > such as fully opaque and fully transparent. > > As far as I could verify and understand, the value set for the ALPHA > property is totally ignored by AMD drivers. I'm not sure if this is a > matter of how we interpret the meaning of the premultiplied blend mode > or the driver's assumptions about the data in these blend modes. > For example, I made a change in the test as here: > https://paste.debian.net/1235620/ > That basically means same framebuffer, but different alpha values for > each plane. And the result was succesful (but I expected it fails). > The intent was that we don't enable global plane alpha along with anything that requires per pixel alpha. The HW does have bits to specify a mode that's intended to work like this, but I don't think we've ever fully supported it in software. I wouldn't necessarily expect that the blending result is correct, but maybe the IGT test result says otherwise. > Besides that, I see that other subtests in kms_plane_alpha_blend are > skipped, use "None" pixel blend mode, or are not changing the > IGT_PLANE_ALPHA property. So, this alpha-7efc seems to be the only one > in the subset that is checking changes on alpha property under a > Pre-multiplied blend mode, and it is failing. > > I see some inputs in this issue: > https://gitlab.freedesktop.org/drm/amd/-/issues/1769. > But them, I guessed there are different interpretations for handling > plane alpha in the pre-multiplied blend mode. Tbh, I'm not clear, but > there's always a chance of different interpretations, and I don't have > a third driver with CRC capabilities for further comparisons. > > I made some experiments on blnd_cfg values, changing alpha_mode vs > global_gain and global_alpha. I think the expected behaviour for the > Pre-multiplied blend mode is achieved by applying this RFC patch (for > Cezanne). > > Does it seems reasonable? Can anyone help me with more inputs to guide > me the right direction or point out what I misunderstood about these > concepts? > > Thanks, > > Signed-off-by: Melissa Wen <mwen@xxxxxxxxxx> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c | 4 ++++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > 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 6633df7682ce..821ffafa441e 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -5438,7 +5438,7 @@ fill_blending_from_plane_state(const struct > drm_plane_state *plane_state, > > if (plane_state->alpha < 0xffff) { > *global_alpha = true; > - *global_alpha_value = plane_state->alpha >> 8; > + *global_alpha_value = plane_state->alpha; Isn't the original behavior here correct? The value into DC should only be an 8-bit value but we have 16-bit precision from the DRM property. This is truncating the bits that we don't support. > } > } > > 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 4290eaf11a04..b4888f91a9d0 100644 > --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c > +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c > @@ -2367,6 +2367,10 @@ void dcn20_update_mpcc(struct dc *dc, struct > pipe_ctx *pipe_ctx) > == SURFACE_PIXEL_FORMAT_GRPH_RGBE_ALPHA) > blnd_cfg.pre_multiplied_alpha = false; > > + if (blnd_cfg.pre_multiplied_alpha) { > + blnd_cfg.alpha_mode = > MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAI > N; > + blnd_cfg.global_gain = blnd_cfg.global_alpha; > + } While I'm not sure we should be exposing/enabling per pixel alpha + combined global gain, I think the correct logical ordering for this would be to modify the check higher up in the function. If (per_pixel_alpha && pipe_ctx->plane_state->global_alpha) blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN; else if (per_pixel_alpha) blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA; else blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA; This should maintain compatibility with scenarios that don't specify any alpha value on the plane. Note that this logic would also need to be carried down into the dcn10_update_mpcc function as well. Regards, Nicholas Kazlauskas > /* > * TODO: remove hack > * Note: currently there is a bug in init_hw such that > -- > 2.35.1
