[PATCH] drm/amd/display: Expose active display color configurations to userspace

[PATCH] drm/amd/display: Expose active display color configurations to userspace

[Werner Sembach]

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,
-- 
2.25.1

[PATCH] drm/amd/display: Expose active display color configurations to userspace

[Werner Sembach]

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,
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Alex Deucher]

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
>

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Alex Deucher]

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
>

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Alex Deucher]

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
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Pekka Paalanen]

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

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

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

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Pekka Paalanen]

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

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

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

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 2305 bytes --]

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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Werner Sembach]

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

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Werner Sembach]

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

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Werner Sembach]

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Pekka Paalanen]

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

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

> 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

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

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Pekka Paalanen]

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

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

> 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

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

Re: [PATCH] drm/amd/display: Expose active display color configurations to userspace

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 7563 bytes --]

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

> 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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx