On 03/28, Melissa Wen wrote: > On 03/28, Kazlauskas, Nicholas wrote: > > [AMD Official Use Only] > > > > > -----Original Message----- > > > From: Melissa Wen <[email protected]> > > > Sent: Friday, March 25, 2022 4:45 PM > > > To: [email protected]; Wentland, Harry > > > <[email protected]>; Deucher, Alexander > > > <[email protected]>; Siqueira, Rodrigo > > > <[email protected]>; Kazlauskas, Nicholas > > > <[email protected]>; Gutierrez, Agustin > > > <[email protected]>; Liu, Zhan <[email protected]> > > > Cc: [email protected]; Simon Ser <[email protected]> > > > 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 <[email protected]> > > > --- > > > 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
> >
signature.asc
Description: PGP signature
