On 03/28, Melissa Wen wrote: > 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. > > Indeed, you're right. I'll remove this change from the patch. > 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. I'll send a new version from your feedback. Thanks, Melissa > > 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
