[PATCH v2 0/4] New DRM properties for output color format

[PATCH v2 0/4] New DRM properties for output color format

[Andri Yngvason]

After some discussion, we decided to drop the "active color format"
property and rename the "preferred color format" property to "force
color format". 

The user can probe available color formats in combination with other
properties using TEST_ONLY commits.

v1: https://lore.kernel.org/dri-devel/20240109181104.1670304-1-andri@yngvason.is/

v2
 - Dropped "active color format"
 - Replaced "preferred color format" with "force color format"


Werner Sembach (4):
  drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check
  drm/uAPI: Add "force color format" drm property as setting for
    userspace
  drm/amd/display: Add handling for new "force color format" property
  drm/i915/display: Add handling for new "force color format" property

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 67 ++++++++++++++++---
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  4 ++
 drivers/gpu/drm/drm_atomic_helper.c           |  4 ++
 drivers/gpu/drm/drm_atomic_uapi.c             |  4 ++
 drivers/gpu/drm/drm_connector.c               | 48 +++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c       | 35 ++++++++--
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  5 ++
 drivers/gpu/drm/i915/display/intel_hdmi.c     | 29 ++++++--
 include/drm/drm_connector.h                   | 16 +++++
 9 files changed, 190 insertions(+), 22 deletions(-)


base-commit: 052d534373b7ed33712a63d5e17b2b6cdbce84fd
-- 
2.43.0

[PATCH v2 1/4] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check

[Andri Yngvason]

From: Werner Sembach <wse@tuxedocomputers.com>

Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check that was performed in the
drm_mode_is_420_only() case, but not in the drm_mode_is_420_also() &&
force_yuv420_output case.

Without further knowledge if YCbCr 4:2:0 is supported outside of HDMI,
there is no reason to use RGB when the display
reports drm_mode_is_420_only() even on a non HDMI connection.

This patch also moves both checks in the same if-case. This  eliminates an
extra else-if-case.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +-----
 1 file changed, 1 insertion(+), 5 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 f6575d7dee971..cc4d1f7f97b98 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5575,11 +5575,7 @@ static void fill_stream_properties_from_drm_display_mode(
 	timing_out->v_border_bottom = 0;
 	/* TODO: un-hardcode */
 	if (drm_mode_is_420_only(info, mode_in)
-			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
-		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
-	else if (drm_mode_is_420_also(info, mode_in)
-			&& aconnector
-			&& aconnector->force_yuv420_output)
+			|| (drm_mode_is_420_also(info, mode_in) && aconnector->force_yuv420_output))
 		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
 	else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
 			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
-- 
2.43.0

[PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Andri Yngvason]

From: Werner Sembach <wse@tuxedocomputers.com>

Add a new general drm property "force color format" which can be used
by userspace to tell the graphics driver which color format to use.

Possible options are:
    - auto (default/current behaviour)
    - rgb
    - ycbcr444
    - ycbcr422 (supported by neither amdgpu or i915)
    - ycbcr420

In theory the auto option should choose the best available option for the
current setup, but because of bad internal conversion some monitors look
better with rgb and some with ycbcr444.

Also, because of bad shielded connectors and/or cables, it might be
preferable to use the less bandwidth heavy ycbcr422 and ycbcr420 formats
for a signal that is less susceptible to interference.

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

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---

Changes in v2:
 - Renamed to "force color format" from "preferred color format"
 - Removed Reported-by pointing to invalid email address

---
 drivers/gpu/drm/drm_atomic_helper.c |  4 +++
 drivers/gpu/drm/drm_atomic_uapi.c   |  4 +++
 drivers/gpu/drm/drm_connector.c     | 48 +++++++++++++++++++++++++++++
 include/drm/drm_connector.h         | 16 ++++++++++
 4 files changed, 72 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 39ef0a6addeba..1dabd164c4f09 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -707,6 +707,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 			if (old_connector_state->max_requested_bpc !=
 			    new_connector_state->max_requested_bpc)
 				new_crtc_state->connectors_changed = true;
+
+			if (old_connector_state->force_color_format !=
+			    new_connector_state->force_color_format)
+				new_crtc_state->connectors_changed = true;
 		}
 
 		if (funcs->atomic_check)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 29d4940188d49..e45949bf4615f 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -776,6 +776,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 		state->max_requested_bpc = val;
 	} else if (property == connector->privacy_screen_sw_state_property) {
 		state->privacy_screen_sw_state = val;
+	} else if (property == connector->force_color_format_property) {
+		state->force_color_format = val;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -859,6 +861,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->max_requested_bpc;
 	} else if (property == connector->privacy_screen_sw_state_property) {
 		*val = state->privacy_screen_sw_state;
+	} else if (property == connector->force_color_format_property) {
+		*val = state->force_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 b0516505f7ae9..e0535e58b4535 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1061,6 +1061,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_force_color_format_enum_list[] = {
+	{ 0, "auto" },
+	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
+	{ DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
+	{ DRM_COLOR_FORMAT_YCBCR422, "ycbcr422" },
+	{ DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
+};
+
 DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
 		 drm_dp_subconnector_enum_list)
 
@@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
  *	drm_connector_attach_max_bpc_property() to create and attach the
  *	property to the connector during initialization.
  *
+ * force color format:
+ *	This property is used by userspace to change the used color format. When
+ *	used the driver will use the selected format if valid for the hardware,
+ *	sink, and current resolution and refresh rate combination. Drivers to
+ *	use the function drm_connector_attach_force_color_format_property()
+ *	to create and attach the property to the connector during
+ *	initialization. Possible values are "auto", "rgb", "ycbcr444",
+ *	"ycbcr422", and "ycbcr420".
+ *
  * Connectors also have one standardized atomic property:
  *
  * CRTC_ID:
@@ -2457,6 +2474,37 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
 
+/**
+ * drm_connector_attach_force_color_format_property - attach "force color format" property
+ * @connector: connector to attach force color format property on.
+ *
+ * This is used to add support for selecting a color format on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_force_color_format_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	if (!connector->force_color_format_property) {
+		prop = drm_property_create_enum(dev, 0, "force color format",
+						drm_force_color_format_enum_list,
+						ARRAY_SIZE(drm_force_color_format_enum_list));
+		if (!prop)
+			return -ENOMEM;
+
+		connector->force_color_format_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, 0);
+	connector->state->force_color_format = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_force_color_format_property);
+
 /**
  * drm_connector_attach_hdr_output_metadata_property - attach "HDR_OUTPUT_METADA" property
  * @connector: connector to attach the property on.
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index fe88d7fc6b8f4..9830e7c09c0ba 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1026,6 +1026,14 @@ struct drm_connector_state {
 	 */
 	enum drm_privacy_screen_status privacy_screen_sw_state;
 
+	/**
+	 * @force_color_format: Property set by userspace to tell the GPU
+	 * driver which color format to use. It only gets applied if hardware,
+	 * meaning both the computer and the monitor, and the driver support the
+	 * given format at the current resolution and refresh rate.
+	 */
+	u32 force_color_format;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1699,6 +1707,12 @@ struct drm_connector {
 	 */
 	struct drm_property *privacy_screen_hw_state_property;
 
+	/**
+	 * @force_color_format_property: Default connector property for the
+	 * force color format to be driven out of the connector.
+	 */
+	struct drm_property *force_color_format_property;
+
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
 #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -2053,6 +2067,8 @@ void drm_connector_attach_privacy_screen_provider(
 	struct drm_connector *connector, struct drm_privacy_screen *priv);
 void drm_connector_update_privacy_screen(const struct drm_connector_state *connector_state);
 
+int drm_connector_attach_force_color_format_property(struct drm_connector *connector);
+
 /**
  * struct drm_tile_group - Tile group metadata
  * @refcount: reference count
-- 
2.43.0

[PATCH v2 3/4] drm/amd/display: Add handling for new "force color format" property

[Andri Yngvason]

From: Werner Sembach <wse@tuxedocomputers.com>

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

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Co-Developed-by: Andri Yngvason <andri@yngvason.is>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---

Changes in v2:
 - Renamed to "force color format" from "preferred color format"
 - Modeset will fail if color format cannot be satisfied

---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++++++++++++++++---
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  4 ++
 2 files changed, 60 insertions(+), 7 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 cc4d1f7f97b98..26c4260c78d7b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5573,15 +5573,32 @@ static void fill_stream_properties_from_drm_display_mode(
 	timing_out->h_border_right = 0;
 	timing_out->v_border_top = 0;
 	timing_out->v_border_bottom = 0;
-	/* TODO: un-hardcode */
-	if (drm_mode_is_420_only(info, mode_in)
-			|| (drm_mode_is_420_also(info, mode_in) && aconnector->force_yuv420_output))
+
+	if (connector_state
+			&& (connector_state->force_color_format == DRM_COLOR_FORMAT_YCBCR420
+			|| aconnector->force_yuv420_output) && drm_mode_is_420(info, mode_in))
 		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
-	else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
-			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
+	else if (connector_state
+			&& connector_state->force_color_format == DRM_COLOR_FORMAT_YCBCR444
+			&& connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
 		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
-	else
+	else if (connector_state
+			&& connector_state->force_color_format == DRM_COLOR_FORMAT_RGB444
+			&& !drm_mode_is_420_only(info, mode_in))
 		timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+	else
+		/*
+		 * connector_state->force_color_format not possible
+		 * || connector_state->force_color_format == 0 (auto)
+		 * || connector_state->force_color_format == DRM_COLOR_FORMAT_YCBCR422
+		 */
+		if (drm_mode_is_420_only(info, mode_in))
+			timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+		else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
+				&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
+			timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
+		else
+			timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
 
 	timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
 	timing_out->display_color_depth = convert_color_depth_from_display_info(
@@ -6685,6 +6702,33 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
 	return dc_result;
 }
 
+static enum dc_status
+dm_validate_stream_color_format(const struct drm_connector_state *drm_state,
+				const struct dc_stream_state *stream)
+{
+	if (!drm_state->force_color_format)
+		return DC_OK;
+
+	enum dc_pixel_encoding encoding = PIXEL_ENCODING_UNDEFINED;
+	switch (drm_state->force_color_format) {
+	case DRM_COLOR_FORMAT_RGB444:
+		encoding = PIXEL_ENCODING_RGB;
+		break;
+	case DRM_COLOR_FORMAT_YCBCR444:
+		encoding = PIXEL_ENCODING_YCBCR444;
+		break;
+	case DRM_COLOR_FORMAT_YCBCR422:
+		encoding = PIXEL_ENCODING_YCBCR422;
+		break;
+	case DRM_COLOR_FORMAT_YCBCR420:
+		encoding = PIXEL_ENCODING_YCBCR420;
+		break;
+	}
+
+	return encoding == stream->timing.pixel_encoding ?
+	       DC_OK : DC_UNSUPPORTED_VALUE;
+}
+
 struct dc_stream_state *
 create_validate_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 				const struct drm_display_mode *drm_mode,
@@ -6717,6 +6761,9 @@ create_validate_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		if (dc_result == DC_OK)
 			dc_result = dm_validate_stream_and_context(adev->dm.dc, stream);
 
+		if (dc_result == DC_OK)
+			dc_result = dm_validate_stream_color_format(drm_state, stream);
+
 		if (dc_result != DC_OK) {
 			DRM_DEBUG_KMS("Mode %dx%d (clk %d) failed DC validation with error %d (%s)\n",
 				      drm_mode->hdisplay,
@@ -7512,8 +7559,10 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				adev->mode_info.underscan_vborder_property,
 				0);
 
-	if (!aconnector->mst_root)
+	if (!aconnector->mst_root) {
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_force_color_format_property(&aconnector->base);
+	}
 
 	aconnector->base.state->max_bpc = 16;
 	aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
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 941e96f100f4e..437d50f53eb97 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
@@ -601,6 +601,10 @@ 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);
 
+	connector->force_color_format_property = master->base.force_color_format_property;
+	if (connector->force_color_format_property)
+		drm_connector_attach_force_color_format_property(&aconnector->base);
+
 	connector->vrr_capable_property = master->base.vrr_capable_property;
 	if (connector->vrr_capable_property)
 		drm_connector_attach_vrr_capable_property(connector);
-- 
2.43.0

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

[Andri Yngvason]

From: Werner Sembach <wse@tuxedocomputers.com>

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

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Co-Developed-by: Andri Yngvason <andri@yngvason.is>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---

Changes in v2:
 - Renamed to "force color format" from "preferred color format"
 - Modeset will fail if color format cannot be satisfied

---
 drivers/gpu/drm/i915/display/intel_dp.c     | 35 ++++++++++++++++-----
 drivers/gpu/drm/i915/display/intel_dp_mst.c |  5 +++
 drivers/gpu/drm/i915/display/intel_hdmi.c   | 29 ++++++++++++++---
 3 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 7d2b8ce48fda1..71e822483572e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2799,6 +2799,16 @@ static bool intel_dp_has_audio(struct intel_encoder *encoder,
 		return intel_conn_state->force_audio == HDMI_AUDIO_ON;
 }
 
+static u32 intel_output_format_to_drm_color_format(enum intel_output_format input)
+{
+	switch (input) {
+	case INTEL_OUTPUT_FORMAT_RGB: return DRM_COLOR_FORMAT_RGB444;
+	case INTEL_OUTPUT_FORMAT_YCBCR444: return DRM_COLOR_FORMAT_YCBCR444;
+	case INTEL_OUTPUT_FORMAT_YCBCR420: return DRM_COLOR_FORMAT_YCBCR420;
+	}
+	return 0;
+}
+
 static int
 intel_dp_compute_output_format(struct intel_encoder *encoder,
 			       struct intel_crtc_state *crtc_state,
@@ -2810,17 +2820,20 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
 	struct intel_connector *connector = intel_dp->attached_connector;
 	const struct drm_display_info *info = &connector->base.display_info;
 	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
-	bool ycbcr_420_only;
+	bool ycbcr_420_output;
 	int ret;
 
-	ycbcr_420_only = drm_mode_is_420_only(info, adjusted_mode);
+	ycbcr_420_output = drm_mode_is_420_only(info, adjusted_mode) ||
+			   (conn_state->force_color_format == DRM_COLOR_FORMAT_YCBCR420 &&
+			    drm_mode_is_420_also(&connector->base.display_info, adjusted_mode));
 
-	if (ycbcr_420_only && !connector->base.ycbcr_420_allowed) {
+	crtc_state->sink_format = ycbcr_420_output ? INTEL_OUTPUT_FORMAT_YCBCR420 :
+						     INTEL_OUTPUT_FORMAT_RGB;
+
+	if (ycbcr_420_output && !connector->base.ycbcr_420_allowed) {
 		drm_dbg_kms(&i915->drm,
 			    "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB.\n");
 		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
-	} else {
-		crtc_state->sink_format = intel_dp_sink_format(connector, adjusted_mode);
 	}
 
 	crtc_state->output_format = intel_dp_output_format(connector, crtc_state->sink_format);
@@ -2840,6 +2853,11 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
 						   respect_downstream_limits);
 	}
 
+	if (conn_state->force_color_format && conn_state->force_color_format !=
+	    intel_output_format_to_drm_color_format(crtc_state->sink_format)) {
+		ret = -EINVAL;
+	}
+
 	return ret;
 }
 
@@ -6179,10 +6197,13 @@ 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_force_color_format_property(connector);
+	} else if (DISPLAY_VER(dev_priv) >= 5) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
+		drm_connector_attach_force_color_format_property(connector);
+	}
 
 	/* Register HDMI colorspace for case of lspcon */
 	if (intel_bios_encoder_is_lspcon(dp_to_dig_port(intel_dp)->base.devdata)) {
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 8a94323350303..dcb3abcc6d83e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1572,6 +1572,11 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 		drm_dbg_kms(&dev_priv->drm, "[%s:%d] HDCP MST init failed, skipping.\n",
 			    connector->name, connector->base.id);
 
+	connector->force_color_format_property =
+		intel_dp->attached_connector->base.force_color_format_property;
+	if (connector->force_color_format_property)
+		drm_connector_attach_force_color_format_property(connector);
+
 	return connector;
 
 err:
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 39e4f5f7c8171..a612173411b26 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2205,6 +2205,16 @@ intel_hdmi_output_format(const struct intel_crtc_state *crtc_state)
 	return crtc_state->sink_format;
 }
 
+static u32 intel_output_format_to_drm_color_format(enum intel_output_format input)
+{
+	switch (input) {
+	case INTEL_OUTPUT_FORMAT_RGB: return DRM_COLOR_FORMAT_RGB444;
+	case INTEL_OUTPUT_FORMAT_YCBCR444: return DRM_COLOR_FORMAT_YCBCR444;
+	case INTEL_OUTPUT_FORMAT_YCBCR420: return DRM_COLOR_FORMAT_YCBCR420;
+	}
+	return 0;
+}
+
 static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
 					    struct intel_crtc_state *crtc_state,
 					    const struct drm_connector_state *conn_state,
@@ -2214,13 +2224,17 @@ static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
 	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
 	const struct drm_display_info *info = &connector->base.display_info;
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
-	bool ycbcr_420_only = drm_mode_is_420_only(info, adjusted_mode);
+	bool ycbcr_420_output;
 	int ret;
 
+	ycbcr_420_output = drm_mode_is_420_only(info, adjusted_mode) ||
+			   (conn_state->force_color_format == DRM_COLOR_FORMAT_YCBCR420 &&
+			    drm_mode_is_420_also(&connector->base.display_info, adjusted_mode));
+
 	crtc_state->sink_format =
-		intel_hdmi_sink_format(crtc_state, connector, ycbcr_420_only);
+		intel_hdmi_sink_format(crtc_state, connector, ycbcr_420_output);
 
-	if (ycbcr_420_only && crtc_state->sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) {
+	if (ycbcr_420_output && crtc_state->sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) {
 		drm_dbg_kms(&i915->drm,
 			    "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB.\n");
 		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
@@ -2240,6 +2254,11 @@ static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
 		ret = intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits);
 	}
 
+	if (conn_state->force_color_format && conn_state->force_color_format !=
+	    intel_output_format_to_drm_color_format(crtc_state->output_format)) {
+		ret = -EINVAL;
+	}
+
 	return ret;
 }
 
@@ -2611,8 +2630,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 	if (DISPLAY_VER(dev_priv) >= 10)
 		drm_connector_attach_hdr_output_metadata_property(connector);
 
-	if (!HAS_GMCH(dev_priv))
+	if (!HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 8, 12);
+		drm_connector_attach_force_color_format_property(connector);
+	}
 }
 
 /*
-- 
2.43.0

Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Sebastian Wick]

On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote:
> From: Werner Sembach <wse@tuxedocomputers.com>
> 
> Add a new general drm property "force color format" which can be used
> by userspace to tell the graphics driver which color format to use.

I don't like the "force" in the name. This just selects the color
format, let's just call it "color format" then.

> Possible options are:
>     - auto (default/current behaviour)
>     - rgb
>     - ycbcr444
>     - ycbcr422 (supported by neither amdgpu or i915)
>     - ycbcr420
> 
> In theory the auto option should choose the best available option for the
> current setup, but because of bad internal conversion some monitors look
> better with rgb and some with ycbcr444.
> 
> Also, because of bad shielded connectors and/or cables, it might be
> preferable to use the less bandwidth heavy ycbcr422 and ycbcr420 formats
> for a signal that is less susceptible to interference.
> 
> In the future, automatic color calibration for screens might also depend on
> this option being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Signed-off-by: Andri Yngvason <andri@yngvason.is>
> Tested-by: Andri Yngvason <andri@yngvason.is>
> ---
> 
> Changes in v2:
>  - Renamed to "force color format" from "preferred color format"
>  - Removed Reported-by pointing to invalid email address
> 
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  4 +++
>  drivers/gpu/drm/drm_atomic_uapi.c   |  4 +++
>  drivers/gpu/drm/drm_connector.c     | 48 +++++++++++++++++++++++++++++
>  include/drm/drm_connector.h         | 16 ++++++++++
>  4 files changed, 72 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 39ef0a6addeba..1dabd164c4f09 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -707,6 +707,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  			if (old_connector_state->max_requested_bpc !=
>  			    new_connector_state->max_requested_bpc)
>  				new_crtc_state->connectors_changed = true;
> +
> +			if (old_connector_state->force_color_format !=
> +			    new_connector_state->force_color_format)
> +				new_crtc_state->connectors_changed = true;
>  		}
>  
>  		if (funcs->atomic_check)
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 29d4940188d49..e45949bf4615f 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -776,6 +776,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		state->max_requested_bpc = val;
>  	} else if (property == connector->privacy_screen_sw_state_property) {
>  		state->privacy_screen_sw_state = val;
> +	} else if (property == connector->force_color_format_property) {
> +		state->force_color_format = val;
>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
>  				state, property, val);
> @@ -859,6 +861,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->max_requested_bpc;
>  	} else if (property == connector->privacy_screen_sw_state_property) {
>  		*val = state->privacy_screen_sw_state;
> +	} else if (property == connector->force_color_format_property) {
> +		*val = state->force_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 b0516505f7ae9..e0535e58b4535 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1061,6 +1061,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_force_color_format_enum_list[] = {
> +	{ 0, "auto" },
> +	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
> +	{ DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
> +	{ DRM_COLOR_FORMAT_YCBCR422, "ycbcr422" },
> +	{ DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
> +};
> +
>  DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>  		 drm_dp_subconnector_enum_list)
>  
> @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
>   *	drm_connector_attach_max_bpc_property() to create and attach the
>   *	property to the connector during initialization.
>   *
> + * force color format:
> + *	This property is used by userspace to change the used color format. When
> + *	used the driver will use the selected format if valid for the hardware,

All properties are always "used", they just can have different values.
You probably want to talk about the auto mode here.

> + *	sink, and current resolution and refresh rate combination. Drivers to

If valid? So when a value is not actually supported user space can still
set it? What happens then? How should user space figure out if the
driver and the sink support the format?

For the Colorspace prop, the kernel just exposes all formats it supports
(independent of the sink) and then makes it the job of user space to
figure out if the sink supports it.

The same could be done here. Property value is exposed if the driver
supports it in general, commits can fail if the driver can't support it
for a specific commit because e.g. the resolution or refresh rate. User
space must look at the EDID/DisplayID/mode to figure out the supported
format for the sink.

> + *	use the function drm_connector_attach_force_color_format_property()
> + *	to create and attach the property to the connector during
> + *	initialization. Possible values are "auto", "rgb", "ycbcr444",
> + *	"ycbcr422", and "ycbcr420".
> + *
>   * Connectors also have one standardized atomic property:
>   *
>   * CRTC_ID:
> @@ -2457,6 +2474,37 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
>  
> +/**
> + * drm_connector_attach_force_color_format_property - attach "force color format" property
> + * @connector: connector to attach force color format property on.
> + *
> + * This is used to add support for selecting a color format on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_force_color_format_property(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	if (!connector->force_color_format_property) {
> +		prop = drm_property_create_enum(dev, 0, "force color format",
> +						drm_force_color_format_enum_list,
> +						ARRAY_SIZE(drm_force_color_format_enum_list));
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->force_color_format_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->force_color_format = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_force_color_format_property);
> +
>  /**
>   * drm_connector_attach_hdr_output_metadata_property - attach "HDR_OUTPUT_METADA" property
>   * @connector: connector to attach the property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f4..9830e7c09c0ba 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1026,6 +1026,14 @@ struct drm_connector_state {
>  	 */
>  	enum drm_privacy_screen_status privacy_screen_sw_state;
>  
> +	/**
> +	 * @force_color_format: Property set by userspace to tell the GPU
> +	 * driver which color format to use. It only gets applied if hardware,
> +	 * meaning both the computer and the monitor, and the driver support the
> +	 * given format at the current resolution and refresh rate.
> +	 */
> +	u32 force_color_format;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1699,6 +1707,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *privacy_screen_hw_state_property;
>  
> +	/**
> +	 * @force_color_format_property: Default connector property for the
> +	 * force color format to be driven out of the connector.
> +	 */
> +	struct drm_property *force_color_format_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -2053,6 +2067,8 @@ void drm_connector_attach_privacy_screen_provider(
>  	struct drm_connector *connector, struct drm_privacy_screen *priv);
>  void drm_connector_update_privacy_screen(const struct drm_connector_state *connector_state);
>  
> +int drm_connector_attach_force_color_format_property(struct drm_connector *connector);
> +
>  /**
>   * struct drm_tile_group - Tile group metadata
>   * @refcount: reference count
> -- 
> 2.43.0
> 

Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Andri Yngvason]

Hi Sebastian,

þri., 16. jan. 2024 kl. 11:42 skrifaði Sebastian Wick
<sebastian.wick@redhat.com>:
>
> On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote:
> > From: Werner Sembach <wse@tuxedocomputers.com>
> >
> > Add a new general drm property "force color format" which can be used
> > by userspace to tell the graphics driver which color format to use.
>
> I don't like the "force" in the name. This just selects the color
> format, let's just call it "color format" then.
>

In previous revisions, this was "preferred color format" and "actual
color format", of which the latter has been dropped. I recommend
reading the discussion for previous revisions.

There are arguments for adding "actual color format" later and if it
is added later, we'd end up with "color format" and "actual color
format", which might be confusing, and it is why I chose to call it
"force color format" because it clearly communicates intent and
disambiguates it from "actual color format".

[...]
> > @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
> >   *   drm_connector_attach_max_bpc_property() to create and attach the
> >   *   property to the connector during initialization.
> >   *
> > + * force color format:
> > + *   This property is used by userspace to change the used color format. When
> > + *   used the driver will use the selected format if valid for the hardware,
>
> All properties are always "used", they just can have different values.
> You probably want to talk about the auto mode here.

Maybe we can say something like: If userspace does not set the
property or if it is explicitly set to zero, the driver will select
the appropriate color format based on other constraints.

>
> > + *   sink, and current resolution and refresh rate combination. Drivers to
>
> If valid? So when a value is not actually supported user space can still
> set it? What happens then? How should user space figure out if the
> driver and the sink support the format?

The kernel does not expose this property unless it's implemented in the driver.

This was originally "preferred color format". Perhaps the
documentation should better reflect that it is now a mandatory
constraint which fails the modeset if not satisfied.

>
> For the Colorspace prop, the kernel just exposes all formats it supports
> (independent of the sink) and then makes it the job of user space to
> figure out if the sink supports it.
>
> The same could be done here. Property value is exposed if the driver
> supports it in general, commits can fail if the driver can't support it
> for a specific commit because e.g. the resolution or refresh rate. User
> space must look at the EDID/DisplayID/mode to figure out the supported
> format for the sink.

Yes, we can make it possible for userspace to discover which modes are
supported by the monitor, but there are other constraints that need to
be satisfied. This was discussed in the previous revision.

In any case, these things can be added later and need not be a part of
this change set.

[...]

Regards,
Andri

Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Sebastian Wick]

On Tue, Jan 16, 2024 at 01:13:13PM +0000, Andri Yngvason wrote:
> Hi Sebastian,
> 
> þri., 16. jan. 2024 kl. 11:42 skrifaði Sebastian Wick
> <sebastian.wick@redhat.com>:
> >
> > On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote:
> > > From: Werner Sembach <wse@tuxedocomputers.com>
> > >
> > > Add a new general drm property "force color format" which can be used
> > > by userspace to tell the graphics driver which color format to use.
> >
> > I don't like the "force" in the name. This just selects the color
> > format, let's just call it "color format" then.
> >
> 
> In previous revisions, this was "preferred color format" and "actual
> color format", of which the latter has been dropped. I recommend
> reading the discussion for previous revisions.

Please don't imply that I didn't read the thread I'm answering to.

> There are arguments for adding "actual color format" later and if it
> is added later, we'd end up with "color format" and "actual color
> format", which might be confusing, and it is why I chose to call it
> "force color format" because it clearly communicates intent and
> disambiguates it from "actual color format".

There is no such thing as "actual color format" in upstream though.
Basing your naming on discarded ideas is not useful. The thing that sets
the color space for example is called "Colorspace", not "force
colorspace". 

> [...]
> > > @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
> > >   *   drm_connector_attach_max_bpc_property() to create and attach the
> > >   *   property to the connector during initialization.
> > >   *
> > > + * force color format:
> > > + *   This property is used by userspace to change the used color format. When
> > > + *   used the driver will use the selected format if valid for the hardware,
> >
> > All properties are always "used", they just can have different values.
> > You probably want to talk about the auto mode here.
> 
> Maybe we can say something like: If userspace does not set the
> property or if it is explicitly set to zero, the driver will select
> the appropriate color format based on other constraints.

The property can be in any state without involvement from user space.
Don't talk about setting it, talk about the state it is in:

  When the color format is auto, the driver will select a format.

> >
> > > + *   sink, and current resolution and refresh rate combination. Drivers to
> >
> > If valid? So when a value is not actually supported user space can still
> > set it? What happens then? How should user space figure out if the
> > driver and the sink support the format?
> 
> The kernel does not expose this property unless it's implemented in the driver.

If the driver simply doesn't support *one format*, the enum value for
that format should not be exposed, period. This isn't about the property
on its own.

> This was originally "preferred color format". Perhaps the
> documentation should better reflect that it is now a mandatory
> constraint which fails the modeset if not satisfied.

That would definitely help.

> >
> > For the Colorspace prop, the kernel just exposes all formats it supports
> > (independent of the sink) and then makes it the job of user space to
> > figure out if the sink supports it.
> >
> > The same could be done here. Property value is exposed if the driver
> > supports it in general, commits can fail if the driver can't support it
> > for a specific commit because e.g. the resolution or refresh rate. User
> > space must look at the EDID/DisplayID/mode to figure out the supported
> > format for the sink.
> 
> Yes, we can make it possible for userspace to discover which modes are
> supported by the monitor, but there are other constraints that need to
> be satisfied. This was discussed in the previous revision.

I mean, yes, that's what I said. User space would then only be
responsible for checking the sink capabilities and the atomic check
would take into account other (non-sink) constraints.

> In any case, these things can be added later and need not be a part of
> this change set.

No, this is the contract between the kernel and user space and has to be
figured out before we can merge new uAPI.

> 
> [...]
> 
> Regards,
> Andri
> 

Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Andri Yngvason]

þri., 16. jan. 2024 kl. 13:29 skrifaði Sebastian Wick
<sebastian.wick@redhat.com>:
>
> On Tue, Jan 16, 2024 at 01:13:13PM +0000, Andri Yngvason wrote:
[...]
> > şri., 16. jan. 2024 kl. 11:42 skrifaği Sebastian Wick
> > <sebastian.wick@redhat.com>:
> > >
> > > On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote:
> > > > From: Werner Sembach <wse@tuxedocomputers.com>
> > > >
> > > > Add a new general drm property "force color format" which can be used
> > > > by userspace to tell the graphics driver which color format to use.
> > >
> > > I don't like the "force" in the name. This just selects the color
> > > format, let's just call it "color format" then.
> > >
> >
> > In previous revisions, this was "preferred color format" and "actual
> > color format", of which the latter has been dropped. I recommend
> > reading the discussion for previous revisions.
>
> Please don't imply that I didn't read the thread I'm answering to.
>
> > There are arguments for adding "actual color format" later and if it
> > is added later, we'd end up with "color format" and "actual color
> > format", which might be confusing, and it is why I chose to call it
> > "force color format" because it clearly communicates intent and
> > disambiguates it from "actual color format".
>
> There is no such thing as "actual color format" in upstream though.
> Basing your naming on discarded ideas is not useful. The thing that sets
> the color space for example is called "Colorspace", not "force
> colorspace".
>

Sure, I'm happy with calling it whatever people want. Maybe we can
have a vote on it?

> > [...]
> > > > @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
> > > >   *   drm_connector_attach_max_bpc_property() to create and attach the
> > > >   *   property to the connector during initialization.
> > > >   *
> > > > + * force color format:
> > > > + *   This property is used by userspace to change the used color format. When
> > > > + *   used the driver will use the selected format if valid for the hardware,
> > >
> > > All properties are always "used", they just can have different values.
> > > You probably want to talk about the auto mode here.
> >
> > Maybe we can say something like: If userspace does not set the
> > property or if it is explicitly set to zero, the driver will select
> > the appropriate color format based on other constraints.
>
> The property can be in any state without involvement from user space.
> Don't talk about setting it, talk about the state it is in:
>
>   When the color format is auto, the driver will select a format.
>

Ok.

> > >
> > > > + *   sink, and current resolution and refresh rate combination. Drivers to
> > >
> > > If valid? So when a value is not actually supported user space can still
> > > set it? What happens then? How should user space figure out if the
> > > driver and the sink support the format?
> >
> > The kernel does not expose this property unless it's implemented in the driver.
>
> If the driver simply doesn't support *one format*, the enum value for
> that format should not be exposed, period. This isn't about the property
> on its own.

Right, understood. You mean that enum should only contain values that
are supported by the driver.

>
> > This was originally "preferred color format". Perhaps the
> > documentation should better reflect that it is now a mandatory
> > constraint which fails the modeset if not satisfied.
>
> That would definitely help.
>
> > >
> > > For the Colorspace prop, the kernel just exposes all formats it supports
> > > (independent of the sink) and then makes it the job of user space to
> > > figure out if the sink supports it.
> > >
> > > The same could be done here. Property value is exposed if the driver
> > > supports it in general, commits can fail if the driver can't support it
> > > for a specific commit because e.g. the resolution or refresh rate. User
> > > space must look at the EDID/DisplayID/mode to figure out the supported
> > > format for the sink.
> >
> > Yes, we can make it possible for userspace to discover which modes are
> > supported by the monitor, but there are other constraints that need to
> > be satisfied. This was discussed in the previous revision.
>
> I mean, yes, that's what I said. User space would then only be
> responsible for checking the sink capabilities and the atomic check
> would take into account other (non-sink) constraints.

Since we need to probe using TEST_ONLY anyway, we'll end up with two
mechanisms to do the same thing where one of them depends on the other
for completeness.

>
> > In any case, these things can be added later and need not be a part of
> > this change set.
>
> No, this is the contract between the kernel and user space and has to be
> figured out before we can merge new uAPI.
>
> >
> > [...]
> >

Thanks,
Andri

Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Pekka Paalanen]

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

On Tue, 16 Jan 2024 14:11:43 +0000
Andri Yngvason <andri@yngvason.is> wrote:

> þri., 16. jan. 2024 kl. 13:29 skrifaði Sebastian Wick
> <sebastian.wick@redhat.com>:
> >
> > On Tue, Jan 16, 2024 at 01:13:13PM +0000, Andri Yngvason wrote:  
> [...]
> > > şri., 16. jan. 2024 kl. 11:42 skrifaği Sebastian Wick
> > > <sebastian.wick@redhat.com>:  
> > > >
> > > > On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote:  
> > > > > From: Werner Sembach <wse@tuxedocomputers.com>
> > > > >
> > > > > Add a new general drm property "force color format" which can be used
> > > > > by userspace to tell the graphics driver which color format to use.  
> > > >
> > > > I don't like the "force" in the name. This just selects the color
> > > > format, let's just call it "color format" then.
> > > >  
> > >
> > > In previous revisions, this was "preferred color format" and "actual
> > > color format", of which the latter has been dropped. I recommend
> > > reading the discussion for previous revisions.  
> >
> > Please don't imply that I didn't read the thread I'm answering to.

FYI, I have not read this thread.

> >  
> > > There are arguments for adding "actual color format" later and if it
> > > is added later, we'd end up with "color format" and "actual color
> > > format", which might be confusing, and it is why I chose to call it
> > > "force color format" because it clearly communicates intent and
> > > disambiguates it from "actual color format".  
> >
> > There is no such thing as "actual color format" in upstream though.
> > Basing your naming on discarded ideas is not useful. The thing that sets
> > the color space for example is called "Colorspace", not "force
> > colorspace".
> >  
> 
> Sure, I'm happy with calling it whatever people want. Maybe we can
> have a vote on it?

It would sound strange to say "force color format" = "auto". Just drop
the "force" of it.

If and when we need the feedback counterpart, it could be an immutable
prop called "active color format" where "auto" is not a valid value, or
something in the new "output properties" design Sima has been thinking
of.

> > > [...]  
> > > > > @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
> > > > >   *   drm_connector_attach_max_bpc_property() to create and attach the
> > > > >   *   property to the connector during initialization.
> > > > >   *
> > > > > + * force color format:
> > > > > + *   This property is used by userspace to change the used color format. When
> > > > > + *   used the driver will use the selected format if valid for the hardware,  
> > > >
> > > > All properties are always "used", they just can have different values.
> > > > You probably want to talk about the auto mode here.  
> > >
> > > Maybe we can say something like: If userspace does not set the
> > > property or if it is explicitly set to zero, the driver will select
> > > the appropriate color format based on other constraints.  
> >
> > The property can be in any state without involvement from user space.
> > Don't talk about setting it, talk about the state it is in:
> >
> >   When the color format is auto, the driver will select a format.
> >  
> 
> Ok.
> 
> > > >  
> > > > > + *   sink, and current resolution and refresh rate combination. Drivers to  
> > > >
> > > > If valid? So when a value is not actually supported user space can still
> > > > set it? What happens then? How should user space figure out if the
> > > > driver and the sink support the format?  
> > >
> > > The kernel does not expose this property unless it's implemented in the driver.  
> >
> > If the driver simply doesn't support *one format*, the enum value for
> > that format should not be exposed, period. This isn't about the property
> > on its own.  
> 
> Right, understood. You mean that enum should only contain values that
> are supported by the driver.

Yes. When a driver installs a property, it can choose which of the enum
entries are exposed. That cannot be changed later though, so the list
cannot live by the currently connected sink, only by what the driver
and display controlled could ever do.

> > > This was originally "preferred color format". Perhaps the
> > > documentation should better reflect that it is now a mandatory
> > > constraint which fails the modeset if not satisfied.  
> >
> > That would definitely help.
> >  
> > > >
> > > > For the Colorspace prop, the kernel just exposes all formats it supports
> > > > (independent of the sink) and then makes it the job of user space to
> > > > figure out if the sink supports it.
> > > >
> > > > The same could be done here. Property value is exposed if the driver
> > > > supports it in general, commits can fail if the driver can't support it
> > > > for a specific commit because e.g. the resolution or refresh rate. User
> > > > space must look at the EDID/DisplayID/mode to figure out the supported
> > > > format for the sink.  
> > >
> > > Yes, we can make it possible for userspace to discover which modes are
> > > supported by the monitor, but there are other constraints that need to
> > > be satisfied. This was discussed in the previous revision.  
> >
> > I mean, yes, that's what I said. User space would then only be
> > responsible for checking the sink capabilities and the atomic check
> > would take into account other (non-sink) constraints.  
> 
> Since we need to probe using TEST_ONLY anyway, we'll end up with two
> mechanisms to do the same thing where one of them depends on the other
> for completeness.

What do you mean by "same thing"?

Neither HDMI nor DisplayPort have a feedback message saying your
infoframe contents are unacceptable, that I know of. Even if there was,
it would come too late for failing the atomic commit ioctl in
non-blocking mode.

In general, display signalling is that you send whatever to the sink,
and hope for the best.

EDID is used to describe what the sink can accept, so in theory the
kernel could parse EDID for all of these details and reject atomic
commits that attempt unsupported configurations. However, EDID are also
notoriously buggy. They are good for a best guess, but I believe it is
useful to be able to try "unsupported" things. IIRC, PS VR2
intentionally lies for instance.

Even if the kernel did reject everything based on EDID, the only way
today for userspace to know what should work is to parse the EDID
itself. TEST_ONLY trials lead to a combinatorial explosion too easily.
So userspace is already expected to parse EDID, with the major
exception being video mode lists that are explicitly provided by the
kernel in UAPI.

EDID and DisplayID standards also evolve. The kernel could be behind
userspace in chasing them, which was the reason why the kernel does not
validate HDR_OUTPUT_METADATA against EDID.

The design of today with HDR_OUTPUT_METADATA and whatnot is
that userspace is responsible for checking sink capabilities, and
atomic check is responsible for driver and display controller
capabilities.

> > > In any case, these things can be added later and need not be a part of
> > > this change set.  
> >
> > No, this is the contract between the kernel and user space and has to be
> > figured out before we can merge new uAPI.

Indeed.


Thanks,
pq

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

Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Andri Yngvason]

mið., 17. jan. 2024 kl. 09:21 skrifaði Pekka Paalanen <ppaalanen@gmail.com>:
>
> On Tue, 16 Jan 2024 14:11:43 +0000
> Andri Yngvason <andri@yngvason.is> wrote:
>
> > þri., 16. jan. 2024 kl. 13:29 skrifaði Sebastian Wick
> > <sebastian.wick@redhat.com>:
> > >
> > > On Tue, Jan 16, 2024 at 01:13:13PM +0000, Andri Yngvason wrote:
> > [...]
> > > > şri., 16. jan. 2024 kl. 11:42 skrifaği Sebastian Wick
> > > > <sebastian.wick@redhat.com>:
> > > > >
> > > > > On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote:
> > > > > > From: Werner Sembach <wse@tuxedocomputers.com>
> > > > > >
> > > > > > Add a new general drm property "force color format" which can be used
> > > > > > by userspace to tell the graphics driver which color format to use.
> > > > >
> > > > > I don't like the "force" in the name. This just selects the color
> > > > > format, let's just call it "color format" then.
> > > > >
> > > >
> > > > In previous revisions, this was "preferred color format" and "actual
> > > > color format", of which the latter has been dropped. I recommend
> > > > reading the discussion for previous revisions.
> > >
> > > Please don't imply that I didn't read the thread I'm answering to.
>
> FYI, I have not read this thread.
>

pq, You did not read this summary?
https://lore.kernel.org/dri-devel/CAFNQBQwjeJaX6B4oewpgASMUd5_nxZYMxUfdOG294CTVGBTd1w@mail.gmail.com/

You partook in the discussion on IRC. Please read it and tell me if I
misunderstood anything.

Sebastian, I apologise. You clearly read it as you even replied to it!

> > >
> > > > There are arguments for adding "actual color format" later and if it
> > > > is added later, we'd end up with "color format" and "actual color
> > > > format", which might be confusing, and it is why I chose to call it
> > > > "force color format" because it clearly communicates intent and
> > > > disambiguates it from "actual color format".
> > >
> > > There is no such thing as "actual color format" in upstream though.
> > > Basing your naming on discarded ideas is not useful. The thing that sets
> > > the color space for example is called "Colorspace", not "force
> > > colorspace".
> > >
> >
> > Sure, I'm happy with calling it whatever people want. Maybe we can
> > have a vote on it?
>
> It would sound strange to say "force color format" = "auto". Just drop
> the "force" of it.
>
> If and when we need the feedback counterpart, it could be an immutable
> prop called "active color format" where "auto" is not a valid value, or
> something in the new "output properties" design Sima has been thinking
> of.

There seems to be consensus for calling it "color format"

>
> > > > [...]
> > > > > > @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
> > > > > >   *   drm_connector_attach_max_bpc_property() to create and attach the
> > > > > >   *   property to the connector during initialization.
> > > > > >   *
> > > > > > + * force color format:
> > > > > > + *   This property is used by userspace to change the used color format. When
> > > > > > + *   used the driver will use the selected format if valid for the hardware,
> > > > >
> > > > > All properties are always "used", they just can have different values.
> > > > > You probably want to talk about the auto mode here.
> > > >
> > > > Maybe we can say something like: If userspace does not set the
> > > > property or if it is explicitly set to zero, the driver will select
> > > > the appropriate color format based on other constraints.
> > >
> > > The property can be in any state without involvement from user space.
> > > Don't talk about setting it, talk about the state it is in:
> > >
> > >   When the color format is auto, the driver will select a format.
> > >
> >
> > Ok.
> >
> > > > >
> > > > > > + *   sink, and current resolution and refresh rate combination. Drivers to
> > > > >
> > > > > If valid? So when a value is not actually supported user space can still
> > > > > set it? What happens then? How should user space figure out if the
> > > > > driver and the sink support the format?
> > > >
> > > > The kernel does not expose this property unless it's implemented in the driver.
> > >
> > > If the driver simply doesn't support *one format*, the enum value for
> > > that format should not be exposed, period. This isn't about the property
> > > on its own.
> >
> > Right, understood. You mean that enum should only contain values that
> > are supported by the driver.
>
> Yes. When a driver installs a property, it can choose which of the enum
> entries are exposed. That cannot be changed later though, so the list
> cannot live by the currently connected sink, only by what the driver
> and display controlled could ever do.

Yes, and I think that basing it also on the connected sink's
capabilities would just add complexity for very little gain. In fact,
I think that limiting it based on the driver's capabilities is also
over-engineering, but I don't mind adding it if that's what people
really want.

>
> > > > This was originally "preferred color format". Perhaps the
> > > > documentation should better reflect that it is now a mandatory
> > > > constraint which fails the modeset if not satisfied.
> > >
> > > That would definitely help.
> > >
> > > > >
> > > > > For the Colorspace prop, the kernel just exposes all formats it supports
> > > > > (independent of the sink) and then makes it the job of user space to
> > > > > figure out if the sink supports it.
> > > > >
> > > > > The same could be done here. Property value is exposed if the driver
> > > > > supports it in general, commits can fail if the driver can't support it
> > > > > for a specific commit because e.g. the resolution or refresh rate. User
> > > > > space must look at the EDID/DisplayID/mode to figure out the supported
> > > > > format for the sink.
> > > >
> > > > Yes, we can make it possible for userspace to discover which modes are
> > > > supported by the monitor, but there are other constraints that need to
> > > > be satisfied. This was discussed in the previous revision.
> > >
> > > I mean, yes, that's what I said. User space would then only be
> > > responsible for checking the sink capabilities and the atomic check
> > > would take into account other (non-sink) constraints.
> >
> > Since we need to probe using TEST_ONLY anyway, we'll end up with two
> > mechanisms to do the same thing where one of them depends on the other
> > for completeness.
>
> What do you mean by "same thing"?

I thought that it would be clear that I did not mean that they were
literally equal. This was discussed on IRC and summarised in the email
message that I linked to above. Excerpt:
"I asked if it made sense to add color format capabilities to the mode info
struct, but the conclusion was that it wouldn't really be useful because we
need TEST_ONLY anyway to see if the color format setting is compatible with
other settings."

>
> Neither HDMI nor DisplayPort have a feedback message saying your
> infoframe contents are unacceptable, that I know of. Even if there was,
> it would come too late for failing the atomic commit ioctl in
> non-blocking mode.
>
> In general, display signalling is that you send whatever to the sink,
> and hope for the best.
>
> EDID is used to describe what the sink can accept, so in theory the
> kernel could parse EDID for all of these details and reject atomic
> commits that attempt unsupported configurations. However, EDID are also
> notoriously buggy. They are good for a best guess, but I believe it is
> useful to be able to try "unsupported" things. IIRC, PS VR2
> intentionally lies for instance.
>
> Even if the kernel did reject everything based on EDID, the only way
> today for userspace to know what should work is to parse the EDID
> itself. TEST_ONLY trials lead to a combinatorial explosion too easily.
> So userspace is already expected to parse EDID, with the major
> exception being video mode lists that are explicitly provided by the
> kernel in UAPI.

I thought that everyone agreed that display settings GUIs don't suffer
from combinatorial explosion because settings are selected in a
predefined order so they don't need to test all permutations.

>
> EDID and DisplayID standards also evolve. The kernel could be behind
> userspace in chasing them, which was the reason why the kernel does not
> validate HDR_OUTPUT_METADATA against EDID.
>
> The design of today with HDR_OUTPUT_METADATA and whatnot is
> that userspace is responsible for checking sink capabilities, and
> atomic check is responsible for driver and display controller
> capabilities.

I'm not really sure where you're going with this. Are you for or
against userspace parsing EDID instead of getting the information from
the kernel?

>
> > > > In any case, these things can be added later and need not be a part of
> > > > this change set.
> > >
> > > No, this is the contract between the kernel and user space and has to be
> > > figured out before we can merge new uAPI.
>
> Indeed.

I don't see how adding something later to cut down on the
combinatorial explosion can possibly break any kind of contract in the
way things are currently implemented. Can anyone provide examples of
how things can go wrong in this particular instance?

Thanks,
Andri

Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Sebastian Wick]

On Wed, Jan 17, 2024 at 12:58:15PM +0000, Andri Yngvason wrote:
> mið., 17. jan. 2024 kl. 09:21 skrifaði Pekka Paalanen <ppaalanen@gmail.com>:
> >
> > On Tue, 16 Jan 2024 14:11:43 +0000
> > Andri Yngvason <andri@yngvason.is> wrote:
> >
> > > þri., 16. jan. 2024 kl. 13:29 skrifaði Sebastian Wick
> > > <sebastian.wick@redhat.com>:
> > > >
> > > > On Tue, Jan 16, 2024 at 01:13:13PM +0000, Andri Yngvason wrote:
> > > [...]
> > > > > şri., 16. jan. 2024 kl. 11:42 skrifaği Sebastian Wick
> > > > > <sebastian.wick@redhat.com>:
> > > > > >
> > > > > > On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote:
> > > > > > > From: Werner Sembach <wse@tuxedocomputers.com>
> > > > > > >
> > > > > > > Add a new general drm property "force color format" which can be used
> > > > > > > by userspace to tell the graphics driver which color format to use.
> > > > > >
> > > > > > I don't like the "force" in the name. This just selects the color
> > > > > > format, let's just call it "color format" then.
> > > > > >
> > > > >
> > > > > In previous revisions, this was "preferred color format" and "actual
> > > > > color format", of which the latter has been dropped. I recommend
> > > > > reading the discussion for previous revisions.
> > > >
> > > > Please don't imply that I didn't read the thread I'm answering to.
> >
> > FYI, I have not read this thread.
> >
> 
> pq, You did not read this summary?
> https://lore.kernel.org/dri-devel/CAFNQBQwjeJaX6B4oewpgASMUd5_nxZYMxUfdOG294CTVGBTd1w@mail.gmail.com/
> 
> You partook in the discussion on IRC. Please read it and tell me if I
> misunderstood anything.
> 
> Sebastian, I apologise. You clearly read it as you even replied to it!

Thank you :)

> > > >
> > > > > There are arguments for adding "actual color format" later and if it
> > > > > is added later, we'd end up with "color format" and "actual color
> > > > > format", which might be confusing, and it is why I chose to call it
> > > > > "force color format" because it clearly communicates intent and
> > > > > disambiguates it from "actual color format".
> > > >
> > > > There is no such thing as "actual color format" in upstream though.
> > > > Basing your naming on discarded ideas is not useful. The thing that sets
> > > > the color space for example is called "Colorspace", not "force
> > > > colorspace".
> > > >
> > >
> > > Sure, I'm happy with calling it whatever people want. Maybe we can
> > > have a vote on it?
> >
> > It would sound strange to say "force color format" = "auto". Just drop
> > the "force" of it.
> >
> > If and when we need the feedback counterpart, it could be an immutable
> > prop called "active color format" where "auto" is not a valid value, or
> > something in the new "output properties" design Sima has been thinking
> > of.
> 
> There seems to be consensus for calling it "color format"
> 
> >
> > > > > [...]
> > > > > > > @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
> > > > > > >   *   drm_connector_attach_max_bpc_property() to create and attach the
> > > > > > >   *   property to the connector during initialization.
> > > > > > >   *
> > > > > > > + * force color format:
> > > > > > > + *   This property is used by userspace to change the used color format. When
> > > > > > > + *   used the driver will use the selected format if valid for the hardware,
> > > > > >
> > > > > > All properties are always "used", they just can have different values.
> > > > > > You probably want to talk about the auto mode here.
> > > > >
> > > > > Maybe we can say something like: If userspace does not set the
> > > > > property or if it is explicitly set to zero, the driver will select
> > > > > the appropriate color format based on other constraints.
> > > >
> > > > The property can be in any state without involvement from user space.
> > > > Don't talk about setting it, talk about the state it is in:
> > > >
> > > >   When the color format is auto, the driver will select a format.
> > > >
> > >
> > > Ok.
> > >
> > > > > >
> > > > > > > + *   sink, and current resolution and refresh rate combination. Drivers to
> > > > > >
> > > > > > If valid? So when a value is not actually supported user space can still
> > > > > > set it? What happens then? How should user space figure out if the
> > > > > > driver and the sink support the format?
> > > > >
> > > > > The kernel does not expose this property unless it's implemented in the driver.
> > > >
> > > > If the driver simply doesn't support *one format*, the enum value for
> > > > that format should not be exposed, period. This isn't about the property
> > > > on its own.
> > >
> > > Right, understood. You mean that enum should only contain values that
> > > are supported by the driver.
> >
> > Yes. When a driver installs a property, it can choose which of the enum
> > entries are exposed. That cannot be changed later though, so the list
> > cannot live by the currently connected sink, only by what the driver
> > and display controlled could ever do.
> 
> Yes, and I think that basing it also on the connected sink's
> capabilities would just add complexity for very little gain. In fact,
> I think that limiting it based on the driver's capabilities is also
> over-engineering, but I don't mind adding it if that's what people
> really want.

Having a bunch of values that will always fail a mode set just makes
life for user space much worse. Might be overengineering from the kernel
pov but it's not for user space.

> >
> > > > > This was originally "preferred color format". Perhaps the
> > > > > documentation should better reflect that it is now a mandatory
> > > > > constraint which fails the modeset if not satisfied.
> > > >
> > > > That would definitely help.
> > > >
> > > > > >
> > > > > > For the Colorspace prop, the kernel just exposes all formats it supports
> > > > > > (independent of the sink) and then makes it the job of user space to
> > > > > > figure out if the sink supports it.
> > > > > >
> > > > > > The same could be done here. Property value is exposed if the driver
> > > > > > supports it in general, commits can fail if the driver can't support it
> > > > > > for a specific commit because e.g. the resolution or refresh rate. User
> > > > > > space must look at the EDID/DisplayID/mode to figure out the supported
> > > > > > format for the sink.
> > > > >
> > > > > Yes, we can make it possible for userspace to discover which modes are
> > > > > supported by the monitor, but there are other constraints that need to
> > > > > be satisfied. This was discussed in the previous revision.
> > > >
> > > > I mean, yes, that's what I said. User space would then only be
> > > > responsible for checking the sink capabilities and the atomic check
> > > > would take into account other (non-sink) constraints.
> > >
> > > Since we need to probe using TEST_ONLY anyway, we'll end up with two
> > > mechanisms to do the same thing where one of them depends on the other
> > > for completeness.
> >
> > What do you mean by "same thing"?
> 
> I thought that it would be clear that I did not mean that they were
> literally equal. This was discussed on IRC and summarised in the email
> message that I linked to above. Excerpt:
> "I asked if it made sense to add color format capabilities to the mode info
> struct, but the conclusion was that it wouldn't really be useful because we
> need TEST_ONLY anyway to see if the color format setting is compatible with
> other settings."

I feel like we're talking past each other.

There are two questions:

1. Should the property expose enum values which will always result in a
   failed commit (because e.g. the hardware doesn't support it)
2. Should the commit fail if the sink doesn't claim to support the
   format

The first issue I believe that we should try to minimize options that
can't work to cut down on the combinatorial explosion problem.

On the second issue, there are good reasons to just not fail commits in
the kernel because:
* user space already has to parse and understand EDIDs
* this information is often wrong
* support for new EDID/DisplayID can get to user space faster

We have to decide on this and make them part of the API. We've seen how
this gets a mess if that's not being done.

> >
> > Neither HDMI nor DisplayPort have a feedback message saying your
> > infoframe contents are unacceptable, that I know of. Even if there was,
> > it would come too late for failing the atomic commit ioctl in
> > non-blocking mode.
> >
> > In general, display signalling is that you send whatever to the sink,
> > and hope for the best.
> >
> > EDID is used to describe what the sink can accept, so in theory the
> > kernel could parse EDID for all of these details and reject atomic
> > commits that attempt unsupported configurations. However, EDID are also
> > notoriously buggy. They are good for a best guess, but I believe it is
> > useful to be able to try "unsupported" things. IIRC, PS VR2
> > intentionally lies for instance.
> >
> > Even if the kernel did reject everything based on EDID, the only way
> > today for userspace to know what should work is to parse the EDID
> > itself. TEST_ONLY trials lead to a combinatorial explosion too easily.
> > So userspace is already expected to parse EDID, with the major
> > exception being video mode lists that are explicitly provided by the
> > kernel in UAPI.
> 
> I thought that everyone agreed that display settings GUIs don't suffer
> from combinatorial explosion because settings are selected in a
> predefined order so they don't need to test all permutations.

Not all permutations doesn't mean no permutations. This is still really
expensive to do right.

> >
> > EDID and DisplayID standards also evolve. The kernel could be behind
> > userspace in chasing them, which was the reason why the kernel does not
> > validate HDR_OUTPUT_METADATA against EDID.
> >
> > The design of today with HDR_OUTPUT_METADATA and whatnot is
> > that userspace is responsible for checking sink capabilities, and
> > atomic check is responsible for driver and display controller
> > capabilities.
> 
> I'm not really sure where you're going with this. Are you for or
> against userspace parsing EDID instead of getting the information from
> the kernel?

The opposite I hope.

Sink capabilities shouldn't influence commits, let user space do what it
can do. We have a bunch of "auto" states but I do consider them a
mistake.

> >
> > > > > In any case, these things can be added later and need not be a part of
> > > > > this change set.
> > > >
> > > > No, this is the contract between the kernel and user space and has to be
> > > > figured out before we can merge new uAPI.
> >
> > Indeed.
> 
> I don't see how adding something later to cut down on the
> combinatorial explosion can possibly break any kind of contract in the
> way things are currently implemented. Can anyone provide examples of
> how things can go wrong in this particular instance?
> 
> Thanks,
> Andri
> 

Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Pekka Paalanen]

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

On Wed, 17 Jan 2024 12:58:15 +0000
Andri Yngvason <andri@yngvason.is> wrote:

> mið., 17. jan. 2024 kl. 09:21 skrifaði Pekka Paalanen <ppaalanen@gmail.com>:

...

> > EDID and DisplayID standards also evolve. The kernel could be behind
> > userspace in chasing them, which was the reason why the kernel does not
> > validate HDR_OUTPUT_METADATA against EDID.
> >
> > The design of today with HDR_OUTPUT_METADATA and whatnot is
> > that userspace is responsible for checking sink capabilities, and
> > atomic check is responsible for driver and display controller
> > capabilities.  
> 
> I'm not really sure where you're going with this. Are you for or
> against userspace parsing EDID instead of getting the information from
> the kernel?

In that specific email, neither. I attempted to describe the current
situation without any bias towards whether I think that is a good or
not design. There is an existing behaviour, and if you want to deviate
from that, you need more justification than for following it.

Even the video modes list that I mentioned as the major example of
things that userspace should not parse from EDID itself is not
exhaustive nor exclusive. Userspace can still craft an arbitrary video
mode and set it. If the driver and display controller can do it, it
passes I believe, even if it would literally destroy the sink (in the
CRT era, you could burn the flyback transistor of an unfortunate
monitor).

If you want me to take a stance, I think the kernel not gating settings
based on EDID for these things is a good idea for these reasons:

- EDID can easily be wrong, and it is easier to test sink "unsupported"
  configurations if you do not need to craft a modified EDID and
  (reboot to?) load it in the kernel first.

- EDID spec gets occasionally extended. If the kernel gated settings,
  you would not be able to test new features without getting an updated
  kernel that allows them to pass. This mostly applies to blob
  properties, and not enums, because you cannot set arbitrary values to
  enum properties.

Finally, as to why userspace parsing EDID at all is a good idea:

- The kernel is not interested in most of the stuff contained in EDIDs,
  so it has no inherent reason to parse everything.

- EDID is a fairly wide "API" and gets occasionally extended.
  Replicating all that in a kernel UAPI is a lot of work that won't
  benefit the kernel itself. There does not seem to be benefit in
  reinventing EDID information encoding in fine-grained UAPI
  structures, but there certainly is risk, because UAPI is written in
  stone once published. Userspace can get the equivalent from libraries
  like libdisplay-info which are much easier to develop and replace
  than UAPI.


Thanks,
pq

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

Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace

[Harry Wentland]

I'm a bit late to the game but I don't think this is merged
yet.

On 2024-01-15 11:05, Andri Yngvason wrote:
> From: Werner Sembach <wse@tuxedocomputers.com>
> 
> Add a new general drm property "force color format" which can be used
> by userspace to tell the graphics driver which color format to use.
> 
> Possible options are:
>     - auto (default/current behaviour)
>     - rgb
>     - ycbcr444
>     - ycbcr422 (supported by neither amdgpu or i915)

If no driver uses this should we expose this now? I would
prefer to leave ycbcr422 out of this until we have a driver
that actually uses it.

I've seen too many properties with ever possible value defined
but they're not used by any (open) userspace and then become
the object of intense discussion on how they should work. I
doubt that this would happen here, but I still feel a slight
aversion to defining things that no open userspace can use at
this point.

I agree with all of Sebastian and Pekka's comments elsewhere in
this thread, in particular with Sebastian's comments to not
advertise color formats that a driver can't support. See this
patch for how I implemented something similar for Colorspace
c265f340eaa8 ("drm/connector: Allow drivers to pass list of supported colorspaces")

Harry

>     - ycbcr420
> 
> In theory the auto option should choose the best available option for the
> current setup, but because of bad internal conversion some monitors look
> better with rgb and some with ycbcr444.
> 
> Also, because of bad shielded connectors and/or cables, it might be
> preferable to use the less bandwidth heavy ycbcr422 and ycbcr420 formats
> for a signal that is less susceptible to interference.
> 
> In the future, automatic color calibration for screens might also depend on
> this option being available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Signed-off-by: Andri Yngvason <andri@yngvason.is>
> Tested-by: Andri Yngvason <andri@yngvason.is>
> ---
> 
> Changes in v2:
>  - Renamed to "force color format" from "preferred color format"
>  - Removed Reported-by pointing to invalid email address
> 
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  4 +++
>  drivers/gpu/drm/drm_atomic_uapi.c   |  4 +++
>  drivers/gpu/drm/drm_connector.c     | 48 +++++++++++++++++++++++++++++
>  include/drm/drm_connector.h         | 16 ++++++++++
>  4 files changed, 72 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 39ef0a6addeba..1dabd164c4f09 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -707,6 +707,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  			if (old_connector_state->max_requested_bpc !=
>  			    new_connector_state->max_requested_bpc)
>  				new_crtc_state->connectors_changed = true;
> +
> +			if (old_connector_state->force_color_format !=
> +			    new_connector_state->force_color_format)
> +				new_crtc_state->connectors_changed = true;
>  		}
>  
>  		if (funcs->atomic_check)
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 29d4940188d49..e45949bf4615f 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -776,6 +776,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		state->max_requested_bpc = val;
>  	} else if (property == connector->privacy_screen_sw_state_property) {
>  		state->privacy_screen_sw_state = val;
> +	} else if (property == connector->force_color_format_property) {
> +		state->force_color_format = val;
>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
>  				state, property, val);
> @@ -859,6 +861,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->max_requested_bpc;
>  	} else if (property == connector->privacy_screen_sw_state_property) {
>  		*val = state->privacy_screen_sw_state;
> +	} else if (property == connector->force_color_format_property) {
> +		*val = state->force_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 b0516505f7ae9..e0535e58b4535 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1061,6 +1061,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_force_color_format_enum_list[] = {
> +	{ 0, "auto" },
> +	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
> +	{ DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
> +	{ DRM_COLOR_FORMAT_YCBCR422, "ycbcr422" },
> +	{ DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
> +};
> +
>  DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>  		 drm_dp_subconnector_enum_list)
>  
> @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
>   *	drm_connector_attach_max_bpc_property() to create and attach the
>   *	property to the connector during initialization.
>   *
> + * force color format:
> + *	This property is used by userspace to change the used color format. When
> + *	used the driver will use the selected format if valid for the hardware,
> + *	sink, and current resolution and refresh rate combination. Drivers to
> + *	use the function drm_connector_attach_force_color_format_property()
> + *	to create and attach the property to the connector during
> + *	initialization. Possible values are "auto", "rgb", "ycbcr444",
> + *	"ycbcr422", and "ycbcr420".
> + *
>   * Connectors also have one standardized atomic property:
>   *
>   * CRTC_ID:
> @@ -2457,6 +2474,37 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
>  
> +/**
> + * drm_connector_attach_force_color_format_property - attach "force color format" property
> + * @connector: connector to attach force color format property on.
> + *
> + * This is used to add support for selecting a color format on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_force_color_format_property(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	if (!connector->force_color_format_property) {
> +		prop = drm_property_create_enum(dev, 0, "force color format",
> +						drm_force_color_format_enum_list,
> +						ARRAY_SIZE(drm_force_color_format_enum_list));
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->force_color_format_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->force_color_format = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_force_color_format_property);
> +
>  /**
>   * drm_connector_attach_hdr_output_metadata_property - attach "HDR_OUTPUT_METADA" property
>   * @connector: connector to attach the property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f4..9830e7c09c0ba 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1026,6 +1026,14 @@ struct drm_connector_state {
>  	 */
>  	enum drm_privacy_screen_status privacy_screen_sw_state;
>  
> +	/**
> +	 * @force_color_format: Property set by userspace to tell the GPU
> +	 * driver which color format to use. It only gets applied if hardware,
> +	 * meaning both the computer and the monitor, and the driver support the
> +	 * given format at the current resolution and refresh rate.
> +	 */
> +	u32 force_color_format;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1699,6 +1707,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *privacy_screen_hw_state_property;
>  
> +	/**
> +	 * @force_color_format_property: Default connector property for the
> +	 * force color format to be driven out of the connector.
> +	 */
> +	struct drm_property *force_color_format_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -2053,6 +2067,8 @@ void drm_connector_attach_privacy_screen_provider(
>  	struct drm_connector *connector, struct drm_privacy_screen *priv);
>  void drm_connector_update_privacy_screen(const struct drm_connector_state *connector_state);
>  
> +int drm_connector_attach_force_color_format_property(struct drm_connector *connector);
> +
>  /**
>   * struct drm_tile_group - Tile group metadata
>   * @refcount: reference count

Re: [PATCH v2 1/4] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check

[Harry Wentland]

On 2024-01-15 11:05, Andri Yngvason wrote:
> From: Werner Sembach <wse@tuxedocomputers.com>
> 
> Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check that was performed in the
> drm_mode_is_420_only() case, but not in the drm_mode_is_420_also() &&
> force_yuv420_output case.
> 
> Without further knowledge if YCbCr 4:2:0 is supported outside of HDMI,
> there is no reason to use RGB when the display
> reports drm_mode_is_420_only() even on a non HDMI connection.
> 
> This patch also moves both checks in the same if-case. This  eliminates an
> extra else-if-case.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Signed-off-by: Andri Yngvason <andri@yngvason.is>
> Tested-by: Andri Yngvason <andri@yngvason.is>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 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 f6575d7dee971..cc4d1f7f97b98 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5575,11 +5575,7 @@ static void fill_stream_properties_from_drm_display_mode(
>  	timing_out->v_border_bottom = 0;
>  	/* TODO: un-hardcode */
>  	if (drm_mode_is_420_only(info, mode_in)
> -			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
> -		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> -	else if (drm_mode_is_420_also(info, mode_in)
> -			&& aconnector
> -			&& aconnector->force_yuv420_output)
> +			|| (drm_mode_is_420_also(info, mode_in) && aconnector->force_yuv420_output))

We need to keep the && aconnector NULL check here, otherwise
writeback connectors will blow up.

Harry

>  		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
>  	else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
>  			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)