[RFC PATCH] drm/amd/display: dont ignore alpha property

[Kazlauskas, Nicholas]

[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.

> 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.

>       }
>  }
>
> 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