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

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

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 3966 bytes --]

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

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@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;
 	}
 }
 
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_GAIN;
+		blnd_cfg.global_gain = blnd_cfg.global_alpha;
+	}
 	/*
 	 * TODO: remove hack
 	 * Note: currently there is a bug in init_hw such that
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 3966 bytes --]

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

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@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;
 	}
 }
 
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_GAIN;
+		blnd_cfg.global_gain = blnd_cfg.global_alpha;
+	}
 	/*
 	 * TODO: remove hack
 	 * Note: currently there is a bug in init_hw such that
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Kazlauskas, Nicholas]

[AMD Official Use Only]

> -----Original Message-----
> From: Melissa Wen <mwen@igalia.com>
> Sent: Friday, March 25, 2022 4:45 PM
> To: amd-gfx@lists.freedesktop.org; Wentland, Harry
> <Harry.Wentland@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Siqueira, Rodrigo
> <Rodrigo.Siqueira@amd.com>; Kazlauskas, Nicholas
> <Nicholas.Kazlauskas@amd.com>; Gutierrez, Agustin
> <Agustin.Gutierrez@amd.com>; Liu, Zhan <Zhan.Liu@amd.com>
> Cc: dri-devel@lists.freedesktop.org; Simon Ser <contact@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@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

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

[Kazlauskas, Nicholas]

[AMD Official Use Only]

> -----Original Message-----
> From: Melissa Wen <mwen@igalia.com>
> Sent: Friday, March 25, 2022 4:45 PM
> To: amd-gfx@lists.freedesktop.org; Wentland, Harry
> <Harry.Wentland@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Siqueira, Rodrigo
> <Rodrigo.Siqueira@amd.com>; Kazlauskas, Nicholas
> <Nicholas.Kazlauskas@amd.com>; Gutierrez, Agustin
> <Agustin.Gutierrez@amd.com>; Liu, Zhan <Zhan.Liu@amd.com>
> Cc: dri-devel@lists.freedesktop.org; Simon Ser <contact@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@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

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

[Simon Ser]

Thanks a lot for you patch! I've noticed as well that amdgpu ignores
the plane alpha property [1]. I'll try to find time to test it.

[1]: https://gitlab.freedesktop.org/drm/amd/-/issues/1734

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

[Simon Ser]

Thanks a lot for you patch! I've noticed as well that amdgpu ignores
the plane alpha property [1]. I'll try to find time to test it.

[1]: https://gitlab.freedesktop.org/drm/amd/-/issues/1734

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

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 8705 bytes --]

On 03/28, Kazlauskas, Nicholas wrote:
> [AMD Official Use Only]
> 
> > -----Original Message-----
> > From: Melissa Wen <mwen@igalia.com>
> > Sent: Friday, March 25, 2022 4:45 PM
> > To: amd-gfx@lists.freedesktop.org; Wentland, Harry
> > <Harry.Wentland@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Siqueira, Rodrigo
> > <Rodrigo.Siqueira@amd.com>; Kazlauskas, Nicholas
> > <Nicholas.Kazlauskas@amd.com>; Gutierrez, Agustin
> > <Agustin.Gutierrez@amd.com>; Liu, Zhan <Zhan.Liu@amd.com>
> > Cc: dri-devel@lists.freedesktop.org; Simon Ser <contact@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@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.
> 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 8705 bytes --]

On 03/28, Kazlauskas, Nicholas wrote:
> [AMD Official Use Only]
> 
> > -----Original Message-----
> > From: Melissa Wen <mwen@igalia.com>
> > Sent: Friday, March 25, 2022 4:45 PM
> > To: amd-gfx@lists.freedesktop.org; Wentland, Harry
> > <Harry.Wentland@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Siqueira, Rodrigo
> > <Rodrigo.Siqueira@amd.com>; Kazlauskas, Nicholas
> > <Nicholas.Kazlauskas@amd.com>; Gutierrez, Agustin
> > <Agustin.Gutierrez@amd.com>; Liu, Zhan <Zhan.Liu@amd.com>
> > Cc: dri-devel@lists.freedesktop.org; Simon Ser <contact@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@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.
> 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On 03/28, Simon Ser wrote:
> Thanks a lot for you patch! I've noticed as well that amdgpu ignores
> the plane alpha property [1]. I'll try to find time to test it.
Hi Simon,

So you've faced this kind of issue many times :/
Let me know the results from your side, so we can use it to find the
correct approach to solve this issue.
> 
> [1]: https://gitlab.freedesktop.org/drm/amd/-/issues/1734
Yeah, it looks like the same problem.

I also think the IGT test case (alpha-7efc) is not the best one because
results can be affected by rounding issues. Maybe the test should be
reworked, but first this alpha property issue should be clarified.

Thanks for the inputs!

Melissa

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On 03/28, Simon Ser wrote:
> Thanks a lot for you patch! I've noticed as well that amdgpu ignores
> the plane alpha property [1]. I'll try to find time to test it.
Hi Simon,

So you've faced this kind of issue many times :/
Let me know the results from your side, so we can use it to find the
correct approach to solve this issue.
> 
> [1]: https://gitlab.freedesktop.org/drm/amd/-/issues/1734
Yeah, it looks like the same problem.

I also think the IGT test case (alpha-7efc) is not the best one because
results can be affected by rounding issues. Maybe the test should be
reworked, but first this alpha property issue should be clarified.

Thanks for the inputs!

Melissa

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Harry Wentland]

On 2022-03-28 12:54, Melissa Wen wrote:
> On 03/28, Simon Ser wrote:
>> Thanks a lot for you patch! I've noticed as well that amdgpu ignores
>> the plane alpha property [1]. I'll try to find time to test it.
> Hi Simon,
> 
> So you've faced this kind of issue many times :/
> Let me know the results from your side, so we can use it to find the
> correct approach to solve this issue.

Thanks, Melissa, for looking into this. It does indeed look like we
should support that in amdgpu.

>>
>> [1]: https://gitlab.freedesktop.org/drm/amd/-/issues/1734
> Yeah, it looks like the same problem.
> 

Ooh, tentative looks neat. I'll have to give it a try.

Harry

> I also think the IGT test case (alpha-7efc) is not the best one because
> results can be affected by rounding issues. Maybe the test should be
> reworked, but first this alpha property issue should be clarified.
> 
> Thanks for the inputs!
> 
> Melissa

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

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 9255 bytes --]

On 03/28, Melissa Wen wrote:
> On 03/28, Kazlauskas, Nicholas wrote:
> > [AMD Official Use Only]
> > 
> > > -----Original Message-----
> > > From: Melissa Wen <mwen@igalia.com>
> > > Sent: Friday, March 25, 2022 4:45 PM
> > > To: amd-gfx@lists.freedesktop.org; Wentland, Harry
> > > <Harry.Wentland@amd.com>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>; Siqueira, Rodrigo
> > > <Rodrigo.Siqueira@amd.com>; Kazlauskas, Nicholas
> > > <Nicholas.Kazlauskas@amd.com>; Gutierrez, Agustin
> > > <Agustin.Gutierrez@amd.com>; Liu, Zhan <Zhan.Liu@amd.com>
> > > Cc: dri-devel@lists.freedesktop.org; Simon Ser <contact@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@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
> > 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 9255 bytes --]

On 03/28, Melissa Wen wrote:
> On 03/28, Kazlauskas, Nicholas wrote:
> > [AMD Official Use Only]
> > 
> > > -----Original Message-----
> > > From: Melissa Wen <mwen@igalia.com>
> > > Sent: Friday, March 25, 2022 4:45 PM
> > > To: amd-gfx@lists.freedesktop.org; Wentland, Harry
> > > <Harry.Wentland@amd.com>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>; Siqueira, Rodrigo
> > > <Rodrigo.Siqueira@amd.com>; Kazlauskas, Nicholas
> > > <Nicholas.Kazlauskas@amd.com>; Gutierrez, Agustin
> > > <Agustin.Gutierrez@amd.com>; Liu, Zhan <Zhan.Liu@amd.com>
> > > Cc: dri-devel@lists.freedesktop.org; Simon Ser <contact@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@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
> > 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]