From: Sebastian Wick <sebastian.wick@redhat.com>
To: Andri Yngvason <andri@yngvason.is>
Cc: dri-devel@lists.freedesktop.org,
"Thomas Zimmermann" <tzimmermann@suse.de>,
"Leo Li" <sunpeng.li@amd.com>, "David Airlie" <airlied@gmail.com>,
intel-gfx@lists.freedesktop.org, "Pan,
Xinhui" <Xinhui.Pan@amd.com>,
"Rodrigo Siqueira" <Rodrigo.Siqueira@amd.com>,
linux-kernel@vger.kernel.org,
"Maxime Ripard" <mripard@kernel.org>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
"Alex Deucher" <alexander.deucher@amd.com>,
amd-gfx@lists.freedesktop.org,
"Harry Wentland" <harry.wentland@amd.com>,
"Christian König" <christian.koenig@amd.com>
Subject: Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace
Date: Tue, 16 Jan 2024 12:42:35 +0100 [thread overview]
Message-ID: <20240116114235.GA311990@toolbox> (raw)
In-Reply-To: <20240115160554.720247-3-andri@yngvason.is>
On Mon, Jan 15, 2024 at 04:05:52PM +0000, Andri Yngvason wrote:
> From: Werner Sembach <wse@tuxedocomputers.com>
>
> Add a new general drm property "force color format" which can be used
> by userspace to tell the graphics driver which color format to use.
I don't like the "force" in the name. This just selects the color
format, let's just call it "color format" then.
> Possible options are:
> - auto (default/current behaviour)
> - rgb
> - ycbcr444
> - ycbcr422 (supported by neither amdgpu or i915)
> - ycbcr420
>
> In theory the auto option should choose the best available option for the
> current setup, but because of bad internal conversion some monitors look
> better with rgb and some with ycbcr444.
>
> Also, because of bad shielded connectors and/or cables, it might be
> preferable to use the less bandwidth heavy ycbcr422 and ycbcr420 formats
> for a signal that is less susceptible to interference.
>
> In the future, automatic color calibration for screens might also depend on
> this option being available.
>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Signed-off-by: Andri Yngvason <andri@yngvason.is>
> Tested-by: Andri Yngvason <andri@yngvason.is>
> ---
>
> Changes in v2:
> - Renamed to "force color format" from "preferred color format"
> - Removed Reported-by pointing to invalid email address
>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 4 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 4 +++
> drivers/gpu/drm/drm_connector.c | 48 +++++++++++++++++++++++++++++
> include/drm/drm_connector.h | 16 ++++++++++
> 4 files changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 39ef0a6addeba..1dabd164c4f09 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -707,6 +707,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> if (old_connector_state->max_requested_bpc !=
> new_connector_state->max_requested_bpc)
> new_crtc_state->connectors_changed = true;
> +
> + if (old_connector_state->force_color_format !=
> + new_connector_state->force_color_format)
> + new_crtc_state->connectors_changed = true;
> }
>
> if (funcs->atomic_check)
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 29d4940188d49..e45949bf4615f 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -776,6 +776,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> state->max_requested_bpc = val;
> } else if (property == connector->privacy_screen_sw_state_property) {
> state->privacy_screen_sw_state = val;
> + } else if (property == connector->force_color_format_property) {
> + state->force_color_format = val;
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> @@ -859,6 +861,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> *val = state->max_requested_bpc;
> } else if (property == connector->privacy_screen_sw_state_property) {
> *val = state->privacy_screen_sw_state;
> + } else if (property == connector->force_color_format_property) {
> + *val = state->force_color_format;
> } else if (connector->funcs->atomic_get_property) {
> return connector->funcs->atomic_get_property(connector,
> state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b0516505f7ae9..e0535e58b4535 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1061,6 +1061,14 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
> { DRM_MODE_SUBCONNECTOR_Native, "Native" }, /* DP */
> };
>
> +static const struct drm_prop_enum_list drm_force_color_format_enum_list[] = {
> + { 0, "auto" },
> + { DRM_COLOR_FORMAT_RGB444, "rgb" },
> + { DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
> + { DRM_COLOR_FORMAT_YCBCR422, "ycbcr422" },
> + { DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
> +};
> +
> DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
> drm_dp_subconnector_enum_list)
>
> @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
> * drm_connector_attach_max_bpc_property() to create and attach the
> * property to the connector during initialization.
> *
> + * force color format:
> + * This property is used by userspace to change the used color format. When
> + * used the driver will use the selected format if valid for the hardware,
All properties are always "used", they just can have different values.
You probably want to talk about the auto mode here.
> + * sink, and current resolution and refresh rate combination. Drivers to
If valid? So when a value is not actually supported user space can still
set it? What happens then? How should user space figure out if the
driver and the sink support the format?
For the Colorspace prop, the kernel just exposes all formats it supports
(independent of the sink) and then makes it the job of user space to
figure out if the sink supports it.
The same could be done here. Property value is exposed if the driver
supports it in general, commits can fail if the driver can't support it
for a specific commit because e.g. the resolution or refresh rate. User
space must look at the EDID/DisplayID/mode to figure out the supported
format for the sink.
> + * use the function drm_connector_attach_force_color_format_property()
> + * to create and attach the property to the connector during
> + * initialization. Possible values are "auto", "rgb", "ycbcr444",
> + * "ycbcr422", and "ycbcr420".
> + *
> * Connectors also have one standardized atomic property:
> *
> * CRTC_ID:
> @@ -2457,6 +2474,37 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
>
> +/**
> + * drm_connector_attach_force_color_format_property - attach "force color format" property
> + * @connector: connector to attach force color format property on.
> + *
> + * This is used to add support for selecting a color format on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_force_color_format_property(struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_property *prop;
> +
> + if (!connector->force_color_format_property) {
> + prop = drm_property_create_enum(dev, 0, "force color format",
> + drm_force_color_format_enum_list,
> + ARRAY_SIZE(drm_force_color_format_enum_list));
> + if (!prop)
> + return -ENOMEM;
> +
> + connector->force_color_format_property = prop;
> + }
> +
> + drm_object_attach_property(&connector->base, prop, 0);
> + connector->state->force_color_format = 0;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_force_color_format_property);
> +
> /**
> * drm_connector_attach_hdr_output_metadata_property - attach "HDR_OUTPUT_METADA" property
> * @connector: connector to attach the property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f4..9830e7c09c0ba 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1026,6 +1026,14 @@ struct drm_connector_state {
> */
> enum drm_privacy_screen_status privacy_screen_sw_state;
>
> + /**
> + * @force_color_format: Property set by userspace to tell the GPU
> + * driver which color format to use. It only gets applied if hardware,
> + * meaning both the computer and the monitor, and the driver support the
> + * given format at the current resolution and refresh rate.
> + */
> + u32 force_color_format;
> +
> /**
> * @hdr_output_metadata:
> * DRM blob property for HDR output metadata
> @@ -1699,6 +1707,12 @@ struct drm_connector {
> */
> struct drm_property *privacy_screen_hw_state_property;
>
> + /**
> + * @force_color_format_property: Default connector property for the
> + * force color format to be driven out of the connector.
> + */
> + struct drm_property *force_color_format_property;
> +
> #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
> #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -2053,6 +2067,8 @@ void drm_connector_attach_privacy_screen_provider(
> struct drm_connector *connector, struct drm_privacy_screen *priv);
> void drm_connector_update_privacy_screen(const struct drm_connector_state *connector_state);
>
> +int drm_connector_attach_force_color_format_property(struct drm_connector *connector);
> +
> /**
> * struct drm_tile_group - Tile group metadata
> * @refcount: reference count
> --
> 2.43.0
>
next prev parent reply other threads:[~2024-01-16 11:42 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-15 16:05 [PATCH v2 0/4] New DRM properties for output color format Andri Yngvason
2024-01-15 16:05 ` [PATCH v2 1/4] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check Andri Yngvason
2024-04-17 19:57 ` Harry Wentland
2024-01-15 16:05 ` [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace Andri Yngvason
2024-01-16 11:42 ` Sebastian Wick [this message]
2024-01-16 13:13 ` Andri Yngvason
2024-01-16 13:29 ` Sebastian Wick
2024-01-16 14:11 ` Andri Yngvason
2024-01-17 9:21 ` Pekka Paalanen
2024-01-17 12:58 ` Andri Yngvason
2024-01-18 21:40 ` Sebastian Wick
2024-01-19 8:42 ` Pekka Paalanen
2024-04-17 19:57 ` Harry Wentland
2024-01-15 16:05 ` [PATCH v2 3/4] drm/amd/display: Add handling for new "force color format" property Andri Yngvason
2024-01-15 16:05 ` [PATCH v2 4/4] drm/i915/display: " Andri Yngvason
2024-01-16 13:08 ` ✗ Fi.CI.CHECKPATCH: warning for New DRM properties for output color format Patchwork
2024-01-16 13:08 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-01-16 13:25 ` ✗ Fi.CI.BAT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240116114235.GA311990@toolbox \
--to=sebastian.wick@redhat.com \
--cc=Rodrigo.Siqueira@amd.com \
--cc=Xinhui.Pan@amd.com \
--cc=airlied@gmail.com \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=andri@yngvason.is \
--cc=christian.koenig@amd.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=harry.wentland@amd.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mripard@kernel.org \
--cc=rodrigo.vivi@intel.com \
--cc=sunpeng.li@amd.com \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).