Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace
On Tue, 11 May 2021 12:03:30 +0200 Werner Sembach wrote: > Am 11.05.21 um 10:07 schrieb Pekka Paalanen: > > On Mon, 10 May 2021 17:47:01 -0400 > > Alex Deucher wrote: > > > >> On Fri, May 7, 2021 at 3:27 PM Werner Sembach > >> wrote: > >>> xrandr --prop and other userspace info tools have currently no way of > >>> telling which color configuration is used on HDMI and DP ports. > >>> > >>> The ongoing transsition from HDMI 1.4 to 2.0 and the different bandwidth > >>> requirements of YCbCr 4:2:0 and RGB color format raise different > >>> incompatibilities. Having these configuration information readily > >>> available is a useful tool in debuging washed out colors, color artefacts > >>> on small fonts and missing refreshrate options. > >> I think we would ideally want these as generic connector properties > >> rather than AMD specific ones since they are not really AMD specific. > >> I believe there is already a generic drm property (max_bpc) for the > >> color depth. At this point, I think having a generic RGB vs YCbCr > >> property would make sense. I'm not sure about the color space. > > Problem is: amdgpu does not really use generic structs for these 3 > properties as far as I can tell. It uses own defines > https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/amd/display/dc/dc_hw_types.h#L647 > in own structs > https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/amd/display/dc/dc_stream.h#L141 > > Intel uses generic defines > https://elixir.bootlin.com/linux/v5.13-rc1/source/include/linux/hdmi.h#L71 > https://elixir.bootlin.com/linux/v5.13-rc1/source/include/drm/drm_dp_helper.h#L1568 > split up between dp and hdmi in own structs > https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/i915/display/intel_display_types.h#L879 > > So the property would need some translation from amd, intel, hdmi, > and dp to one enumeration representing all? If so, much better to do that in the kernel in one place per driver that actually know what they mean, rather than in half of the Wayland compositors by people who barely understand even the general concept. Like me. > > Hi, > > > > I believe that userspace will definitely want to know what exactly is > > going on on the monitor cable, so I think exposing more information is > > good, but I agree that it needs to be done with generic KMS properties > > as much as possible. Userspace is not going to bother having explicit > > code for driver-specific properties. > > > > I think a major use case will be Wayland color management, where a > > Wayland compositor will want to make sure that the video signal > > parameters have not changed since the monitor was last measured > > (profiled). If the signal configuration is different, the measured > > color profile may be invalid and therefore the end user needs to be > > warned. See some ideas in > > https://gitlab.freedesktop.org/wayland/weston/-/issues/467 > > under the heading "Color calibration auditing system". > > > > About the color space: is that something a kernel driver will decide > > on its own? I mean in the same sense as the driver will negotiate > > HDMI/DP link parameters, perhaps falling back to smaller requirements > > if higher requirements signal does not seem to work. > > > > We only need readback properties for things that generic userspace > > won't or cannot control explicitly, e.g. because the kernel driver has > > room to make a choice rather than fail. > > Some explanation why I choose these 3 properties: > > output color space: Mainly to see if full or limited RGB was chosen. IOW, the driver makes the decision. Therefore userspace will need to know what it picked. Ok. > While in theory the driver should choose the right one automatically, > I read articles that in some cases it doesn't (hence why the > "Broadcast RGB" property for intel-gfx driver and "output_csc" for > the old radeon driver exist). The next step ofc would be to bring > over the "Broadcast RGB" property to amdgpu/make it a generic > property. But then still: having a feedback channel to see if the > chosen setting got correctly applied should not hurt in any way, > shape, or form? Feedback is good IMO, yes, when it's not clearly redundant. As long as you don't tie the driver developers' hands in a knot or preclude support for hardware not invented yet. But I think the option to not expose a specific KMS property is an escape hatch enough. You might have problems with the requirements for DRM UAPI additions though, since they call for a proper userspace consumer. What defines what is "the right one"? I believe that we also need to aim for a fully known display pipeline, so if the driver is doing compression from full range 8-bit to limited range 8-bit, losing a little precision, userspace needs to know. This is a long stretch and maybe not even fully feasible, but I think it would be good to get as close as possible - within the limits of being driver-agnostic UAPI.
Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace
Am 11.05.21 um 10:07 schrieb Pekka Paalanen: > On Mon, 10 May 2021 17:47:01 -0400 > Alex Deucher wrote: > >> On Fri, May 7, 2021 at 3:27 PM Werner Sembach >> wrote: >>> xrandr --prop and other userspace info tools have currently no way of >>> telling which color configuration is used on HDMI and DP ports. >>> >>> The ongoing transsition from HDMI 1.4 to 2.0 and the different bandwidth >>> requirements of YCbCr 4:2:0 and RGB color format raise different >>> incompatibilities. Having these configuration information readily >>> available is a useful tool in debuging washed out colors, color artefacts >>> on small fonts and missing refreshrate options. >> I think we would ideally want these as generic connector properties >> rather than AMD specific ones since they are not really AMD specific. >> I believe there is already a generic drm property (max_bpc) for the >> color depth. At this point, I think having a generic RGB vs YCbCr >> property would make sense. I'm not sure about the color space. Problem is: amdgpu does not really use generic structs for these 3 properties as far as I can tell. It uses own defines https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/amd/display/dc/dc_hw_types.h#L647 in own structs https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/amd/display/dc/dc_stream.h#L141 Intel uses generic defines https://elixir.bootlin.com/linux/v5.13-rc1/source/include/linux/hdmi.h#L71 https://elixir.bootlin.com/linux/v5.13-rc1/source/include/drm/drm_dp_helper.h#L1568 split up between dp and hdmi in own structs https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/i915/display/intel_display_types.h#L879 So the property would need some translation from amd, intel, hdmi, and dp to one enumeration representing all? > Hi, > > I believe that userspace will definitely want to know what exactly is > going on on the monitor cable, so I think exposing more information is > good, but I agree that it needs to be done with generic KMS properties > as much as possible. Userspace is not going to bother having explicit > code for driver-specific properties. > > I think a major use case will be Wayland color management, where a > Wayland compositor will want to make sure that the video signal > parameters have not changed since the monitor was last measured > (profiled). If the signal configuration is different, the measured > color profile may be invalid and therefore the end user needs to be > warned. See some ideas in > https://gitlab.freedesktop.org/wayland/weston/-/issues/467 > under the heading "Color calibration auditing system". > > About the color space: is that something a kernel driver will decide > on its own? I mean in the same sense as the driver will negotiate > HDMI/DP link parameters, perhaps falling back to smaller requirements > if higher requirements signal does not seem to work. > > We only need readback properties for things that generic userspace > won't or cannot control explicitly, e.g. because the kernel driver has > room to make a choice rather than fail. Some explanation why I choose these 3 properties: output color space: Mainly to see if full or limited RGB was chosen. While in theory the driver should choose the right one automatically, I read articles that in some cases it doesn't (hence why the "Broadcast RGB" property for intel-gfx driver and "output_csc" for the old radeon driver exist). The next step ofc would be to bring over the "Broadcast RGB" property to amdgpu/make it a generic property. But then still: having a feedback channel to see if the chosen setting got correctly applied should not hurt in any way, shape, or form? pixel encoding: Probably the most important of the 3: This should be accompanied with a "preferred pixel encoding" user controllable setting and is mainly thought as a feedback channel for that, because it might not always the obvious if the Display + GPU + Driver + Link encoder combination actually supports and therefore applies the selected "preferred pixel encoding". For example: I have a display here that can display 4k@60Hz or WQHD@120Hz, but YCbCr is only supported by it for 4k@60Hz (also it's not supported for 4k30Hz). The "preferred pixel encoding" setting is required because certain devices (both PC's and display's) might wrongly advertise their capabilities. The current fix in this case is to write a custom edid which is a kinda hacky solution. Examples: 1. RGB and YCbCr4:4:4 in theory carry the same amount of color information, but some displays look worse in one or the other, because they do bad internal conversion. 2. A laptop wants to output YCbCr4:4:4 but because of bad shielding of the port/the cable/the display, the screen goes black every few seconds. Using YCbCr4:2:0, and therefore a lower signal clock, stabilizes the connection without changing hardware. color depth: While "max bpc" can be used to change the color depth, there is currently no way to
Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace
On Mon, 10 May 2021 17:47:01 -0400 Alex Deucher wrote: > On Fri, May 7, 2021 at 3:27 PM Werner Sembach > wrote: > > > > xrandr --prop and other userspace info tools have currently no way of > > telling which color configuration is used on HDMI and DP ports. > > > > The ongoing transsition from HDMI 1.4 to 2.0 and the different bandwidth > > requirements of YCbCr 4:2:0 and RGB color format raise different > > incompatibilities. Having these configuration information readily > > available is a useful tool in debuging washed out colors, color artefacts > > on small fonts and missing refreshrate options. > > I think we would ideally want these as generic connector properties > rather than AMD specific ones since they are not really AMD specific. > I believe there is already a generic drm property (max_bpc) for the > color depth. At this point, I think having a generic RGB vs YCbCr > property would make sense. I'm not sure about the color space. Hi, I believe that userspace will definitely want to know what exactly is going on on the monitor cable, so I think exposing more information is good, but I agree that it needs to be done with generic KMS properties as much as possible. Userspace is not going to bother having explicit code for driver-specific properties. I think a major use case will be Wayland color management, where a Wayland compositor will want to make sure that the video signal parameters have not changed since the monitor was last measured (profiled). If the signal configuration is different, the measured color profile may be invalid and therefore the end user needs to be warned. See some ideas in https://gitlab.freedesktop.org/wayland/weston/-/issues/467 under the heading "Color calibration auditing system". About the color space: is that something a kernel driver will decide on its own? I mean in the same sense as the driver will negotiate HDMI/DP link parameters, perhaps falling back to smaller requirements if higher requirements signal does not seem to work. We only need readback properties for things that generic userspace won't or cannot control explicitly, e.g. because the kernel driver has room to make a choice rather than fail. Thanks, pq pgpgvvG8vUxbd.pgp Description: OpenPGP digital signature
Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace
On Fri, May 7, 2021 at 3:27 PM Werner Sembach wrote:
>
> xrandr --prop and other userspace info tools have currently no way of
> telling which color configuration is used on HDMI and DP ports.
>
> The ongoing transsition from HDMI 1.4 to 2.0 and the different bandwidth
> requirements of YCbCr 4:2:0 and RGB color format raise different
> incompatibilities. Having these configuration information readily
> available is a useful tool in debuging washed out colors, color artefacts
> on small fonts and missing refreshrate options.
I think we would ideally want these as generic connector properties
rather than AMD specific ones since they are not really AMD specific.
I believe there is already a generic drm property (max_bpc) for the
color depth. At this point, I think having a generic RGB vs YCbCr
property would make sense. I'm not sure about the color space.
Alex
>
> Signed-off-by: Werner Sembach
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 58 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 4 ++
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36
> 3 files changed, 98 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index f753e04fee99..c0404bcda31b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -986,6 +986,40 @@ static const struct drm_prop_enum_list
> amdgpu_dither_enum_list[] =
> { AMDGPU_FMT_DITHER_ENABLE, "on" },
> };
>
> +static const struct drm_prop_enum_list
> amdgpu_active_pixel_encoding_enum_list[] = {
> + { PIXEL_ENCODING_UNDEFINED, "undefined" },
> + { PIXEL_ENCODING_RGB, "RGB" },
> + { PIXEL_ENCODING_YCBCR422, "YCbCr 4:2:2" },
> + { PIXEL_ENCODING_YCBCR444, "YCbCr 4:4:4" },
> + { PIXEL_ENCODING_YCBCR420, "YCbCr 4:2:0" },
> +};
> +
> +static const struct drm_prop_enum_list
> amdgpu_active_display_color_depth_enum_list[] = {
> + { COLOR_DEPTH_UNDEFINED, "undefined" },
> + { COLOR_DEPTH_666, "6 bit" },
> + { COLOR_DEPTH_888, "8 bit" },
> + { COLOR_DEPTH_101010, "10 bit" },
> + { COLOR_DEPTH_121212, "12 bit" },
> + { COLOR_DEPTH_141414, "14 bit" },
> + { COLOR_DEPTH_161616, "16 bit" },
> + { COLOR_DEPTH_999, "9 bit" },
> + { COLOR_DEPTH_11, "11 bit" },
> +};
> +
> +static const struct drm_prop_enum_list
> amdgpu_active_output_color_space_enum_list[] = {
> + { COLOR_SPACE_UNKNOWN, "unknown" },
> + { COLOR_SPACE_SRGB, "sRGB" },
> + { COLOR_SPACE_SRGB_LIMITED, "sRGB limited" },
> + { COLOR_SPACE_YCBCR601, "YCbCr 601" },
> + { COLOR_SPACE_YCBCR709, "YCbCr 709" },
> + { COLOR_SPACE_YCBCR601_LIMITED, "YCbCr 601 limited" },
> + { COLOR_SPACE_YCBCR709_LIMITED, "YCbCr 709 limited" },
> + { COLOR_SPACE_2020_RGB_FULLRANGE, "RGB 2020" },
> + { COLOR_SPACE_2020_RGB_LIMITEDRANGE, "RGB 2020 limited" },
> + { COLOR_SPACE_2020_YCBCR, "YCbCr 2020" },
> + { COLOR_SPACE_ADOBERGB, "Adobe RGB" },
> +};
> +
> int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
> {
> int sz;
> @@ -1038,6 +1072,30 @@ int amdgpu_display_modeset_create_props(struct
> amdgpu_device *adev)
> "abm level", 0, 4);
> if (!adev->mode_info.abm_level_property)
> return -ENOMEM;
> +
> + sz = ARRAY_SIZE(amdgpu_active_pixel_encoding_enum_list);
> + adev->mode_info.active_pixel_encoding_property =
> + drm_property_create_enum(adev_to_drm(adev), 0,
> + "active pixel encoding",
> + amdgpu_active_pixel_encoding_enum_list, sz);
> + if (!adev->mode_info.active_pixel_encoding_property)
> + return -ENOMEM;
> +
> + sz = ARRAY_SIZE(amdgpu_active_display_color_depth_enum_list);
> + adev->mode_info.active_display_color_depth_property =
> + drm_property_create_enum(adev_to_drm(adev), 0,
> + "active display color depth",
> + amdgpu_active_display_color_depth_enum_list,
> sz);
> + if (!adev->mode_info.active_display_color_depth_property)
> + return -ENOMEM;
> +
> + sz = ARRAY_SIZE(amdgpu_active_output_color_space_enum_list);
> + adev->mode_info.active_output_color_space_property =
> + drm_property_create_enum(adev_to_drm(adev), 0,
> + "active output color space",
> + amdgpu_active_output_color_space_enum_list,
> sz);
> + if (!adev->mode_info.active_output_color_space_property)
> + return -ENOMEM;
> }
>
> return 0;
> diff --git
[PATCH] drm/amd/display: Expose active display color configurations to userspace
xrandr --prop and other userspace info tools have currently no way of
telling which color configuration is used on HDMI and DP ports.
The ongoing transsition from HDMI 1.4 to 2.0 and the different bandwidth
requirements of YCbCr 4:2:0 and RGB color format raise different
incompatibilities. Having these configuration information readily
available is a useful tool in debuging washed out colors, color artefacts
on small fonts and missing refreshrate options.
Signed-off-by: Werner Sembach
---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 58 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 4 ++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36
3 files changed, 98 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index f753e04fee99..c0404bcda31b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -986,6 +986,40 @@ static const struct drm_prop_enum_list
amdgpu_dither_enum_list[] =
{ AMDGPU_FMT_DITHER_ENABLE, "on" },
};
+static const struct drm_prop_enum_list
amdgpu_active_pixel_encoding_enum_list[] = {
+ { PIXEL_ENCODING_UNDEFINED, "undefined" },
+ { PIXEL_ENCODING_RGB, "RGB" },
+ { PIXEL_ENCODING_YCBCR422, "YCbCr 4:2:2" },
+ { PIXEL_ENCODING_YCBCR444, "YCbCr 4:4:4" },
+ { PIXEL_ENCODING_YCBCR420, "YCbCr 4:2:0" },
+};
+
+static const struct drm_prop_enum_list
amdgpu_active_display_color_depth_enum_list[] = {
+ { COLOR_DEPTH_UNDEFINED, "undefined" },
+ { COLOR_DEPTH_666, "6 bit" },
+ { COLOR_DEPTH_888, "8 bit" },
+ { COLOR_DEPTH_101010, "10 bit" },
+ { COLOR_DEPTH_121212, "12 bit" },
+ { COLOR_DEPTH_141414, "14 bit" },
+ { COLOR_DEPTH_161616, "16 bit" },
+ { COLOR_DEPTH_999, "9 bit" },
+ { COLOR_DEPTH_11, "11 bit" },
+};
+
+static const struct drm_prop_enum_list
amdgpu_active_output_color_space_enum_list[] = {
+ { COLOR_SPACE_UNKNOWN, "unknown" },
+ { COLOR_SPACE_SRGB, "sRGB" },
+ { COLOR_SPACE_SRGB_LIMITED, "sRGB limited" },
+ { COLOR_SPACE_YCBCR601, "YCbCr 601" },
+ { COLOR_SPACE_YCBCR709, "YCbCr 709" },
+ { COLOR_SPACE_YCBCR601_LIMITED, "YCbCr 601 limited" },
+ { COLOR_SPACE_YCBCR709_LIMITED, "YCbCr 709 limited" },
+ { COLOR_SPACE_2020_RGB_FULLRANGE, "RGB 2020" },
+ { COLOR_SPACE_2020_RGB_LIMITEDRANGE, "RGB 2020 limited" },
+ { COLOR_SPACE_2020_YCBCR, "YCbCr 2020" },
+ { COLOR_SPACE_ADOBERGB, "Adobe RGB" },
+};
+
int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
{
int sz;
@@ -1038,6 +1072,30 @@ int amdgpu_display_modeset_create_props(struct
amdgpu_device *adev)
"abm level", 0, 4);
if (!adev->mode_info.abm_level_property)
return -ENOMEM;
+
+ sz = ARRAY_SIZE(amdgpu_active_pixel_encoding_enum_list);
+ adev->mode_info.active_pixel_encoding_property =
+ drm_property_create_enum(adev_to_drm(adev), 0,
+ "active pixel encoding",
+ amdgpu_active_pixel_encoding_enum_list, sz);
+ if (!adev->mode_info.active_pixel_encoding_property)
+ return -ENOMEM;
+
+ sz = ARRAY_SIZE(amdgpu_active_display_color_depth_enum_list);
+ adev->mode_info.active_display_color_depth_property =
+ drm_property_create_enum(adev_to_drm(adev), 0,
+ "active display color depth",
+ amdgpu_active_display_color_depth_enum_list,
sz);
+ if (!adev->mode_info.active_display_color_depth_property)
+ return -ENOMEM;
+
+ sz = ARRAY_SIZE(amdgpu_active_output_color_space_enum_list);
+ adev->mode_info.active_output_color_space_property =
+ drm_property_create_enum(adev_to_drm(adev), 0,
+ "active output color space",
+ amdgpu_active_output_color_space_enum_list, sz);
+ if (!adev->mode_info.active_output_color_space_property)
+ return -ENOMEM;
}
return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 319cb19e1b99..ad43af6a878d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -337,6 +337,10 @@ struct amdgpu_mode_info {
struct drm_property *dither_property;
/* Adaptive Backlight Modulation (power feature) */
struct drm_property *abm_level_property;
+ /* Color settings */
+ struct drm_property *active_pixel_encoding_property;
+ struct drm_property *active_display_color_depth_property;
+ struct drm_property
