[v2,18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB

Commit Message

Harry Wentland Jan. 13, 2023, 4:24 p.m. UTC

From: Joshua Ashton <[e-mail]>

Userspace might not aware whether we're sending RGB or YCbCr
data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
requested but the output encoding is YCbCr we should
send COLOR_SPACE_2020_YCBCR.

Signed-off-by: Joshua Ashton <[e-mail]>
Signed-off-by: Harry Wentland <[e-mail]>
Cc: Pekka Paalanen <[e-mail]>
Cc: Sebastian Wick <[e-mail]>
Cc: [e-mail]
Cc: Joshua Ashton <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Reviewed-by: Harry Wentland <[e-mail]>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Sebastian Wick Jan. 23, 2023, 8:30 p.m. UTC | #1

A new property to control YCC and subsampling would be the more
complete path here. If we actually want to fix this in the short-term
though, we should handle the YCC and RGB Colorspace values as
equivalent, everywhere. Technically we're breaking the user space API
here so it should be documented on the KMS property and other drivers
must be adjusted accordingly as well.

On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <[e-mail]> wrote:
>
> From: Joshua Ashton <[e-mail]>
>
> Userspace might not aware whether we're sending RGB or YCbCr
> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
> requested but the output encoding is YCbCr we should
> send COLOR_SPACE_2020_YCBCR.
>
> Signed-off-by: Joshua Ashton <[e-mail]>
> Signed-off-by: Harry Wentland <[e-mail]>
> Cc: Pekka Paalanen <[e-mail]>
> Cc: Sebastian Wick <[e-mail]>
> Cc: [e-mail]
> Cc: Joshua Ashton <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Reviewed-by: Harry Wentland <[e-mail]>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
>  1 file changed, 4 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 f74b125af31f..16940ea61b59 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>                 color_space = COLOR_SPACE_ADOBERGB;
>                 break;
>         case DRM_MODE_COLORIMETRY_BT2020_RGB:
> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> +               else
> +                       color_space = COLOR_SPACE_2020_YCBCR;
>                 break;
>         case DRM_MODE_COLORIMETRY_BT2020_YCC:
>                 color_space = COLOR_SPACE_2020_YCBCR;
> --
> 2.39.0
>

Harry Wentland Jan. 24, 2023, 3:37 p.m. UTC | #2

On 1/23/23 15:30, Sebastian Wick wrote:
> A new property to control YCC and subsampling would be the more
> complete path here. If we actually want to fix this in the short-term
> though, we should handle the YCC and RGB Colorspace values as
> equivalent, everywhere. Technically we're breaking the user space API
> here so it should be documented on the KMS property and other drivers
> must be adjusted accordingly as well.
> 

Could someone point me to a userspace that uses this currently?

Harry

> On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <[e-mail]> wrote:
>>
>> From: Joshua Ashton <[e-mail]>
>>
>> Userspace might not aware whether we're sending RGB or YCbCr
>> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
>> requested but the output encoding is YCbCr we should
>> send COLOR_SPACE_2020_YCBCR.
>>
>> Signed-off-by: Joshua Ashton <[e-mail]>
>> Signed-off-by: Harry Wentland <[e-mail]>
>> Cc: Pekka Paalanen <[e-mail]>
>> Cc: Sebastian Wick <[e-mail]>
>> Cc: [e-mail]
>> Cc: Joshua Ashton <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Reviewed-by: Harry Wentland <[e-mail]>
>> ---
>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
>>  1 file changed, 4 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 f74b125af31f..16940ea61b59 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>>                 color_space = COLOR_SPACE_ADOBERGB;
>>                 break;
>>         case DRM_MODE_COLORIMETRY_BT2020_RGB:
>> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
>> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>> +               else
>> +                       color_space = COLOR_SPACE_2020_YCBCR;
>>                 break;
>>         case DRM_MODE_COLORIMETRY_BT2020_YCC:
>>                 color_space = COLOR_SPACE_2020_YCBCR;
>> --
>> 2.39.0
>>
>

Harry Wentland Jan. 24, 2023, 6:57 p.m. UTC | #3

On 1/24/23 10:37, Harry Wentland wrote:
> 
> 
> On 1/23/23 15:30, Sebastian Wick wrote:
>> A new property to control YCC and subsampling would be the more
>> complete path here. If we actually want to fix this in the short-term
>> though, we should handle the YCC and RGB Colorspace values as
>> equivalent, everywhere. Technically we're breaking the user space API
>> here so it should be documented on the KMS property and other drivers
>> must be adjusted accordingly as well.
>>
> 
> Could someone point me to a userspace that uses this currently?
> 

To elaborate a bit more...

A driver has always had the ability to pick the wire format, whether
it'd be RGB or YCbCr (444, or 420). In some cases that selection
is required in order to satisfy bandwidth requirements. In others
we follow a certain policy to ensure similar behaviors between our
Windows and Linux drivers. I don't think it makes sense for userspace
to control this.

Based on what I see I am not convinced the entirety of the
colorspace definition has a corresponding implementation in an
upstream, canonical userspace, hence my question. Not even an IGT
test existed when I started looking at this. In the absence of a
missing userspace implementation I am not convinced we're breaking
anything. Even then, this was never implemented in amdgpu so
there is no way this regresses any existing behavior.

Harry

> Harry
> 
>> On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <[e-mail]> wrote:
>>>
>>> From: Joshua Ashton <[e-mail]>
>>>
>>> Userspace might not aware whether we're sending RGB or YCbCr
>>> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
>>> requested but the output encoding is YCbCr we should
>>> send COLOR_SPACE_2020_YCBCR.
>>>
>>> Signed-off-by: Joshua Ashton <[e-mail]>
>>> Signed-off-by: Harry Wentland <[e-mail]>
>>> Cc: Pekka Paalanen <[e-mail]>
>>> Cc: Sebastian Wick <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: Joshua Ashton <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> Reviewed-by: Harry Wentland <[e-mail]>
>>> ---
>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
>>>  1 file changed, 4 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 f74b125af31f..16940ea61b59 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>>>                 color_space = COLOR_SPACE_ADOBERGB;
>>>                 break;
>>>         case DRM_MODE_COLORIMETRY_BT2020_RGB:
>>> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>>> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
>>> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>>> +               else
>>> +                       color_space = COLOR_SPACE_2020_YCBCR;
>>>                 break;
>>>         case DRM_MODE_COLORIMETRY_BT2020_YCC:
>>>                 color_space = COLOR_SPACE_2020_YCBCR;
>>> --
>>> 2.39.0
>>>
>>
>

Joshua Ashton Jan. 25, 2023, 12:59 p.m. UTC | #4

On 1/23/23 20:30, Sebastian Wick wrote:
> A new property to control YCC and subsampling would be the more
> complete path here. If we actually want to fix this in the short-term
> though, we should handle the YCC and RGB Colorspace values as
> equivalent, everywhere. Technically we're breaking the user space API
> here so it should be documented on the KMS property and other drivers
> must be adjusted accordingly as well.

I am happy with treating 2020_YCC and 2020_RGB as the same.

I think having the YCC/RGB split here in Colorspace is a mistake.
Pixel encoding should be completely separate from colorspace from a uAPI 
perspective when we want to expose that.
It's just a design flaw from when it was added as it just mirrors the 
values in the colorimetry packets 1:1. I understand why this happened, 
and I don't think it's a big deal for us to correct ourselves now:

I suggest we deprecate the _YCC variants, treat them the same as the RGB 
enum to avoid any potential uAPI breakage and key the split entirely off 
the pixel_encoding value.

That way when we do want to plumb more explicit pixel encoding down the 
line, userspace only has to manage one thing. There's no advantage for 
anything more here.

- Joshie 

Sebastian Wick Jan. 26, 2023, 1:38 a.m. UTC | #5

On Wed, Jan 25, 2023 at 2:00 PM Joshua Ashton <[e-mail]> wrote:
>
>
>
> On 1/23/23 20:30, Sebastian Wick wrote:
> > A new property to control YCC and subsampling would be the more
> > complete path here. If we actually want to fix this in the short-term
> > though, we should handle the YCC and RGB Colorspace values as
> > equivalent, everywhere. Technically we're breaking the user space API
> > here so it should be documented on the KMS property and other drivers
> > must be adjusted accordingly as well.
>
> I am happy with treating 2020_YCC and 2020_RGB as the same.
>
> I think having the YCC/RGB split here in Colorspace is a mistake.
> Pixel encoding should be completely separate from colorspace from a uAPI
> perspective when we want to expose that.
> It's just a design flaw from when it was added as it just mirrors the
> values in the colorimetry packets 1:1. I understand why this happened,
> and I don't think it's a big deal for us to correct ourselves now:
>
> I suggest we deprecate the _YCC variants, treat them the same as the RGB
> enum to avoid any potential uAPI breakage and key the split entirely off
> the pixel_encoding value.

Yeah, I agree. The kernel must know the wire encoding and can thus
always choose the correct variant. If anyone wants to provide a patch
I'll review it.

> That way when we do want to plumb more explicit pixel encoding down the
> line, userspace only has to manage one thing. There's no advantage for
> anything more here.
>
> - Joshie 

Sebastian Wick Jan. 26, 2023, 1:48 a.m. UTC | #6

On Tue, Jan 24, 2023 at 7:57 PM Harry Wentland <[e-mail]> wrote:
>
>
>
> On 1/24/23 10:37, Harry Wentland wrote:
> >
> >
> > On 1/23/23 15:30, Sebastian Wick wrote:
> >> A new property to control YCC and subsampling would be the more
> >> complete path here. If we actually want to fix this in the short-term
> >> though, we should handle the YCC and RGB Colorspace values as
> >> equivalent, everywhere. Technically we're breaking the user space API
> >> here so it should be documented on the KMS property and other drivers
> >> must be adjusted accordingly as well.
> >>
> >
> > Could someone point me to a userspace that uses this currently?
> >
>
> To elaborate a bit more...
>
> A driver has always had the ability to pick the wire format, whether
> it'd be RGB or YCbCr (444, or 420). In some cases that selection
> is required in order to satisfy bandwidth requirements. In others
> we follow a certain policy to ensure similar behaviors between our
> Windows and Linux drivers. I don't think it makes sense for userspace
> to control this.

I disagree. The subsampling is degrading the image considerably in
some cases. We need control over this.

It does mean that user space has to be smart and try to reduce the
bandwidth if a KMS commit fails, but the same is true for resolution
and refresh rate right now and will be true for a min bpc property as
well.

> Based on what I see I am not convinced the entirety of the
> colorspace definition has a corresponding implementation in an
> upstream, canonical userspace, hence my question. Not even an IGT
> test existed when I started looking at this. In the absence of a
> missing userspace implementation I am not convinced we're breaking
> anything. Even then, this was never implemented in amdgpu so
> there is no way this regresses any existing behavior.

I don't think this breaks anything in practice. The change would only
break use cases where you want to set the infoframe to a variant which
does not match the wire format and that would be broken. So yes, I
agree.

We can't just remove the enum values though. If we deprecate the YCC
variants that must be documented and user space has to understand that
choosing the RGB variant will result in the kernel just doing the
"right thing".

>
> Harry
>
> > Harry
> >
> >> On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <[e-mail]> wrote:
> >>>
> >>> From: Joshua Ashton <[e-mail]>
> >>>
> >>> Userspace might not aware whether we're sending RGB or YCbCr
> >>> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
> >>> requested but the output encoding is YCbCr we should
> >>> send COLOR_SPACE_2020_YCBCR.
> >>>
> >>> Signed-off-by: Joshua Ashton <[e-mail]>
> >>> Signed-off-by: Harry Wentland <[e-mail]>
> >>> Cc: Pekka Paalanen <[e-mail]>
> >>> Cc: Sebastian Wick <[e-mail]>
> >>> Cc: [e-mail]
> >>> Cc: Joshua Ashton <[e-mail]>
> >>> Cc: [e-mail]
> >>> Cc: [e-mail]
> >>> Reviewed-by: Harry Wentland <[e-mail]>
> >>> ---
> >>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
> >>>  1 file changed, 4 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 f74b125af31f..16940ea61b59 100644
> >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
> >>>                 color_space = COLOR_SPACE_ADOBERGB;
> >>>                 break;
> >>>         case DRM_MODE_COLORIMETRY_BT2020_RGB:
> >>> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> >>> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
> >>> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> >>> +               else
> >>> +                       color_space = COLOR_SPACE_2020_YCBCR;
> >>>                 break;
> >>>         case DRM_MODE_COLORIMETRY_BT2020_YCC:
> >>>                 color_space = COLOR_SPACE_2020_YCBCR;
> >>> --
> >>> 2.39.0
> >>>
> >>
> >
>

Pekka Paalanen Feb. 7, 2023, 12:42 p.m. UTC | #7

On Wed, 25 Jan 2023 12:59:53 +0000
Joshua Ashton <[e-mail]> wrote:

> On 1/23/23 20:30, Sebastian Wick wrote:
> > A new property to control YCC and subsampling would be the more
> > complete path here. If we actually want to fix this in the short-term
> > though, we should handle the YCC and RGB Colorspace values as
> > equivalent, everywhere. Technically we're breaking the user space API
> > here so it should be documented on the KMS property and other drivers
> > must be adjusted accordingly as well.  
> 
> I am happy with treating 2020_YCC and 2020_RGB as the same.
> 
> I think having the YCC/RGB split here in Colorspace is a mistake.
> Pixel encoding should be completely separate from colorspace from a uAPI 
> perspective when we want to expose that.
> It's just a design flaw from when it was added as it just mirrors the 
> values in the colorimetry packets 1:1. I understand why this happened, 
> and I don't think it's a big deal for us to correct ourselves now:
> 
> I suggest we deprecate the _YCC variants, treat them the same as the RGB 
> enum to avoid any potential uAPI breakage and key the split entirely off 
> the pixel_encoding value.
> 
> That way when we do want to plumb more explicit pixel encoding down the 
> line, userspace only has to manage one thing. There's no advantage for 
> anything more here.

Sounds good to me!


Thanks,
pq

> 
> - Joshie 

Patch

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 f74b125af31f..16940ea61b59 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5184,7 +5184,10 @@  get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
 		color_space = COLOR_SPACE_ADOBERGB;
 		break;
 	case DRM_MODE_COLORIMETRY_BT2020_RGB:
-		color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
+		if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
+			color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
+		else
+			color_space = COLOR_SPACE_2020_YCBCR;
 		break;
 	case DRM_MODE_COLORIMETRY_BT2020_YCC:
 		color_space = COLOR_SPACE_2020_YCBCR;