* [PATCH 1/7] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check
2024-01-09 18:10 [PATCH 0/7] New DRM properties for output color format Andri Yngvason
@ 2024-01-09 18:10 ` Andri Yngvason
2024-01-09 18:10 ` [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace Andri Yngvason
` (5 subsequent siblings)
6 siblings, 0 replies; 26+ messages in thread
From: Andri Yngvason @ 2024-01-09 18:10 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin
Cc: amd-gfx, dri-devel, linux-kernel, intel-gfx, Simon Ser,
Werner Sembach, Andri Yngvason
From: Werner Sembach <wse@tuxedocomputers.com>
Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check that was performed in the
drm_mode_is_420_only() case, but not in the drm_mode_is_420_also() &&
force_yuv420_output case.
Without further knowledge if YCbCr 4:2:0 is supported outside of HDMI,
there is no reason to use RGB when the display
reports drm_mode_is_420_only() even on a non HDMI connection.
This patch also moves both checks in the same if-case. This eliminates an
extra else-if-case.
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 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
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace
2024-01-09 18:10 [PATCH 0/7] New DRM properties for output color format Andri Yngvason
2024-01-09 18:10 ` [PATCH 1/7] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check Andri Yngvason
@ 2024-01-09 18:10 ` Andri Yngvason
2024-01-09 22:32 ` Daniel Stone
2024-01-10 13:39 ` Werner Sembach
2024-01-09 18:11 ` [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property Andri Yngvason
` (4 subsequent siblings)
6 siblings, 2 replies; 26+ messages in thread
From: Andri Yngvason @ 2024-01-09 18:10 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin
Cc: amd-gfx, dri-devel, linux-kernel, intel-gfx, Simon Ser,
Werner Sembach, Andri Yngvason
From: Werner Sembach <wse@tuxedocomputers.com>
Add a new general drm property "active color format" which can be used by
graphic drivers to report the used color format back to userspace.
There was no way to check which color format got actually used on a given
monitor. To surely predict this, one must know the exact capabilities of
the monitor, the GPU, and the connection used and what the default
behaviour of the used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915
prefers RGB). This property helps eliminating the guessing on this point.
In the future, automatic color calibration for screens might also depend on
this information being available.
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---
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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace
2024-01-09 18:10 ` [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace Andri Yngvason
@ 2024-01-09 22:32 ` Daniel Stone
[not found] ` <CAFNQBQwiqqSRqzXAnC035UWCGF3=GGFR5SpDd=biPTOEA+cWbQ@mail.gmail.com>
2024-01-10 13:39 ` Werner Sembach
1 sibling, 1 reply; 26+ messages in thread
From: Daniel Stone @ 2024-01-09 22:32 UTC (permalink / raw)
To: Andri Yngvason
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser, Werner Sembach
Hi,
On Tue, 9 Jan 2024 at 18:12, Andri Yngvason <andri@yngvason.is> 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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace
2024-01-09 18:10 ` [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace Andri Yngvason
2024-01-09 22:32 ` Daniel Stone
@ 2024-01-10 13:39 ` Werner Sembach
1 sibling, 0 replies; 26+ messages in thread
From: Werner Sembach @ 2024-01-10 13:39 UTC (permalink / raw)
To: Andri Yngvason, Harry Wentland, Leo Li, Rodrigo Siqueira,
Alex Deucher, Christian König, Pan, Xinhui, David Airlie,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin
Cc: amd-gfx, intel-gfx, linux-kernel, dri-devel
Hi,
Am 09.01.24 um 19:10 schrieb Andri Yngvason:
> From: Werner Sembach <wse@tuxedocomputers.com>
>
> Add a new general drm property "active color format" which can be used by
> graphic drivers to report the used color format back to userspace.
>
> There was no way to check which color format got actually used on a given
> monitor. To surely predict this, one must know the exact capabilities of
> the monitor, the GPU, and the connection used and what the default
> behaviour of the used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915
> prefers RGB). This property helps eliminating the guessing on this point.
>
> In the future, automatic color calibration for screens might also depend on
> this information being available.
>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Signed-off-by: Andri Yngvason <andri@yngvason.is>
> Tested-by: Andri Yngvason <andri@yngvason.is>
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property
2024-01-09 18:10 [PATCH 0/7] New DRM properties for output color format Andri Yngvason
2024-01-09 18:10 ` [PATCH 1/7] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check Andri Yngvason
2024-01-09 18:10 ` [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace Andri Yngvason
@ 2024-01-09 18:11 ` Andri Yngvason
2024-01-10 11:10 ` Daniel Vetter
2024-01-09 18:11 ` [PATCH 4/7] drm/i915/display: " Andri Yngvason
` (3 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Andri Yngvason @ 2024-01-09 18:11 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin
Cc: amd-gfx, dri-devel, linux-kernel, intel-gfx, Simon Ser,
Werner Sembach, Andri Yngvason
From: Werner Sembach <wse@tuxedocomputers.com>
This commit implements the "active color format" drm property for the AMD
GPU driver.
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---
.../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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property
2024-01-09 18:11 ` [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property Andri Yngvason
@ 2024-01-10 11:10 ` Daniel Vetter
2024-01-10 12:52 ` Andri Yngvason
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Vetter @ 2024-01-10 11:10 UTC (permalink / raw)
To: Andri Yngvason
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser, Werner Sembach
On Tue, Jan 09, 2024 at 06:11:00PM +0000, Andri Yngvason wrote:
> From: Werner Sembach <wse@tuxedocomputers.com>
>
> This commit implements the "active color format" drm property for the AMD
> GPU driver.
>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Signed-off-by: Andri Yngvason <andri@yngvason.is>
> Tested-by: Andri Yngvason <andri@yngvason.is>
> ---
> .../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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property
2024-01-10 11:10 ` Daniel Vetter
@ 2024-01-10 12:52 ` Andri Yngvason
2024-01-10 13:09 ` Daniel Vetter
0 siblings, 1 reply; 26+ messages in thread
From: Andri Yngvason @ 2024-01-10 12:52 UTC (permalink / raw)
To: Andri Yngvason, Harry Wentland, Leo Li, Rodrigo Siqueira,
Alex Deucher, Christian König, Pan, Xinhui, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser, Werner Sembach
Cc: Daniel Vetter
mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <daniel@ffwll.ch>:
>
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property
2024-01-10 12:52 ` Andri Yngvason
@ 2024-01-10 13:09 ` Daniel Vetter
2024-01-10 13:14 ` Werner Sembach
0 siblings, 1 reply; 26+ messages in thread
From: Daniel Vetter @ 2024-01-10 13:09 UTC (permalink / raw)
To: Andri Yngvason
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser, Werner Sembach
On Wed, 10 Jan 2024 at 13:53, Andri Yngvason <andri@yngvason.is> wrote:
>
> mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <daniel@ffwll.ch>:
> >
> > 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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property
2024-01-10 13:09 ` Daniel Vetter
@ 2024-01-10 13:14 ` Werner Sembach
2024-01-10 17:15 ` Andri Yngvason
0 siblings, 1 reply; 26+ messages in thread
From: Werner Sembach @ 2024-01-10 13:14 UTC (permalink / raw)
To: Daniel Vetter, Andri Yngvason
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser
Hi,
Am 10.01.24 um 14:09 schrieb Daniel Vetter:
> On Wed, 10 Jan 2024 at 13:53, Andri Yngvason <andri@yngvason.is> wrote:
>> mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <daniel@ffwll.ch>:
>>> 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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property
2024-01-10 13:14 ` Werner Sembach
@ 2024-01-10 17:15 ` Andri Yngvason
2024-01-11 23:54 ` Werner Sembach
0 siblings, 1 reply; 26+ messages in thread
From: Andri Yngvason @ 2024-01-10 17:15 UTC (permalink / raw)
To: Werner Sembach
Cc: Daniel Vetter, Harry Wentland, Leo Li, Rodrigo Siqueira,
Alex Deucher, Christian König, Pan, Xinhui, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser
Hi Werner,
mið., 10. jan. 2024 kl. 13:14 skrifaði Werner Sembach <wse@tuxedocomputers.com>:
>
> Hi,
>
> Am 10.01.24 um 14:09 schrieb Daniel Vetter:
> > On Wed, 10 Jan 2024 at 13:53, Andri Yngvason <andri@yngvason.is> wrote:
> >> mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <daniel@ffwll.ch>:
> >>> 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. */
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property
2024-01-10 17:15 ` Andri Yngvason
@ 2024-01-11 23:54 ` Werner Sembach
0 siblings, 0 replies; 26+ messages in thread
From: Werner Sembach @ 2024-01-11 23:54 UTC (permalink / raw)
To: Andri Yngvason
Cc: Tvrtko Ursulin, Joonas Lahtinen, Thomas Zimmermann, intel-gfx,
Leo Li, David Airlie, dri-devel, Pan, Xinhui, Rodrigo Siqueira,
linux-kernel, Maxime Ripard, Jani Nikula, Daniel Vetter,
Rodrigo Vivi, Alex Deucher, Maarten Lankhorst, Simon Ser, amd-gfx,
Harry Wentland, Christian König
Hi,
Am 10.01.24 um 18:15 schrieb Andri Yngvason:
> Hi Werner,
>
> mið., 10. jan. 2024 kl. 13:14 skrifaði Werner Sembach <wse@tuxedocomputers.com>:
>> Hi,
>>
>> Am 10.01.24 um 14:09 schrieb Daniel Vetter:
>>> On Wed, 10 Jan 2024 at 13:53, Andri Yngvason <andri@yngvason.is> wrote:
>>>> mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter <daniel@ffwll.ch>:
>>>>> 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. */
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/7] drm/i915/display: Add handling for new "active color format" property
2024-01-09 18:10 [PATCH 0/7] New DRM properties for output color format Andri Yngvason
` (2 preceding siblings ...)
2024-01-09 18:11 ` [PATCH 3/7] drm/amd/display: Add handling for new "active color format" property Andri Yngvason
@ 2024-01-09 18:11 ` Andri Yngvason
2024-01-09 18:11 ` [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace Andri Yngvason
` (2 subsequent siblings)
6 siblings, 0 replies; 26+ messages in thread
From: Andri Yngvason @ 2024-01-09 18:11 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin
Cc: amd-gfx, dri-devel, linux-kernel, intel-gfx, Simon Ser,
Werner Sembach, Andri Yngvason
From: Werner Sembach <wse@tuxedocomputers.com>
This commit implements the "active color format" drm property for the Intel
GPU driver.
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---
drivers/gpu/drm/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
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace
2024-01-09 18:10 [PATCH 0/7] New DRM properties for output color format Andri Yngvason
` (3 preceding siblings ...)
2024-01-09 18:11 ` [PATCH 4/7] drm/i915/display: " Andri Yngvason
@ 2024-01-09 18:11 ` Andri Yngvason
2024-01-10 9:27 ` Maxime Ripard
2024-01-09 18:11 ` [PATCH 6/7] drm/amd/display: Add handling for new "preferred color format" property Andri Yngvason
2024-01-09 18:11 ` [PATCH 7/7] drm/i915/display: " Andri Yngvason
6 siblings, 1 reply; 26+ messages in thread
From: Andri Yngvason @ 2024-01-09 18:11 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin
Cc: amd-gfx, dri-devel, linux-kernel, intel-gfx, Simon Ser,
Werner Sembach, Dan Carpenter, Andri Yngvason
From: Werner Sembach <wse@tuxedocomputers.com>
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 <wse@tuxedocomputers.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---
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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace
2024-01-09 18:11 ` [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace Andri Yngvason
@ 2024-01-10 9:27 ` Maxime Ripard
2024-01-10 10:11 ` Andri Yngvason
0 siblings, 1 reply; 26+ messages in thread
From: Maxime Ripard @ 2024-01-10 9:27 UTC (permalink / raw)
To: Andri Yngvason
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser, Werner Sembach, Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 7843 bytes --]
Hi,
On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
> From: Werner Sembach <wse@tuxedocomputers.com>
>
> 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 <wse@tuxedocomputers.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Andri Yngvason <andri@yngvason.is>
> Tested-by: Andri Yngvason <andri@yngvason.is>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace
2024-01-10 9:27 ` Maxime Ripard
@ 2024-01-10 10:11 ` Andri Yngvason
2024-01-10 13:09 ` Werner Sembach
0 siblings, 1 reply; 26+ messages in thread
From: Andri Yngvason @ 2024-01-10 10:11 UTC (permalink / raw)
To: Maxime Ripard
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser, Werner Sembach, Dan Carpenter
Hi,
mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard <mripard@kernel.org>:
> On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
> > From: Werner Sembach <wse@tuxedocomputers.com>
> >
> > 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace
2024-01-10 10:11 ` Andri Yngvason
@ 2024-01-10 13:09 ` Werner Sembach
2024-01-10 13:42 ` Andri Yngvason
0 siblings, 1 reply; 26+ messages in thread
From: Werner Sembach @ 2024-01-10 13:09 UTC (permalink / raw)
To: Andri Yngvason, Maxime Ripard
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser, Dan Carpenter
Hi,
Am 10.01.24 um 11:11 schrieb Andri Yngvason:
> Hi,
>
> mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard <mripard@kernel.org>:
>> On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
>>> From: Werner Sembach <wse@tuxedocomputers.com>
>>>
>>> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace
2024-01-10 13:09 ` Werner Sembach
@ 2024-01-10 13:42 ` Andri Yngvason
2024-01-10 14:54 ` Werner Sembach
0 siblings, 1 reply; 26+ messages in thread
From: Andri Yngvason @ 2024-01-10 13:42 UTC (permalink / raw)
To: Werner Sembach
Cc: Maxime Ripard, Harry Wentland, Leo Li, Rodrigo Siqueira,
Alex Deucher, Christian König, Pan, Xinhui, David Airlie,
Daniel Vetter, Maarten Lankhorst, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser, Dan Carpenter
mið., 10. jan. 2024 kl. 13:09 skrifaði Werner Sembach <wse@tuxedocomputers.com>:
>
> Hi,
>
> Am 10.01.24 um 11:11 schrieb Andri Yngvason:
> > Hi,
> >
> > mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard <mripard@kernel.org>:
> >> On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
> >>> From: Werner Sembach <wse@tuxedocomputers.com>
> >>>
> >>> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace
2024-01-10 13:42 ` Andri Yngvason
@ 2024-01-10 14:54 ` Werner Sembach
0 siblings, 0 replies; 26+ messages in thread
From: Werner Sembach @ 2024-01-10 14:54 UTC (permalink / raw)
To: Andri Yngvason
Cc: Maxime Ripard, Harry Wentland, Leo Li, Rodrigo Siqueira,
Alex Deucher, Christian König, Pan, Xinhui, David Airlie,
Daniel Vetter, Maarten Lankhorst, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, amd-gfx, dri-devel,
linux-kernel, intel-gfx, Simon Ser, Dan Carpenter
[-- Attachment #1.1: Type: text/plain, Size: 3516 bytes --]
Hi,
Am 10.01.24 um 14:42 schrieb Andri Yngvason:
> mið., 10. jan. 2024 kl. 13:09 skrifaði Werner Sembach<wse@tuxedocomputers.com>:
>> Hi,
>>
>> Am 10.01.24 um 11:11 schrieb Andri Yngvason:
>>> Hi,
>>>
>>> mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard<mripard@kernel.org>:
>>>> On Tue, Jan 09, 2024 at 06:11:02PM +0000, Andri Yngvason wrote:
>>>>> From: Werner Sembach<wse@tuxedocomputers.com>
>>>>>
>>>>> 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
[-- Attachment #1.2: Type: text/html, Size: 5492 bytes --]
[-- Attachment #2: hacking_drm_color_management_no_immutable_flag.tar.gz --]
[-- Type: application/gzip, Size: 10913 bytes --]
[-- Attachment #3: rejected_drm_color_management.tar.gz --]
[-- Type: application/gzip, Size: 19541 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 6/7] drm/amd/display: Add handling for new "preferred color format" property
2024-01-09 18:10 [PATCH 0/7] New DRM properties for output color format Andri Yngvason
` (4 preceding siblings ...)
2024-01-09 18:11 ` [PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace Andri Yngvason
@ 2024-01-09 18:11 ` Andri Yngvason
2024-01-09 18:11 ` [PATCH 7/7] drm/i915/display: " Andri Yngvason
6 siblings, 0 replies; 26+ messages in thread
From: Andri Yngvason @ 2024-01-09 18:11 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin
Cc: amd-gfx, dri-devel, linux-kernel, intel-gfx, Simon Ser,
Werner Sembach, Andri Yngvason
From: Werner Sembach <wse@tuxedocomputers.com>
This commit implements the "preferred color format" drm property for the
AMD GPU driver.
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---
.../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
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 7/7] drm/i915/display: Add handling for new "preferred color format" property
2024-01-09 18:10 [PATCH 0/7] New DRM properties for output color format Andri Yngvason
` (5 preceding siblings ...)
2024-01-09 18:11 ` [PATCH 6/7] drm/amd/display: Add handling for new "preferred color format" property Andri Yngvason
@ 2024-01-09 18:11 ` Andri Yngvason
6 siblings, 0 replies; 26+ messages in thread
From: Andri Yngvason @ 2024-01-09 18:11 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Pan, Xinhui, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin
Cc: amd-gfx, dri-devel, linux-kernel, intel-gfx, Simon Ser,
Werner Sembach, Andri Yngvason
From: Werner Sembach <wse@tuxedocomputers.com>
This commit implements the "preferred color format" drm property for the
Intel GPU driver.
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Co-developed-by: Andri Yngvason <andri@yngvason.is>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
