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 : > > Hi, > > Am 10.01.24 um 11:11 schrieb Andri Yngvason: > > Hi, > > > > mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard : > >> On Tue, Jan 09, 2024 at 06:11:02PM +, Andri Yngvason wrote: > >>> From: Werner Sembach > >>> > >>> 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
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 : > On Tue, Jan 09, 2024 at 06:11:02PM +, Andri Yngvason wrote: > > From: Werner Sembach > > > > 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
[PATCH 5/7] drm/uAPI: Add "preferred color format" drm property as setting for userspace
From: Werner Sembach
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
Reported-by: Dan Carpenter
Signed-off-by: Andri Yngvason
Tested-by: Andri Yngvason
---
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 tel
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: Hi, Am 10.01.24 um 11:11 schrieb Andri Yngvason: Hi, mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard: On Tue, Jan 09, 2024 at 06:11:02PM +, Andri Yngvason wrote: From: Werner Sembach 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 hacking_drm_color_management_no_immutable_flag.tar.gz Description: application/gzip rejected_drm_color_management.tar.gz Description: application/gzip
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 : On Tue, Jan 09, 2024 at 06:11:02PM +, Andri Yngvason wrote: From: Werner Sembach 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
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 +, Andri Yngvason wrote:
> From: Werner Sembach
>
> 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
> Reported-by: Dan Carpenter
> Signed-off-by: Andri Yngvason
> Tested-by: Andri Yngvason
> ---
> 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
