[RFC PATCH] drm/amd/display: dont ignore alpha property
Melissa Wen
mwen at igalia.com
Tue Mar 29 16:42:44 UTC 2022
On 03/28, Melissa Wen wrote:
> On 03/28, Kazlauskas, Nicholas wrote:
> > [AMD Official Use Only]
> >
> > > -----Original Message-----
> > > From: Melissa Wen <mwen at igalia.com>
> > > Sent: Friday, March 25, 2022 4:45 PM
> > > To: amd-gfx at lists.freedesktop.org; Wentland, Harry
> > > <Harry.Wentland at amd.com>; Deucher, Alexander
> > > <Alexander.Deucher at amd.com>; Siqueira, Rodrigo
> > > <Rodrigo.Siqueira at amd.com>; Kazlauskas, Nicholas
> > > <Nicholas.Kazlauskas at amd.com>; Gutierrez, Agustin
> > > <Agustin.Gutierrez at amd.com>; Liu, Zhan <Zhan.Liu at amd.com>
> > > Cc: dri-devel at lists.freedesktop.org; Simon Ser <contact at emersion.fr>
> > > 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 at igalia.com>
> > > ---
> > > 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
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220329/352935e0/attachment-0001.sig>
More information about the dri-devel
mailing list