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

[Andri Yngvason]

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

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

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

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

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

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

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

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

[Andri Yngvason]

Hi Sebastian,

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

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

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

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

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

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

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

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

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

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

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

[...]

Regards,
Andri

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

[Andri Yngvason]

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

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

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

Ok.

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

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

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

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

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

Thanks,
Andri

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

[Andri Yngvason]

From: Werner Sembach 

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

Signed-off-by: Werner Sembach 
Co-Developed-by: Andri Yngvason 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---

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

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

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

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

[Andri Yngvason]

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

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

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

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


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

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


base-commit: 052d534373b7ed33712a63d5e17b2b6cdbce84fd
-- 
2.43.0

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

[Andri Yngvason]

From: Werner Sembach 

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

Signed-off-by: Werner Sembach 
Co-Developed-by: Andri Yngvason 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---

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

---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ---
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  4 ++
 2 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index cc4d1f7f97b98..26c4260c78d7b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5573,15 +5573,32 @@ static void 
fill_stream_properties_from_drm_display_mode(
timing_out->h_border_right = 0;
timing_out->v_border_top = 0;
timing_out->v_border_bottom = 0;
-   /* TODO: un-hardcode */
-   if (drm_mode_is_420_only(info, mode_in)
-   || (drm_mode_is_420_also(info, mode_in) && 
aconnector->force_yuv420_output))
+
+   if (connector_state
+   && (connector_state->force_color_format == 
DRM_COLOR_FORMAT_YCBCR420
+   || aconnector->force_yuv420_output) && 
drm_mode_is_420(info, mode_in))
timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
-   else if ((connector->display_info.color_formats & 
DRM_COLOR_FORMAT_YCBCR444)
-   && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
+   else if (connector_state
+   && connector_state->force_color_format == 
DRM_COLOR_FORMAT_YCBCR444
+   && connector->display_info.color_formats & 
DRM_COLOR_FORMAT_YCBCR444)
timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
-   else
+   else if (connector_state
+   && connector_state->force_color_format == 
DRM_COLOR_FORMAT_RGB444
+   && !drm_mode_is_420_only(info, mode_in))
timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+   else
+   /*
+* connector_state->force_color_format not possible
+* || connector_state->force_color_format == 0 (auto)
+* || connector_state->force_color_format == 
DRM_COLOR_FORMAT_YCBCR422
+*/
+   if (drm_mode_is_420_only(info, mode_in))
+   timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+   else if ((connector->display_info.color_formats & 
DRM_COLOR_FORMAT_YCBCR444)
+   && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
+   timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
+   else
+   timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
 
timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
timing_out->display_color_depth = convert_color_depth_from_display_info(
@@ -6685,6 +6702,33 @@ static enum dc_status 
dm_validate_stream_and_context(struct dc *dc,
return dc_result;
 }
 
+static enum dc_status
+dm_validate_stream_color_format(const struct drm_connector_state *drm_state,
+   const struct dc_stream_state *stream)
+{
+   if (!drm_state->force_color_format)
+   return DC_OK;
+
+   enum dc_pixel_encoding encoding = PIXEL_ENCODING_UNDEFINED;
+   switch (drm_state->force_color_format) {
+   case DRM_COLOR_FORMAT_RGB444:
+   encoding = PIXEL_ENCODING_RGB;
+   break;
+   case DRM_COLOR_FORMAT_YCBCR444:
+   encoding = PIXEL_ENCODING_YCBCR444;
+   break;
+   case DRM_COLOR_FORMAT_YCBCR422:
+   encoding = PIXEL_ENCODING_YCBCR422;
+   break;
+   case DRM_COLOR_FORMAT_YCBCR420:
+   encoding = PIXEL_ENCODING_YCBCR420;
+   break;
+   }
+
+   return encoding == stream->timing.pixel_encoding ?
+  DC_OK : DC_UNSUPPORTED_VALUE;
+}
+
 struct dc_stream_state *
 create_validate_stream_for_sink(struct amdgpu_dm_connector *aconnector,
const struct drm_display_mode *drm_mode,
@@ -6717,6 +6761,9 @@ create_validate_stream_for_sink(struct 
amdgpu_dm_connector *aconnector,
if (dc_result == DC_OK)
dc_result = dm_validate_stream_and_context(adev->dm.dc, 
stream);
 
+   if (dc_result == DC_OK)
+   dc_result = dm_validate_stream_color_format(drm_state, 
stream);
+
if (dc_result != DC_OK) {
DRM_DE

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

[Andri Yngvason]

From: Werner Sembach 

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

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

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

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

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

Signed-off-by: Werner Sembach 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---

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

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 39ef0a6addeba..1dabd164c4f09 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -707,6 +707,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
if (old_connector_state->max_requested_bpc !=
new_connector_state->max_requested_bpc)
new_crtc_state->connectors_changed = true;
+
+   if (old_connector_state->force_color_format !=
+   new_connector_state->force_color_format)
+   new_crtc_state->connectors_changed = true;
}
 
if (funcs->atomic_check)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index 29d4940188d49..e45949bf4615f 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -776,6 +776,8 @@ static int drm_atomic_connector_set_property(struct 
drm_connector *connector,
state->max_requested_bpc = val;
} else if (property == connector->privacy_screen_sw_state_property) {
state->privacy_screen_sw_state = val;
+   } else if (property == connector->force_color_format_property) {
+   state->force_color_format = val;
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
@@ -859,6 +861,8 @@ drm_atomic_connector_get_property(struct drm_connector 
*connector,
*val = state->max_requested_bpc;
} else if (property == connector->privacy_screen_sw_state_property) {
*val = state->privacy_screen_sw_state;
+   } else if (property == connector->force_color_format_property) {
+   *val = state->force_color_format;
} else if (connector->funcs->atomic_get_property) {
return connector->funcs->atomic_get_property(connector,
state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b0516505f7ae9..e0535e58b4535 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1061,6 +1061,14 @@ static const struct drm_prop_enum_list 
drm_dp_subconnector_enum_list[] = {
{ DRM_MODE_SUBCONNECTOR_Native,  "Native"}, /* DP */
 };
 
+static const struct drm_prop_enum_list drm_force_color_format_enum_list[] = {
+   { 0, "auto" },
+   { DRM_COLOR_FORMAT_RGB444, "rgb" },
+   { DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
+   { DRM_COLOR_FORMAT_YCBCR422, "ycbcr422" },
+   { DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
+};
+
 DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
 drm_dp_subconnector_enum_list)
 
@@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
  * drm_connector_attach_max_bpc_property() to create and attach the
  * property to the connector during initialization.
  *
+ * force color format:
+ * This property is used by userspace to change the used color format. When
+ * used the driver will use the selected format if valid for the hardware,
+ * sink, and current resolution and refresh rate combination. Drivers to
+ * use the function drm_connector_attach_force_color_format_property()
+ * to create and attach the property to the connector during
+ *

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

[Andri Yngvason]

From: Werner Sembach 

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 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index f6575d7dee971..cc4d1f7f97b98 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5575,11 +5575,7 @@ static void fill_stream_properties_from_drm_display_mode(
timing_out->v_border_bottom = 0;
/* TODO: un-hardcode */
if (drm_mode_is_420_only(info, mode_in)
-   && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
-   timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
-   else if (drm_mode_is_420_also(info, mode_in)
-   && aconnector
-   && aconnector->force_yuv420_output)
+   || (drm_mode_is_420_also(info, mode_in) && 
aconnector->force_yuv420_output))
timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
else if ((connector->display_info.color_formats & 
DRM_COLOR_FORMAT_YCBCR444)
&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
-- 
2.43.0

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

[Andri Yngvason]

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

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

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

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

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

I asked if it made sense to add color format capabilities to the mode info
struct, but the conclusion was that it wouldn't really be useful because we
need TEST_ONLY anyway to see if the color format setting is compatible with
other settings.

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

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

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

Cheers,
Andri

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

[Andri Yngvason]

Hi Werner,

mið., 10. jan. 2024 kl. 13:14 skrifaði Werner Sembach 
:
>
> Hi,
>
> Am 10.01.24 um 14:09 schrieb Daniel Vetter:
> > On Wed, 10 Jan 2024 at 13:53, Andri Yngvason  wrote:
> >> mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter :
> >>> On Tue, Jan 09, 2024 at 06:11:00PM +, 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. */

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

[Andri Yngvason]

Hi,

mið., 10. jan. 2024 kl. 09:27 skrifaði Maxime Ripard :
> On Tue, Jan 09, 2024 at 06:11:02PM +0000, 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

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

[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.

Thanks,
Andri

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

[Andri Yngvason]

mið., 10. jan. 2024 kl. 11:10 skrifaði Daniel Vetter :
>
> On Tue, Jan 09, 2024 at 06:11:00PM +, 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

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

[Andri Yngvason]

Hi Daniel,

þri., 9. jan. 2024 kl. 22:32 skrifaði Daniel Stone :

> On Tue, 9 Jan 2024 at 18:12, Andri Yngvason  wrote:
> > + * active color format:
> > + * This read-only property tells userspace the color format
> actually used
> > + * by the hardware display engine "on the cable" on a connector.
> The chosen
> > + * value depends on hardware capabilities, both display engine and
> > + * connected monitor. Drivers shall use
> > + * drm_connector_attach_active_color_format_property() to install
> this
> > + * property. Possible values are "not applicable", "rgb",
> "ycbcr444",
> > + * "ycbcr422", and "ycbcr420".
>
> How does userspace determine what's happened without polling? Will it
> only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> updated after the commit has completed and the event being sent?
> Should it send a HOTPLUG event? Other?
>

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

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

Regards,
Andri

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

[Andri Yngvason]

Hi Daniel,

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

þri., 9. jan. 2024 kl. 22:32 skrifaði Daniel Stone :
>
> On Tue, 9 Jan 2024 at 18:12, Andri Yngvason  wrote:
> > + * active color format:
> > + * This read-only property tells userspace the color format actually 
> > used
> > + * by the hardware display engine "on the cable" on a connector. The 
> > chosen
> > + * value depends on hardware capabilities, both display engine and
> > + * connected monitor. Drivers shall use
> > + * drm_connector_attach_active_color_format_property() to install this
> > + * property. Possible values are "not applicable", "rgb", "ycbcr444",
> > + * "ycbcr422", and "ycbcr420".
>
> How does userspace determine what's happened without polling? Will it
> only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> updated after the commit has completed and the event being sent?
> Should it send a HOTPLUG event? Other?

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

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

Regards,
Andri

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

[Andri Yngvason]

From: Werner Sembach 

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

Signed-off-by: Werner Sembach 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---
 .../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(>base, 8, 16);
+   
drm_connector_attach_preferred_color_format_property(>base);

drm_connector_attach_active_color_format_property(>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(>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(>base);
-- 
2.43.0

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

[Andri Yngvason]

From: Werner Sembach 

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

Signed-off-by: Werner Sembach 
Co-developed-by: Andri Yngvason 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---
 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 = >base.display_info;
const struct drm_display_mode *adjusted_mode = 
_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(>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(>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(_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 = 
_state->hw.adjusted_mode;
const struct drm_display_info *info = >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_COLO

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

[Andri Yngvason]

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

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

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

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

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

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

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


base-commit: 1f874787ed9a2d78ed59cb21d0d90ac0178eceb0
-- 
2.43.0

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

[Andri Yngvason]

From: Werner Sembach 

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

Signed-off-by: Werner Sembach 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---
 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(_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(>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(>base);
INIT_WORK(>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(_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/int

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

[Andri Yngvason]

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
+ 

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

[Andri Yngvason]

From: Werner Sembach 

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 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---
 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(>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(>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 {
 

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

[Andri Yngvason]

From: Werner Sembach 

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 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---
 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

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

[Andri Yngvason]

From: Werner Sembach 

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

Signed-off-by: Werner Sembach 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 
---
 .../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(>base, 8, 16);
+   
drm_connector_attach_active_color_format_property(>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(>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