On 03/28, Kazlauskas, Nicholas wrote: > [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. hmm... afaiu, I think the issue here is that no formula of pixel blend mode ignores the "global plane alpha". Looking at the description here: https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_blend.c#n142 I understand the global plane alpha is the plane_alpha, described as: "Plane alpha value set by the plane "alpha" property." And the pixel alpha is the fg.alpha, right? So, the "None" mode doesn't care of pixel alpha, but still considers (global) plane alpha, and "Pre-multiplied" mode is considering plane alpha and pixel alpha to calculate how background RGB affects the resulted composition... > > > 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. > From what I could verify (printed), the driver reads a 8-bit value, and the shift is actually clearing the global_alpha_value: alpha_value >> 8; [ 38.296885] amdgpu: Changing global alpha: ppa 1, ps/a 0x007e [ 38.296887] amdgpu: Global alpha resulted: global_alpha_value 0x0000 [ 38.297071] amdgpu: Changing global alpha: ppa 1, ps/a 0x007e [ 38.297072] amdgpu: Global alpha resulted: global_alpha_value 0x0000 [ 38.297601] DCN20 update mpcc: 1, 0x00, 0x00, 1 [ 38.297660] DCN20 update mpcc: 1, 0x00, 0x00, 1 ppa = per_pixel_alpha ps/a = plane_state->alpha Values in the last prints are: per_pixel_alpha, pipe_ctx->plane_state->global_alpha_value, blnd_cfg.global_gain, blnd_cfg.pre_multiplied_alpha alpha_value; (no shift) [ +0.000003] amdgpu: Changing global alpha: ppa 1, ps/a 0x007e [ +0.000002] amdgpu: Global alpha resulted: global_alpha_value 0x007e [ +0.000001] amdgpu: Changing global alpha: ppa 1, ps/a 0x007e [ +0.000001] amdgpu: Global alpha resulted: global_alpha_value 0x007e [ +0.000553] DCN20 update mpcc: 1, 0x7e, 0x7e, 1 [ +0.000068] DCN20 update mpcc: 1, 0x7e, 0x7e, 1 > > } > > } > > > > 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. Thanks! So, in the case that global_gain is previously set as 0xff in the code, is it ok to always set global_gain = global_alpha or should it only happens when _COMBINED_GLOBAL_GAIN mode is defined? > > Note that this logic would also need to be carried down into the dcn10_update_mpcc function as well. yeah, sure! Thanks a lot for your feedback! I'll try to provide more inputs. Also, let me know what you think about the expected behaviour of global plane alpha from these pixel blend mode formulas. Best Regards, Melissa > > Regards, > Nicholas Kazlauskas > > > /* > > * TODO: remove hack > > * Note: currently there is a bug in init_hw such that > > -- > > 2.35.1 >
Attachment:
signature.asc
Description: PGP signature
