2024-01-09 18:12:00

by Andri Yngvason

Subject: [PATCH 0/7] New DRM properties for output color format

This is a subset of patches, originally submitted by Werner Sembach
titled: New uAPI drm properties for color management [1]

I've rebased against the current master branch, made modifications where
needed, and tested with both HDMI and DP on both Intel and AMD hardware,
using modified sway [2] and wlroots [3].

The original patch set added the following properties:
- active bpc
- active color format
- active color range
- preferred color format
and consolidated "Broadcast RGB" into a single property.

I've elected to only include active and preferred color format in this
patch set as I've very little interest in the other properties and,
hopefully, this will be easier for others to review.

[1]: https://lore.kernel.org/dri-devel/[email protected]/
[2]: https://github.com/swaywm/sway/pull/7903
[3]: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4509

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

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 75 ++++++++++--
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 8 ++
drivers/gpu/drm/drm_atomic_helper.c | 4 +
drivers/gpu/drm/drm_atomic_uapi.c | 4 +
drivers/gpu/drm/drm_connector.c | 111 ++++++++++++++++++
drivers/gpu/drm/i915/display/intel_display.c | 33 ++++++
drivers/gpu/drm/i915/display/intel_dp.c | 23 ++--
drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++
drivers/gpu/drm/i915/display/intel_hdmi.c | 16 ++-
include/drm/drm_connector.h | 27 +++++
10 files changed, 289 insertions(+), 22 deletions(-)


base-commit: 1f874787ed9a2d78ed59cb21d0d90ac0178eceb0
--
2.43.0

2024-01-09 18:12:15

by Andri Yngvason

Subject: [PATCH 1/7] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check

From: Werner Sembach <[email protected]>

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 <[email protected]>
Signed-off-by: Andri Yngvason <[email protected]>
Tested-by: Andri Yngvason <[email protected]>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +----
1 file changed, 1 insertion(+), 4 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 c8c00c2a5224a..10e041a3b2545 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5524,10 +5524,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->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

2024-01-09 18:12:31

by Andri Yngvason

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

From: Werner Sembach <[email protected]>

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 <[email protected]>
Signed-off-by: Andri Yngvason <[email protected]>
Tested-by: Andri Yngvason <[email protected]>
---
drivers/gpu/drm/drm_connector.c | 63 +++++++++++++++++++++++++++++++++
include/drm/drm_connector.h | 10 ++++++
2 files changed, 73 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index c3725086f4132..30d62e505d188 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_active_color_format_enum_list[] = {
+ { 0, "not applicable" },
+ { 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)

@@ -1390,6 +1398,15 @@ static const u32 dp_colorspaces =
* drm_connector_attach_max_bpc_property() to create and attach the
* property to the connector during initialization.
*
+ * 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. Possible values are "not applicable", "rgb", "ycbcr444",
+ * "ycbcr422", and "ycbcr420".
+ *
* Connectors also have one standardized atomic property:
*
* CRTC_ID:
@@ -2451,6 +2468,52 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
}
EXPORT_SYMBOL(drm_connector_attach_max_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;
+
+ if (!connector->active_color_format_property) {
+ prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE, "active color format",
+ drm_active_color_format_enum_list,
+ ARRAY_SIZE(drm_active_color_format_enum_list));
+ if (!prop)
+ return -ENOMEM;
+
+ connector->active_color_format_property = prop;
+ }
+
+ drm_object_attach_property(&connector->base, prop, 0);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
+
+/**
+ * drm_connector_set_active_color_format_property - sets the active color format property for a
+ * connector
+ * @connector: drm connector
+ * @active_color_format: color format for the connector currently active "on the cable"
+ *
+ * Should be used by atomic drivers to update the active color format over a connector.
+ */
+void drm_connector_set_active_color_format_property(struct drm_connector *connector,
+ u32 active_color_format)
+{
+ drm_object_property_set_value(&connector->base, connector->active_color_format_property,
+ active_color_format);
+}
+EXPORT_SYMBOL(drm_connector_set_active_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..9ae73cfdceeb1 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1699,6 +1699,12 @@ struct drm_connector {
*/
struct drm_property *privacy_screen_hw_state_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)
@@ -2053,6 +2059,10 @@ 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_active_color_format_property(struct drm_connector *connector);
+void drm_connector_set_active_color_format_property(struct drm_connector *connector,
+ u32 active_color_format);
+
/**
* struct drm_tile_group - Tile group metadata
* @refcount: reference count
--
2.43.0

2024-01-09 18:12:47

by Andri Yngvason

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

From: Werner Sembach <[email protected]>

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

Signed-off-by: Werner Sembach <[email protected]>
Signed-off-by: Andri Yngvason <[email protected]>
Tested-by: Andri Yngvason <[email protected]>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 ++++++++++++++++++-
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++
2 files changed, 45 insertions(+), 1 deletion(-)

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 10e041a3b2545..b44d06c3b1706 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6882,6 +6882,24 @@ int convert_dc_color_depth_into_bpc(enum dc_color_depth display_color_depth)
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_YCBCR422;
+ case PIXEL_ENCODING_YCBCR444:
+ return DRM_COLOR_FORMAT_YCBCR444;
+ case PIXEL_ENCODING_YCBCR420:
+ return DRM_COLOR_FORMAT_YCBCR420;
+ 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)
@@ -7436,8 +7454,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_active_color_format_property(&aconnector->base);
+ }

aconnector->base.state->max_bpc = 16;
aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
@@ -8969,6 +8989,26 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
kfree(dummy_updates);
}

+ /* 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;
+ struct dc_stream_state *stream;
+
+ if (crtc) {
+ new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+ stream = dm_new_crtc_state->stream;
+
+ if (stream) {
+ drm_connector_set_active_color_format_property(connector,
+ convert_dc_pixel_encoding_into_drm_color_format(
+ dm_new_crtc_state->stream->timing.pixel_encoding));
+ }
+ } else {
+ drm_connector_set_active_color_format_property(connector, 0);
+ }
+ }
+
/**
* Enable interrupts for CRTCs that are newly enabled or went through
* a modeset. It was intentionally deferred until after the front end
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 11da0eebee6c4..a4d1b3ea8f81c 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
@@ -600,6 +600,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->active_color_format_property = master->base.active_color_format_property;
+ if (connector->active_color_format_property)
+ drm_connector_attach_active_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

2024-01-09 18:13:05

by Andri Yngvason

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

From: Werner Sembach <[email protected]>

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

Signed-off-by: Werner Sembach <[email protected]>
Signed-off-by: Andri Yngvason <[email protected]>
Tested-by: Andri Yngvason <[email protected]>
---
drivers/gpu/drm/i915/display/intel_display.c | 33 ++++++++++++++++++++
drivers/gpu/drm/i915/display/intel_dp.c | 7 +++--
drivers/gpu/drm/i915/display/intel_dp_mst.c | 5 +++
drivers/gpu/drm/i915/display/intel_hdmi.c | 4 ++-
4 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index df582ff81b45f..79cc258db8f09 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7167,6 +7167,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_YCBCR420;
+ case INTEL_OUTPUT_FORMAT_YCBCR444:
+ return DRM_COLOR_FORMAT_YCBCR444;
+ default:
+ break;
+ }
+ return 0;
+}
+
static void intel_atomic_commit_tail(struct intel_atomic_state *state)
{
struct drm_device *dev = state->base.dev;
@@ -7438,6 +7453,9 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state,
{
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);
@@ -7506,6 +7524,21 @@ int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *_state,
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);
+
+ drm_connector_set_active_color_format_property(connector,
+ convert_intel_output_format_into_drm_color_format(
+ new_crtc_state->output_format));
+ } else
+ drm_connector_set_active_color_format_property(connector, 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 62ce92772367f..c40fe8a847614 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5910,10 +5910,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_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_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 aa10612626136..e7574ca0604e6 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1210,6 +1210,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->active_color_format_property =
+ intel_dp->attached_connector->base.active_color_format_property;
+ if (connector->active_color_format_property)
+ drm_connector_attach_active_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 bfa456fa7d25c..ce0221f90de92 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2611,8 +2611,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_active_color_format_property(connector);
+ }
}

/*
--
2.43.0

2024-01-09 18:14:07

by Andri Yngvason

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

From: Werner Sembach <[email protected]>

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

Signed-off-by: Werner Sembach <[email protected]>
Co-developed-by: Andri Yngvason <[email protected]>
Signed-off-by: Andri Yngvason <[email protected]>
Tested-by: Andri Yngvason <[email protected]>
---
drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++++++------
drivers/gpu/drm/i915/display/intel_dp_mst.c | 5 +++++
drivers/gpu/drm/i915/display/intel_hdmi.c | 12 +++++++++---
3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index c40fe8a847614..f241798660d0b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2698,21 +2698,23 @@ 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->preferred_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);
-
ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
respect_downstream_limits);
if (ret) {
@@ -5912,9 +5914,11 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
intel_attach_broadcast_rgb_property(connector);
if (HAS_GMCH(dev_priv)) {
drm_connector_attach_max_bpc_property(connector, 6, 10);
+ drm_connector_attach_preferred_color_format_property(connector);
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_preferred_color_format_property(connector);
drm_connector_attach_active_color_format_property(connector);
}

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index e7574ca0604e6..4a850eb9b8d4d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1210,6 +1210,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->preferred_color_format_property =
+ intel_dp->attached_connector->base.preferred_color_format_property;
+ if (connector->preferred_color_format_property)
+ drm_connector_attach_preferred_color_format_property(connector);
+
connector->active_color_format_property =
intel_dp->attached_connector->base.active_color_format_property;
if (connector->active_color_format_property)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index ce0221f90de92..3030589d245d7 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2214,19 +2214,24 @@ 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->preferred_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;
}

crtc_state->output_format = intel_hdmi_output_format(crtc_state);
+
ret = intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits);
if (ret) {
if (crtc_state->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
@@ -2613,6 +2618,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_preferred_color_format_property(connector);
drm_connector_attach_active_color_format_property(connector);
}
}
--
2.43.0

2024-01-09 18:32:55

by Andri Yngvason

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

From: Werner Sembach <[email protected]>

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

Signed-off-by: Werner Sembach <[email protected]>
Signed-off-by: Andri Yngvason <[email protected]>
Tested-by: Andri Yngvason <[email protected]>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 30 +++++++++++++++----
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 +++
2 files changed, 28 insertions(+), 6 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 b44d06c3b1706..262d420877c91 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5522,15 +5522,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->preferred_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->preferred_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->preferred_color_format == DRM_COLOR_FORMAT_RGB444
+ && !drm_mode_is_420_only(info, mode_in))
timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+ else
+ /*
+ * connector_state->preferred_color_format not possible
+ * || connector_state->preferred_color_format == 0 (auto)
+ * || connector_state->preferred_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(
@@ -7456,6 +7473,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,

if (!aconnector->mst_root) {
drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
+ drm_connector_attach_preferred_color_format_property(&aconnector->base);
drm_connector_attach_active_color_format_property(&aconnector->base);
}

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 a4d1b3ea8f81c..dc8cea0ac2c11 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
@@ -600,6 +600,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->preferred_color_format_property = master->base.preferred_color_format_property;
+ if (connector->preferred_color_format_property)
+ drm_connector_attach_preferred_color_format_property(&aconnector->base);
+
connector->active_color_format_property = master->base.active_color_format_property;
if (connector->active_color_format_property)
drm_connector_attach_active_color_format_property(&aconnector->base);
--
2.43.0

2024-01-09 18:33:33

by Andri Yngvason

Subject: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace

From: Werner Sembach <[email protected]>

Add a new general drm property "preferred color format" which can be used
by userspace to tell the graphic drivers to which color format to use.

Possible options are:
- auto (default/current behaviour)
- rgb
- ycbcr444
- ycbcr422 (not supported by both amdgpu and 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 deceptible to interference.

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

Signed-off-by: Werner Sembach <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Andri Yngvason <[email protected]>
Tested-by: Andri Yngvason <[email protected]>
---
drivers/gpu/drm/drm_atomic_helper.c | 4 +++
drivers/gpu/drm/drm_atomic_uapi.c | 4 +++
drivers/gpu/drm/drm_connector.c | 50 ++++++++++++++++++++++++++++-
include/drm/drm_connector.h | 17 ++++++++++
4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 68ffcc0b00dca..745a43d9c5da3 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->preferred_color_format !=
+ new_connector_state->preferred_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 98d3b10c08ae1..eba5dea1249e5 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -798,6 +798,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->preferred_color_format_property) {
+ state->preferred_color_format = val;
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
@@ -881,6 +883,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->preferred_color_format_property) {
+ *val = state->preferred_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 30d62e505d188..4de48a38792cf 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_preferred_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" },
+};
+
static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = {
{ 0, "not applicable" },
{ DRM_COLOR_FORMAT_RGB444, "rgb" },
@@ -1398,11 +1406,20 @@ static const u32 dp_colorspaces =
* drm_connector_attach_max_bpc_property() to create and attach the
* property to the connector during initialization.
*
+ * preferred 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_preferred_color_format_property()
+ * to create and attach the property to the connector during
+ * initialization. Possible values are "auto", "rgb", "ycbcr444",
+ * "ycbcr422", and "ycbcr420".
+ *
* 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
+ * connected monitor, and the "preferred color format". Drivers shall use
* drm_connector_attach_active_color_format_property() to install this
* property. Possible values are "not applicable", "rgb", "ycbcr444",
* "ycbcr422", and "ycbcr420".
@@ -2468,6 +2485,37 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
}
EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);

+/**
+ * drm_connector_attach_preferred_color_format_property - attach "preferred color format" property
+ * @connector: connector to attach preferred 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_preferred_color_format_property(struct drm_connector *connector)
+{
+ struct drm_device *dev = connector->dev;
+ struct drm_property *prop;
+
+ if (!connector->preferred_color_format_property) {
+ prop = drm_property_create_enum(dev, 0, "preferred color format",
+ drm_preferred_color_format_enum_list,
+ ARRAY_SIZE(drm_preferred_color_format_enum_list));
+ if (!prop)
+ return -ENOMEM;
+
+ connector->preferred_color_format_property = prop;
+ }
+
+ drm_object_attach_property(&connector->base, prop, 0);
+ connector->state->preferred_color_format = 0;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_preferred_color_format_property);
+
/**
* drm_connector_attach_active_color_format_property - attach "active color format" property
* @connector: connector to attach active color format property on.
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 9ae73cfdceeb1..d7bc54c8b42cb 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1026,6 +1026,16 @@ struct drm_connector_state {
*/
enum drm_privacy_screen_status privacy_screen_sw_state;

+ /**
+ * preferred_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. Userspace
+ * can check for (un-)successful application via the "active color
+ * format" property.
+ */
+ u32 preferred_color_format;
+
/**
* @hdr_output_metadata:
* DRM blob property for HDR output metadata
@@ -1699,6 +1709,12 @@ struct drm_connector {
*/
struct drm_property *privacy_screen_hw_state_property;

+ /**
+ * @preferred_color_format_property: Default connector property for the
+ * preferred color format to be driven out of the connector.
+ */
+ struct drm_property *preferred_color_format_property;
+
/**
* @active_color_format_property: Default connector property for the
* active color format to be driven out of the connector.
@@ -2059,6 +2075,7 @@ 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_preferred_color_format_property(struct drm_connector *connector);
int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
void drm_connector_set_active_color_format_property(struct drm_connector *connector,
u32 active_color_format);
--
2.43.0

2024-01-09 22:32:41

by Daniel Stone

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

Hi,

On Tue, 9 Jan 2024 at 18:12, Andri Yngvason <[email protected]> wrote:
> + * 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. Possible values are "not applicable", "rgb", "ycbcr444",
> + * "ycbcr422", and "ycbcr420".

How does userspace determine what's happened without polling? Will it
only change after an `ALLOW_MODESET` commit, and be guaranteed to be
updated after the commit has completed and the event being sent?
Should it send a HOTPLUG event? Other?

Cheers,
Daniel

2024-01-09 23:22:24

by Andri Yngvason

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

Hi Daniel,

Please excuse my misconfigured email client. HTML was accidentally
enabled in my previous messages, so I'll re-send it for the benefit of
mailing lists.

þri., 9. jan. 2024 kl. 22:32 skrifaði Daniel Stone <[email protected]>:
>
> On Tue, 9 Jan 2024 at 18:12, Andri Yngvason <[email protected]> wrote:
> > + * 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. Possible values are "not applicable", "rgb", "ycbcr444",
> > + * "ycbcr422", and "ycbcr420".
>
> How does userspace determine what's happened without polling? Will it
> only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> updated after the commit has completed and the event being sent?
> Should it send a HOTPLUG event? Other?

Userspace does not determine what's happened without polling. The
purpose of this property is not for programmatic verification that the
preferred property was applied. It is my understanding that it's
mostly intended for debugging purposes. It should only change as a
consequence of modesetting, although I didn't actually look into what
happens if you set the "preferred color format" outside of a modeset.

The way I've implemented things in sway, calling the
"preferred_signal_format" command triggers a modeset with the
"preferred color format" set and calling "get_outputs", immediately
queries the "actual color format" and displays it.

Regards,
Andri

2024-01-10 09:27:25

by Maxime Ripard

Subject: Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace

Hi,

On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
> From: Werner Sembach <[email protected]>
>
> Add a new general drm property "preferred color format" which can be used
> by userspace to tell the graphic drivers to which color format to use.
>
> Possible options are:
> - auto (default/current behaviour)
> - rgb
> - ycbcr444
> - ycbcr422 (not supported by both amdgpu and 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.

I looked at the patch and I couldn't find what is supposed to happen if
you set it to something else than auto, and the driver can't match that.
Are we supposed to fallback to the "auto" behaviour, or are we suppose
to reject the mode entirely?

The combination with the active output format property suggests the
former, but we should document it explicitly.

> 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 deceptible to interference.
>
> In the future, automatic color calibration for screens might also depend on
> this option being available.
>
> Signed-off-by: Werner Sembach <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> Signed-off-by: Andri Yngvason <[email protected]>
> Tested-by: Andri Yngvason <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 4 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 4 +++
> drivers/gpu/drm/drm_connector.c | 50 ++++++++++++++++++++++++++++-
> include/drm/drm_connector.h | 17 ++++++++++
> 4 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 68ffcc0b00dca..745a43d9c5da3 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->preferred_color_format !=
> + new_connector_state->preferred_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 98d3b10c08ae1..eba5dea1249e5 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -798,6 +798,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->preferred_color_format_property) {
> + state->preferred_color_format = val;
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> @@ -881,6 +883,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->preferred_color_format_property) {
> + *val = state->preferred_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 30d62e505d188..4de48a38792cf 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_preferred_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" },
> +};
> +
> static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = {
> { 0, "not applicable" },
> { DRM_COLOR_FORMAT_RGB444, "rgb" },
> @@ -1398,11 +1406,20 @@ static const u32 dp_colorspaces =
> * drm_connector_attach_max_bpc_property() to create and attach the
> * property to the connector during initialization.
> *
> + * preferred 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_preferred_color_format_property()
> + * to create and attach the property to the connector during
> + * initialization. Possible values are "auto", "rgb", "ycbcr444",
> + * "ycbcr422", and "ycbcr420".
> + *
> * 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
> + * connected monitor, and the "preferred color format". Drivers shall use
> * drm_connector_attach_active_color_format_property() to install this
> * property. Possible values are "not applicable", "rgb", "ycbcr444",
> * "ycbcr422", and "ycbcr420".
> @@ -2468,6 +2485,37 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
>
> +/**
> + * drm_connector_attach_preferred_color_format_property - attach "preferred color format" property
> + * @connector: connector to attach preferred 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_preferred_color_format_property(struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_property *prop;
> +
> + if (!connector->preferred_color_format_property) {
> + prop = drm_property_create_enum(dev, 0, "preferred color format",
> + drm_preferred_color_format_enum_list,
> + ARRAY_SIZE(drm_preferred_color_format_enum_list));
> + if (!prop)
> + return -ENOMEM;
> +
> + connector->preferred_color_format_property = prop;
> + }
> +
> + drm_object_attach_property(&connector->base, prop, 0);
> + connector->state->preferred_color_format = 0;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_preferred_color_format_property);
> +
> /**
> * drm_connector_attach_active_color_format_property - attach "active color format" property
> * @connector: connector to attach active color format property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 9ae73cfdceeb1..d7bc54c8b42cb 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1026,6 +1026,16 @@ struct drm_connector_state {
> */
> enum drm_privacy_screen_status privacy_screen_sw_state;
>
> + /**
> + * preferred_color_format: Property set by userspace to tell the GPU

That's not the proper doc format, you're missing a @

Maxime

Attachments:

2024-01-10 10:13:19

by Andri Yngvason

Subject: Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace

Hi,

mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard <[email protected]>:
> On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
> > From: Werner Sembach <[email protected]>
> >
> > Add a new general drm property "preferred color format" which can be used
> > by userspace to tell the graphic drivers to which color format to use.
> >
> > Possible options are:
> > - auto (default/current behaviour)
> > - rgb
> > - ycbcr444
> > - ycbcr422 (not supported by both amdgpu and 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.
>
> I looked at the patch and I couldn't find what is supposed to happen if
> you set it to something else than auto, and the driver can't match that.
> Are we supposed to fallback to the "auto" behaviour, or are we suppose
> to reject the mode entirely?
>
> The combination with the active output format property suggests the
> former, but we should document it explicitly.

It is also my understanding that it should fall back to the "auto"
behaviour. I will add this to the documentation.

Thanks,
Andri

2024-01-10 10:44:21

by Daniel Vetter

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

On Tue, Jan 09, 2024 at 11:12:11PM +0000, Andri Yngvason wrote:
> Hi Daniel,
>
> ?ri., 9. jan. 2024 kl. 22:32 skrifa?i Daniel Stone <[email protected]>:
>
> > On Tue, 9 Jan 2024 at 18:12, Andri Yngvason <[email protected]> wrote:
> > > + * 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. Possible values are "not applicable", "rgb",
> > "ycbcr444",
> > > + * "ycbcr422", and "ycbcr420".
> >
> > How does userspace determine what's happened without polling? Will it
> > only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> > updated after the commit has completed and the event being sent?
> > Should it send a HOTPLUG event? Other?
> >
>
> Userspace does not determine what's happened without polling. The purpose
> of this property is not for programmatic verification that the preferred
> property was applied. It is my understanding that it's mostly intended for
> debugging purposes. It should only change as a consequence of modesetting,
> although I didn't actually look into what happens if you set the "preferred
> color format" outside of a modeset.

This feels a bit irky to me, since we don't have any synchronization and
it kinda breaks how userspace gets to know about stuff.

For context the current immutable properties are all stuff that's derived
from the sink (like edid, or things like that). Userspace is guaranteed to
get a hotplug event (minus driver bugs as usual) if any of these change,
and we've added infrastructure so that the hotplug event even contains the
specific property so that userspace can avoid re-read (which can cause
some costly re-probing) them all.

As an example you can look at drm_connector_set_link_status_property,
which drivers follow by a call to drm_kms_helper_connector_hotplug_event
to make sure userspace knows about what's up. Could be optimized I think.

This thing here works entirely differently, and I think we need somewhat
new semantics for this:

- I agree it should be read-only for userspace, so immutable sounds right.

- But I also agree with Daniel Stone that this should be tied more
directly to the modeset state.

So I think the better approach would be to put the output type into
drm_connector_state, require that drivers compute it in their
->atomic_check code (which in the future would allow us to report it out
for TEST_ONLY commits too), and so guarantee that the value is updated
right after the kms ioctl returns (and not somewhen later for non-blocking
commits).

You probably need a bit of work to be able to handle immutable properties
with the atomic state infrastructure, but I think otherwise this should
fit all rather neatly.

Cheers, Sima
>
> The way I've implemented things in sway, calling the
> "preferred_signal_format" command triggers a modeset with the "preferred
> color format" set and calling "get_outputs", immediately queries the
> "actual color format" and displays it.
>
> Regards,
> Andri

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2024-01-10 11:11:09

by Daniel Vetter

Subject: Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property

On Tue, Jan 09, 2024 at 06:11:00PM +0000, Andri Yngvason wrote:
> From: Werner Sembach <[email protected]>
>
> This commit implements the "active color format" drm property for the AMD
> GPU driver.
>
> Signed-off-by: Werner Sembach <[email protected]>
> Signed-off-by: Andri Yngvason <[email protected]>
> Tested-by: Andri Yngvason <[email protected]>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 ++++++++++++++++++-
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++
> 2 files changed, 45 insertions(+), 1 deletion(-)
>
> 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 10e041a3b2545..b44d06c3b1706 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6882,6 +6882,24 @@ int convert_dc_color_depth_into_bpc(enum dc_color_depth display_color_depth)
> 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_YCBCR422;
> + case PIXEL_ENCODING_YCBCR444:
> + return DRM_COLOR_FORMAT_YCBCR444;
> + case PIXEL_ENCODING_YCBCR420:
> + return DRM_COLOR_FORMAT_YCBCR420;
> + 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)
> @@ -7436,8 +7454,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_active_color_format_property(&aconnector->base);
> + }
>
> aconnector->base.state->max_bpc = 16;
> aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
> @@ -8969,6 +8989,26 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> kfree(dummy_updates);
> }
>
> + /* 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;
> + struct dc_stream_state *stream;
> +
> + if (crtc) {
> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> + stream = dm_new_crtc_state->stream;
> +
> + if (stream) {
> + drm_connector_set_active_color_format_property(connector,
> + convert_dc_pixel_encoding_into_drm_color_format(
> + dm_new_crtc_state->stream->timing.pixel_encoding));
> + }
> + } else {
> + drm_connector_set_active_color_format_property(connector, 0);

Just realized an even bigger reason why your current design doesn't work:
You don't have locking here.

And you cannot grab the required lock, which is
drm_dev->mode_config.mutex, because that would result in deadlocks. So
this really needs to use the atomic state based design I've described.

A bit a tanget, but it would be really good to add a lockdep assert into
drm_object_property_set_value, that at least for atomic drivers and
connectors the above lock must be held for changing property values. But
it will be quite a bit of audit to make sure all current users obey that
rule.

Cheers, Sima
> + }
> + }
> +
> /**
> * Enable interrupts for CRTCs that are newly enabled or went through
> * a modeset. It was intentionally deferred until after the front end
> 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 11da0eebee6c4..a4d1b3ea8f81c 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
> @@ -600,6 +600,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->active_color_format_property = master->base.active_color_format_property;
> + if (connector->active_color_format_property)
> + drm_connector_attach_active_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
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2024-01-10 12:53:33

by Andri Yngvason

Subject: Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property

mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <[email protected]>:
>
> On Tue, Jan 09, 2024 at 06:11:00PM +0000, Andri Yngvason wrote:
> > + /* 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;
> > + struct dc_stream_state *stream;
> > +
> > + if (crtc) {
> > + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > + dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> > + stream = dm_new_crtc_state->stream;
> > +
> > + if (stream) {
> > + drm_connector_set_active_color_format_property(connector,
> > + convert_dc_pixel_encoding_into_drm_color_format(
> > + dm_new_crtc_state->stream->timing.pixel_encoding));
> > + }
> > + } else {
> > + drm_connector_set_active_color_format_property(connector, 0);
>
> Just realized an even bigger reason why your current design doesn't work:
> You don't have locking here.
>
> And you cannot grab the required lock, which is
> drm_dev->mode_config.mutex, because that would result in deadlocks. So
> this really needs to use the atomic state based design I've described.
>

Maybe we should just drop "actual color format" and instead fail the
modeset if the "preferred color format" property cannot be satisfied?
It seems like the simplest thing to do here, though it is perhaps less
convenient for userspace. In that case, the "preferred color format"
property should just be called "color format".

Thanks,
Andri

2024-01-10 13:09:32

by Werner Sembach

Subject: Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace

Hi,

Am 10.01.24 um 11:11 schrieb Andri Yngvason:
> Hi,
>
> mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard <[email protected]>:
>> On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
>>> From: Werner Sembach <[email protected]>
>>>
>>> Add a new general drm property "preferred color format" which can be used
>>> by userspace to tell the graphic drivers to which color format to use.
>>>
>>> Possible options are:
>>> - auto (default/current behaviour)
>>> - rgb
>>> - ycbcr444
>>> - ycbcr422 (not supported by both amdgpu and 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.
>> I looked at the patch and I couldn't find what is supposed to happen if
>> you set it to something else than auto, and the driver can't match that.
>> Are we supposed to fallback to the "auto" behaviour, or are we suppose
>> to reject the mode entirely?
>>
>> The combination with the active output format property suggests the
>> former, but we should document it explicitly.
> It is also my understanding that it should fall back to the "auto"
> behaviour. I will add this to the documentation.

Yes, that was the intention, and then userspace can check, but it wasn't well
received: https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_964530

Actually a lot of the thoughts that went into the original patch set can be
found in that topic.

There was another iteration of the patch set that I never finished and sent to
the LKML because I got discouraged by this:
https://lore.kernel.org/dri-devel/20210623102923.70877c1a@eldfell/

I can try to dig it up, but it is completely untested and I don't think I still
have the respective TODO list anymore, so I don't know if it is a better or
worst starting point than the last iteration I sent to the LKML.

Greetings

Werner

>
> Thanks,
> Andri

2024-01-10 13:09:54

by Daniel Vetter

Subject: Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property

On Wed, 10 Jan 2024 at 13:53, Andri Yngvason <[email protected]> wrote:
>
> mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <[email protected]>:
> >
> > On Tue, Jan 09, 2024 at 06:11:00PM +0000, Andri Yngvason wrote:
> > > + /* 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;
> > > + struct dc_stream_state *stream;
> > > +
> > > + if (crtc) {
> > > + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > + dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> > > + stream = dm_new_crtc_state->stream;
> > > +
> > > + if (stream) {
> > > + drm_connector_set_active_color_format_property(connector,
> > > + convert_dc_pixel_encoding_into_drm_color_format(
> > > + dm_new_crtc_state->stream->timing.pixel_encoding));
> > > + }
> > > + } else {
> > > + drm_connector_set_active_color_format_property(connector, 0);
> >
> > Just realized an even bigger reason why your current design doesn't work:
> > You don't have locking here.
> >
> > And you cannot grab the required lock, which is
> > drm_dev->mode_config.mutex, because that would result in deadlocks. So
> > this really needs to use the atomic state based design I've described.
> >
>
> Maybe we should just drop "actual color format" and instead fail the
> modeset if the "preferred color format" property cannot be satisfied?
> It seems like the simplest thing to do here, though it is perhaps less
> convenient for userspace. In that case, the "preferred color format"
> property should just be called "color format".

Yeah that's more in line with how other atomic properties work. This
way userspace can figure out what works with a TEST_ONLY commit too.
And for this to work you probably want to have an "automatic" setting
too.
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2024-01-10 13:15:54

by Werner Sembach

Subject: Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property

Hi,

Am 10.01.24 um 14:09 schrieb Daniel Vetter:
> On Wed, 10 Jan 2024 at 13:53, Andri Yngvason <[email protected]> wrote:
>> mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <[email protected]>:
>>> On Tue, Jan 09, 2024 at 06:11:00PM +0000, Andri Yngvason wrote:
>>>> + /* 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;
>>>> + struct dc_stream_state *stream;
>>>> +
>>>> + if (crtc) {
>>>> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>>>> + dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>>>> + stream = dm_new_crtc_state->stream;
>>>> +
>>>> + if (stream) {
>>>> + drm_connector_set_active_color_format_property(connector,
>>>> + convert_dc_pixel_encoding_into_drm_color_format(
>>>> + dm_new_crtc_state->stream->timing.pixel_encoding));
>>>> + }
>>>> + } else {
>>>> + drm_connector_set_active_color_format_property(connector, 0);
>>> Just realized an even bigger reason why your current design doesn't work:
>>> You don't have locking here.
>>>
>>> And you cannot grab the required lock, which is
>>> drm_dev->mode_config.mutex, because that would result in deadlocks. So
>>> this really needs to use the atomic state based design I've described.
>>>
>> Maybe we should just drop "actual color format" and instead fail the
>> modeset if the "preferred color format" property cannot be satisfied?
>> It seems like the simplest thing to do here, though it is perhaps less
>> convenient for userspace. In that case, the "preferred color format"
>> property should just be called "color format".
> Yeah that's more in line with how other atomic properties work. This
> way userspace can figure out what works with a TEST_ONLY commit too.
> And for this to work you probably want to have an "automatic" setting
> too.
> -Sima

The problem with TEST_ONLY probing is that color format settings are
interdependent: https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_966634

So changing any other setting may require every color format to be TEST_ONLY
probed again.

Greetings

Werner

2024-01-10 13:27:07

by Daniel Stone

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

Hi,

On Wed, 10 Jan 2024 at 10:44, Daniel Vetter <[email protected]> wrote:
> On Tue, Jan 09, 2024 at 11:12:11PM +0000, Andri Yngvason wrote:
> > ţri., 9. jan. 2024 kl. 22:32 skrifađi Daniel Stone <[email protected]>:
> > > How does userspace determine what's happened without polling? Will it
> > > only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> > > updated after the commit has completed and the event being sent?
> > > Should it send a HOTPLUG event? Other?
> > >
> >
> > Userspace does not determine what's happened without polling. The purpose
> > of this property is not for programmatic verification that the preferred
> > property was applied. It is my understanding that it's mostly intended for
> > debugging purposes. It should only change as a consequence of modesetting,
> > although I didn't actually look into what happens if you set the "preferred
> > color format" outside of a modeset.
>
> This feels a bit irky to me, since we don't have any synchronization and
> it kinda breaks how userspace gets to know about stuff.
>
> For context the current immutable properties are all stuff that's derived
> from the sink (like edid, or things like that). Userspace is guaranteed to
> get a hotplug event (minus driver bugs as usual) if any of these change,
> and we've added infrastructure so that the hotplug event even contains the
> specific property so that userspace can avoid re-read (which can cause
> some costly re-probing) them all.

Right.

> [...]
>
> This thing here works entirely differently, and I think we need somewhat
> new semantics for this:
>
> - I agree it should be read-only for userspace, so immutable sounds right.
>
> - But I also agree with Daniel Stone that this should be tied more
> directly to the modeset state.
>
> So I think the better approach would be to put the output type into
> drm_connector_state, require that drivers compute it in their
> ->atomic_check code (which in the future would allow us to report it out
> for TEST_ONLY commits too), and so guarantee that the value is updated
> right after the kms ioctl returns (and not somewhen later for non-blocking
> commits).

That's exactly the point. Whether userspace gets an explicit
notification or it has to 'know' when to read isn't much of an issue -
just as long as it's well defined. I think the suggestion of 'do it in
atomic_check and then it's guaranteed to be readable when the commit
completes' is a good one.

I do still have some reservations - for instance, why do we have the
fallback to auto when userspace has explicitly requested a certain
type? - but they may have been covered previously.

Cheers,
Daniel

2024-01-10 13:42:04

by Werner Sembach

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

Hi,

Am 09.01.24 um 19:10 schrieb Andri Yngvason:
> From: Werner Sembach <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Andri Yngvason <[email protected]>
> Tested-by: Andri Yngvason <[email protected]>

One suggestion from back then was, instead picking out singular properties like
"active color format", to just expose whatever the last HDMI or DP metadata
block(s)/frame(s) that was sent over the display wire was to userspace and
accompanying it with a parsing script.

Question: Does the driver really actually know what the GPU is ultimatively
sending out the wire, or is that decided by a final firmware blob we have no
info about?

Greetings

Werner

> ---
> drivers/gpu/drm/drm_connector.c | 63 +++++++++++++++++++++++++++++++++
> include/drm/drm_connector.h | 10 ++++++
> 2 files changed, 73 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index c3725086f4132..30d62e505d188 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_active_color_format_enum_list[] = {
> + { 0, "not applicable" },
> + { 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)
>
> @@ -1390,6 +1398,15 @@ static const u32 dp_colorspaces =
> * drm_connector_attach_max_bpc_property() to create and attach the
> * property to the connector during initialization.
> *
> + * 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. Possible values are "not applicable", "rgb", "ycbcr444",
> + * "ycbcr422", and "ycbcr420".
> + *
> * Connectors also have one standardized atomic property:
> *
> * CRTC_ID:
> @@ -2451,6 +2468,52 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_connector_attach_max_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;
> +
> + if (!connector->active_color_format_property) {
> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE, "active color format",
> + drm_active_color_format_enum_list,
> + ARRAY_SIZE(drm_active_color_format_enum_list));
> + if (!prop)
> + return -ENOMEM;
> +
> + connector->active_color_format_property = prop;
> + }
> +
> + drm_object_attach_property(&connector->base, prop, 0);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
> +
> +/**
> + * drm_connector_set_active_color_format_property - sets the active color format property for a
> + * connector
> + * @connector: drm connector
> + * @active_color_format: color format for the connector currently active "on the cable"
> + *
> + * Should be used by atomic drivers to update the active color format over a connector.
> + */
> +void drm_connector_set_active_color_format_property(struct drm_connector *connector,
> + u32 active_color_format)
> +{
> + drm_object_property_set_value(&connector->base, connector->active_color_format_property,
> + active_color_format);
> +}
> +EXPORT_SYMBOL(drm_connector_set_active_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..9ae73cfdceeb1 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1699,6 +1699,12 @@ struct drm_connector {
> */
> struct drm_property *privacy_screen_hw_state_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)
> @@ -2053,6 +2059,10 @@ 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_active_color_format_property(struct drm_connector *connector);
> +void drm_connector_set_active_color_format_property(struct drm_connector *connector,
> + u32 active_color_format);
> +
> /**
> * struct drm_tile_group - Tile group metadata
> * @refcount: reference count

2024-01-10 13:43:23

by Andri Yngvason

Subject: Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace

mið., 10. jan. 2024 kl. 13:09 skrifaði Werner Sembach <[email protected]>:
>
> Hi,
>
> Am 10.01.24 um 11:11 schrieb Andri Yngvason:
> > Hi,
> >
> > mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard <[email protected]>:
> >> On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
> >>> From: Werner Sembach <[email protected]>
> >>>
> >>> Add a new general drm property "preferred color format" which can be used
> >>> by userspace to tell the graphic drivers to which color format to use.
> >>>
> >>> Possible options are:
> >>> - auto (default/current behaviour)
> >>> - rgb
> >>> - ycbcr444
> >>> - ycbcr422 (not supported by both amdgpu and 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.
> >> I looked at the patch and I couldn't find what is supposed to happen if
> >> you set it to something else than auto, and the driver can't match that.
> >> Are we supposed to fallback to the "auto" behaviour, or are we suppose
> >> to reject the mode entirely?
> >>
> >> The combination with the active output format property suggests the
> >> former, but we should document it explicitly.
> > It is also my understanding that it should fall back to the "auto"
> > behaviour. I will add this to the documentation.
>
> Yes, that was the intention, and then userspace can check, but it wasn't well
> received: https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_964530
>
> Actually a lot of the thoughts that went into the original patch set can be
> found in that topic.
>
> There was another iteration of the patch set that I never finished and sent to
> the LKML because I got discouraged by this:
> https://lore.kernel.org/dri-devel/20210623102923.70877c1a@eldfell/

Well, I've implemented this for sway and wlroots now and Simon has
reacted positively, so this does appear likely to end up as a feature
in wlroots based compositors.

>
> I can try to dig it up, but it is completely untested and I don't think I still
> have the respective TODO list anymore, so I don't know if it is a better or
> worst starting point than the last iteration I sent to the LKML.
>

You can send the patches to me if you want and I can see if they're
useful. I'm really only interested in the color format part though.
Alternatively, you can continue your work and post it to LKML and I
can focus on the userspace side and testing. By the way, I have an
HDMI analyzer that can tell me the actual color format.

Thanks,
Andri

2024-01-10 14:54:34

by Werner Sembach

Subject: Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace

Hi,

Am 10.01.24 um 14:42 schrieb Andri Yngvason:
> mið., 10. jan. 2024 kl. 13:09 skrifaði Werner Sembach<[email protected]>:
>> Hi,
>>
>> Am 10.01.24 um 11:11 schrieb Andri Yngvason:
>>> Hi,
>>>
>>> mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard<[email protected]>:
>>>> On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
>>>>> From: Werner Sembach<[email protected]>
>>>>>
>>>>> Add a new general drm property "preferred color format" which can be used
>>>>> by userspace to tell the graphic drivers to which color format to use.
>>>>>
>>>>> Possible options are:
>>>>> - auto (default/current behaviour)
>>>>> - rgb
>>>>> - ycbcr444
>>>>> - ycbcr422 (not supported by both amdgpu and 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.
>>>> I looked at the patch and I couldn't find what is supposed to happen if
>>>> you set it to something else than auto, and the driver can't match that.
>>>> Are we supposed to fallback to the "auto" behaviour, or are we suppose
>>>> to reject the mode entirely?
>>>>
>>>> The combination with the active output format property suggests the
>>>> former, but we should document it explicitly.
>>> It is also my understanding that it should fall back to the "auto"
>>> behaviour. I will add this to the documentation.
>> Yes, that was the intention, and then userspace can check, but it wasn't well
>> received:https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_964530
>>
>> Actually a lot of the thoughts that went into the original patch set can be
>> found in that topic.
>>
>> There was another iteration of the patch set that I never finished and sent to
>> the LKML because I got discouraged by this:
>> https://lore.kernel.org/dri-devel/20210623102923.70877c1a@eldfell/
> Well, I've implemented this for sway and wlroots now and Simon has
> reacted positively, so this does appear likely to end up as a feature
> in wlroots based compositors.
>
>> I can try to dig it up, but it is completely untested and I don't think I still
>> have the respective TODO list anymore, so I don't know if it is a better or
>> worst starting point than the last iteration I sent to the LKML.
>>
> You can send the patches to me if you want and I can see if they're
> useful. I'm really only interested in the color format part though.
> Alternatively, you can continue your work and post it to LKML and I
> can focus on the userspace side and testing. By the way, I have an
> HDMI analyzer that can tell me the actual color format.

Searched for what I still had in my private repo, see attachments, filename is
the branch name I used and like I said: I don't know which state these branches
are in.

The hacking_ branch was based on 25fe90f43fa312213b653dc1f12fd2d80f855883 from
linux-next and the rejected_ one on 132b189b72a94328f17fd70321bfe63e5b4208e9
from drm-tip.

And the rejected_ one is 2 weeks newer.

To pick it up again I would first need to allocate some time for it, ... which
could take some time.

With a HDMI analyzer at hand you are better equipped then me already. I was
working with printf statements, Monitor OSD's and test patterns like
https://media.extron.com/public/technology/landing/vector4k/img/scalfe-444Chroma.png
and http://www.lagom.nl/lcd-test/ while being red blind xD.

>
> Thanks,
> Andri

Attachments:

2024-01-10 17:16:05

by Andri Yngvason

Subject: Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property

Hi Werner,

mið., 10. jan. 2024 kl. 13:14 skrifaði Werner Sembach <[email protected]>:
>
> Hi,
>
> Am 10.01.24 um 14:09 schrieb Daniel Vetter:
> > On Wed, 10 Jan 2024 at 13:53, Andri Yngvason <[email protected]> wrote:
> >> mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <[email protected]>:
> >>> On Tue, Jan 09, 2024 at 06:11:00PM +0000, Andri Yngvason wrote:
> >>>> + /* 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;
> >>>> + struct dc_stream_state *stream;
> >>>> +
> >>>> + if (crtc) {
> >>>> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> >>>> + dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> >>>> + stream = dm_new_crtc_state->stream;
> >>>> +
> >>>> + if (stream) {
> >>>> + drm_connector_set_active_color_format_property(connector,
> >>>> + convert_dc_pixel_encoding_into_drm_color_format(
> >>>> + dm_new_crtc_state->stream->timing.pixel_encoding));
> >>>> + }
> >>>> + } else {
> >>>> + drm_connector_set_active_color_format_property(connector, 0);
> >>> Just realized an even bigger reason why your current design doesn't work:
> >>> You don't have locking here.
> >>>
> >>> And you cannot grab the required lock, which is
> >>> drm_dev->mode_config.mutex, because that would result in deadlocks. So
> >>> this really needs to use the atomic state based design I've described.
> >>>
> >> Maybe we should just drop "actual color format" and instead fail the
> >> modeset if the "preferred color format" property cannot be satisfied?
> >> It seems like the simplest thing to do here, though it is perhaps less
> >> convenient for userspace. In that case, the "preferred color format"
> >> property should just be called "color format".
> > Yeah that's more in line with how other atomic properties work. This
> > way userspace can figure out what works with a TEST_ONLY commit too.
> > And for this to work you probably want to have an "automatic" setting
> > too.
> > -Sima
>
> The problem with TEST_ONLY probing is that color format settings are
> interdependent: https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_966634
>
> So changing any other setting may require every color format to be TEST_ONLY
> probed again.
>

If we put a bit map containing the possible color formats into
drm_mode_mode_info (I'm thinking that it could go into flags), we'd be
able to eliminate a bunch of combinations early on. Do you think that
would make things more bearable?

I'm thinking, something like this:
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 128d09138ceb3..59980803cb89e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -124,6 +124,13 @@ extern "C" {
#define DRM_MODE_FLAG_PIC_AR_256_135 \
(DRM_MODE_PICTURE_ASPECT_256_135<<19)

+/* Possible color formats (4 bits) */
+#define DRM_MODE_FLAG_COLOR_FORMAT_MASK (0x0f << 22)
+#define DRM_MODE_FLAG_COLOR_FORMAT_RGB (1 << 22)
+#define DRM_MODE_FLAG_COLOR_FORMAT_YCBCR444 (1 << 23)
+#define DRM_MODE_FLAG_COLOR_FORMAT_YCBCR422 (1 << 24)
+#define DRM_MODE_FLAG_COLOR_FORMAT_YCBCR420 (1 << 25)
+
#define DRM_MODE_FLAG_ALL (DRM_MODE_FLAG_PHSYNC | \
DRM_MODE_FLAG_NHSYNC | \
DRM_MODE_FLAG_PVSYNC | \
@@ -136,7 +143,8 @@ extern "C" {
DRM_MODE_FLAG_HSKEW | \
DRM_MODE_FLAG_DBLCLK | \
DRM_MODE_FLAG_CLKDIV2 | \
- DRM_MODE_FLAG_3D_MASK)
+ DRM_MODE_FLAG_3D_MASK | \
+ DRM_MODE_FLAG_COLOR_FORMAT_MASK)

/* DPMS flags */
/* bit compatible with the xorg definitions. */

2024-01-11 17:18:34

by Andri Yngvason

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

mið., 10. jan. 2024 kl. 13:26 skrifaði Daniel Stone <[email protected]>:
> >
> > This thing here works entirely differently, and I think we need somewhat
> > new semantics for this:
> >
> > - I agree it should be read-only for userspace, so immutable sounds right.
> >
> > - But I also agree with Daniel Stone that this should be tied more
> > directly to the modeset state.
> >
> > So I think the better approach would be to put the output type into
> > drm_connector_state, require that drivers compute it in their
> > ->atomic_check code (which in the future would allow us to report it out
> > for TEST_ONLY commits too), and so guarantee that the value is updated
> > right after the kms ioctl returns (and not somewhen later for non-blocking
> > commits).
>
> That's exactly the point. Whether userspace gets an explicit
> notification or it has to 'know' when to read isn't much of an issue -
> just as long as it's well defined. I think the suggestion of 'do it in
> atomic_check and then it's guaranteed to be readable when the commit
> completes' is a good one.
>
> I do still have some reservations - for instance, why do we have the
> fallback to auto when userspace has explicitly requested a certain
> type? - but they may have been covered previously.
>

We discussed this further on IRC and this is summary of that discussion:

Sima proposed a new type of property that can be used to git feedback to
userspace after atomic ioctl. The user supplies a list of output properties
that they want to query and the kernel fills it in before returning from the
ioctl. This would help to get some information about why things failed
during TEST_ONLY.

Emersion raised the point that you might not know how much memory is needed
beforehand for the returned properties, to which sima replied: blob
property. There was some discussion about how that makes it possible to leak
kernel memory, especially if userspace does not know about a new property
blob. Emersion pointed out that userspace should only request properties
that it understands and pq agreed.

Emersion asked how the user should inform the kernel that it's done with the
blob, to which sima replied: DRM_IOCTL_MODE_DESTROYPROPBLOB. Sima also
mentioned using some sort of weak reference garbage collection scheme for
properties and there was some further discussion, but I'm not sure there was
any conclusion.

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 asked again if we should drop the "active color format" property as it
seems to be more trouble than it's worth for now. pq mentioned that there
are 2 separate use cases (in his words):
- People playing with setting UI would like to know what "auto" would result
in, but that's just nice to have
- The other use case is the flicker-free boot into known configuration He
went on to point out that the main problem with "auto" is that any modeset
could make the driver decide differently. This means that we cannot fully
rely on the previously set property.

However, leaving out "active color property" did not put us in a worse
situation than before, so the conclusion was that we should leave it out for
now. For GUI selectors, the current TEST_ONLY should be good enough, and all
the fancy stuff discussed previously isn't needed for now.

To summarise the summary: this means that we will drop the "active
color format" property and rename the "preferred color format"
property to "force color format" or just "color format" and failing to
satisfy that constraint will fail the modeset rather than falling back
to the "auto" behaviour.

Cheers,
Andri

2024-01-11 23:54:41

by Werner Sembach

Subject: Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property

Hi,

Am 10.01.24 um 18:15 schrieb Andri Yngvason:
> Hi Werner,
>
> mið., 10. jan. 2024 kl. 13:14 skrifaði Werner Sembach <[email protected]>:
>> Hi,
>>
>> Am 10.01.24 um 14:09 schrieb Daniel Vetter:
>>> On Wed, 10 Jan 2024 at 13:53, Andri Yngvason <[email protected]> wrote:
>>>> mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <[email protected]>:
>>>>> On Tue, Jan 09, 2024 at 06:11:00PM +0000, Andri Yngvason wrote:
>>>>>> + /* 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;
>>>>>> + struct dc_stream_state *stream;
>>>>>> +
>>>>>> + if (crtc) {
>>>>>> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>>>>>> + dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>>>>>> + stream = dm_new_crtc_state->stream;
>>>>>> +
>>>>>> + if (stream) {
>>>>>> + drm_connector_set_active_color_format_property(connector,
>>>>>> + convert_dc_pixel_encoding_into_drm_color_format(
>>>>>> + dm_new_crtc_state->stream->timing.pixel_encoding));
>>>>>> + }
>>>>>> + } else {
>>>>>> + drm_connector_set_active_color_format_property(connector, 0);
>>>>> Just realized an even bigger reason why your current design doesn't work:
>>>>> You don't have locking here.
>>>>>
>>>>> And you cannot grab the required lock, which is
>>>>> drm_dev->mode_config.mutex, because that would result in deadlocks. So
>>>>> this really needs to use the atomic state based design I've described.
>>>>>
>>>> Maybe we should just drop "actual color format" and instead fail the
>>>> modeset if the "preferred color format" property cannot be satisfied?
>>>> It seems like the simplest thing to do here, though it is perhaps less
>>>> convenient for userspace. In that case, the "preferred color format"
>>>> property should just be called "color format".
>>> Yeah that's more in line with how other atomic properties work. This
>>> way userspace can figure out what works with a TEST_ONLY commit too.
>>> And for this to work you probably want to have an "automatic" setting
>>> too.
>>> -Sima
>> The problem with TEST_ONLY probing is that color format settings are
>> interdependent: https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_966634
>>
>> So changing any other setting may require every color format to be TEST_ONLY
>> probed again.
>>
> If we put a bit map containing the possible color formats into
> drm_mode_mode_info (I'm thinking that it could go into flags), we'd be
> able to eliminate a bunch of combinations early on. Do you think that
> would make things more bearable?

That would eliminate some, but note that for example YCBCR444 needs a faster
pixel clock then YCBCR420, so it's interdependent with everything else that
changes the required pixel clock like bpc, resolution and refresh rate.

So the config space is n-dimensional with no "right angle box" clearly
separating working from non working combinations.

But I just had the idea:

Currently in KDE and Gnome UI you first select the resolution, to then wee what
refresh rates are available. So I guess this concept could be appended to color
properties -> Define a sequence for the different properties to be applied
across all drivers and as soon as you select one, the next property in the
sequence is TEST_ONLYed.

e.g.:

1. Select resolution -> Available refresh rates are updated

2. Select refresh rate -> Available color formats are updated

3. Select color format -> Available bpc are updated

etc.

So you can't select a bpc that doesn't fit your current color format. Changing
color format can change the available bpc. One maybe counter intuitive thing
here is that color format "auto" might not have all bpc settings available, as
auto will for example actually be RGB which has higher pixel clock requirements
then ycbcr420. And in this model, color format is always decided first. Or vice
versa if bpc is decided to be before color format in the sequence.

>
> I'm thinking, something like this:
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 128d09138ceb3..59980803cb89e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -124,6 +124,13 @@ extern "C" {
> #define DRM_MODE_FLAG_PIC_AR_256_135 \
> (DRM_MODE_PICTURE_ASPECT_256_135<<19)
>
> +/* Possible color formats (4 bits) */
> +#define DRM_MODE_FLAG_COLOR_FORMAT_MASK (0x0f << 22)
> +#define DRM_MODE_FLAG_COLOR_FORMAT_RGB (1 << 22)
> +#define DRM_MODE_FLAG_COLOR_FORMAT_YCBCR444 (1 << 23)
> +#define DRM_MODE_FLAG_COLOR_FORMAT_YCBCR422 (1 << 24)
> +#define DRM_MODE_FLAG_COLOR_FORMAT_YCBCR420 (1 << 25)
> +
> #define DRM_MODE_FLAG_ALL (DRM_MODE_FLAG_PHSYNC | \
> DRM_MODE_FLAG_NHSYNC | \
> DRM_MODE_FLAG_PVSYNC | \
> @@ -136,7 +143,8 @@ extern "C" {
> DRM_MODE_FLAG_HSKEW | \
> DRM_MODE_FLAG_DBLCLK | \
> DRM_MODE_FLAG_CLKDIV2 | \
> - DRM_MODE_FLAG_3D_MASK)
> + DRM_MODE_FLAG_3D_MASK | \
> + DRM_MODE_FLAG_COLOR_FORMAT_MASK)
>
> /* DPMS flags */
> /* bit compatible with the xorg definitions. */

2024-01-15 15:01:57

by Sebastian Wick

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

On Thu, Jan 11, 2024 at 05:17:46PM +0000, Andri Yngvason wrote:
> mi?., 10. jan. 2024 kl. 13:26 skrifa?i Daniel Stone <[email protected]>:
> > >
> > > This thing here works entirely differently, and I think we need somewhat
> > > new semantics for this:
> > >
> > > - I agree it should be read-only for userspace, so immutable sounds right.
> > >
> > > - But I also agree with Daniel Stone that this should be tied more
> > > directly to the modeset state.
> > >
> > > So I think the better approach would be to put the output type into
> > > drm_connector_state, require that drivers compute it in their
> > > ->atomic_check code (which in the future would allow us to report it out
> > > for TEST_ONLY commits too), and so guarantee that the value is updated
> > > right after the kms ioctl returns (and not somewhen later for non-blocking
> > > commits).
> >
> > That's exactly the point. Whether userspace gets an explicit
> > notification or it has to 'know' when to read isn't much of an issue -
> > just as long as it's well defined. I think the suggestion of 'do it in
> > atomic_check and then it's guaranteed to be readable when the commit
> > completes' is a good one.
> >
> > I do still have some reservations - for instance, why do we have the
> > fallback to auto when userspace has explicitly requested a certain
> > type? - but they may have been covered previously.
> >
>
> We discussed this further on IRC and this is summary of that discussion:
>
> Sima proposed a new type of property that can be used to git feedback to
> userspace after atomic ioctl. The user supplies a list of output properties
> that they want to query and the kernel fills it in before returning from the
> ioctl. This would help to get some information about why things failed
> during TEST_ONLY.
>
> Emersion raised the point that you might not know how much memory is needed
> beforehand for the returned properties, to which sima replied: blob
> property. There was some discussion about how that makes it possible to leak
> kernel memory, especially if userspace does not know about a new property
> blob. Emersion pointed out that userspace should only request properties
> that it understands and pq agreed.
>
> Emersion asked how the user should inform the kernel that it's done with the
> blob, to which sima replied: DRM_IOCTL_MODE_DESTROYPROPBLOB. Sima also
> mentioned using some sort of weak reference garbage collection scheme for
> properties and there was some further discussion, but I'm not sure there was
> any conclusion.
>
> 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 asked again if we should drop the "active color format" property as it
> seems to be more trouble than it's worth for now. pq mentioned that there
> are 2 separate use cases (in his words):
> - People playing with setting UI would like to know what "auto" would result
> in, but that's just nice to have
> - The other use case is the flicker-free boot into known configuration He
> went on to point out that the main problem with "auto" is that any modeset
> could make the driver decide differently. This means that we cannot fully
> rely on the previously set property.
>
> However, leaving out "active color property" did not put us in a worse
> situation than before, so the conclusion was that we should leave it out for
> now. For GUI selectors, the current TEST_ONLY should be good enough, and all
> the fancy stuff discussed previously isn't needed for now.
>
> To summarise the summary: this means that we will drop the "active
> color format" property and rename the "preferred color format"
> property to "force color format" or just "color format" and failing to
> satisfy that constraint will fail the modeset rather than falling back
> to the "auto" behaviour.

That's a good idea.

Anything else won't work with the new color pipeline API anyways because
user space will be responsible for setting up the color pipeline API in
a way so that the monitor will receive the correctly encoded data.

> Cheers,
> Andri
>