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 wse@tuxedocomputers.com --- 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_111111, "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 *active_output_color_space_property; /* hardcoded DFP edid from BIOS */ struct edid *bios_hardcoded_edid; int bios_hardcoded_edid_size; 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 d699a5cf6c11..89465f74ca59 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5592,8 +5592,13 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector, struct amdgpu_device *adev = drm_to_adev(dev); struct dm_connector_state *dm_state = to_dm_connector_state(state); + struct dm_crtc_state *dm_crtc_state = NULL; int ret = -EINVAL;
+ if (state->crtc != NULL && state->crtc->state != NULL) { + dm_crtc_state = to_dm_crtc_state(state->crtc->state); + } + if (property == dev->mode_config.scaling_mode_property) { switch (dm_state->scaling) { case RMX_CENTER: @@ -5623,6 +5628,21 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector, } else if (property == adev->mode_info.abm_level_property) { *val = dm_state->abm_level; ret = 0; + } else if (property == adev->mode_info.active_pixel_encoding_property) { + *val = PIXEL_ENCODING_UNDEFINED; + if (dm_crtc_state != NULL && dm_crtc_state->stream != NULL) + *val = dm_crtc_state->stream->timing.pixel_encoding; + ret = 0; + } else if (property == adev->mode_info.active_display_color_depth_property) { + *val = COLOR_DEPTH_UNDEFINED; + if (dm_crtc_state != NULL && dm_crtc_state->stream != NULL) + *val = dm_crtc_state->stream->timing.display_color_depth; + ret = 0; + } else if (property == adev->mode_info.active_output_color_space_property) { + *val = COLOR_SPACE_UNKNOWN; + if (dm_crtc_state != NULL && dm_crtc_state->stream != NULL) + *val = dm_crtc_state->stream->output_color_space; + ret = 0; }
return ret; @@ -7083,6 +7103,22 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, drm_connector_attach_content_protection_property(&aconnector->base, true); #endif } + + if (adev->mode_info.active_pixel_encoding_property) { + drm_object_attach_property(&aconnector->base.base, + adev->mode_info.active_pixel_encoding_property, 0); + DRM_DEBUG_DRIVER("amdgpu: attached active pixel encoding drm property"); + } + if (adev->mode_info.active_display_color_depth_property) { + drm_object_attach_property(&aconnector->base.base, + adev->mode_info.active_display_color_depth_property, 0); + DRM_DEBUG_DRIVER("amdgpu: attached active color depth drm property"); + } + if (adev->mode_info.active_output_color_space_property) { + drm_object_attach_property(&aconnector->base.base, + adev->mode_info.active_output_color_space_property, 0); + DRM_DEBUG_DRIVER("amdgpu: attached active output color space drm property"); + } }
static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
On Fri, May 7, 2021 at 3:27 PM Werner Sembach wse@tuxedocomputers.com 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 wse@tuxedocomputers.com
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_111111, "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 *active_output_color_space_property; /* hardcoded DFP edid from BIOS */ struct edid *bios_hardcoded_edid; int bios_hardcoded_edid_size;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 d699a5cf6c11..89465f74ca59 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5592,8 +5592,13 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector, struct amdgpu_device *adev = drm_to_adev(dev); struct dm_connector_state *dm_state = to_dm_connector_state(state);
struct dm_crtc_state *dm_crtc_state = NULL; int ret = -EINVAL;if (state->crtc != NULL && state->crtc->state != NULL) {dm_crtc_state = to_dm_crtc_state(state->crtc->state);}if (property == dev->mode_config.scaling_mode_property) { switch (dm_state->scaling) { case RMX_CENTER:@@ -5623,6 +5628,21 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector, } else if (property == adev->mode_info.abm_level_property) { *val = dm_state->abm_level; ret = 0;
} else if (property == adev->mode_info.active_pixel_encoding_property) {*val = PIXEL_ENCODING_UNDEFINED;if (dm_crtc_state != NULL && dm_crtc_state->stream != NULL)*val = dm_crtc_state->stream->timing.pixel_encoding;ret = 0;} else if (property == adev->mode_info.active_display_color_depth_property) {*val = COLOR_DEPTH_UNDEFINED;if (dm_crtc_state != NULL && dm_crtc_state->stream != NULL)*val = dm_crtc_state->stream->timing.display_color_depth;ret = 0;} else if (property == adev->mode_info.active_output_color_space_property) {*val = COLOR_SPACE_UNKNOWN;if (dm_crtc_state != NULL && dm_crtc_state->stream != NULL)*val = dm_crtc_state->stream->output_color_space;ret = 0; } return ret;@@ -7083,6 +7103,22 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, drm_connector_attach_content_protection_property(&aconnector->base, true); #endif }
if (adev->mode_info.active_pixel_encoding_property) {drm_object_attach_property(&aconnector->base.base,adev->mode_info.active_pixel_encoding_property, 0);DRM_DEBUG_DRIVER("amdgpu: attached active pixel encoding drm property");}if (adev->mode_info.active_display_color_depth_property) {drm_object_attach_property(&aconnector->base.base,adev->mode_info.active_display_color_depth_property, 0);DRM_DEBUG_DRIVER("amdgpu: attached active color depth drm property");}if (adev->mode_info.active_output_color_space_property) {drm_object_attach_property(&aconnector->base.base,adev->mode_info.active_output_color_space_property, 0);DRM_DEBUG_DRIVER("amdgpu: attached active output color space drm property");}}
static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
2.25.1
On Mon, 10 May 2021 17:47:01 -0400 Alex Deucher alexdeucher@gmail.com wrote:
On Fri, May 7, 2021 at 3:27 PM Werner Sembach wse@tuxedocomputers.com 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
Am 11.05.21 um 10:07 schrieb Pekka Paalanen:
On Mon, 10 May 2021 17:47:01 -0400 Alex Deucher alexdeucher@gmail.com wrote:
On Fri, May 7, 2021 at 3:27 PM Werner Sembach wse@tuxedocomputers.com 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/displa... in own structs https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/amd/displa...
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.... split up between dp and hdmi in own structs https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/i915/displ...
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 check which one actually gets used. While in theory the driver chooses the best/highest bpc within the max bpc setting a user might not be fully aware what his hardware is or isn't capable off. This is meant as a quick way to double check the setup.
Thanks, pq
On Tue, 11 May 2021 12:03:30 +0200 Werner Sembach wse@tuxedocomputers.com wrote:
Am 11.05.21 um 10:07 schrieb Pekka Paalanen:
On Mon, 10 May 2021 17:47:01 -0400 Alex Deucher alexdeucher@gmail.com wrote:
On Fri, May 7, 2021 at 3:27 PM Werner Sembach wse@tuxedocomputers.com 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/displa... in own structs https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/amd/displa...
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.... split up between dp and hdmi in own structs https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/gpu/drm/i915/displ...
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.
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).
This feedback is useful, IMO.
The alternative approach would be to just let KMS atomic commit fail if the exact given configuration does not work, but it would be really difficult for userspace to implement the necessary fallbacks and to try alternative configurations. Therefore I think setting preference and then checking what the driver actually achieved seems like a good compromise to me.
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.
That totally makes sense, and example 1 is actually a good reason why color management needs to be interested about these properties.
color depth: While "max bpc" can be used to change the color depth, there is currently no way to check which one actually gets used. While in theory the driver chooses the best/highest bpc within the max bpc setting a user might not be fully aware what his hardware is or isn't capable off. This is meant as a quick way to double check the setup.
Likewise, this is important information for color management.
I'm really happy to see these come up. The Weston CM&HDR work isn't quite there yet to start making use of these, but some month/year I bet it will.
Thanks, pq
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Pekka Paalanen -
Werner Sembach