[PATCH 0/7] Add "activ bpc" and "active color format" drm property

[PATCH 0/7] Add "activ bpc" and "active color format" drm property

[Werner Sembach]

I started work on my proposal for better color handling in Linux display
drivers: https://lkml.org/lkml/2021/5/12/764

In this 2nd revision the first two read-only properties are now implemented for
amdgpu and i915. I post it here to collect collect some additional feedback, if
someone sees an improvement opportunity.

I have already commited the first patch in this series independently as it fixes
a function already in use.

Some of the feedback from the first post is already implemented.

The actual update of the values is implemented in patch three and four and six
and seven in the atomic_commit_tail() function of amdgpu and atomic_commit()
function of i915 respectively. They do get updated more often than needed with
the current approach, but without harm since just the same value is written
again. A check if the update is required would be the same amount of
computation.

[PATCH 0/7] Add "activ bpc" and "active color format" drm property

[Werner Sembach]

I started work on my proposal for better color handling in Linux display
drivers: https://lkml.org/lkml/2021/5/12/764

In this 2nd revision the first two read-only properties are now implemented for
amdgpu and i915. I post it here to collect collect some additional feedback, if
someone sees an improvement opportunity.

I have already commited the first patch in this series independently as it fixes
a function already in use.

Some of the feedback from the first post is already implemented.

The actual update of the values is implemented in patch three and four and six
and seven in the atomic_commit_tail() function of amdgpu and atomic_commit()
function of i915 respectively. They do get updated more often than needed with
the current approach, but without harm since just the same value is written
again. A check if the update is required would be the same amount of
computation.


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

[Intel-gfx] [PATCH 0/7] Add "activ bpc" and "active color format" drm property

[Werner Sembach]

I started work on my proposal for better color handling in Linux display
drivers: https://lkml.org/lkml/2021/5/12/764

In this 2nd revision the first two read-only properties are now implemented for
amdgpu and i915. I post it here to collect collect some additional feedback, if
someone sees an improvement opportunity.

I have already commited the first patch in this series independently as it fixes
a function already in use.

Some of the feedback from the first post is already implemented.

The actual update of the values is implemented in patch three and four and six
and seven in the atomic_commit_tail() function of amdgpu and atomic_commit()
function of i915 respectively. They do get updated more often than needed with
the current approach, but without harm since just the same value is written
again. A check if the update is required would be the same amount of
computation.


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

[PATCH v2 1/7] drm/amd/display: Add missing cases convert_dc_color_depth_into_bpc

[Werner Sembach]

convert_dc_color_depth_into_bpc() that converts the enum dc_color_depth to an
integer had the casses for COLOR_DEPTH_999 and COLOR_DEPTH_111111 missing.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
 1 file changed, 4 insertions(+)

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 652cc1a0e450..dbbccbed7b20 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6515,6 +6515,10 @@ static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_de
 			return 14;
 		case COLOR_DEPTH_161616:
 			return 16;
+		case COLOR_DEPTH_999:
+			return 9;
+		case COLOR_DEPTH_111111:
+			return 11;
 		default:
 			break;
 		}
-- 
2.25.1

[PATCH v2 1/7] drm/amd/display: Add missing cases convert_dc_color_depth_into_bpc

[Werner Sembach]

convert_dc_color_depth_into_bpc() that converts the enum dc_color_depth to an
integer had the casses for COLOR_DEPTH_999 and COLOR_DEPTH_111111 missing.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
 1 file changed, 4 insertions(+)

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 652cc1a0e450..dbbccbed7b20 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6515,6 +6515,10 @@ static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_de
 			return 14;
 		case COLOR_DEPTH_161616:
 			return 16;
+		case COLOR_DEPTH_999:
+			return 9;
+		case COLOR_DEPTH_111111:
+			return 11;
 		default:
 			break;
 		}
-- 
2.25.1

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

[Intel-gfx] [PATCH v2 1/7] drm/amd/display: Add missing cases convert_dc_color_depth_into_bpc

[Werner Sembach]

convert_dc_color_depth_into_bpc() that converts the enum dc_color_depth to an
integer had the casses for COLOR_DEPTH_999 and COLOR_DEPTH_111111 missing.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
 1 file changed, 4 insertions(+)

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 652cc1a0e450..dbbccbed7b20 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6515,6 +6515,10 @@ static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_de
 			return 14;
 		case COLOR_DEPTH_161616:
 			return 16;
+		case COLOR_DEPTH_999:
+			return 9;
+		case COLOR_DEPTH_111111:
+			return 11;
 		default:
 			break;
 		}
-- 
2.25.1

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

[PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Werner Sembach]

Add a new general drm property "active bpc" which can be used by graphic drivers
to report the applied bit depth per pixel back to userspace.

While "max bpc" can be used to change the color depth, there was no way to check
which one actually got used. While in theory the driver chooses the best/highest
color depth 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.

In the future, automatic color calibration for screens might also depend on this
information being available.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
 drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 15 +++++++++++
 3 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 268bb69c2e2f..7ae4e40936b5 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = 0;
 	} else if (property == connector->max_bpc_property) {
 		*val = state->max_requested_bpc;
+	} else if (property == connector->active_bpc_property) {
+		*val = state->active_bpc;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 7631f76e7f34..c0c3c09bfed0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
  *	drm_connector_attach_max_bpc_property() to create and attach the
  *	property to the connector during initialization.
  *
+ * active bpc:
+ *	This read-only range property tells userspace the pixel color bit depth
+ *	actually used by the hardware display engine on "the cable" on a
+ *	connector. The chosen value depends on hardware capabilities, both
+ *	display engine and connected monitor, and the "max bpc" property.
+ *	Drivers shall use drm_connector_attach_active_bpc_property() to install
+ *	this property.
+ *
  * Connectors also have one standardized atomic property:
  *
  * CRTC_ID:
@@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
 
+/**
+ * drm_connector_attach_active_bpc_property - attach "active bpc" property
+ * @connector: connector to attach active bpc property on.
+ * @min: The minimum bit depth supported by the connector.
+ * @max: The maximum bit depth supported by the connector.
+ *
+ * This is used to check the applied bit depth on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
+					  int min, int max)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	prop = connector->active_bpc_property;
+	if (!prop) {
+		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
+		if (!prop)
+			return -ENOMEM;
+
+		connector->active_bpc_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, 0);
+	connector->state->active_bpc = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1922b278ffad..c58cba2b6afe 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -781,6 +781,13 @@ struct drm_connector_state {
 	 */
 	u8 max_bpc;
 
+	/**
+	 * @active_bpc: Read only property set by the GPU driver to the actually
+	 * applied bit depth of the pixels after evaluating all hardware
+	 * limitations.
+	 */
+	u8 active_bpc;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1380,6 +1387,12 @@ struct drm_connector {
 	 */
 	struct drm_property *max_bpc_property;
 
+	/**
+	 * @active_bpc_property: Default connector property for the active bpc
+	 * to be driven out of the connector.
+	 */
+	struct drm_property *active_bpc_property;
+
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
 #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
 	int width, int height);
 int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
+int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
+					  int min, int max);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.25.1

[PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Werner Sembach]

Add a new general drm property "active bpc" which can be used by graphic drivers
to report the applied bit depth per pixel back to userspace.

While "max bpc" can be used to change the color depth, there was no way to check
which one actually got used. While in theory the driver chooses the best/highest
color depth 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.

In the future, automatic color calibration for screens might also depend on this
information being available.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
 drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 15 +++++++++++
 3 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 268bb69c2e2f..7ae4e40936b5 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = 0;
 	} else if (property == connector->max_bpc_property) {
 		*val = state->max_requested_bpc;
+	} else if (property == connector->active_bpc_property) {
+		*val = state->active_bpc;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 7631f76e7f34..c0c3c09bfed0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
  *	drm_connector_attach_max_bpc_property() to create and attach the
  *	property to the connector during initialization.
  *
+ * active bpc:
+ *	This read-only range property tells userspace the pixel color bit depth
+ *	actually used by the hardware display engine on "the cable" on a
+ *	connector. The chosen value depends on hardware capabilities, both
+ *	display engine and connected monitor, and the "max bpc" property.
+ *	Drivers shall use drm_connector_attach_active_bpc_property() to install
+ *	this property.
+ *
  * Connectors also have one standardized atomic property:
  *
  * CRTC_ID:
@@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
 
+/**
+ * drm_connector_attach_active_bpc_property - attach "active bpc" property
+ * @connector: connector to attach active bpc property on.
+ * @min: The minimum bit depth supported by the connector.
+ * @max: The maximum bit depth supported by the connector.
+ *
+ * This is used to check the applied bit depth on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
+					  int min, int max)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	prop = connector->active_bpc_property;
+	if (!prop) {
+		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
+		if (!prop)
+			return -ENOMEM;
+
+		connector->active_bpc_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, 0);
+	connector->state->active_bpc = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1922b278ffad..c58cba2b6afe 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -781,6 +781,13 @@ struct drm_connector_state {
 	 */
 	u8 max_bpc;
 
+	/**
+	 * @active_bpc: Read only property set by the GPU driver to the actually
+	 * applied bit depth of the pixels after evaluating all hardware
+	 * limitations.
+	 */
+	u8 active_bpc;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1380,6 +1387,12 @@ struct drm_connector {
 	 */
 	struct drm_property *max_bpc_property;
 
+	/**
+	 * @active_bpc_property: Default connector property for the active bpc
+	 * to be driven out of the connector.
+	 */
+	struct drm_property *active_bpc_property;
+
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
 #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
 	int width, int height);
 int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
+int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
+					  int min, int max);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.25.1

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

[Intel-gfx] [PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Werner Sembach]

Add a new general drm property "active bpc" which can be used by graphic drivers
to report the applied bit depth per pixel back to userspace.

While "max bpc" can be used to change the color depth, there was no way to check
which one actually got used. While in theory the driver chooses the best/highest
color depth 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.

In the future, automatic color calibration for screens might also depend on this
information being available.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
 drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 15 +++++++++++
 3 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 268bb69c2e2f..7ae4e40936b5 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = 0;
 	} else if (property == connector->max_bpc_property) {
 		*val = state->max_requested_bpc;
+	} else if (property == connector->active_bpc_property) {
+		*val = state->active_bpc;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 7631f76e7f34..c0c3c09bfed0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
  *	drm_connector_attach_max_bpc_property() to create and attach the
  *	property to the connector during initialization.
  *
+ * active bpc:
+ *	This read-only range property tells userspace the pixel color bit depth
+ *	actually used by the hardware display engine on "the cable" on a
+ *	connector. The chosen value depends on hardware capabilities, both
+ *	display engine and connected monitor, and the "max bpc" property.
+ *	Drivers shall use drm_connector_attach_active_bpc_property() to install
+ *	this property.
+ *
  * Connectors also have one standardized atomic property:
  *
  * CRTC_ID:
@@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
 
+/**
+ * drm_connector_attach_active_bpc_property - attach "active bpc" property
+ * @connector: connector to attach active bpc property on.
+ * @min: The minimum bit depth supported by the connector.
+ * @max: The maximum bit depth supported by the connector.
+ *
+ * This is used to check the applied bit depth on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
+					  int min, int max)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	prop = connector->active_bpc_property;
+	if (!prop) {
+		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
+		if (!prop)
+			return -ENOMEM;
+
+		connector->active_bpc_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, 0);
+	connector->state->active_bpc = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1922b278ffad..c58cba2b6afe 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -781,6 +781,13 @@ struct drm_connector_state {
 	 */
 	u8 max_bpc;
 
+	/**
+	 * @active_bpc: Read only property set by the GPU driver to the actually
+	 * applied bit depth of the pixels after evaluating all hardware
+	 * limitations.
+	 */
+	u8 active_bpc;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1380,6 +1387,12 @@ struct drm_connector {
 	 */
 	struct drm_property *max_bpc_property;
 
+	/**
+	 * @active_bpc_property: Default connector property for the active bpc
+	 * to be driven out of the connector.
+	 */
+	struct drm_property *active_bpc_property;
+
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
 #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
 	int width, int height);
 int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
+int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
+					  int min, int max);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.25.1

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

Re: [PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Pekka Paalanen]

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

On Tue,  8 Jun 2021 19:43:15 +0200
Werner Sembach <wse@tuxedocomputers.com> wrote:

> Add a new general drm property "active bpc" which can be used by graphic drivers
> to report the applied bit depth per pixel back to userspace.
> 
> While "max bpc" can be used to change the color depth, there was no way to check
> which one actually got used. While in theory the driver chooses the best/highest
> color depth 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.
> 
> In the future, automatic color calibration for screens might also depend on this
> information being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 15 +++++++++++
>  3 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..7ae4e40936b5 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = 0;
>  	} else if (property == connector->max_bpc_property) {
>  		*val = state->max_requested_bpc;
> +	} else if (property == connector->active_bpc_property) {
> +		*val = state->active_bpc;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 7631f76e7f34..c0c3c09bfed0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	drm_connector_attach_max_bpc_property() to create and attach the
>   *	property to the connector during initialization.
>   *
> + * active bpc:
> + *	This read-only range property tells userspace the pixel color bit depth
> + *	actually used by the hardware display engine on "the cable" on a
> + *	connector. The chosen value depends on hardware capabilities, both
> + *	display engine and connected monitor, and the "max bpc" property.
> + *	Drivers shall use drm_connector_attach_active_bpc_property() to install
> + *	this property.
> + *

This description is now clear to me, but I wonder, is it also how
others understand it wrt. dithering?

Dithering done on monitor is irrelevant, because we are talking about
"on the cable" pixels. But since we are talking about "on the cable"
pixels, also dithering done by the display engine must not factor in.
Should the dithering done by display engine result in higher "active
bpc" number than what is actually transmitted on the cable?

I cannot guess what userspace would want exactly. I think the
strict "on the cable" interpretation is a safe bet, because it then
gives a lower limit on observed bpc. Dithering settings should be
exposed with other KMS properties, so userspace can factor those in.
But to be absolutely sure, we'd have to ask some color management
experts.

Cc'ing Mario in case he has an opinion.

Since "active bpc" is related to "max bpc", the both should follow the
same definition. Do they do that now?

Maybe a clarifying note about interaction with dithering would still be
good to have here.


I recall reading some comments from you about having problems with
making this immutable. Is it properly immutable now?

That is, drm_info reports the property as "(immutable)".
https://github.com/ascent12/drm_info

If we are not sure if DSC could result in lower observed bpc than
"active bpc", then DSC state would need to be exposed as a KMS property
too, with a note that it invalidates what "active bpc" shows. Or maybe
"active bpc" should be "unknown" in that case?


Thanks,
pq

>   * Connectors also have one standardized atomic property:
>   *
>   * CRTC_ID:
> @@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
>  
> +/**
> + * drm_connector_attach_active_bpc_property - attach "active bpc" property
> + * @connector: connector to attach active bpc property on.
> + * @min: The minimum bit depth supported by the connector.
> + * @max: The maximum bit depth supported by the connector.
> + *
> + * This is used to check the applied bit depth on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> +					  int min, int max)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = connector->active_bpc_property;
> +	if (!prop) {
> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_bpc_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_bpc = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1922b278ffad..c58cba2b6afe 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -781,6 +781,13 @@ struct drm_connector_state {
>  	 */
>  	u8 max_bpc;
>  
> +	/**
> +	 * @active_bpc: Read only property set by the GPU driver to the actually
> +	 * applied bit depth of the pixels after evaluating all hardware
> +	 * limitations.
> +	 */
> +	u8 active_bpc;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1380,6 +1387,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *max_bpc_property;
>  
> +	/**
> +	 * @active_bpc_property: Default connector property for the active bpc
> +	 * to be driven out of the connector.
> +	 */
> +	struct drm_property *active_bpc_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
>  	int width, int height);
>  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> +					  int min, int max);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata


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

Re: [PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Pekka Paalanen]

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

On Tue,  8 Jun 2021 19:43:15 +0200
Werner Sembach <wse@tuxedocomputers.com> wrote:

> Add a new general drm property "active bpc" which can be used by graphic drivers
> to report the applied bit depth per pixel back to userspace.
> 
> While "max bpc" can be used to change the color depth, there was no way to check
> which one actually got used. While in theory the driver chooses the best/highest
> color depth 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.
> 
> In the future, automatic color calibration for screens might also depend on this
> information being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 15 +++++++++++
>  3 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..7ae4e40936b5 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = 0;
>  	} else if (property == connector->max_bpc_property) {
>  		*val = state->max_requested_bpc;
> +	} else if (property == connector->active_bpc_property) {
> +		*val = state->active_bpc;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 7631f76e7f34..c0c3c09bfed0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	drm_connector_attach_max_bpc_property() to create and attach the
>   *	property to the connector during initialization.
>   *
> + * active bpc:
> + *	This read-only range property tells userspace the pixel color bit depth
> + *	actually used by the hardware display engine on "the cable" on a
> + *	connector. The chosen value depends on hardware capabilities, both
> + *	display engine and connected monitor, and the "max bpc" property.
> + *	Drivers shall use drm_connector_attach_active_bpc_property() to install
> + *	this property.
> + *

This description is now clear to me, but I wonder, is it also how
others understand it wrt. dithering?

Dithering done on monitor is irrelevant, because we are talking about
"on the cable" pixels. But since we are talking about "on the cable"
pixels, also dithering done by the display engine must not factor in.
Should the dithering done by display engine result in higher "active
bpc" number than what is actually transmitted on the cable?

I cannot guess what userspace would want exactly. I think the
strict "on the cable" interpretation is a safe bet, because it then
gives a lower limit on observed bpc. Dithering settings should be
exposed with other KMS properties, so userspace can factor those in.
But to be absolutely sure, we'd have to ask some color management
experts.

Cc'ing Mario in case he has an opinion.

Since "active bpc" is related to "max bpc", the both should follow the
same definition. Do they do that now?

Maybe a clarifying note about interaction with dithering would still be
good to have here.


I recall reading some comments from you about having problems with
making this immutable. Is it properly immutable now?

That is, drm_info reports the property as "(immutable)".
https://github.com/ascent12/drm_info

If we are not sure if DSC could result in lower observed bpc than
"active bpc", then DSC state would need to be exposed as a KMS property
too, with a note that it invalidates what "active bpc" shows. Or maybe
"active bpc" should be "unknown" in that case?


Thanks,
pq

>   * Connectors also have one standardized atomic property:
>   *
>   * CRTC_ID:
> @@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
>  
> +/**
> + * drm_connector_attach_active_bpc_property - attach "active bpc" property
> + * @connector: connector to attach active bpc property on.
> + * @min: The minimum bit depth supported by the connector.
> + * @max: The maximum bit depth supported by the connector.
> + *
> + * This is used to check the applied bit depth on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> +					  int min, int max)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = connector->active_bpc_property;
> +	if (!prop) {
> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_bpc_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_bpc = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1922b278ffad..c58cba2b6afe 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -781,6 +781,13 @@ struct drm_connector_state {
>  	 */
>  	u8 max_bpc;
>  
> +	/**
> +	 * @active_bpc: Read only property set by the GPU driver to the actually
> +	 * applied bit depth of the pixels after evaluating all hardware
> +	 * limitations.
> +	 */
> +	u8 active_bpc;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1380,6 +1387,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *max_bpc_property;
>  
> +	/**
> +	 * @active_bpc_property: Default connector property for the active bpc
> +	 * to be driven out of the connector.
> +	 */
> +	struct drm_property *active_bpc_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
>  	int width, int height);
>  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> +					  int min, int max);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata


[-- 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: [Intel-gfx] [PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Pekka Paalanen]

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

On Tue,  8 Jun 2021 19:43:15 +0200
Werner Sembach <wse@tuxedocomputers.com> wrote:

> Add a new general drm property "active bpc" which can be used by graphic drivers
> to report the applied bit depth per pixel back to userspace.
> 
> While "max bpc" can be used to change the color depth, there was no way to check
> which one actually got used. While in theory the driver chooses the best/highest
> color depth 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.
> 
> In the future, automatic color calibration for screens might also depend on this
> information being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 15 +++++++++++
>  3 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..7ae4e40936b5 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = 0;
>  	} else if (property == connector->max_bpc_property) {
>  		*val = state->max_requested_bpc;
> +	} else if (property == connector->active_bpc_property) {
> +		*val = state->active_bpc;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 7631f76e7f34..c0c3c09bfed0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	drm_connector_attach_max_bpc_property() to create and attach the
>   *	property to the connector during initialization.
>   *
> + * active bpc:
> + *	This read-only range property tells userspace the pixel color bit depth
> + *	actually used by the hardware display engine on "the cable" on a
> + *	connector. The chosen value depends on hardware capabilities, both
> + *	display engine and connected monitor, and the "max bpc" property.
> + *	Drivers shall use drm_connector_attach_active_bpc_property() to install
> + *	this property.
> + *

This description is now clear to me, but I wonder, is it also how
others understand it wrt. dithering?

Dithering done on monitor is irrelevant, because we are talking about
"on the cable" pixels. But since we are talking about "on the cable"
pixels, also dithering done by the display engine must not factor in.
Should the dithering done by display engine result in higher "active
bpc" number than what is actually transmitted on the cable?

I cannot guess what userspace would want exactly. I think the
strict "on the cable" interpretation is a safe bet, because it then
gives a lower limit on observed bpc. Dithering settings should be
exposed with other KMS properties, so userspace can factor those in.
But to be absolutely sure, we'd have to ask some color management
experts.

Cc'ing Mario in case he has an opinion.

Since "active bpc" is related to "max bpc", the both should follow the
same definition. Do they do that now?

Maybe a clarifying note about interaction with dithering would still be
good to have here.


I recall reading some comments from you about having problems with
making this immutable. Is it properly immutable now?

That is, drm_info reports the property as "(immutable)".
https://github.com/ascent12/drm_info

If we are not sure if DSC could result in lower observed bpc than
"active bpc", then DSC state would need to be exposed as a KMS property
too, with a note that it invalidates what "active bpc" shows. Or maybe
"active bpc" should be "unknown" in that case?


Thanks,
pq

>   * Connectors also have one standardized atomic property:
>   *
>   * CRTC_ID:
> @@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
>  
> +/**
> + * drm_connector_attach_active_bpc_property - attach "active bpc" property
> + * @connector: connector to attach active bpc property on.
> + * @min: The minimum bit depth supported by the connector.
> + * @max: The maximum bit depth supported by the connector.
> + *
> + * This is used to check the applied bit depth on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> +					  int min, int max)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = connector->active_bpc_property;
> +	if (!prop) {
> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_bpc_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_bpc = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1922b278ffad..c58cba2b6afe 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -781,6 +781,13 @@ struct drm_connector_state {
>  	 */
>  	u8 max_bpc;
>  
> +	/**
> +	 * @active_bpc: Read only property set by the GPU driver to the actually
> +	 * applied bit depth of the pixels after evaluating all hardware
> +	 * limitations.
> +	 */
> +	u8 active_bpc;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1380,6 +1387,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *max_bpc_property;
>  
> +	/**
> +	 * @active_bpc_property: Default connector property for the active bpc
> +	 * to be driven out of the connector.
> +	 */
> +	struct drm_property *active_bpc_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
>  	int width, int height);
>  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> +					  int min, int max);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata


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

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

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

Re: [PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Pekka Paalanen]

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

On Tue,  8 Jun 2021 19:43:15 +0200
Werner Sembach <wse@tuxedocomputers.com> wrote:

> Add a new general drm property "active bpc" which can be used by graphic drivers
> to report the applied bit depth per pixel back to userspace.
> 
> While "max bpc" can be used to change the color depth, there was no way to check
> which one actually got used. While in theory the driver chooses the best/highest
> color depth 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.
> 
> In the future, automatic color calibration for screens might also depend on this
> information being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 15 +++++++++++
>  3 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..7ae4e40936b5 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = 0;
>  	} else if (property == connector->max_bpc_property) {
>  		*val = state->max_requested_bpc;
> +	} else if (property == connector->active_bpc_property) {
> +		*val = state->active_bpc;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 7631f76e7f34..c0c3c09bfed0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	drm_connector_attach_max_bpc_property() to create and attach the
>   *	property to the connector during initialization.
>   *
> + * active bpc:
> + *	This read-only range property tells userspace the pixel color bit depth
> + *	actually used by the hardware display engine on "the cable" on a
> + *	connector. The chosen value depends on hardware capabilities, both
> + *	display engine and connected monitor, and the "max bpc" property.
> + *	Drivers shall use drm_connector_attach_active_bpc_property() to install
> + *	this property.
> + *

This description is now clear to me, but I wonder, is it also how
others understand it wrt. dithering?

Dithering done on monitor is irrelevant, because we are talking about
"on the cable" pixels. But since we are talking about "on the cable"
pixels, also dithering done by the display engine must not factor in.
Should the dithering done by display engine result in higher "active
bpc" number than what is actually transmitted on the cable?

I cannot guess what userspace would want exactly. I think the
strict "on the cable" interpretation is a safe bet, because it then
gives a lower limit on observed bpc. Dithering settings should be
exposed with other KMS properties, so userspace can factor those in.
But to be absolutely sure, we'd have to ask some color management
experts.

Cc'ing Mario in case he has an opinion.

Since "active bpc" is related to "max bpc", the both should follow the
same definition. Do they do that now?

Maybe a clarifying note about interaction with dithering would still be
good to have here.


I recall reading some comments from you about having problems with
making this immutable. Is it properly immutable now?

That is, drm_info reports the property as "(immutable)".
https://github.com/ascent12/drm_info

If we are not sure if DSC could result in lower observed bpc than
"active bpc", then DSC state would need to be exposed as a KMS property
too, with a note that it invalidates what "active bpc" shows. Or maybe
"active bpc" should be "unknown" in that case?


Thanks,
pq

>   * Connectors also have one standardized atomic property:
>   *
>   * CRTC_ID:
> @@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
>  
> +/**
> + * drm_connector_attach_active_bpc_property - attach "active bpc" property
> + * @connector: connector to attach active bpc property on.
> + * @min: The minimum bit depth supported by the connector.
> + * @max: The maximum bit depth supported by the connector.
> + *
> + * This is used to check the applied bit depth on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> +					  int min, int max)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = connector->active_bpc_property;
> +	if (!prop) {
> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_bpc_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_bpc = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1922b278ffad..c58cba2b6afe 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -781,6 +781,13 @@ struct drm_connector_state {
>  	 */
>  	u8 max_bpc;
>  
> +	/**
> +	 * @active_bpc: Read only property set by the GPU driver to the actually
> +	 * applied bit depth of the pixels after evaluating all hardware
> +	 * limitations.
> +	 */
> +	u8 active_bpc;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1380,6 +1387,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *max_bpc_property;
>  
> +	/**
> +	 * @active_bpc_property: Default connector property for the active bpc
> +	 * to be driven out of the connector.
> +	 */
> +	struct drm_property *active_bpc_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
>  	int width, int height);
>  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
> +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> +					  int min, int max);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata


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

Re: [PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Mario Kleiner]

On Thu, Jun 10, 2021 at 9:55 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Tue,  8 Jun 2021 19:43:15 +0200
> Werner Sembach <wse@tuxedocomputers.com> wrote:
>
> > Add a new general drm property "active bpc" which can be used by graphic drivers
> > to report the applied bit depth per pixel back to userspace.
> >

Maybe "bit depth per pixel" -> "bit depth per pixel color component"
for slightly more clarity?

> > While "max bpc" can be used to change the color depth, there was no way to check
> > which one actually got used. While in theory the driver chooses the best/highest
> > color depth 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.
> >
> > In the future, automatic color calibration for screens might also depend on this
> > information being available.
> >
> > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
> >  drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       | 15 +++++++++++
> >  3 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 268bb69c2e2f..7ae4e40936b5 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >               *val = 0;
> >       } else if (property == connector->max_bpc_property) {
> >               *val = state->max_requested_bpc;
> > +     } else if (property == connector->active_bpc_property) {
> > +             *val = state->active_bpc;
> >       } else if (connector->funcs->atomic_get_property) {
> >               return connector->funcs->atomic_get_property(connector,
> >                               state, property, val);
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 7631f76e7f34..c0c3c09bfed0 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
> >   *   drm_connector_attach_max_bpc_property() to create and attach the
> >   *   property to the connector during initialization.
> >   *
> > + * active bpc:
> > + *   This read-only range property tells userspace the pixel color bit depth
> > + *   actually used by the hardware display engine on "the cable" on a
> > + *   connector. The chosen value depends on hardware capabilities, both
> > + *   display engine and connected monitor, and the "max bpc" property.
> > + *   Drivers shall use drm_connector_attach_active_bpc_property() to install
> > + *   this property.
> > + *
>
> This description is now clear to me, but I wonder, is it also how
> others understand it wrt. dithering?
>
> Dithering done on monitor is irrelevant, because we are talking about
> "on the cable" pixels. But since we are talking about "on the cable"
> pixels, also dithering done by the display engine must not factor in.
> Should the dithering done by display engine result in higher "active
> bpc" number than what is actually transmitted on the cable?
>
> I cannot guess what userspace would want exactly. I think the
> strict "on the cable" interpretation is a safe bet, because it then
> gives a lower limit on observed bpc. Dithering settings should be
> exposed with other KMS properties, so userspace can factor those in.
> But to be absolutely sure, we'd have to ask some color management
> experts.
>
> Cc'ing Mario in case he has an opinion.
>

Thanks. I like this a lot, in fact such a connector property was on my
todo list / wish list for something like that!

I agree with the "active bpc" definition here in this patch and
Pekka's comments. I want what goes out over the cable, not including
any effects of dithering. At least AMD's amdpu-kms driver exposes
"active bpc" already as a per-connector property in debugfs, and i use
reported output from there a lot to debug problems with respect to HDR
display or high color precision output, and to verify i'm not fooling
myself wrt. what goes out, compared to what dithering may "fake" on
top of it.

Software like mine would greatly benefit from getting this directly
off the connector, ie. as a RandR output property, just like with "max
bpc", as mapping X11 output names to driver output names is a guessing
game, directing regular users to those debugfs files is tedious and
error prone, and many regular users don't have root permissions
anyway.

Sometimes one wants to prioritize "active bpc" over resolution or
refresh rate, and especially on now more common HDR displays, and
actual bit depth also changes depending on bandwidth requirements vs.
availability, and how well DP link training went with a flaky or loose
cable, like only getting 10 bpc for HDR-10 when running on less than
maximum resolution or refresh rate, and the cable "just right". This
can be very puzzling without actual feedback over true "active bpc".

It would also be very beneficial to also have reporting and control
over gpu dithering state via a read/write property. Some drivers like
nouveau-kms have that, and i think some older non-atomic amd drivers
had it at some point in time iirc, which was useful, also for
debugging of dithering induced issues, when one wants to pass-through
a 8 bpc framebuffer unmodified to special display equipment, and
dithering silently kicked in and is messing things up.

And a read only property for DSC active would be good to account for the future.

> Since "active bpc" is related to "max bpc", the both should follow the
> same definition. Do they do that now?
>
> Maybe a clarifying note about interaction with dithering would still be
> good to have here.
>
+1

>
> I recall reading some comments from you about having problems with
> making this immutable. Is it properly immutable now?
>
> That is, drm_info reports the property as "(immutable)".
> https://github.com/ascent12/drm_info
>
> If we are not sure if DSC could result in lower observed bpc than
> "active bpc", then DSC state would need to be exposed as a KMS property
> too, with a note that it invalidates what "active bpc" shows. Or maybe
> "active bpc" should be "unknown" in that case?
>

Yes. Or could we have some way of disabling DSC per connector in the
future? I'm not familiar with current implementations, but i'd very
much would like to have a selectable tradeoff if i want a "pure" video
signal and maybe not get some high resolution / refresh rate modes on
low-bandwidth cables, or if i want max resolution/refresh but some
lossy perceptual compression.

thanks,
-mario

>
> Thanks,
> pq
>
> >   * Connectors also have one standardized atomic property:
> >   *
> >   * CRTC_ID:
> > @@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> >  }
> >  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
> >
> > +/**
> > + * drm_connector_attach_active_bpc_property - attach "active bpc" property
> > + * @connector: connector to attach active bpc property on.
> > + * @min: The minimum bit depth supported by the connector.
> > + * @max: The maximum bit depth supported by the connector.
> > + *
> > + * This is used to check the applied bit depth on a connector.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> > +                                       int min, int max)
> > +{
> > +     struct drm_device *dev = connector->dev;
> > +     struct drm_property *prop;
> > +
> > +     prop = connector->active_bpc_property;
> > +     if (!prop) {
> > +             prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> > +             if (!prop)
> > +                     return -ENOMEM;
> > +
> > +             connector->active_bpc_property = prop;
> > +     }
> > +
> > +     drm_object_attach_property(&connector->base, prop, 0);
> > +     connector->state->active_bpc = 0;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> > +
> >  /**
> >   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
> >   * capable property for a connector
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 1922b278ffad..c58cba2b6afe 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -781,6 +781,13 @@ struct drm_connector_state {
> >        */
> >       u8 max_bpc;
> >
> > +     /**
> > +      * @active_bpc: Read only property set by the GPU driver to the actually
> > +      * applied bit depth of the pixels after evaluating all hardware
> > +      * limitations.
> > +      */
> > +     u8 active_bpc;
> > +
> >       /**
> >        * @hdr_output_metadata:
> >        * DRM blob property for HDR output metadata
> > @@ -1380,6 +1387,12 @@ struct drm_connector {
> >        */
> >       struct drm_property *max_bpc_property;
> >
> > +     /**
> > +      * @active_bpc_property: Default connector property for the active bpc
> > +      * to be driven out of the connector.
> > +      */
> > +     struct drm_property *active_bpc_property;
> > +
> >  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> >  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
> >  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> > @@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
> >       int width, int height);
> >  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> >                                         int min, int max);
> > +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> > +                                       int min, int max);
> >
> >  /**
> >   * struct drm_tile_group - Tile group metadata
>

Re: [PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Mario Kleiner]

On Thu, Jun 10, 2021 at 9:55 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Tue,  8 Jun 2021 19:43:15 +0200
> Werner Sembach <wse@tuxedocomputers.com> wrote:
>
> > Add a new general drm property "active bpc" which can be used by graphic drivers
> > to report the applied bit depth per pixel back to userspace.
> >

Maybe "bit depth per pixel" -> "bit depth per pixel color component"
for slightly more clarity?

> > While "max bpc" can be used to change the color depth, there was no way to check
> > which one actually got used. While in theory the driver chooses the best/highest
> > color depth 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.
> >
> > In the future, automatic color calibration for screens might also depend on this
> > information being available.
> >
> > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
> >  drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       | 15 +++++++++++
> >  3 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 268bb69c2e2f..7ae4e40936b5 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >               *val = 0;
> >       } else if (property == connector->max_bpc_property) {
> >               *val = state->max_requested_bpc;
> > +     } else if (property == connector->active_bpc_property) {
> > +             *val = state->active_bpc;
> >       } else if (connector->funcs->atomic_get_property) {
> >               return connector->funcs->atomic_get_property(connector,
> >                               state, property, val);
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 7631f76e7f34..c0c3c09bfed0 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
> >   *   drm_connector_attach_max_bpc_property() to create and attach the
> >   *   property to the connector during initialization.
> >   *
> > + * active bpc:
> > + *   This read-only range property tells userspace the pixel color bit depth
> > + *   actually used by the hardware display engine on "the cable" on a
> > + *   connector. The chosen value depends on hardware capabilities, both
> > + *   display engine and connected monitor, and the "max bpc" property.
> > + *   Drivers shall use drm_connector_attach_active_bpc_property() to install
> > + *   this property.
> > + *
>
> This description is now clear to me, but I wonder, is it also how
> others understand it wrt. dithering?
>
> Dithering done on monitor is irrelevant, because we are talking about
> "on the cable" pixels. But since we are talking about "on the cable"
> pixels, also dithering done by the display engine must not factor in.
> Should the dithering done by display engine result in higher "active
> bpc" number than what is actually transmitted on the cable?
>
> I cannot guess what userspace would want exactly. I think the
> strict "on the cable" interpretation is a safe bet, because it then
> gives a lower limit on observed bpc. Dithering settings should be
> exposed with other KMS properties, so userspace can factor those in.
> But to be absolutely sure, we'd have to ask some color management
> experts.
>
> Cc'ing Mario in case he has an opinion.
>

Thanks. I like this a lot, in fact such a connector property was on my
todo list / wish list for something like that!

I agree with the "active bpc" definition here in this patch and
Pekka's comments. I want what goes out over the cable, not including
any effects of dithering. At least AMD's amdpu-kms driver exposes
"active bpc" already as a per-connector property in debugfs, and i use
reported output from there a lot to debug problems with respect to HDR
display or high color precision output, and to verify i'm not fooling
myself wrt. what goes out, compared to what dithering may "fake" on
top of it.

Software like mine would greatly benefit from getting this directly
off the connector, ie. as a RandR output property, just like with "max
bpc", as mapping X11 output names to driver output names is a guessing
game, directing regular users to those debugfs files is tedious and
error prone, and many regular users don't have root permissions
anyway.

Sometimes one wants to prioritize "active bpc" over resolution or
refresh rate, and especially on now more common HDR displays, and
actual bit depth also changes depending on bandwidth requirements vs.
availability, and how well DP link training went with a flaky or loose
cable, like only getting 10 bpc for HDR-10 when running on less than
maximum resolution or refresh rate, and the cable "just right". This
can be very puzzling without actual feedback over true "active bpc".

It would also be very beneficial to also have reporting and control
over gpu dithering state via a read/write property. Some drivers like
nouveau-kms have that, and i think some older non-atomic amd drivers
had it at some point in time iirc, which was useful, also for
debugging of dithering induced issues, when one wants to pass-through
a 8 bpc framebuffer unmodified to special display equipment, and
dithering silently kicked in and is messing things up.

And a read only property for DSC active would be good to account for the future.

> Since "active bpc" is related to "max bpc", the both should follow the
> same definition. Do they do that now?
>
> Maybe a clarifying note about interaction with dithering would still be
> good to have here.
>
+1

>
> I recall reading some comments from you about having problems with
> making this immutable. Is it properly immutable now?
>
> That is, drm_info reports the property as "(immutable)".
> https://github.com/ascent12/drm_info
>
> If we are not sure if DSC could result in lower observed bpc than
> "active bpc", then DSC state would need to be exposed as a KMS property
> too, with a note that it invalidates what "active bpc" shows. Or maybe
> "active bpc" should be "unknown" in that case?
>

Yes. Or could we have some way of disabling DSC per connector in the
future? I'm not familiar with current implementations, but i'd very
much would like to have a selectable tradeoff if i want a "pure" video
signal and maybe not get some high resolution / refresh rate modes on
low-bandwidth cables, or if i want max resolution/refresh but some
lossy perceptual compression.

thanks,
-mario

>
> Thanks,
> pq
>
> >   * Connectors also have one standardized atomic property:
> >   *
> >   * CRTC_ID:
> > @@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> >  }
> >  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
> >
> > +/**
> > + * drm_connector_attach_active_bpc_property - attach "active bpc" property
> > + * @connector: connector to attach active bpc property on.
> > + * @min: The minimum bit depth supported by the connector.
> > + * @max: The maximum bit depth supported by the connector.
> > + *
> > + * This is used to check the applied bit depth on a connector.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> > +                                       int min, int max)
> > +{
> > +     struct drm_device *dev = connector->dev;
> > +     struct drm_property *prop;
> > +
> > +     prop = connector->active_bpc_property;
> > +     if (!prop) {
> > +             prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> > +             if (!prop)
> > +                     return -ENOMEM;
> > +
> > +             connector->active_bpc_property = prop;
> > +     }
> > +
> > +     drm_object_attach_property(&connector->base, prop, 0);
> > +     connector->state->active_bpc = 0;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> > +
> >  /**
> >   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
> >   * capable property for a connector
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 1922b278ffad..c58cba2b6afe 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -781,6 +781,13 @@ struct drm_connector_state {
> >        */
> >       u8 max_bpc;
> >
> > +     /**
> > +      * @active_bpc: Read only property set by the GPU driver to the actually
> > +      * applied bit depth of the pixels after evaluating all hardware
> > +      * limitations.
> > +      */
> > +     u8 active_bpc;
> > +
> >       /**
> >        * @hdr_output_metadata:
> >        * DRM blob property for HDR output metadata
> > @@ -1380,6 +1387,12 @@ struct drm_connector {
> >        */
> >       struct drm_property *max_bpc_property;
> >
> > +     /**
> > +      * @active_bpc_property: Default connector property for the active bpc
> > +      * to be driven out of the connector.
> > +      */
> > +     struct drm_property *active_bpc_property;
> > +
> >  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> >  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
> >  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> > @@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
> >       int width, int height);
> >  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> >                                         int min, int max);
> > +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> > +                                       int min, int max);
> >
> >  /**
> >   * struct drm_tile_group - Tile group metadata
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Mario Kleiner]

On Thu, Jun 10, 2021 at 9:55 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Tue,  8 Jun 2021 19:43:15 +0200
> Werner Sembach <wse@tuxedocomputers.com> wrote:
>
> > Add a new general drm property "active bpc" which can be used by graphic drivers
> > to report the applied bit depth per pixel back to userspace.
> >

Maybe "bit depth per pixel" -> "bit depth per pixel color component"
for slightly more clarity?

> > While "max bpc" can be used to change the color depth, there was no way to check
> > which one actually got used. While in theory the driver chooses the best/highest
> > color depth 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.
> >
> > In the future, automatic color calibration for screens might also depend on this
> > information being available.
> >
> > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
> >  drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       | 15 +++++++++++
> >  3 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 268bb69c2e2f..7ae4e40936b5 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >               *val = 0;
> >       } else if (property == connector->max_bpc_property) {
> >               *val = state->max_requested_bpc;
> > +     } else if (property == connector->active_bpc_property) {
> > +             *val = state->active_bpc;
> >       } else if (connector->funcs->atomic_get_property) {
> >               return connector->funcs->atomic_get_property(connector,
> >                               state, property, val);
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 7631f76e7f34..c0c3c09bfed0 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
> >   *   drm_connector_attach_max_bpc_property() to create and attach the
> >   *   property to the connector during initialization.
> >   *
> > + * active bpc:
> > + *   This read-only range property tells userspace the pixel color bit depth
> > + *   actually used by the hardware display engine on "the cable" on a
> > + *   connector. The chosen value depends on hardware capabilities, both
> > + *   display engine and connected monitor, and the "max bpc" property.
> > + *   Drivers shall use drm_connector_attach_active_bpc_property() to install
> > + *   this property.
> > + *
>
> This description is now clear to me, but I wonder, is it also how
> others understand it wrt. dithering?
>
> Dithering done on monitor is irrelevant, because we are talking about
> "on the cable" pixels. But since we are talking about "on the cable"
> pixels, also dithering done by the display engine must not factor in.
> Should the dithering done by display engine result in higher "active
> bpc" number than what is actually transmitted on the cable?
>
> I cannot guess what userspace would want exactly. I think the
> strict "on the cable" interpretation is a safe bet, because it then
> gives a lower limit on observed bpc. Dithering settings should be
> exposed with other KMS properties, so userspace can factor those in.
> But to be absolutely sure, we'd have to ask some color management
> experts.
>
> Cc'ing Mario in case he has an opinion.
>

Thanks. I like this a lot, in fact such a connector property was on my
todo list / wish list for something like that!

I agree with the "active bpc" definition here in this patch and
Pekka's comments. I want what goes out over the cable, not including
any effects of dithering. At least AMD's amdpu-kms driver exposes
"active bpc" already as a per-connector property in debugfs, and i use
reported output from there a lot to debug problems with respect to HDR
display or high color precision output, and to verify i'm not fooling
myself wrt. what goes out, compared to what dithering may "fake" on
top of it.

Software like mine would greatly benefit from getting this directly
off the connector, ie. as a RandR output property, just like with "max
bpc", as mapping X11 output names to driver output names is a guessing
game, directing regular users to those debugfs files is tedious and
error prone, and many regular users don't have root permissions
anyway.

Sometimes one wants to prioritize "active bpc" over resolution or
refresh rate, and especially on now more common HDR displays, and
actual bit depth also changes depending on bandwidth requirements vs.
availability, and how well DP link training went with a flaky or loose
cable, like only getting 10 bpc for HDR-10 when running on less than
maximum resolution or refresh rate, and the cable "just right". This
can be very puzzling without actual feedback over true "active bpc".

It would also be very beneficial to also have reporting and control
over gpu dithering state via a read/write property. Some drivers like
nouveau-kms have that, and i think some older non-atomic amd drivers
had it at some point in time iirc, which was useful, also for
debugging of dithering induced issues, when one wants to pass-through
a 8 bpc framebuffer unmodified to special display equipment, and
dithering silently kicked in and is messing things up.

And a read only property for DSC active would be good to account for the future.

> Since "active bpc" is related to "max bpc", the both should follow the
> same definition. Do they do that now?
>
> Maybe a clarifying note about interaction with dithering would still be
> good to have here.
>
+1

>
> I recall reading some comments from you about having problems with
> making this immutable. Is it properly immutable now?
>
> That is, drm_info reports the property as "(immutable)".
> https://github.com/ascent12/drm_info
>
> If we are not sure if DSC could result in lower observed bpc than
> "active bpc", then DSC state would need to be exposed as a KMS property
> too, with a note that it invalidates what "active bpc" shows. Or maybe
> "active bpc" should be "unknown" in that case?
>

Yes. Or could we have some way of disabling DSC per connector in the
future? I'm not familiar with current implementations, but i'd very
much would like to have a selectable tradeoff if i want a "pure" video
signal and maybe not get some high resolution / refresh rate modes on
low-bandwidth cables, or if i want max resolution/refresh but some
lossy perceptual compression.

thanks,
-mario

>
> Thanks,
> pq
>
> >   * Connectors also have one standardized atomic property:
> >   *
> >   * CRTC_ID:
> > @@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> >  }
> >  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
> >
> > +/**
> > + * drm_connector_attach_active_bpc_property - attach "active bpc" property
> > + * @connector: connector to attach active bpc property on.
> > + * @min: The minimum bit depth supported by the connector.
> > + * @max: The maximum bit depth supported by the connector.
> > + *
> > + * This is used to check the applied bit depth on a connector.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> > +                                       int min, int max)
> > +{
> > +     struct drm_device *dev = connector->dev;
> > +     struct drm_property *prop;
> > +
> > +     prop = connector->active_bpc_property;
> > +     if (!prop) {
> > +             prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> > +             if (!prop)
> > +                     return -ENOMEM;
> > +
> > +             connector->active_bpc_property = prop;
> > +     }
> > +
> > +     drm_object_attach_property(&connector->base, prop, 0);
> > +     connector->state->active_bpc = 0;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> > +
> >  /**
> >   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
> >   * capable property for a connector
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 1922b278ffad..c58cba2b6afe 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -781,6 +781,13 @@ struct drm_connector_state {
> >        */
> >       u8 max_bpc;
> >
> > +     /**
> > +      * @active_bpc: Read only property set by the GPU driver to the actually
> > +      * applied bit depth of the pixels after evaluating all hardware
> > +      * limitations.
> > +      */
> > +     u8 active_bpc;
> > +
> >       /**
> >        * @hdr_output_metadata:
> >        * DRM blob property for HDR output metadata
> > @@ -1380,6 +1387,12 @@ struct drm_connector {
> >        */
> >       struct drm_property *max_bpc_property;
> >
> > +     /**
> > +      * @active_bpc_property: Default connector property for the active bpc
> > +      * to be driven out of the connector.
> > +      */
> > +     struct drm_property *active_bpc_property;
> > +
> >  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> >  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
> >  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> > @@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
> >       int width, int height);
> >  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> >                                         int min, int max);
> > +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> > +                                       int min, int max);
> >
> >  /**
> >   * struct drm_tile_group - Tile group metadata
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/7] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Mario Kleiner]

On Thu, Jun 10, 2021 at 9:55 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Tue,  8 Jun 2021 19:43:15 +0200
> Werner Sembach <wse@tuxedocomputers.com> wrote:
>
> > Add a new general drm property "active bpc" which can be used by graphic drivers
> > to report the applied bit depth per pixel back to userspace.
> >

Maybe "bit depth per pixel" -> "bit depth per pixel color component"
for slightly more clarity?

> > While "max bpc" can be used to change the color depth, there was no way to check
> > which one actually got used. While in theory the driver chooses the best/highest
> > color depth 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.
> >
> > In the future, automatic color calibration for screens might also depend on this
> > information being available.
> >
> > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
> >  drivers/gpu/drm/drm_connector.c   | 41 +++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       | 15 +++++++++++
> >  3 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 268bb69c2e2f..7ae4e40936b5 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >               *val = 0;
> >       } else if (property == connector->max_bpc_property) {
> >               *val = state->max_requested_bpc;
> > +     } else if (property == connector->active_bpc_property) {
> > +             *val = state->active_bpc;
> >       } else if (connector->funcs->atomic_get_property) {
> >               return connector->funcs->atomic_get_property(connector,
> >                               state, property, val);
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 7631f76e7f34..c0c3c09bfed0 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1195,6 +1195,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
> >   *   drm_connector_attach_max_bpc_property() to create and attach the
> >   *   property to the connector during initialization.
> >   *
> > + * active bpc:
> > + *   This read-only range property tells userspace the pixel color bit depth
> > + *   actually used by the hardware display engine on "the cable" on a
> > + *   connector. The chosen value depends on hardware capabilities, both
> > + *   display engine and connected monitor, and the "max bpc" property.
> > + *   Drivers shall use drm_connector_attach_active_bpc_property() to install
> > + *   this property.
> > + *
>
> This description is now clear to me, but I wonder, is it also how
> others understand it wrt. dithering?
>
> Dithering done on monitor is irrelevant, because we are talking about
> "on the cable" pixels. But since we are talking about "on the cable"
> pixels, also dithering done by the display engine must not factor in.
> Should the dithering done by display engine result in higher "active
> bpc" number than what is actually transmitted on the cable?
>
> I cannot guess what userspace would want exactly. I think the
> strict "on the cable" interpretation is a safe bet, because it then
> gives a lower limit on observed bpc. Dithering settings should be
> exposed with other KMS properties, so userspace can factor those in.
> But to be absolutely sure, we'd have to ask some color management
> experts.
>
> Cc'ing Mario in case he has an opinion.
>

Thanks. I like this a lot, in fact such a connector property was on my
todo list / wish list for something like that!

I agree with the "active bpc" definition here in this patch and
Pekka's comments. I want what goes out over the cable, not including
any effects of dithering. At least AMD's amdpu-kms driver exposes
"active bpc" already as a per-connector property in debugfs, and i use
reported output from there a lot to debug problems with respect to HDR
display or high color precision output, and to verify i'm not fooling
myself wrt. what goes out, compared to what dithering may "fake" on
top of it.

Software like mine would greatly benefit from getting this directly
off the connector, ie. as a RandR output property, just like with "max
bpc", as mapping X11 output names to driver output names is a guessing
game, directing regular users to those debugfs files is tedious and
error prone, and many regular users don't have root permissions
anyway.

Sometimes one wants to prioritize "active bpc" over resolution or
refresh rate, and especially on now more common HDR displays, and
actual bit depth also changes depending on bandwidth requirements vs.
availability, and how well DP link training went with a flaky or loose
cable, like only getting 10 bpc for HDR-10 when running on less than
maximum resolution or refresh rate, and the cable "just right". This
can be very puzzling without actual feedback over true "active bpc".

It would also be very beneficial to also have reporting and control
over gpu dithering state via a read/write property. Some drivers like
nouveau-kms have that, and i think some older non-atomic amd drivers
had it at some point in time iirc, which was useful, also for
debugging of dithering induced issues, when one wants to pass-through
a 8 bpc framebuffer unmodified to special display equipment, and
dithering silently kicked in and is messing things up.

And a read only property for DSC active would be good to account for the future.

> Since "active bpc" is related to "max bpc", the both should follow the
> same definition. Do they do that now?
>
> Maybe a clarifying note about interaction with dithering would still be
> good to have here.
>
+1

>
> I recall reading some comments from you about having problems with
> making this immutable. Is it properly immutable now?
>
> That is, drm_info reports the property as "(immutable)".
> https://github.com/ascent12/drm_info
>
> If we are not sure if DSC could result in lower observed bpc than
> "active bpc", then DSC state would need to be exposed as a KMS property
> too, with a note that it invalidates what "active bpc" shows. Or maybe
> "active bpc" should be "unknown" in that case?
>

Yes. Or could we have some way of disabling DSC per connector in the
future? I'm not familiar with current implementations, but i'd very
much would like to have a selectable tradeoff if i want a "pure" video
signal and maybe not get some high resolution / refresh rate modes on
low-bandwidth cables, or if i want max resolution/refresh but some
lossy perceptual compression.

thanks,
-mario

>
> Thanks,
> pq
>
> >   * Connectors also have one standardized atomic property:
> >   *
> >   * CRTC_ID:
> > @@ -2150,6 +2158,39 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> >  }
> >  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
> >
> > +/**
> > + * drm_connector_attach_active_bpc_property - attach "active bpc" property
> > + * @connector: connector to attach active bpc property on.
> > + * @min: The minimum bit depth supported by the connector.
> > + * @max: The maximum bit depth supported by the connector.
> > + *
> > + * This is used to check the applied bit depth on a connector.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> > +                                       int min, int max)
> > +{
> > +     struct drm_device *dev = connector->dev;
> > +     struct drm_property *prop;
> > +
> > +     prop = connector->active_bpc_property;
> > +     if (!prop) {
> > +             prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> > +             if (!prop)
> > +                     return -ENOMEM;
> > +
> > +             connector->active_bpc_property = prop;
> > +     }
> > +
> > +     drm_object_attach_property(&connector->base, prop, 0);
> > +     connector->state->active_bpc = 0;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> > +
> >  /**
> >   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
> >   * capable property for a connector
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 1922b278ffad..c58cba2b6afe 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -781,6 +781,13 @@ struct drm_connector_state {
> >        */
> >       u8 max_bpc;
> >
> > +     /**
> > +      * @active_bpc: Read only property set by the GPU driver to the actually
> > +      * applied bit depth of the pixels after evaluating all hardware
> > +      * limitations.
> > +      */
> > +     u8 active_bpc;
> > +
> >       /**
> >        * @hdr_output_metadata:
> >        * DRM blob property for HDR output metadata
> > @@ -1380,6 +1387,12 @@ struct drm_connector {
> >        */
> >       struct drm_property *max_bpc_property;
> >
> > +     /**
> > +      * @active_bpc_property: Default connector property for the active bpc
> > +      * to be driven out of the connector.
> > +      */
> > +     struct drm_property *active_bpc_property;
> > +
> >  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> >  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
> >  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> > @@ -1698,6 +1711,8 @@ int drm_connector_set_panel_orientation_with_quirk(
> >       int width, int height);
> >  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> >                                         int min, int max);
> > +int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
> > +                                       int min, int max);
> >
> >  /**
> >   * struct drm_tile_group - Tile group metadata
>

[PATCH v2 3/7] drm/amd/display: Add handling for new "active bpc" property

[Werner Sembach]

This commit implements the "active bpc" drm property for the AMD GPU driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 18 +++++++++++++++++-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c    |  4 +++-
 2 files changed, 20 insertions(+), 2 deletions(-)

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 dbbccbed7b20..e57b2b743d36 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7519,8 +7519,10 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				adev->mode_info.underscan_vborder_property,
 				0);
 
-	if (!aconnector->mst_port)
+	if (!aconnector->mst_port) {
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+	}
 
 	/* This defaults to the max in the range, but we want 8bpc for non-edp. */
 	aconnector->base.state->max_bpc = (connector_type == DRM_MODE_CONNECTOR_eDP) ? 16 : 8;
@@ -8890,6 +8892,20 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		mutex_unlock(&dm->dc_lock);
 	}
 
+	/* Extract information from crtc to communicate it to userspace as connector properties */
+	for_each_new_connector_in_state(state, connector, new_con_state, i) {
+		struct drm_crtc *crtc = new_con_state->crtc;
+		if (crtc) {
+			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+			if (dm_new_crtc_state->stream)
+				new_con_state->active_bpc = convert_dc_color_depth_into_bpc(
+					dm_new_crtc_state->stream->timing.display_color_depth);
+		}
+		else
+			new_con_state->active_bpc = 0;
+	}
+
 	/* Count number of newly disabled CRTCs for dropping PM refs later. */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
 				      new_crtc_state, i) {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 9b221db526dc..2a8dc6b2c6c7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -397,8 +397,10 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	}
 
 	connector->max_bpc_property = master->base.max_bpc_property;
-	if (connector->max_bpc_property)
+	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 8, 16);
+		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+	}
 
 	connector->vrr_capable_property = master->base.vrr_capable_property;
 	if (connector->vrr_capable_property)
-- 
2.25.1

[PATCH v2 3/7] drm/amd/display: Add handling for new "active bpc" property

[Werner Sembach]

This commit implements the "active bpc" drm property for the AMD GPU driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 18 +++++++++++++++++-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c    |  4 +++-
 2 files changed, 20 insertions(+), 2 deletions(-)

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 dbbccbed7b20..e57b2b743d36 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7519,8 +7519,10 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				adev->mode_info.underscan_vborder_property,
 				0);
 
-	if (!aconnector->mst_port)
+	if (!aconnector->mst_port) {
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+	}
 
 	/* This defaults to the max in the range, but we want 8bpc for non-edp. */
 	aconnector->base.state->max_bpc = (connector_type == DRM_MODE_CONNECTOR_eDP) ? 16 : 8;
@@ -8890,6 +8892,20 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		mutex_unlock(&dm->dc_lock);
 	}
 
+	/* Extract information from crtc to communicate it to userspace as connector properties */
+	for_each_new_connector_in_state(state, connector, new_con_state, i) {
+		struct drm_crtc *crtc = new_con_state->crtc;
+		if (crtc) {
+			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+			if (dm_new_crtc_state->stream)
+				new_con_state->active_bpc = convert_dc_color_depth_into_bpc(
+					dm_new_crtc_state->stream->timing.display_color_depth);
+		}
+		else
+			new_con_state->active_bpc = 0;
+	}
+
 	/* Count number of newly disabled CRTCs for dropping PM refs later. */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
 				      new_crtc_state, i) {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 9b221db526dc..2a8dc6b2c6c7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -397,8 +397,10 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	}
 
 	connector->max_bpc_property = master->base.max_bpc_property;
-	if (connector->max_bpc_property)
+	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 8, 16);
+		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+	}
 
 	connector->vrr_capable_property = master->base.vrr_capable_property;
 	if (connector->vrr_capable_property)
-- 
2.25.1

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

[Intel-gfx] [PATCH v2 3/7] drm/amd/display: Add handling for new "active bpc" property

[Werner Sembach]

This commit implements the "active bpc" drm property for the AMD GPU driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 18 +++++++++++++++++-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c    |  4 +++-
 2 files changed, 20 insertions(+), 2 deletions(-)

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 dbbccbed7b20..e57b2b743d36 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7519,8 +7519,10 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				adev->mode_info.underscan_vborder_property,
 				0);
 
-	if (!aconnector->mst_port)
+	if (!aconnector->mst_port) {
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+	}
 
 	/* This defaults to the max in the range, but we want 8bpc for non-edp. */
 	aconnector->base.state->max_bpc = (connector_type == DRM_MODE_CONNECTOR_eDP) ? 16 : 8;
@@ -8890,6 +8892,20 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		mutex_unlock(&dm->dc_lock);
 	}
 
+	/* Extract information from crtc to communicate it to userspace as connector properties */
+	for_each_new_connector_in_state(state, connector, new_con_state, i) {
+		struct drm_crtc *crtc = new_con_state->crtc;
+		if (crtc) {
+			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+			if (dm_new_crtc_state->stream)
+				new_con_state->active_bpc = convert_dc_color_depth_into_bpc(
+					dm_new_crtc_state->stream->timing.display_color_depth);
+		}
+		else
+			new_con_state->active_bpc = 0;
+	}
+
 	/* Count number of newly disabled CRTCs for dropping PM refs later. */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
 				      new_crtc_state, i) {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 9b221db526dc..2a8dc6b2c6c7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -397,8 +397,10 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	}
 
 	connector->max_bpc_property = master->base.max_bpc_property;
-	if (connector->max_bpc_property)
+	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 8, 16);
+		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+	}
 
 	connector->vrr_capable_property = master->base.vrr_capable_property;
 	if (connector->vrr_capable_property)
-- 
2.25.1

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

[PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Werner Sembach]

This commits implements the "active bpc" drm property for the Intel GPU driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
 drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
 drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
 4 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 64e9107d70f7..50c11b8770a7 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 {
 	struct intel_atomic_state *state = to_intel_atomic_state(_state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
 	int ret = 0;
 
 	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
@@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
 	intel_shared_dpll_swap_state(state);
 	intel_atomic_track_fbs(state);
 
+	/* Extract information from crtc to communicate it to userspace as connector properties */
+	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
+		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
+		if (crtc) {
+			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
+			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
+		}
+		else
+			new_conn_state->active_bpc = 0;
+	}
+
 	drm_atomic_state_get(&state->base);
 	INIT_WORK(&state->base.commit_work, intel_atomic_commit_work);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 642c60f3d9b1..67826ba976ed 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4671,10 +4671,14 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 		intel_attach_force_audio_property(connector);
 
 	intel_attach_broadcast_rgb_property(connector);
-	if (HAS_GMCH(dev_priv))
+	if (HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 6, 10);
-	else if (DISPLAY_VER(dev_priv) >= 5)
+		drm_connector_attach_active_bpc_property(connector, 6, 10);
+	}
+	else if (DISPLAY_VER(dev_priv) >= 5) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_bpc_property(connector, 6, 12);
+	}
 
 	/* Register HDMI colorspace for case of lspcon */
 	if (intel_bios_is_lspcon_present(dev_priv, port)) {
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 2daa3f67791e..5a1869dc2210 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -844,8 +844,10 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	 */
 	connector->max_bpc_property =
 		intel_dp->attached_connector->base.max_bpc_property;
-	if (connector->max_bpc_property)
+	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_bpc_property(connector, 6, 12);
+	}
 
 	return connector;
 
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index d69f0a6dc26d..8af78b27b6ce 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2463,8 +2463,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 		drm_object_attach_property(&connector->base,
 			connector->dev->mode_config.hdr_output_metadata_property, 0);
 
-	if (!HAS_GMCH(dev_priv))
+	if (!HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 8, 12);
+		drm_connector_attach_active_bpc_property(connector, 8, 12);
+	}
 }
 
 /*
-- 
2.25.1

[PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Werner Sembach]

This commits implements the "active bpc" drm property for the Intel GPU driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
 drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
 drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
 4 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 64e9107d70f7..50c11b8770a7 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 {
 	struct intel_atomic_state *state = to_intel_atomic_state(_state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
 	int ret = 0;
 
 	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
@@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
 	intel_shared_dpll_swap_state(state);
 	intel_atomic_track_fbs(state);
 
+	/* Extract information from crtc to communicate it to userspace as connector properties */
+	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
+		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
+		if (crtc) {
+			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
+			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
+		}
+		else
+			new_conn_state->active_bpc = 0;
+	}
+
 	drm_atomic_state_get(&state->base);
 	INIT_WORK(&state->base.commit_work, intel_atomic_commit_work);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 642c60f3d9b1..67826ba976ed 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4671,10 +4671,14 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 		intel_attach_force_audio_property(connector);
 
 	intel_attach_broadcast_rgb_property(connector);
-	if (HAS_GMCH(dev_priv))
+	if (HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 6, 10);
-	else if (DISPLAY_VER(dev_priv) >= 5)
+		drm_connector_attach_active_bpc_property(connector, 6, 10);
+	}
+	else if (DISPLAY_VER(dev_priv) >= 5) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_bpc_property(connector, 6, 12);
+	}
 
 	/* Register HDMI colorspace for case of lspcon */
 	if (intel_bios_is_lspcon_present(dev_priv, port)) {
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 2daa3f67791e..5a1869dc2210 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -844,8 +844,10 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	 */
 	connector->max_bpc_property =
 		intel_dp->attached_connector->base.max_bpc_property;
-	if (connector->max_bpc_property)
+	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_bpc_property(connector, 6, 12);
+	}
 
 	return connector;
 
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index d69f0a6dc26d..8af78b27b6ce 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2463,8 +2463,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 		drm_object_attach_property(&connector->base,
 			connector->dev->mode_config.hdr_output_metadata_property, 0);
 
-	if (!HAS_GMCH(dev_priv))
+	if (!HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 8, 12);
+		drm_connector_attach_active_bpc_property(connector, 8, 12);
+	}
 }
 
 /*
-- 
2.25.1

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

[Intel-gfx] [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Werner Sembach]

This commits implements the "active bpc" drm property for the Intel GPU driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
 drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
 drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
 4 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 64e9107d70f7..50c11b8770a7 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 {
 	struct intel_atomic_state *state = to_intel_atomic_state(_state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
 	int ret = 0;
 
 	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
@@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
 	intel_shared_dpll_swap_state(state);
 	intel_atomic_track_fbs(state);
 
+	/* Extract information from crtc to communicate it to userspace as connector properties */
+	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
+		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
+		if (crtc) {
+			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
+			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
+		}
+		else
+			new_conn_state->active_bpc = 0;
+	}
+
 	drm_atomic_state_get(&state->base);
 	INIT_WORK(&state->base.commit_work, intel_atomic_commit_work);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 642c60f3d9b1..67826ba976ed 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4671,10 +4671,14 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 		intel_attach_force_audio_property(connector);
 
 	intel_attach_broadcast_rgb_property(connector);
-	if (HAS_GMCH(dev_priv))
+	if (HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 6, 10);
-	else if (DISPLAY_VER(dev_priv) >= 5)
+		drm_connector_attach_active_bpc_property(connector, 6, 10);
+	}
+	else if (DISPLAY_VER(dev_priv) >= 5) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_bpc_property(connector, 6, 12);
+	}
 
 	/* Register HDMI colorspace for case of lspcon */
 	if (intel_bios_is_lspcon_present(dev_priv, port)) {
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 2daa3f67791e..5a1869dc2210 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -844,8 +844,10 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	 */
 	connector->max_bpc_property =
 		intel_dp->attached_connector->base.max_bpc_property;
-	if (connector->max_bpc_property)
+	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_bpc_property(connector, 6, 12);
+	}
 
 	return connector;
 
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index d69f0a6dc26d..8af78b27b6ce 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2463,8 +2463,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 		drm_object_attach_property(&connector->base,
 			connector->dev->mode_config.hdr_output_metadata_property, 0);
 
-	if (!HAS_GMCH(dev_priv))
+	if (!HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 8, 12);
+		drm_connector_attach_active_bpc_property(connector, 8, 12);
+	}
 }
 
 /*
-- 
2.25.1

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

Re: [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Maxime Ripard]

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

Hi

On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote:
> This commits implements the "active bpc" drm property for the Intel GPU driver.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
>  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
>  4 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 64e9107d70f7..50c11b8770a7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>  {
>  	struct intel_atomic_state *state = to_intel_atomic_state(_state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
>  	int ret = 0;
>  
>  	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	intel_shared_dpll_swap_state(state);
>  	intel_atomic_track_fbs(state);
>  
> +	/* Extract information from crtc to communicate it to userspace as connector properties */
> +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
> +		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
> +		if (crtc) {
> +			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> +		}
> +		else
> +			new_conn_state->active_bpc = 0;
> +	}
> +

This seems fairly intrusive, but also commit / commit_tail might not be
the best place to put this, we want to support it at the connector
level.

Indeed, this will cause some issue if your HDMI output is a bridge for
example, where the commit will be in an entirely different driver that
has no dependency on the HDMI controller one.

I think it would be best to do that assignment in atomic_check. That
way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it
would know what the output state would have been like.

Also, all of your patches don't follow the kernel coding style. Make
sure you fix the issues reported by checkpatch --strict

Maxime

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

Re: [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Maxime Ripard]

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

Hi

On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote:
> This commits implements the "active bpc" drm property for the Intel GPU driver.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
>  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
>  4 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 64e9107d70f7..50c11b8770a7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>  {
>  	struct intel_atomic_state *state = to_intel_atomic_state(_state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
>  	int ret = 0;
>  
>  	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	intel_shared_dpll_swap_state(state);
>  	intel_atomic_track_fbs(state);
>  
> +	/* Extract information from crtc to communicate it to userspace as connector properties */
> +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
> +		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
> +		if (crtc) {
> +			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> +		}
> +		else
> +			new_conn_state->active_bpc = 0;
> +	}
> +

This seems fairly intrusive, but also commit / commit_tail might not be
the best place to put this, we want to support it at the connector
level.

Indeed, this will cause some issue if your HDMI output is a bridge for
example, where the commit will be in an entirely different driver that
has no dependency on the HDMI controller one.

I think it would be best to do that assignment in atomic_check. That
way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it
would know what the output state would have been like.

Also, all of your patches don't follow the kernel coding style. Make
sure you fix the issues reported by checkpatch --strict

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 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: [Intel-gfx] [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Maxime Ripard]

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

Hi

On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote:
> This commits implements the "active bpc" drm property for the Intel GPU driver.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
>  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
>  4 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 64e9107d70f7..50c11b8770a7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>  {
>  	struct intel_atomic_state *state = to_intel_atomic_state(_state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
>  	int ret = 0;
>  
>  	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	intel_shared_dpll_swap_state(state);
>  	intel_atomic_track_fbs(state);
>  
> +	/* Extract information from crtc to communicate it to userspace as connector properties */
> +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
> +		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
> +		if (crtc) {
> +			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> +		}
> +		else
> +			new_conn_state->active_bpc = 0;
> +	}
> +

This seems fairly intrusive, but also commit / commit_tail might not be
the best place to put this, we want to support it at the connector
level.

Indeed, this will cause some issue if your HDMI output is a bridge for
example, where the commit will be in an entirely different driver that
has no dependency on the HDMI controller one.

I think it would be best to do that assignment in atomic_check. That
way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it
would know what the output state would have been like.

Also, all of your patches don't follow the kernel coding style. Make
sure you fix the issues reported by checkpatch --strict

Maxime

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

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

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

Re: [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Maxime Ripard]

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

Hi

On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote:
> This commits implements the "active bpc" drm property for the Intel GPU driver.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
>  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
>  4 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 64e9107d70f7..50c11b8770a7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>  {
>  	struct intel_atomic_state *state = to_intel_atomic_state(_state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
>  	int ret = 0;
>  
>  	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	intel_shared_dpll_swap_state(state);
>  	intel_atomic_track_fbs(state);
>  
> +	/* Extract information from crtc to communicate it to userspace as connector properties */
> +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
> +		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
> +		if (crtc) {
> +			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> +		}
> +		else
> +			new_conn_state->active_bpc = 0;
> +	}
> +

This seems fairly intrusive, but also commit / commit_tail might not be
the best place to put this, we want to support it at the connector
level.

Indeed, this will cause some issue if your HDMI output is a bridge for
example, where the commit will be in an entirely different driver that
has no dependency on the HDMI controller one.

I think it would be best to do that assignment in atomic_check. That
way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it
would know what the output state would have been like.

Also, all of your patches don't follow the kernel coding style. Make
sure you fix the issues reported by checkpatch --strict

Maxime

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

Re: [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Ville Syrjälä]

On Thu, Jun 10, 2021 at 02:50:36PM +0200, Maxime Ripard wrote:
> Hi
> 
> On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote:
> > This commits implements the "active bpc" drm property for the Intel GPU driver.
> > 
> > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
> >  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
> >  4 files changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 64e9107d70f7..50c11b8770a7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  {
> >  	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *new_conn_state;
> > +	int i;
> >  	int ret = 0;
> >  
> >  	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> > @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  	intel_shared_dpll_swap_state(state);
> >  	intel_atomic_track_fbs(state);
> >  
> > +	/* Extract information from crtc to communicate it to userspace as connector properties */
> > +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
> > +		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
> > +		if (crtc) {
> > +			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> > +		}
> > +		else
> > +			new_conn_state->active_bpc = 0;
> > +	}
> > +
> 
> This seems fairly intrusive, but also commit / commit_tail might not be
> the best place to put this, we want to support it at the connector
> level.
> 
> Indeed, this will cause some issue if your HDMI output is a bridge for
> example, where the commit will be in an entirely different driver that
> has no dependency on the HDMI controller one.
> 
> I think it would be best to do that assignment in atomic_check. That
> way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it
> would know what the output state would have been like.

DRM_MODE_ATOMIC_TEST_ONLY isn't allowed to change anything.

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Ville Syrjälä]

On Thu, Jun 10, 2021 at 02:50:36PM +0200, Maxime Ripard wrote:
> Hi
> 
> On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote:
> > This commits implements the "active bpc" drm property for the Intel GPU driver.
> > 
> > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
> >  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
> >  4 files changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 64e9107d70f7..50c11b8770a7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  {
> >  	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *new_conn_state;
> > +	int i;
> >  	int ret = 0;
> >  
> >  	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> > @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  	intel_shared_dpll_swap_state(state);
> >  	intel_atomic_track_fbs(state);
> >  
> > +	/* Extract information from crtc to communicate it to userspace as connector properties */
> > +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
> > +		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
> > +		if (crtc) {
> > +			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> > +		}
> > +		else
> > +			new_conn_state->active_bpc = 0;
> > +	}
> > +
> 
> This seems fairly intrusive, but also commit / commit_tail might not be
> the best place to put this, we want to support it at the connector
> level.
> 
> Indeed, this will cause some issue if your HDMI output is a bridge for
> example, where the commit will be in an entirely different driver that
> has no dependency on the HDMI controller one.
> 
> I think it would be best to do that assignment in atomic_check. That
> way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it
> would know what the output state would have been like.

DRM_MODE_ATOMIC_TEST_ONLY isn't allowed to change anything.

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Ville Syrjälä]

On Thu, Jun 10, 2021 at 02:50:36PM +0200, Maxime Ripard wrote:
> Hi
> 
> On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote:
> > This commits implements the "active bpc" drm property for the Intel GPU driver.
> > 
> > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
> >  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
> >  4 files changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 64e9107d70f7..50c11b8770a7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  {
> >  	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *new_conn_state;
> > +	int i;
> >  	int ret = 0;
> >  
> >  	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> > @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  	intel_shared_dpll_swap_state(state);
> >  	intel_atomic_track_fbs(state);
> >  
> > +	/* Extract information from crtc to communicate it to userspace as connector properties */
> > +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
> > +		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
> > +		if (crtc) {
> > +			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> > +		}
> > +		else
> > +			new_conn_state->active_bpc = 0;
> > +	}
> > +
> 
> This seems fairly intrusive, but also commit / commit_tail might not be
> the best place to put this, we want to support it at the connector
> level.
> 
> Indeed, this will cause some issue if your HDMI output is a bridge for
> example, where the commit will be in an entirely different driver that
> has no dependency on the HDMI controller one.
> 
> I think it would be best to do that assignment in atomic_check. That
> way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it
> would know what the output state would have been like.

DRM_MODE_ATOMIC_TEST_ONLY isn't allowed to change anything.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property

[Ville Syrjälä]

On Thu, Jun 10, 2021 at 02:50:36PM +0200, Maxime Ripard wrote:
> Hi
> 
> On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote:
> > This commits implements the "active bpc" drm property for the Intel GPU driver.
> > 
> > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
> >  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
> >  4 files changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 64e9107d70f7..50c11b8770a7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  {
> >  	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *new_conn_state;
> > +	int i;
> >  	int ret = 0;
> >  
> >  	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> > @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  	intel_shared_dpll_swap_state(state);
> >  	intel_atomic_track_fbs(state);
> >  
> > +	/* Extract information from crtc to communicate it to userspace as connector properties */
> > +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
> > +		struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
> > +		if (crtc) {
> > +			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> > +		}
> > +		else
> > +			new_conn_state->active_bpc = 0;
> > +	}
> > +
> 
> This seems fairly intrusive, but also commit / commit_tail might not be
> the best place to put this, we want to support it at the connector
> level.
> 
> Indeed, this will cause some issue if your HDMI output is a bridge for
> example, where the commit will be in an entirely different driver that
> has no dependency on the HDMI controller one.
> 
> I think it would be best to do that assignment in atomic_check. That
> way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it
> would know what the output state would have been like.

DRM_MODE_ATOMIC_TEST_ONLY isn't allowed to change anything.

-- 
Ville Syrjälä
Intel

[PATCH v2 5/7] drm/uAPI: Add "active color format" drm property as feedback for userspace

[Werner Sembach]

Add a new general drm property "active color format" which can be used by
graphic drivers to report the used color format back to userspace.

There was no way to check which color format got actually used on a given
monitor. To surely predict this, one must know the exact capabilities of the
monitor, the GPU, and the connection used and what the default behaviour of the
used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915 prefers RGB). This
property helps eliminating the guessing on this point.

In the future, automatic color calibration for screens might also depend on this
information being available.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
 drivers/gpu/drm/drm_connector.c   | 46 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 13 +++++++++
 3 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 7ae4e40936b5..bb78da2405f9 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -875,6 +875,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->max_requested_bpc;
 	} else if (property == connector->active_bpc_property) {
 		*val = state->active_bpc;
+	} else if (property == connector->active_color_format_property) {
+		*val = state->active_color_format;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index c0c3c09bfed0..f4f35c4117b6 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -887,6 +887,14 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
 	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
 };
 
+static const struct drm_prop_enum_list drm_color_format_enum_list[] = {
+	{ 0, "none" },
+	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
+	{ DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
+	{ DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
+	{ DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
+};
+
 DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
 		 drm_dp_subconnector_enum_list)
 
@@ -1202,6 +1210,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
  *	display engine and connected monitor, and the "max bpc" property.
  *	Drivers shall use drm_connector_attach_active_bpc_property() to install
  *	this property.
+
+ * active color format:
+ *	This read-only property tells userspace the color format actually used
+ *	by the hardware display engine on "the cable" on a connector. The chosen
+ *	value depends on hardware capabilities, both display engine and
+ *	connected monitor. Drivers shall use
+ *	drm_connector_attach_active_color_format_property() to install this
+ *	property.
  *
  * Connectors also have one standardized atomic property:
  *
@@ -2191,6 +2207,36 @@ int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
 
+/**
+ * drm_connector_attach_active_color_format_property - attach "active color format" property
+ * @connector: connector to attach active color format property on.
+ *
+ * This is used to check the applied color format on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_active_color_format_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	prop = connector->active_color_format_property;
+	if (!prop) {
+		prop = drm_property_create_enum(dev, 0, "active color format", drm_color_format_enum_list, ARRAY_SIZE(drm_color_format_enum_list));
+		if (!prop)
+			return -ENOMEM;
+
+		connector->active_color_format_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, 0);
+	connector->state->active_color_format = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c58cba2b6afe..167cd36129ae 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -788,6 +788,12 @@ struct drm_connector_state {
 	 */
 	u8 active_bpc;
 
+	/**
+	 * active_color_format: Read only property set by the GPU driver to the
+	 * actually used color format after evaluating all hardware limitations.
+	 */
+	u32 active_color_format;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1393,6 +1399,12 @@ struct drm_connector {
 	 */
 	struct drm_property *active_bpc_property;
 
+	/**
+	 * @active_color_format_property: Default connector property for the
+	 * active color format to be driven out of the connector.
+	 */
+	struct drm_property *active_color_format_property;
+
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
 #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -1713,6 +1725,7 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
 int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
 					  int min, int max);
+int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.25.1

[PATCH v2 5/7] drm/uAPI: Add "active color format" drm property as feedback for userspace

[Werner Sembach]

Add a new general drm property "active color format" which can be used by
graphic drivers to report the used color format back to userspace.

There was no way to check which color format got actually used on a given
monitor. To surely predict this, one must know the exact capabilities of the
monitor, the GPU, and the connection used and what the default behaviour of the
used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915 prefers RGB). This
property helps eliminating the guessing on this point.

In the future, automatic color calibration for screens might also depend on this
information being available.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
 drivers/gpu/drm/drm_connector.c   | 46 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 13 +++++++++
 3 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 7ae4e40936b5..bb78da2405f9 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -875,6 +875,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->max_requested_bpc;
 	} else if (property == connector->active_bpc_property) {
 		*val = state->active_bpc;
+	} else if (property == connector->active_color_format_property) {
+		*val = state->active_color_format;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index c0c3c09bfed0..f4f35c4117b6 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -887,6 +887,14 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
 	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
 };
 
+static const struct drm_prop_enum_list drm_color_format_enum_list[] = {
+	{ 0, "none" },
+	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
+	{ DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
+	{ DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
+	{ DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
+};
+
 DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
 		 drm_dp_subconnector_enum_list)
 
@@ -1202,6 +1210,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
  *	display engine and connected monitor, and the "max bpc" property.
  *	Drivers shall use drm_connector_attach_active_bpc_property() to install
  *	this property.
+
+ * active color format:
+ *	This read-only property tells userspace the color format actually used
+ *	by the hardware display engine on "the cable" on a connector. The chosen
+ *	value depends on hardware capabilities, both display engine and
+ *	connected monitor. Drivers shall use
+ *	drm_connector_attach_active_color_format_property() to install this
+ *	property.
  *
  * Connectors also have one standardized atomic property:
  *
@@ -2191,6 +2207,36 @@ int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
 
+/**
+ * drm_connector_attach_active_color_format_property - attach "active color format" property
+ * @connector: connector to attach active color format property on.
+ *
+ * This is used to check the applied color format on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_active_color_format_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	prop = connector->active_color_format_property;
+	if (!prop) {
+		prop = drm_property_create_enum(dev, 0, "active color format", drm_color_format_enum_list, ARRAY_SIZE(drm_color_format_enum_list));
+		if (!prop)
+			return -ENOMEM;
+
+		connector->active_color_format_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, 0);
+	connector->state->active_color_format = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c58cba2b6afe..167cd36129ae 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -788,6 +788,12 @@ struct drm_connector_state {
 	 */
 	u8 active_bpc;
 
+	/**
+	 * active_color_format: Read only property set by the GPU driver to the
+	 * actually used color format after evaluating all hardware limitations.
+	 */
+	u32 active_color_format;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1393,6 +1399,12 @@ struct drm_connector {
 	 */
 	struct drm_property *active_bpc_property;
 
+	/**
+	 * @active_color_format_property: Default connector property for the
+	 * active color format to be driven out of the connector.
+	 */
+	struct drm_property *active_color_format_property;
+
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
 #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -1713,6 +1725,7 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
 int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
 					  int min, int max);
+int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.25.1

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

[Intel-gfx] [PATCH v2 5/7] drm/uAPI: Add "active color format" drm property as feedback for userspace

[Werner Sembach]

Add a new general drm property "active color format" which can be used by
graphic drivers to report the used color format back to userspace.

There was no way to check which color format got actually used on a given
monitor. To surely predict this, one must know the exact capabilities of the
monitor, the GPU, and the connection used and what the default behaviour of the
used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915 prefers RGB). This
property helps eliminating the guessing on this point.

In the future, automatic color calibration for screens might also depend on this
information being available.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
 drivers/gpu/drm/drm_connector.c   | 46 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 13 +++++++++
 3 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 7ae4e40936b5..bb78da2405f9 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -875,6 +875,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->max_requested_bpc;
 	} else if (property == connector->active_bpc_property) {
 		*val = state->active_bpc;
+	} else if (property == connector->active_color_format_property) {
+		*val = state->active_color_format;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index c0c3c09bfed0..f4f35c4117b6 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -887,6 +887,14 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
 	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
 };
 
+static const struct drm_prop_enum_list drm_color_format_enum_list[] = {
+	{ 0, "none" },
+	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
+	{ DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
+	{ DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
+	{ DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
+};
+
 DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
 		 drm_dp_subconnector_enum_list)
 
@@ -1202,6 +1210,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
  *	display engine and connected monitor, and the "max bpc" property.
  *	Drivers shall use drm_connector_attach_active_bpc_property() to install
  *	this property.
+
+ * active color format:
+ *	This read-only property tells userspace the color format actually used
+ *	by the hardware display engine on "the cable" on a connector. The chosen
+ *	value depends on hardware capabilities, both display engine and
+ *	connected monitor. Drivers shall use
+ *	drm_connector_attach_active_color_format_property() to install this
+ *	property.
  *
  * Connectors also have one standardized atomic property:
  *
@@ -2191,6 +2207,36 @@ int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
 
+/**
+ * drm_connector_attach_active_color_format_property - attach "active color format" property
+ * @connector: connector to attach active color format property on.
+ *
+ * This is used to check the applied color format on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_active_color_format_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	prop = connector->active_color_format_property;
+	if (!prop) {
+		prop = drm_property_create_enum(dev, 0, "active color format", drm_color_format_enum_list, ARRAY_SIZE(drm_color_format_enum_list));
+		if (!prop)
+			return -ENOMEM;
+
+		connector->active_color_format_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, 0);
+	connector->state->active_color_format = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c58cba2b6afe..167cd36129ae 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -788,6 +788,12 @@ struct drm_connector_state {
 	 */
 	u8 active_bpc;
 
+	/**
+	 * active_color_format: Read only property set by the GPU driver to the
+	 * actually used color format after evaluating all hardware limitations.
+	 */
+	u32 active_color_format;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1393,6 +1399,12 @@ struct drm_connector {
 	 */
 	struct drm_property *active_bpc_property;
 
+	/**
+	 * @active_color_format_property: Default connector property for the
+	 * active color format to be driven out of the connector.
+	 */
+	struct drm_property *active_color_format_property;
+
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
 #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -1713,6 +1725,7 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
 int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
 					  int min, int max);
+int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.25.1

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

Re: [PATCH v2 5/7] drm/uAPI: Add "active color format" drm property as feedback for userspace

[Pekka Paalanen]

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

On Tue,  8 Jun 2021 19:43:18 +0200
Werner Sembach <wse@tuxedocomputers.com> wrote:

> Add a new general drm property "active color format" which can be used by
> graphic drivers to report the used color format back to userspace.
> 
> There was no way to check which color format got actually used on a given
> monitor. To surely predict this, one must know the exact capabilities of the
> monitor, the GPU, and the connection used and what the default behaviour of the
> used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915 prefers RGB). This
> property helps eliminating the guessing on this point.
> 
> In the future, automatic color calibration for screens might also depend on this
> information being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 46 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 13 +++++++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 7ae4e40936b5..bb78da2405f9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -875,6 +875,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->max_requested_bpc;
>  	} else if (property == connector->active_bpc_property) {
>  		*val = state->active_bpc;
> +	} else if (property == connector->active_color_format_property) {
> +		*val = state->active_color_format;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index c0c3c09bfed0..f4f35c4117b6 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -887,6 +887,14 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
>  	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
>  };
>  
> +static const struct drm_prop_enum_list drm_color_format_enum_list[] = {
> +	{ 0, "none" },
> +	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
> +	{ DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
> +	{ DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
> +	{ DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
> +};
> +
>  DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>  		 drm_dp_subconnector_enum_list)
>  
> @@ -1202,6 +1210,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	display engine and connected monitor, and the "max bpc" property.
>   *	Drivers shall use drm_connector_attach_active_bpc_property() to install
>   *	this property.
> +
> + * active color format:
> + *	This read-only property tells userspace the color format actually used
> + *	by the hardware display engine on "the cable" on a connector. The chosen
> + *	value depends on hardware capabilities, both display engine and
> + *	connected monitor. Drivers shall use
> + *	drm_connector_attach_active_color_format_property() to install this
> + *	property.

Hi,

I think also the enum values should be documented in the UAPI docs. Or
listed at the very least. Otherwise userspace developers "do not know"
what strings to decode.


Thanks,
pq


>   *
>   * Connectors also have one standardized atomic property:
>   *
> @@ -2191,6 +2207,36 @@ int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
>  
> +/**
> + * drm_connector_attach_active_color_format_property - attach "active color format" property
> + * @connector: connector to attach active color format property on.
> + *
> + * This is used to check the applied color format on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_active_color_format_property(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = connector->active_color_format_property;
> +	if (!prop) {
> +		prop = drm_property_create_enum(dev, 0, "active color format", drm_color_format_enum_list, ARRAY_SIZE(drm_color_format_enum_list));
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_color_format_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_color_format = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index c58cba2b6afe..167cd36129ae 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -788,6 +788,12 @@ struct drm_connector_state {
>  	 */
>  	u8 active_bpc;
>  
> +	/**
> +	 * active_color_format: Read only property set by the GPU driver to the
> +	 * actually used color format after evaluating all hardware limitations.
> +	 */
> +	u32 active_color_format;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1393,6 +1399,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *active_bpc_property;
>  
> +	/**
> +	 * @active_color_format_property: Default connector property for the
> +	 * active color format to be driven out of the connector.
> +	 */
> +	struct drm_property *active_color_format_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1713,6 +1725,7 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
>  int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
> +int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata


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

Re: [PATCH v2 5/7] drm/uAPI: Add "active color format" drm property as feedback for userspace

[Pekka Paalanen]

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

On Tue,  8 Jun 2021 19:43:18 +0200
Werner Sembach <wse@tuxedocomputers.com> wrote:

> Add a new general drm property "active color format" which can be used by
> graphic drivers to report the used color format back to userspace.
> 
> There was no way to check which color format got actually used on a given
> monitor. To surely predict this, one must know the exact capabilities of the
> monitor, the GPU, and the connection used and what the default behaviour of the
> used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915 prefers RGB). This
> property helps eliminating the guessing on this point.
> 
> In the future, automatic color calibration for screens might also depend on this
> information being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 46 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 13 +++++++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 7ae4e40936b5..bb78da2405f9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -875,6 +875,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->max_requested_bpc;
>  	} else if (property == connector->active_bpc_property) {
>  		*val = state->active_bpc;
> +	} else if (property == connector->active_color_format_property) {
> +		*val = state->active_color_format;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index c0c3c09bfed0..f4f35c4117b6 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -887,6 +887,14 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
>  	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
>  };
>  
> +static const struct drm_prop_enum_list drm_color_format_enum_list[] = {
> +	{ 0, "none" },
> +	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
> +	{ DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
> +	{ DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
> +	{ DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
> +};
> +
>  DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>  		 drm_dp_subconnector_enum_list)
>  
> @@ -1202,6 +1210,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	display engine and connected monitor, and the "max bpc" property.
>   *	Drivers shall use drm_connector_attach_active_bpc_property() to install
>   *	this property.
> +
> + * active color format:
> + *	This read-only property tells userspace the color format actually used
> + *	by the hardware display engine on "the cable" on a connector. The chosen
> + *	value depends on hardware capabilities, both display engine and
> + *	connected monitor. Drivers shall use
> + *	drm_connector_attach_active_color_format_property() to install this
> + *	property.

Hi,

I think also the enum values should be documented in the UAPI docs. Or
listed at the very least. Otherwise userspace developers "do not know"
what strings to decode.


Thanks,
pq


>   *
>   * Connectors also have one standardized atomic property:
>   *
> @@ -2191,6 +2207,36 @@ int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
>  
> +/**
> + * drm_connector_attach_active_color_format_property - attach "active color format" property
> + * @connector: connector to attach active color format property on.
> + *
> + * This is used to check the applied color format on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_active_color_format_property(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = connector->active_color_format_property;
> +	if (!prop) {
> +		prop = drm_property_create_enum(dev, 0, "active color format", drm_color_format_enum_list, ARRAY_SIZE(drm_color_format_enum_list));
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_color_format_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_color_format = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index c58cba2b6afe..167cd36129ae 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -788,6 +788,12 @@ struct drm_connector_state {
>  	 */
>  	u8 active_bpc;
>  
> +	/**
> +	 * active_color_format: Read only property set by the GPU driver to the
> +	 * actually used color format after evaluating all hardware limitations.
> +	 */
> +	u32 active_color_format;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1393,6 +1399,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *active_bpc_property;
>  
> +	/**
> +	 * @active_color_format_property: Default connector property for the
> +	 * active color format to be driven out of the connector.
> +	 */
> +	struct drm_property *active_color_format_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1713,6 +1725,7 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
>  int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
> +int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata


[-- 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: [Intel-gfx] [PATCH v2 5/7] drm/uAPI: Add "active color format" drm property as feedback for userspace

[Pekka Paalanen]

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

On Tue,  8 Jun 2021 19:43:18 +0200
Werner Sembach <wse@tuxedocomputers.com> wrote:

> Add a new general drm property "active color format" which can be used by
> graphic drivers to report the used color format back to userspace.
> 
> There was no way to check which color format got actually used on a given
> monitor. To surely predict this, one must know the exact capabilities of the
> monitor, the GPU, and the connection used and what the default behaviour of the
> used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915 prefers RGB). This
> property helps eliminating the guessing on this point.
> 
> In the future, automatic color calibration for screens might also depend on this
> information being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 46 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 13 +++++++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 7ae4e40936b5..bb78da2405f9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -875,6 +875,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->max_requested_bpc;
>  	} else if (property == connector->active_bpc_property) {
>  		*val = state->active_bpc;
> +	} else if (property == connector->active_color_format_property) {
> +		*val = state->active_color_format;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index c0c3c09bfed0..f4f35c4117b6 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -887,6 +887,14 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
>  	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
>  };
>  
> +static const struct drm_prop_enum_list drm_color_format_enum_list[] = {
> +	{ 0, "none" },
> +	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
> +	{ DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
> +	{ DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
> +	{ DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
> +};
> +
>  DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>  		 drm_dp_subconnector_enum_list)
>  
> @@ -1202,6 +1210,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	display engine and connected monitor, and the "max bpc" property.
>   *	Drivers shall use drm_connector_attach_active_bpc_property() to install
>   *	this property.
> +
> + * active color format:
> + *	This read-only property tells userspace the color format actually used
> + *	by the hardware display engine on "the cable" on a connector. The chosen
> + *	value depends on hardware capabilities, both display engine and
> + *	connected monitor. Drivers shall use
> + *	drm_connector_attach_active_color_format_property() to install this
> + *	property.

Hi,

I think also the enum values should be documented in the UAPI docs. Or
listed at the very least. Otherwise userspace developers "do not know"
what strings to decode.


Thanks,
pq


>   *
>   * Connectors also have one standardized atomic property:
>   *
> @@ -2191,6 +2207,36 @@ int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
>  
> +/**
> + * drm_connector_attach_active_color_format_property - attach "active color format" property
> + * @connector: connector to attach active color format property on.
> + *
> + * This is used to check the applied color format on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_active_color_format_property(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = connector->active_color_format_property;
> +	if (!prop) {
> +		prop = drm_property_create_enum(dev, 0, "active color format", drm_color_format_enum_list, ARRAY_SIZE(drm_color_format_enum_list));
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_color_format_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_color_format = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index c58cba2b6afe..167cd36129ae 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -788,6 +788,12 @@ struct drm_connector_state {
>  	 */
>  	u8 active_bpc;
>  
> +	/**
> +	 * active_color_format: Read only property set by the GPU driver to the
> +	 * actually used color format after evaluating all hardware limitations.
> +	 */
> +	u32 active_color_format;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1393,6 +1399,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *active_bpc_property;
>  
> +	/**
> +	 * @active_color_format_property: Default connector property for the
> +	 * active color format to be driven out of the connector.
> +	 */
> +	struct drm_property *active_color_format_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1713,6 +1725,7 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
>  int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
> +int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata


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

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

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

Re: [PATCH v2 5/7] drm/uAPI: Add "active color format" drm property as feedback for userspace

[Pekka Paalanen]

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

On Tue,  8 Jun 2021 19:43:18 +0200
Werner Sembach <wse@tuxedocomputers.com> wrote:

> Add a new general drm property "active color format" which can be used by
> graphic drivers to report the used color format back to userspace.
> 
> There was no way to check which color format got actually used on a given
> monitor. To surely predict this, one must know the exact capabilities of the
> monitor, the GPU, and the connection used and what the default behaviour of the
> used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915 prefers RGB). This
> property helps eliminating the guessing on this point.
> 
> In the future, automatic color calibration for screens might also depend on this
> information being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 46 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 13 +++++++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 7ae4e40936b5..bb78da2405f9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -875,6 +875,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->max_requested_bpc;
>  	} else if (property == connector->active_bpc_property) {
>  		*val = state->active_bpc;
> +	} else if (property == connector->active_color_format_property) {
> +		*val = state->active_color_format;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index c0c3c09bfed0..f4f35c4117b6 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -887,6 +887,14 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
>  	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
>  };
>  
> +static const struct drm_prop_enum_list drm_color_format_enum_list[] = {
> +	{ 0, "none" },
> +	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
> +	{ DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
> +	{ DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
> +	{ DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
> +};
> +
>  DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>  		 drm_dp_subconnector_enum_list)
>  
> @@ -1202,6 +1210,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	display engine and connected monitor, and the "max bpc" property.
>   *	Drivers shall use drm_connector_attach_active_bpc_property() to install
>   *	this property.
> +
> + * active color format:
> + *	This read-only property tells userspace the color format actually used
> + *	by the hardware display engine on "the cable" on a connector. The chosen
> + *	value depends on hardware capabilities, both display engine and
> + *	connected monitor. Drivers shall use
> + *	drm_connector_attach_active_color_format_property() to install this
> + *	property.

Hi,

I think also the enum values should be documented in the UAPI docs. Or
listed at the very least. Otherwise userspace developers "do not know"
what strings to decode.


Thanks,
pq


>   *
>   * Connectors also have one standardized atomic property:
>   *
> @@ -2191,6 +2207,36 @@ int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
>  
> +/**
> + * drm_connector_attach_active_color_format_property - attach "active color format" property
> + * @connector: connector to attach active color format property on.
> + *
> + * This is used to check the applied color format on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_active_color_format_property(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = connector->active_color_format_property;
> +	if (!prop) {
> +		prop = drm_property_create_enum(dev, 0, "active color format", drm_color_format_enum_list, ARRAY_SIZE(drm_color_format_enum_list));
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_color_format_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_color_format = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index c58cba2b6afe..167cd36129ae 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -788,6 +788,12 @@ struct drm_connector_state {
>  	 */
>  	u8 active_bpc;
>  
> +	/**
> +	 * active_color_format: Read only property set by the GPU driver to the
> +	 * actually used color format after evaluating all hardware limitations.
> +	 */
> +	u32 active_color_format;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1393,6 +1399,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *active_bpc_property;
>  
> +	/**
> +	 * @active_color_format_property: Default connector property for the
> +	 * active color format to be driven out of the connector.
> +	 */
> +	struct drm_property *active_color_format_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1713,6 +1725,7 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
>  int drm_connector_attach_active_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
> +int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata


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

[PATCH v2 6/7] drm/amd/display: Add handling for new "active color format" property

[Werner Sembach]

This commit implements the "active color format" drm property for the AMD GPU
driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 27 +++++++++++++++++--
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  1 +
 2 files changed, 26 insertions(+), 2 deletions(-)

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 e57b2b743d36..019be46def1d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6525,6 +6525,23 @@ static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_de
 	return 0;
 }
 
+static int convert_dc_pixel_encoding_into_drm_color_format(enum dc_pixel_encoding display_pixel_encoding)
+{
+	switch (display_pixel_encoding) {
+		case PIXEL_ENCODING_RGB:
+			return DRM_COLOR_FORMAT_RGB444;
+		case PIXEL_ENCODING_YCBCR422:
+			return DRM_COLOR_FORMAT_YCRCB422;
+		case PIXEL_ENCODING_YCBCR444:
+			return DRM_COLOR_FORMAT_YCRCB444;
+		case PIXEL_ENCODING_YCBCR420:
+			return DRM_COLOR_FORMAT_YCRCB420;
+		default:
+			break;
+		}
+	return 0;
+}
+
 static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
 					  struct drm_crtc_state *crtc_state,
 					  struct drm_connector_state *conn_state)
@@ -7522,6 +7539,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 	if (!aconnector->mst_port) {
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
 		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_color_format_property(&aconnector->base);
 	}
 
 	/* This defaults to the max in the range, but we want 8bpc for non-edp. */
@@ -8898,12 +8916,17 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		if (crtc) {
 			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
-			if (dm_new_crtc_state->stream)
+			if (dm_new_crtc_state->stream) {
 				new_con_state->active_bpc = convert_dc_color_depth_into_bpc(
 					dm_new_crtc_state->stream->timing.display_color_depth);
+				new_con_state->active_color_format = convert_dc_pixel_encoding_into_drm_color_format(
+					dm_new_crtc_state->stream->timing.pixel_encoding);
+			}
 		}
-		else
+		else {
 			new_con_state->active_bpc = 0;
+			new_con_state->active_color_format = 0;
+		}
 	}
 
 	/* Count number of newly disabled CRTCs for dropping PM refs later. */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 2a8dc6b2c6c7..f68950da9ff8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -400,6 +400,7 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 8, 16);
 		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_color_format_property(&aconnector->base);
 	}
 
 	connector->vrr_capable_property = master->base.vrr_capable_property;
-- 
2.25.1

[PATCH v2 6/7] drm/amd/display: Add handling for new "active color format" property

[Werner Sembach]

This commit implements the "active color format" drm property for the AMD GPU
driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 27 +++++++++++++++++--
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  1 +
 2 files changed, 26 insertions(+), 2 deletions(-)

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 e57b2b743d36..019be46def1d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6525,6 +6525,23 @@ static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_de
 	return 0;
 }
 
+static int convert_dc_pixel_encoding_into_drm_color_format(enum dc_pixel_encoding display_pixel_encoding)
+{
+	switch (display_pixel_encoding) {
+		case PIXEL_ENCODING_RGB:
+			return DRM_COLOR_FORMAT_RGB444;
+		case PIXEL_ENCODING_YCBCR422:
+			return DRM_COLOR_FORMAT_YCRCB422;
+		case PIXEL_ENCODING_YCBCR444:
+			return DRM_COLOR_FORMAT_YCRCB444;
+		case PIXEL_ENCODING_YCBCR420:
+			return DRM_COLOR_FORMAT_YCRCB420;
+		default:
+			break;
+		}
+	return 0;
+}
+
 static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
 					  struct drm_crtc_state *crtc_state,
 					  struct drm_connector_state *conn_state)
@@ -7522,6 +7539,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 	if (!aconnector->mst_port) {
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
 		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_color_format_property(&aconnector->base);
 	}
 
 	/* This defaults to the max in the range, but we want 8bpc for non-edp. */
@@ -8898,12 +8916,17 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		if (crtc) {
 			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
-			if (dm_new_crtc_state->stream)
+			if (dm_new_crtc_state->stream) {
 				new_con_state->active_bpc = convert_dc_color_depth_into_bpc(
 					dm_new_crtc_state->stream->timing.display_color_depth);
+				new_con_state->active_color_format = convert_dc_pixel_encoding_into_drm_color_format(
+					dm_new_crtc_state->stream->timing.pixel_encoding);
+			}
 		}
-		else
+		else {
 			new_con_state->active_bpc = 0;
+			new_con_state->active_color_format = 0;
+		}
 	}
 
 	/* Count number of newly disabled CRTCs for dropping PM refs later. */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 2a8dc6b2c6c7..f68950da9ff8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -400,6 +400,7 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 8, 16);
 		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_color_format_property(&aconnector->base);
 	}
 
 	connector->vrr_capable_property = master->base.vrr_capable_property;
-- 
2.25.1

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

[Intel-gfx] [PATCH v2 6/7] drm/amd/display: Add handling for new "active color format" property

[Werner Sembach]

This commit implements the "active color format" drm property for the AMD GPU
driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 27 +++++++++++++++++--
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  1 +
 2 files changed, 26 insertions(+), 2 deletions(-)

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 e57b2b743d36..019be46def1d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6525,6 +6525,23 @@ static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_de
 	return 0;
 }
 
+static int convert_dc_pixel_encoding_into_drm_color_format(enum dc_pixel_encoding display_pixel_encoding)
+{
+	switch (display_pixel_encoding) {
+		case PIXEL_ENCODING_RGB:
+			return DRM_COLOR_FORMAT_RGB444;
+		case PIXEL_ENCODING_YCBCR422:
+			return DRM_COLOR_FORMAT_YCRCB422;
+		case PIXEL_ENCODING_YCBCR444:
+			return DRM_COLOR_FORMAT_YCRCB444;
+		case PIXEL_ENCODING_YCBCR420:
+			return DRM_COLOR_FORMAT_YCRCB420;
+		default:
+			break;
+		}
+	return 0;
+}
+
 static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
 					  struct drm_crtc_state *crtc_state,
 					  struct drm_connector_state *conn_state)
@@ -7522,6 +7539,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 	if (!aconnector->mst_port) {
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
 		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_color_format_property(&aconnector->base);
 	}
 
 	/* This defaults to the max in the range, but we want 8bpc for non-edp. */
@@ -8898,12 +8916,17 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		if (crtc) {
 			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
-			if (dm_new_crtc_state->stream)
+			if (dm_new_crtc_state->stream) {
 				new_con_state->active_bpc = convert_dc_color_depth_into_bpc(
 					dm_new_crtc_state->stream->timing.display_color_depth);
+				new_con_state->active_color_format = convert_dc_pixel_encoding_into_drm_color_format(
+					dm_new_crtc_state->stream->timing.pixel_encoding);
+			}
 		}
-		else
+		else {
 			new_con_state->active_bpc = 0;
+			new_con_state->active_color_format = 0;
+		}
 	}
 
 	/* Count number of newly disabled CRTCs for dropping PM refs later. */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 2a8dc6b2c6c7..f68950da9ff8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -400,6 +400,7 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 8, 16);
 		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_color_format_property(&aconnector->base);
 	}
 
 	connector->vrr_capable_property = master->base.vrr_capable_property;
-- 
2.25.1

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

[PATCH v2 7/7] drm/i915/display: Add handling for new "active color format" property

[Werner Sembach]

This commit implements the "active color format" drm property for the Intel GPU
driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 20 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_dp.c      |  2 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c  |  1 +
 drivers/gpu/drm/i915/display/intel_hdmi.c    |  1 +
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 50c11b8770a7..e3e98c959cb4 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10158,6 +10158,21 @@ static void intel_atomic_prepare_plane_clear_colors(struct intel_atomic_state *s
 	}
 }
 
+static int convert_intel_output_format_into_drm_color_format(enum intel_output_format output_format)
+{
+	switch (output_format) {
+		case INTEL_OUTPUT_FORMAT_RGB:
+			return DRM_COLOR_FORMAT_RGB444;
+		case INTEL_OUTPUT_FORMAT_YCBCR420:
+			return DRM_COLOR_FORMAT_YCRCB420;
+		case INTEL_OUTPUT_FORMAT_YCBCR444:
+			return DRM_COLOR_FORMAT_YCRCB444;
+		default:
+			break;
+		}
+	return 0;
+}
+
 static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 {
 	struct drm_device *dev = state->base.dev;
@@ -10465,9 +10480,12 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (crtc) {
 			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
 			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
+			new_conn_state->active_color_format = convert_intel_output_format_into_drm_color_format(new_crtc_state->output_format);
 		}
-		else
+		else {
 			new_conn_state->active_bpc = 0;
+			new_conn_state->active_color_format = 0;
+		}
 	}
 
 	drm_atomic_state_get(&state->base);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 67826ba976ed..7d58bc7972d0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4674,10 +4674,12 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	if (HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 6, 10);
 		drm_connector_attach_active_bpc_property(connector, 6, 10);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 	else if (DISPLAY_VER(dev_priv) >= 5) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
 		drm_connector_attach_active_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 
 	/* Register HDMI colorspace for case of lspcon */
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 5a1869dc2210..9143adccb5d0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -847,6 +847,7 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
 		drm_connector_attach_active_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 
 	return connector;
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 8af78b27b6ce..0b57d924987e 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2466,6 +2466,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 	if (!HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 8, 12);
 		drm_connector_attach_active_bpc_property(connector, 8, 12);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 }
 
-- 
2.25.1

[PATCH v2 7/7] drm/i915/display: Add handling for new "active color format" property

[Werner Sembach]

This commit implements the "active color format" drm property for the Intel GPU
driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 20 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_dp.c      |  2 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c  |  1 +
 drivers/gpu/drm/i915/display/intel_hdmi.c    |  1 +
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 50c11b8770a7..e3e98c959cb4 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10158,6 +10158,21 @@ static void intel_atomic_prepare_plane_clear_colors(struct intel_atomic_state *s
 	}
 }
 
+static int convert_intel_output_format_into_drm_color_format(enum intel_output_format output_format)
+{
+	switch (output_format) {
+		case INTEL_OUTPUT_FORMAT_RGB:
+			return DRM_COLOR_FORMAT_RGB444;
+		case INTEL_OUTPUT_FORMAT_YCBCR420:
+			return DRM_COLOR_FORMAT_YCRCB420;
+		case INTEL_OUTPUT_FORMAT_YCBCR444:
+			return DRM_COLOR_FORMAT_YCRCB444;
+		default:
+			break;
+		}
+	return 0;
+}
+
 static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 {
 	struct drm_device *dev = state->base.dev;
@@ -10465,9 +10480,12 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (crtc) {
 			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
 			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
+			new_conn_state->active_color_format = convert_intel_output_format_into_drm_color_format(new_crtc_state->output_format);
 		}
-		else
+		else {
 			new_conn_state->active_bpc = 0;
+			new_conn_state->active_color_format = 0;
+		}
 	}
 
 	drm_atomic_state_get(&state->base);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 67826ba976ed..7d58bc7972d0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4674,10 +4674,12 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	if (HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 6, 10);
 		drm_connector_attach_active_bpc_property(connector, 6, 10);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 	else if (DISPLAY_VER(dev_priv) >= 5) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
 		drm_connector_attach_active_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 
 	/* Register HDMI colorspace for case of lspcon */
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 5a1869dc2210..9143adccb5d0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -847,6 +847,7 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
 		drm_connector_attach_active_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 
 	return connector;
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 8af78b27b6ce..0b57d924987e 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2466,6 +2466,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 	if (!HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 8, 12);
 		drm_connector_attach_active_bpc_property(connector, 8, 12);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 }
 
-- 
2.25.1

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

[Intel-gfx] [PATCH v2 7/7] drm/i915/display: Add handling for new "active color format" property

[Werner Sembach]

This commit implements the "active color format" drm property for the Intel GPU
driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 20 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_dp.c      |  2 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c  |  1 +
 drivers/gpu/drm/i915/display/intel_hdmi.c    |  1 +
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 50c11b8770a7..e3e98c959cb4 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10158,6 +10158,21 @@ static void intel_atomic_prepare_plane_clear_colors(struct intel_atomic_state *s
 	}
 }
 
+static int convert_intel_output_format_into_drm_color_format(enum intel_output_format output_format)
+{
+	switch (output_format) {
+		case INTEL_OUTPUT_FORMAT_RGB:
+			return DRM_COLOR_FORMAT_RGB444;
+		case INTEL_OUTPUT_FORMAT_YCBCR420:
+			return DRM_COLOR_FORMAT_YCRCB420;
+		case INTEL_OUTPUT_FORMAT_YCBCR444:
+			return DRM_COLOR_FORMAT_YCRCB444;
+		default:
+			break;
+		}
+	return 0;
+}
+
 static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 {
 	struct drm_device *dev = state->base.dev;
@@ -10465,9 +10480,12 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (crtc) {
 			struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
 			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
+			new_conn_state->active_color_format = convert_intel_output_format_into_drm_color_format(new_crtc_state->output_format);
 		}
-		else
+		else {
 			new_conn_state->active_bpc = 0;
+			new_conn_state->active_color_format = 0;
+		}
 	}
 
 	drm_atomic_state_get(&state->base);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 67826ba976ed..7d58bc7972d0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4674,10 +4674,12 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	if (HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 6, 10);
 		drm_connector_attach_active_bpc_property(connector, 6, 10);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 	else if (DISPLAY_VER(dev_priv) >= 5) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
 		drm_connector_attach_active_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 
 	/* Register HDMI colorspace for case of lspcon */
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 5a1869dc2210..9143adccb5d0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -847,6 +847,7 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
 		drm_connector_attach_active_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 
 	return connector;
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 8af78b27b6ce..0b57d924987e 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2466,6 +2466,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 	if (!HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 8, 12);
 		drm_connector_attach_active_bpc_property(connector, 8, 12);
+		drm_connector_attach_active_color_format_property(connector);
 	}
 }
 
-- 
2.25.1

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

[Intel-gfx] ✗ Fi.CI.BUILD: failure for Add "activ bpc" and "active color format" drm property

[Patchwork]

== Series Details ==

Series: Add "activ bpc" and "active color format" drm property
URL   : https://patchwork.freedesktop.org/series/91188/
State : failure

== Summary ==

Applying: drm/amd/display: Add missing cases convert_dc_color_depth_into_bpc
Applying: drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/drm_atomic_uapi.c
M	drivers/gpu/drm/drm_connector.c
M	include/drm/drm_connector.h
Falling back to patching base and 3-way merge...
Auto-merging include/drm/drm_connector.h
Auto-merging drivers/gpu/drm/drm_connector.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/drm_connector.c
Auto-merging drivers/gpu/drm/drm_atomic_uapi.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


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