On Fri, 18 Jun 2021 11:11:02 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

Add a new general drm property "active bpc" which can be used by graphic drivers to report the applied bit depth per pixel back to userspace.

While "max bpc" can be used to change the color depth, there was no way to check which one actually got used. While in theory the driver chooses the best/highest color depth within the max bpc setting a user might not be fully aware what his hardware is or isn't capable off. This is meant as a quick way to double check the setup.

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

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

drivers/gpu/drm/drm_connector.c | 51 +++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 8 ++++++ 2 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index da39e7ff6965..943f6b61053b 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1197,6 +1197,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• drm_connector_attach_max_bpc_property() to create and attach the
• property to the connector during initialization.

• • active bpc:
• • This read-only range property tells userspace the pixel color bit depth
• • 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, and the "max bpc" property.
• • Drivers shall use drm_connector_attach_active_bpc_property() to install
• • this property.
• • Connectors also have one standardized atomic property:
  • CRTC_ID:

@@ -2152,6 +2160,49 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);

+/**

• • drm_connector_attach_active_bpc_property - attach "active bpc" property
• • @connector: connector to attach active bpc property on.
• • @min: The minimum bit depth supported by the connector.
• • @max: The maximum bit depth supported by the connector.
• • This is used to check the applied bit depth on a connector.
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max) +{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop;
• if (!connector->active_bpc_property) {

• prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "active bpc",
  

• 				 min, max);
  

• if (!prop)
  

• 	return -ENOMEM;
  

• connector->active_bpc_property = prop;
  

• drm_object_attach_property(&connector->base, prop, 0);
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);

+/**

• • drm_connector_set_active_bpc_property - sets the active bits per color property for a connector
• • @connector: drm connector
• • @active_bpc: bits per color for the connector currently active on "the cable"
• • Should be used by atomic drivers to update the active bits per color over a connector.
• */

+void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc) +{

• drm_object_property_set_value(&connector->base, connector->active_bpc_property, active_bpc);

+} +EXPORT_SYMBOL(drm_connector_set_active_bpc_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 714d1a01c065..eee86de62a5f 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1380,6 +1380,12 @@ struct drm_connector { */ struct drm_property *max_bpc_property;

• /**

• * @active_bpc_property: Default connector property for the active bpc
  

• * to be driven out of the connector.
  

• */
  

• struct drm_property *active_bpc_property;

#define DRM_CONNECTOR_POLL_HPD (1 << 0) #define DRM_CONNECTOR_POLL_CONNECT (1 << 1) #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2) @@ -1702,6 +1708,8 @@ int drm_connector_set_panel_orientation_with_quirk( int width, int height); int drm_connector_attach_max_bpc_property(struct drm_connector *connector, int min, int max); +int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max); +void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc);

/**

• struct drm_tile_group - Tile group metadata

Acked-by: Pekka Paalanen pekka.paalanen@collabora.com

Thanks, pq

Am 28.06.21 um 19:03 schrieb Werner Sembach:

Am 18.06.21 um 11:11 schrieb Werner Sembach:

...

Add a new general drm property "active bpc" which can be used by graphic drivers to report the applied bit depth per pixel back to userspace.

While "max bpc" can be used to change the color depth, there was no way to check which one actually got used. While in theory the driver chooses the best/highest color depth within the max bpc setting a user might not be fully aware what his hardware is or isn't capable off. This is meant as a quick way to double check the setup.

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

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

drivers/gpu/drm/drm_connector.c | 51 +++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 8 ++++++ 2 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index da39e7ff6965..943f6b61053b 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1197,6 +1197,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• drm_connector_attach_max_bpc_property() to create and attach the
• property to the connector during initialization.

• • active bpc:
• • This read-only range property tells userspace the pixel color bit depth
• • 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, and the "max bpc" property.
• • Drivers shall use drm_connector_attach_active_bpc_property() to install
• • this property.

Regarding "on the cable" and dithering: As far as I can tell, what the dithering option does, is setting a hardware register here:

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/i...

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/i...

So dithering seems to be calculated by fixed purpose hardware/firmware outside of the driver?

The Intel driver does not seem to set a target bpc/bpp for this hardware so I guess it defaults to 6 or 8 bpc?

Never mind it does. This switch-case does affect the dithering output: https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/i...

As found in this documentation p.548: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-lkf-vol0...

So max bpc and active bpc are affecting/affected by the bpc after dithering.

Similar things happen on amd. Here the output dither depth seems to be written to a fixed value however:

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc...

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc...

Does anyone know about a resource where I can read up on the used registers and what this hardware actually does?

Searching now for a similar register reference for AMD GPUs.

My proposal for now: "max bpc" affects what happens before dither, so I would keep "active bpc" the same and add another drm property "dither active: true/false". No additional property to control dither, as amdgpu does have one already (which isn't always active?) and Intel driver does only seem prepared for dithering at 6bpc (albeit I don't know why to dither at 6bpc and what depth to dither to?).

...

• Connectors also have one standardized atomic property:
• CRTC_ID:

@@ -2152,6 +2160,49 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);

+/**

• • drm_connector_attach_active_bpc_property - attach "active bpc" property
• • @connector: connector to attach active bpc property on.
• • @min: The minimum bit depth supported by the connector.
• • @max: The maximum bit depth supported by the connector.
• • This is used to check the applied bit depth on a connector.
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max) +{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop;
• if (!connector->active_bpc_property) {

• prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "active bpc",
  

• 				 min, max);
  

• if (!prop)
  

• 	return -ENOMEM;
  

• connector->active_bpc_property = prop;
  

• drm_object_attach_property(&connector->base, prop, 0);
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);

+/**

• • drm_connector_set_active_bpc_property - sets the active bits per color property for a connector
• • @connector: drm connector
• • @active_bpc: bits per color for the connector currently active on "the cable"
• • Should be used by atomic drivers to update the active bits per color over a connector.
• */

+void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc) +{

• drm_object_property_set_value(&connector->base, connector->active_bpc_property, active_bpc);

+} +EXPORT_SYMBOL(drm_connector_set_active_bpc_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 714d1a01c065..eee86de62a5f 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1380,6 +1380,12 @@ struct drm_connector { */ struct drm_property *max_bpc_property;

• /**

• * @active_bpc_property: Default connector property for the active bpc
  

• * to be driven out of the connector.
  

• */
  

• struct drm_property *active_bpc_property;

#define DRM_CONNECTOR_POLL_HPD (1 << 0) #define DRM_CONNECTOR_POLL_CONNECT (1 << 1) #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2) @@ -1702,6 +1708,8 @@ int drm_connector_set_panel_orientation_with_quirk( int width, int height); int drm_connector_attach_max_bpc_property(struct drm_connector *connector, int min, int max); +int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max); +void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc);

/**

• struct drm_tile_group - Tile group metadata

---

amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Am 30.06.21 um 10:21 schrieb Pekka Paalanen:

On Tue, 29 Jun 2021 13:02:05 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

...

Am 28.06.21 um 19:03 schrieb Werner Sembach:

...

Am 18.06.21 um 11:11 schrieb Werner Sembach:

...

Add a new general drm property "active bpc" which can be used by graphic drivers to report the applied bit depth per pixel back to userspace.

While "max bpc" can be used to change the color depth, there was no way to check which one actually got used. While in theory the driver chooses the best/highest color depth within the max bpc setting a user might not be fully aware what his hardware is or isn't capable off. This is meant as a quick way to double check the setup.

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

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

drivers/gpu/drm/drm_connector.c | 51 +++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 8 ++++++ 2 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index da39e7ff6965..943f6b61053b 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1197,6 +1197,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• drm_connector_attach_max_bpc_property() to create and attach the
• property to the connector during initialization.

• • active bpc:
• • This read-only range property tells userspace the pixel color bit depth
• • 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, and the "max bpc" property.
• • Drivers shall use drm_connector_attach_active_bpc_property() to install
• • this property.

Regarding "on the cable" and dithering: As far as I can tell, what the dithering option does, is setting a hardware register here:

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/i...

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/i...

So dithering seems to be calculated by fixed purpose hardware/firmware outside of the driver?

The Intel driver does not seem to set a target bpc/bpp for this hardware so I guess it defaults to 6 or 8 bpc?

Never mind it does. This switch-case does affect the dithering output: https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/i...

Hi,

I obviously do not know the intel driver or hardware at all, but to me that just looks like translating from bits per pixel to bits per channel in RGB mapping?

No, if i understand the documentation correctly: Writing bit depth here with dithering enabled sets the dithering target bpc.

...

As found in this documentation p.548: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-lkf-vol0...

So max bpc and active bpc are affecting/affected by the bpc after dithering.

By definition, if the cable carries N bpc, then dithering does not change that. The cable still carries N bpc, but due to spatial or temporal dithering, the *observed* color resolution may or may not be higher than the cable bpc.

Yes, and max bpc and active bpc tell the cable bpc ist not the *observed* bpc.

Of course, if the cable bpc is 8, and dithering targets 6 bpc, then 2 LSB on the cable are always zero, right?

I would assume that in this case only 6 bpc are actually send? Isn't the whole thing of dithering that you can't send, for example, 8 bpc?

Maybe one would want to do that if the monitor has a 6 bit panel and it simply ignored the 2 LSB, and the cable cannot go down to 6 bpc.

Is there dithering actually doing this? aka is my assumption above wrong?

AMD code that confused me before, is hinting that you might be right: https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc...

there is a set_clamp depth and a separate DCP_SPATIAL_DITHER_DEPTH_30BPP

So, what does "max bpc" mean right now?

It seems like dither on/off is insufficient information, one would also need to control the dithering target bpc. I suppose the driver has a policy on how it chooses the target bpc, but what is that policy? Is the dither target bpc the cable bpc or the sink bpc?

Needless to say, I'm quite confused.

... We need someone who knows what dithering on intel and amd gpu actually means.

But I don't want this to become a blocker for this patchset, because if there is no dithering, which seems to be the norm, the active bpc property is already really usefull as it is. So add a note to the docs that the value might be invalid when dithering is active for now?

Thanks, pq

...

...

Similar things happen on amd. Here the output dither depth seems to be written to a fixed value however:

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc...

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc...

Does anyone know about a resource where I can read up on the used registers and what this hardware actually does?

Searching now for a similar register reference for AMD GPUs.

...

My proposal for now: "max bpc" affects what happens before dither, so I would keep "active bpc" the same and add another drm property "dither active: true/false". No additional property to control dither, as amdgpu does have one already (which isn't always active?) and Intel driver does only seem prepared for dithering at 6bpc (albeit I don't know why to dither at 6bpc and what depth to dither to?).

...

• Connectors also have one standardized atomic property:
• CRTC_ID:

@@ -2152,6 +2160,49 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);

+/**

• • drm_connector_attach_active_bpc_property - attach "active bpc" property
• • @connector: connector to attach active bpc property on.
• • @min: The minimum bit depth supported by the connector.
• • @max: The maximum bit depth supported by the connector.
• • This is used to check the applied bit depth on a connector.
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max) +{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop;
• if (!connector->active_bpc_property) {

• prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "active bpc",
  

• 				 min, max);
  

• if (!prop)
  

• 	return -ENOMEM;
  

• connector->active_bpc_property = prop;
  

• drm_object_attach_property(&connector->base, prop, 0);
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);

+/**

• • drm_connector_set_active_bpc_property - sets the active bits per color property for a connector
• • @connector: drm connector
• • @active_bpc: bits per color for the connector currently active on "the cable"
• • Should be used by atomic drivers to update the active bits per color over a connector.
• */

+void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc) +{

• drm_object_property_set_value(&connector->base, connector->active_bpc_property, active_bpc);

+} +EXPORT_SYMBOL(drm_connector_set_active_bpc_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 714d1a01c065..eee86de62a5f 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1380,6 +1380,12 @@ struct drm_connector { */ struct drm_property *max_bpc_property;

• /**

• * @active_bpc_property: Default connector property for the active bpc
  

• * to be driven out of the connector.
  

• */
  

• struct drm_property *active_bpc_property;
• #define DRM_CONNECTOR_POLL_HPD (1 << 0) #define DRM_CONNECTOR_POLL_CONNECT (1 << 1) #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)

@@ -1702,6 +1708,8 @@ int drm_connector_set_panel_orientation_with_quirk( int width, int height); int drm_connector_attach_max_bpc_property(struct drm_connector *connector, int min, int max); +int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max); +void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc);

/**

• struct drm_tile_group - Tile group metadata

---

amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Am 01.07.21 um 09:42 schrieb Pekka Paalanen:

On Wed, 30 Jun 2021 11:42:10 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

...

Am 30.06.21 um 10:21 schrieb Pekka Paalanen:

...

On Tue, 29 Jun 2021 13:02:05 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

...

Am 28.06.21 um 19:03 schrieb Werner Sembach:

...

Am 18.06.21 um 11:11 schrieb Werner Sembach:

...

Add a new general drm property "active bpc" which can be used by graphic drivers to report the applied bit depth per pixel back to userspace.

While "max bpc" can be used to change the color depth, there was no way to check which one actually got used. While in theory the driver chooses the best/highest color depth within the max bpc setting a user might not be fully aware what his hardware is or isn't capable off. This is meant as a quick way to double check the setup.

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

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

drivers/gpu/drm/drm_connector.c | 51 +++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 8 ++++++ 2 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index da39e7ff6965..943f6b61053b 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1197,6 +1197,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• drm_connector_attach_max_bpc_property() to create and attach the
• property to the connector during initialization.

• • active bpc:
• • This read-only range property tells userspace the pixel color bit depth
• • 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, and the "max bpc" property.
• • Drivers shall use drm_connector_attach_active_bpc_property() to install
• • this property.

Regarding "on the cable" and dithering: As far as I can tell, what the dithering option does, is setting a hardware register here:

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/i...

• https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/i...

So dithering seems to be calculated by fixed purpose hardware/firmware outside of the driver?

The Intel driver does not seem to set a target bpc/bpp for this hardware so I guess it defaults to 6 or 8 bpc?

Never mind it does. This switch-case does affect the dithering output: https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/i...

Hi,

I obviously do not know the intel driver or hardware at all, but to me that just looks like translating from bits per pixel to bits per channel in RGB mapping?

No, if i understand the documentation correctly: Writing bit depth here with dithering enabled sets the dithering target bpc.

...

...

As found in this documentation p.548: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-lkf-vol0...

So max bpc and active bpc are affecting/affected by the bpc after dithering.

By definition, if the cable carries N bpc, then dithering does not change that. The cable still carries N bpc, but due to spatial or temporal dithering, the *observed* color resolution may or may not be higher than the cable bpc.

Yes, and max bpc and active bpc tell the cable bpc ist not the *observed* bpc.

...

Of course, if the cable bpc is 8, and dithering targets 6 bpc, then 2 LSB on the cable are always zero, right?

I would assume that in this case only 6 bpc are actually send? Isn't the whole thing of dithering that you can't send, for example, 8 bpc?

...

Maybe one would want to do that if the monitor has a 6 bit panel and it simply ignored the 2 LSB, and the cable cannot go down to 6 bpc.

Is there dithering actually doing this? aka is my assumption above wrong?

AMD code that confused me before, is hinting that you might be right: https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc...

there is a set_clamp depth and a separate DCP_SPATIAL_DITHER_DEPTH_30BPP

...

So, what does "max bpc" mean right now?

It seems like dither on/off is insufficient information, one would also need to control the dithering target bpc. I suppose the driver has a policy on how it chooses the target bpc, but what is that policy? Is the dither target bpc the cable bpc or the sink bpc?

Needless to say, I'm quite confused.

... We need someone who knows what dithering on intel and amd gpu actually means.

But I don't want this to become a blocker for this patchset, because if there is no dithering, which seems to be the norm, the active bpc property is already really usefull as it is. So add a note to the docs that the value might be invalid when dithering is active for now?

Hi,

not necessarily invalid. It all depends on how "max bpc" and "active bpc" are defined.

If they are defined and implemented as "on the cable", then they are both well-defined and always valid, regardless of what dithering or bit clamping does, so this is the semantics I'd would prefer. It's clear, but of course does not tell full story.

When better properties for dithering are added, those can then define how it works on top of cable bpc, with no impact on "max bpc" or "active bpc" properties.

So if we cannot tell what "max bpc" is, then "active bpc" should just be defined as the same thing as "max bpc" affects, and leave the precise definition of both for later.

But as long as I don't know exactly how dithering is affected by max bpc I can't tell for sure if active bpc is acting the same. That's why I wrote it is "undefined" to not run into a trap where the actual behavior have to change after the fact.

If the definition was observed bpc, then we would have problems and would need to know everything right now. But you can't really make promises of observed bpc anyway, because you don't know what the monitor does to the video signal I suppose. Unless you define it "as if observed through an ideal theoretical monitor" which then gets awkward to explain.

Yes, that's why I think describing the "raw" signal is the best and leave it to the user to know what his or her Monitor is making out of it.

Thanks, pq

On Fri, 18 Jun 2021 11:11:05 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

Add a new general drm property "active color format" which can be used by graphic drivers to report the used color format back to userspace.

There was no way to check which color format got actually used on a given monitor. To surely predict this, one must know the exact capabilities of the monitor, the GPU, and the connection used and what the default behaviour of the used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915 prefers RGB). This property helps eliminating the guessing on this point.

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

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

drivers/gpu/drm/drm_connector.c | 61 +++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 9 +++++ 2 files changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 943f6b61053b..684d7abdf0eb 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -889,6 +889,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, "unknown" },
• { DRM_COLOR_FORMAT_RGB444, "rgb" },
• { DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
• { DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
• { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },

+};

DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name, drm_dp_subconnector_enum_list)

@@ -1205,6 +1213,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• Drivers shall use drm_connector_attach_active_bpc_property() to install
• this property.

• • 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.
• • Connectors also have one standardized atomic property:
  • CRTC_ID:

@@ -2203,6 +2219,51 @@ void drm_connector_set_active_bpc_property(struct drm_connector *connector, int } EXPORT_SYMBOL(drm_connector_set_active_bpc_property);

+/**

• • drm_connector_attach_active_color_format_property - attach "active color format" property
• • @connector: connector to attach active color format property on.
• • This is used to check the applied color format on a connector.
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int drm_connector_attach_active_color_format_property(struct drm_connector *connector) +{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop;
• if (!connector->active_color_format_property) {

• prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE, "active color format",
  

• 				drm_active_color_format_enum_list,
  

• 				ARRAY_SIZE(drm_active_color_format_enum_list));
  

• if (!prop)
  

• 	return -ENOMEM;
  

• connector->active_color_format_property = prop;
  

• drm_object_attach_property(&connector->base, prop, 0);
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);

+/**

• • drm_connector_set_active_color_format_property - sets the active color format property for a
• • connector
• • @connector: drm connector
• • @active_color_format: color format for the connector currently active on "the cable"
• • Should be used by atomic drivers to update the active color format over a connector.
• */

+void drm_connector_set_active_color_format_property(struct drm_connector *connector,

• 				    u32 active_color_format)
  

+{

• drm_object_property_set_value(&connector->base, connector->active_color_format_property,

• 		      active_color_format);
  

+} +EXPORT_SYMBOL(drm_connector_set_active_color_format_property);

/**

• drm_connector_attach_hdr_output_metadata_property - attach "HDR_OUTPUT_METADA" property
• @connector: connector to attach the property on.

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index eee86de62a5f..8a5197f14e87 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1386,6 +1386,12 @@ struct drm_connector { */ struct drm_property *active_bpc_property;

• /**

• * @active_color_format_property: Default connector property for the
  

• * active color format to be driven out of the connector.
  

• */
  

• struct drm_property *active_color_format_property;

#define DRM_CONNECTOR_POLL_HPD (1 << 0) #define DRM_CONNECTOR_POLL_CONNECT (1 << 1) #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2) @@ -1710,6 +1716,9 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector, int min, int max); int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max); void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc); +int drm_connector_attach_active_color_format_property(struct drm_connector *connector); +void drm_connector_set_active_color_format_property(struct drm_connector *connector,

• 				    u32 active_color_format);
  
  

/**

• struct drm_tile_group - Tile group metadata

Acked-by: Pekka Paalanen pekka.paalanen@collabora.com

Thanks, pq

On Fri, 18 Jun 2021 11:11:08 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

Add a new general drm property "active color range" which can be used by graphic drivers to report the used color range back to userspace.

There was no way to check which color range got actually used on a given monitor. To surely predict this, one must know the exact capabilities of the monitor and what the default behaviour of the used driver is. This property helps eliminating the guessing at this point.

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

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

drivers/gpu/drm/drm_connector.c | 59 +++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 27 +++++++++++++++ 2 files changed, 86 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 684d7abdf0eb..818de58d972f 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -897,6 +897,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, };

+static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = {

• { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" },
• { DRM_MODE_COLOR_RANGE_FULL, "Full" },
• { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" },

Doesn't "limited" mean different numbers on RGB vs. Y vs. CbCr? I have a vague recollection that at least one of them was different from the others.

Documenting DRM_MODE_COLOR_RANGE_UNSET as "unspecified/default" while the string for it is "Unknown" seems inconsistent to me. I would recommend to avoid the word "default" because "reset to defaults" might become a thing one day, and that probably is not the same default as here.

Is there actually a case for "unknown"? How can it be not known? Or does it mean "not applicable"?

Otherwise looks good to me.

Thanks, pq

+};

DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name, drm_dp_subconnector_enum_list)

@@ -1221,6 +1227,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• drm_connector_attach_active_color_format_property() to install this
• property.

• • active color range:
• • This read-only property tells userspace the color range actually used by
• • the hardware display engine on "the cable" on a connector. The chosen
• • value depends on hardware capabilities of the monitor and the used color
• • format. Drivers shall use
• • drm_connector_attach_active_color_range_property() to install this
• • property.
• • Connectors also have one standardized atomic property:
  • CRTC_ID:

@@ -2264,6 +2278,51 @@ void drm_connector_set_active_color_format_property(struct drm_connector *connec } EXPORT_SYMBOL(drm_connector_set_active_color_format_property);

+/**

• • drm_connector_attach_active_color_range_property - attach "active color range" property
• • @connector: connector to attach active color range property on.
• • This is used to check the applied color range on a connector.
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int drm_connector_attach_active_color_range_property(struct drm_connector *connector) +{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop;
• if (!connector->active_color_range_property) {

• prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE, "active color range",
  

• 				drm_active_color_range_enum_list,
  

• 				ARRAY_SIZE(drm_active_color_range_enum_list));
  

• if (!prop)
  

• 	return -ENOMEM;
  

• connector->active_color_range_property = prop;
  

• drm_object_attach_property(&connector->base, prop, DRM_MODE_COLOR_RANGE_UNSET);
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_connector_attach_active_color_range_property);

+/**

• • drm_connector_set_active_color_range_property - sets the active color range property for a
• • connector
• • @connector: drm connector
• • @active_color_range: color range for the connector currently active on "the cable"
• • Should be used by atomic drivers to update the active color range over a connector.
• */

+void drm_connector_set_active_color_range_property(struct drm_connector *connector,

• 				   enum drm_mode_color_range active_color_range)
  

+{

• drm_object_property_set_value(&connector->base, connector->active_color_range_property,

• 		      active_color_range);
  

+} +EXPORT_SYMBOL(drm_connector_set_active_color_range_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 8a5197f14e87..9fb7119b7a02 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -648,6 +648,24 @@ struct drm_tv_connector_state { unsigned int hue; };

+/**

• • enum drm_mode_color_range - color_range info for &drm_connector
• • This enum is used to represent full or limited color range on the display
• • connector signal.
• • @DRM_MODE_COLOR_RANGE_UNSET: Color range is unspecified/default.
• • @DRM_MODE_COLOR_RANGE_FULL: Color range is full range, 0-255 for

• • 			8-Bit color depth.
    

• • DRM_MODE_COLOR_RANGE_LIMITED_16_235: Color range is limited range, 16-235 for

• • 			8-Bit color depth.
    

• */

+enum drm_mode_color_range {

• DRM_MODE_COLOR_RANGE_UNSET,
• DRM_MODE_COLOR_RANGE_FULL,
• DRM_MODE_COLOR_RANGE_LIMITED_16_235,

+};

/**

• struct drm_connector_state - mutable connector state

*/ @@ -1392,6 +1410,12 @@ struct drm_connector { */ struct drm_property *active_color_format_property;

• /**

• * @active_color_range_property: Default connector property for the
  

• * active color range to be driven out of the connector.
  

• */
  

• struct drm_property *active_color_range_property;

#define DRM_CONNECTOR_POLL_HPD (1 << 0) #define DRM_CONNECTOR_POLL_CONNECT (1 << 1) #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2) @@ -1719,6 +1743,9 @@ void drm_connector_set_active_bpc_property(struct drm_connector *connector, int int drm_connector_attach_active_color_format_property(struct drm_connector *connector); void drm_connector_set_active_color_format_property(struct drm_connector *connector, u32 active_color_format); +int drm_connector_attach_active_color_range_property(struct drm_connector *connector); +void drm_connector_set_active_color_range_property(struct drm_connector *connector,

• 				   enum drm_mode_color_range active_color_range);
  
  

/**

• struct drm_tile_group - Tile group metadata

On Fri, 18 Jun 2021 11:11:11 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

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

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

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

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

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

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

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 | 17 ++++++++++ 4 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index bc3487964fb5..90d62f305257 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -687,6 +687,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 438e9585b225..c536f5e22016 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -796,6 +796,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, fence_ptr); } else if (property == connector->max_bpc_property) { state->max_requested_bpc = 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);

@@ -873,6 +875,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = 0; } else if (property == connector->max_bpc_property) { *val = state->max_requested_bpc;

• } 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 818de58d972f..aea03dd02e33 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -889,6 +889,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_YCRCB444, "ycbcr444" },
• { DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
• { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },

+};

static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { { 0, "unknown" }, { DRM_COLOR_FORMAT_RGB444, "rgb" }, @@ -1219,11 +1227,19 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• Drivers shall use drm_connector_attach_active_bpc_property() to install
• this property.

• • 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.
• • active color format:
  • This read-only property tells userspace the color format actually used
  • by the hardware display engine on "the cable" on a connector. The chosen
  • value depends on hardware capabilities, both display engine and

• • connected monitor. Drivers shall use

• • connected monitor, and the "preferred color format". Drivers shall use
  • drm_connector_attach_active_color_format_property() to install this
  • property.

@@ -2233,6 +2249,36 @@ void drm_connector_set_active_bpc_property(struct drm_connector *connector, int } EXPORT_SYMBOL(drm_connector_set_active_bpc_property);

+/**

• • drm_connector_attach_preferred_color_format_property - attach "preferred color format" property
• • @connector: connector to attach active color format property on.
• • This is used to add support for selecting a color format on a connector.
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int drm_connector_attach_preferred_color_format_property(struct drm_connector *connector) +{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop;
• if (!connector->preferred_color_format_property) {

• prop = drm_property_create_enum(dev, 0, "preferred color format",
  

• 				drm_preferred_color_format_enum_list,
  

• 				ARRAY_SIZE(drm_preferred_color_format_enum_list));
  

• if (!prop)
  

• 	return -ENOMEM;
  

• connector->preferred_color_format_property = prop;
  

• drm_object_attach_property(&connector->base, prop, 0);
  

• connector->state->preferred_color_format = 0;
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_connector_attach_preferred_color_format_property);

/**

• drm_connector_attach_active_color_format_property - attach "active color format" property
• @connector: connector to attach active color format property on.

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 9fb7119b7a02..7b85407ba45c 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -799,6 +799,16 @@ struct drm_connector_state { */ u8 max_bpc;

• /**

• * preferred_color_format: Property set by userspace to tell the GPU
  

• * driver which color format to use. It only gets applied if hardware,
  

• * meaning both the computer and the monitor, and the driver support the
  

• * given format at the current resolution and refresh rate. Userspace
  

• * can check for (un-)successful application via the active_color_format
  

• * property.
  

• */
  

• u32 preferred_color_format;

Hi,

yes, I think this makes sense, even if it is a property that one can't tell for sure what it does before hand.

Using a pair of properties, preference and active, to ask for something and then check what actually worked is good for reducing the combinatorial explosion caused by needing to "atomic TEST_ONLY commit" test different KMS configurations. Userspace has a better chance of finding a configuration that is possible.

OTOH, this has the problem than in UI one cannot tell the user in advance which options are truly possible. Given that KMS properties are rarely completely independent, and in this case known to depend on several other KMS properties, I think it is good enough to know after the fact.

If a driver does not use what userspace prefers, there is no way to understand why, or what else to change to make it happen. That problem exists anyway, because TEST_ONLY commits do not give useful feedback but only a yes/no.

Acked-by: Pekka Paalanen pekka.paalanen@collabora.com

Thanks, pq

• /**
  • @hdr_output_metadata:
  • DRM blob property for HDR output metadata

@@ -1404,6 +1414,12 @@ struct drm_connector { */ struct drm_property *active_bpc_property;

• /**

• * @preferred_color_format_property: Default connector property for the
  

• * preferred color format to be driven out of the connector.
  

• */
  

• struct drm_property *preferred_color_format_property;
• /**
  • @active_color_format_property: Default connector property for the
  • active color format to be driven out of the connector.

@@ -1740,6 +1756,7 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector, int min, int max); int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max); void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc); +int drm_connector_attach_preferred_color_format_property(struct drm_connector *connector); int drm_connector_attach_active_color_format_property(struct drm_connector *connector); void drm_connector_set_active_color_format_property(struct drm_connector *connector, u32 active_color_format);

On Fri, 18 Jun 2021 11:11:14 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

Add "Broadcast RGB" to general drm context so that more drivers besides i915 and gma500 can implement it without duplicating code.

Userspace can use this property to tell the graphic driver to use full or limited color range for a given connector, overwriting the default behaviour/automatic detection.

Possible options are: - Automatic (default/current behaviour) - Full - Limited 16:235

In theory the driver should be able to automatically detect the monitors capabilities, but because of flawed standard implementations in Monitors, this might fail. In this case a manual overwrite is required to not have washed out colors or lose details in very dark or bright scenes.

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 90d62f305257..0c89d32efbd0 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (old_connector_state->preferred_color_format != new_connector_state->preferred_color_format) new_crtc_state->connectors_changed = true;

• 	if (old_connector_state->preferred_color_range !=
  

• 	    new_connector_state->preferred_color_range)
  

• 		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 c536f5e22016..c589bb1a8163 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->preferred_color_format_property) { state->preferred_color_format = val;

• } else if (property == connector->preferred_color_range_property) {

• state->preferred_color_range = val;
  

  } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val);

@@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->max_requested_bpc; } else if (property == connector->preferred_color_format_property) { *val = state->preferred_color_format;

• } else if (property == connector->preferred_color_range_property) {

• *val = state->preferred_color_range;
  

  } 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 aea03dd02e33..9bc596638613 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, };

+static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = {

• { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" },
• { DRM_MODE_COLOR_RANGE_FULL, "Full" },
• { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" },

Hi,

the same question here about these numbers as I asked on the "active color range" property.

+};

static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, { DRM_MODE_COLOR_RANGE_FULL, "Full" }, @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• drm_connector_attach_active_color_format_property() to install this
• property.

• • Broadcast RGB:
• • This property is used by userspace to change the used color range. When
• • used the driver will use the selected range if valid for the current
• • color format. Drivers to use the function
• • drm_connector_attach_preferred_color_format_property() to create and
• • attach the property to the connector during initialization.

An important detail to document here is: does userspace need to take care that pixel data at the connector will already match the set range, or will the driver program the hardware to produce the set range?

If the former, then I'm afraid the preference/active property pair design does not work. Userspace needs to make sure the content is in the right range, so the driver cannot second-guess that afterwards.

If the latter, then what does the driver assume about color range just before the automatic conversion to the final color range, and does the range conversion happen as the final step in the color pipeline?

If I remember the discussion about Intel right, then the driver does the latter and assume that userspace programs KMS to always produce full range pixels. There is no provision for userspace to produce limited range pixels, IIRC.

Thanks, pq

• • active color range:
  • This read-only property tells userspace the color range actually used by
  • the hardware display engine on "the cable" on a connector. The chosen

@@ -2324,6 +2337,36 @@ void drm_connector_set_active_color_format_property(struct drm_connector *connec } EXPORT_SYMBOL(drm_connector_set_active_color_format_property);

+/**

• • drm_connector_attach_preferred_color_range_property - attach "Broadcast RGB" property
• • @connector: connector to attach preferred color range property on.
• • This is used to add support for selecting a color range on a connector.
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int drm_connector_attach_preferred_color_range_property(struct drm_connector *connector) +{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop;
• if (!connector->preferred_color_range_property) {

• prop = drm_property_create_enum(dev, 0, "Broadcast RGB",
  

• 				drm_preferred_color_range_enum_list,
  

• 				ARRAY_SIZE(drm_preferred_color_range_enum_list));
  

• if (!prop)
  

• 	return -ENOMEM;
  

• connector->preferred_color_range_property = prop;
  

• drm_object_attach_property(&connector->base, prop, DRM_MODE_COLOR_RANGE_UNSET);
  

• connector->state->preferred_color_range = DRM_MODE_COLOR_RANGE_UNSET;
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_connector_attach_preferred_color_range_property);

/**

• drm_connector_attach_active_color_range_property - attach "active color range" property
• @connector: connector to attach active color range property on.

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 7b85407ba45c..b319760d4a8c 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -809,6 +809,15 @@ struct drm_connector_state { */ u32 preferred_color_format;

• /**

• * preferred_color_range: Property set by userspace via "Broadcast RGB"
  

• * property to tell the GPU driver which color range to use. It
  

• * overwrites existing automatic detection mechanisms, if set and valid
  

• * for the current color format. Userspace can check for (un-)successful
  

• * application via the "active color range" property.
  

• */
  

• enum drm_mode_color_range preferred_color_range;
• /**
  • @hdr_output_metadata:
  • DRM blob property for HDR output metadata

@@ -1426,6 +1435,12 @@ struct drm_connector { */ struct drm_property *active_color_format_property;

• /**

• * @preferred_color_range_property: Default connector property for the
  

• * preferred color range to be driven out of the connector.
  

• */
  

• struct drm_property *preferred_color_range_property;
• /**
  • @active_color_range_property: Default connector property for the
  • active color range to be driven out of the connector.

@@ -1760,6 +1775,7 @@ int drm_connector_attach_preferred_color_format_property(struct drm_connector *c int drm_connector_attach_active_color_format_property(struct drm_connector *connector); void drm_connector_set_active_color_format_property(struct drm_connector *connector, u32 active_color_format); +int drm_connector_attach_preferred_color_range_property(struct drm_connector *connector); int drm_connector_attach_active_color_range_property(struct drm_connector *connector); void drm_connector_set_active_color_range_property(struct drm_connector *connector, enum drm_mode_color_range active_color_range);

On Tue, 22 Jun 2021 11:57:53 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

Am 22.06.21 um 09:25 schrieb Pekka Paalanen:

...

On Fri, 18 Jun 2021 11:11:14 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

...

Add "Broadcast RGB" to general drm context so that more drivers besides i915 and gma500 can implement it without duplicating code.

Userspace can use this property to tell the graphic driver to use full or limited color range for a given connector, overwriting the default behaviour/automatic detection.

Possible options are: - Automatic (default/current behaviour) - Full - Limited 16:235

In theory the driver should be able to automatically detect the monitors capabilities, but because of flawed standard implementations in Monitors, this might fail. In this case a manual overwrite is required to not have washed out colors or lose details in very dark or bright scenes.

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 90d62f305257..0c89d32efbd0 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (old_connector_state->preferred_color_format != new_connector_state->preferred_color_format) new_crtc_state->connectors_changed = true;

• 	if (old_connector_state->preferred_color_range !=
  

• 	    new_connector_state->preferred_color_range)
  

• 		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 c536f5e22016..c589bb1a8163 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->preferred_color_format_property) { state->preferred_color_format = val;

• } else if (property == connector->preferred_color_range_property) {

• state->preferred_color_range = val;
  

  } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val);

@@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->max_requested_bpc; } else if (property == connector->preferred_color_format_property) { *val = state->preferred_color_format;

• } else if (property == connector->preferred_color_range_property) {

• *val = state->preferred_color_range;
  

  } 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 aea03dd02e33..9bc596638613 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, };

+static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = {

• { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" },
• { DRM_MODE_COLOR_RANGE_FULL, "Full" },
• { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" },

Hi,

the same question here about these numbers as I asked on the "active color range" property.

...

+};

static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, { DRM_MODE_COLOR_RANGE_FULL, "Full" }, @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• drm_connector_attach_active_color_format_property() to install this
• property.

• • Broadcast RGB:
• • This property is used by userspace to change the used color range. When
• • used the driver will use the selected range if valid for the current
• • color format. Drivers to use the function
• • drm_connector_attach_preferred_color_format_property() to create and
• • attach the property to the connector during initialization.

An important detail to document here is: does userspace need to take care that pixel data at the connector will already match the set range, or will the driver program the hardware to produce the set range?

Since until now, the userspace didn't even know for sure if RGB or YCbCr and therefore which full/limited format was used I guess it's all kernel space conversion.

...

If the former, then I'm afraid the preference/active property pair design does not work. Userspace needs to make sure the content is in the right range, so the driver cannot second-guess that afterwards.

If the latter, then what does the driver assume about color range just before the automatic conversion to the final color range, and does the range conversion happen as the final step in the color pipeline?

If I remember the discussion about Intel right, then the driver does the latter and assume that userspace programs KMS to always produce full range pixels. There is no provision for userspace to produce limited range pixels, IIRC.

I think I remember this too from an answer to one of the revisions of this patchset.

As long as you keep the old KMS property as is, just moving code so it is used by more drivers, this is fine and one can't do otherwise anyway.

(The rest of this email is merely pondering the future, so not about this patch in particular.)

But if we had a new, more general property for the range reported to monitors via infoframes, then it would be worth to re-visit the design. The HDR properties only set the infoframe and expect userspace to make sure that the pixels actually correspond to what the infoframes tell the monitor. One can't do HDR tone mapping automatically in the kernel, so in that sense the HDR property behaviour is obvious. But which behaviour would fit range property or others better, I'm not sure.

Generally there seems to be two approaches to designing KMS properties:

- Let userspace describe what data it has and what data should be sent to a monitor, and let the kernel driver magically come up with a conversion.

- Only userspace understands how the pixel data is encoded, and programs the transformations (DEGAMMA/CTM/GAMMA etc.) such, that the result is what a monitor expects based on e.g. infoframes.

Doing the former requires policy in the kernel. If there is a specification that uniquely defines what the conversion is, this is good. But if not or if there are artistic decisions to be made, like with HDR tone mapping, then it doesn't work.

OTOH, the former approach allows the driver to use any and all hardware features it has to realize the conversion, perhaps taking advantage of even fixed-function hardware blocks. The latter approach is much harder to map to hardware features.

This dilemma has been discussed in length in https://lists.freedesktop.org/archives/dri-devel/2021-June/311689.html

Thanks, pq

On Wed, 23 Jun 2021 12:10:14 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

Am 23.06.21 um 09:48 schrieb Pekka Paalanen:

...

On Tue, 22 Jun 2021 11:57:53 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

...

Am 22.06.21 um 09:25 schrieb Pekka Paalanen:

...

On Fri, 18 Jun 2021 11:11:14 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

...

Add "Broadcast RGB" to general drm context so that more drivers besides i915 and gma500 can implement it without duplicating code.

Userspace can use this property to tell the graphic driver to use full or limited color range for a given connector, overwriting the default behaviour/automatic detection.

Possible options are: - Automatic (default/current behaviour) - Full - Limited 16:235

In theory the driver should be able to automatically detect the monitors capabilities, but because of flawed standard implementations in Monitors, this might fail. In this case a manual overwrite is required to not have washed out colors or lose details in very dark or bright scenes.

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 90d62f305257..0c89d32efbd0 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (old_connector_state->preferred_color_format != new_connector_state->preferred_color_format) new_crtc_state->connectors_changed = true;

• 	if (old_connector_state->preferred_color_range !=
  

• 	    new_connector_state->preferred_color_range)
  

• 		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 c536f5e22016..c589bb1a8163 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->preferred_color_format_property) { state->preferred_color_format = val;

• } else if (property == connector->preferred_color_range_property) {

• state->preferred_color_range = val;
  

  } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val);

@@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->max_requested_bpc; } else if (property == connector->preferred_color_format_property) { *val = state->preferred_color_format;

• } else if (property == connector->preferred_color_range_property) {

• *val = state->preferred_color_range;
  

  } 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 aea03dd02e33..9bc596638613 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, };

+static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = {

• { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" },
• { DRM_MODE_COLOR_RANGE_FULL, "Full" },
• { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" },

Hi,

the same question here about these numbers as I asked on the "active color range" property.

...

+};

static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, { DRM_MODE_COLOR_RANGE_FULL, "Full" }, @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {

• drm_connector_attach_active_color_format_property() to install this
• property.

• • Broadcast RGB:
• • This property is used by userspace to change the used color range. When
• • used the driver will use the selected range if valid for the current
• • color format. Drivers to use the function
• • drm_connector_attach_preferred_color_format_property() to create and
• • attach the property to the connector during initialization.

An important detail to document here is: does userspace need to take care that pixel data at the connector will already match the set range, or will the driver program the hardware to produce the set range?

Since until now, the userspace didn't even know for sure if RGB or YCbCr and therefore which full/limited format was used I guess it's all kernel space conversion.

...

If the former, then I'm afraid the preference/active property pair design does not work. Userspace needs to make sure the content is in the right range, so the driver cannot second-guess that afterwards.

If the latter, then what does the driver assume about color range just before the automatic conversion to the final color range, and does the range conversion happen as the final step in the color pipeline?

If I remember the discussion about Intel right, then the driver does the latter and assume that userspace programs KMS to always produce full range pixels. There is no provision for userspace to produce limited range pixels, IIRC.

I think I remember this too from an answer to one of the revisions of this patchset.

As long as you keep the old KMS property as is, just moving code so it is used by more drivers, this is fine and one can't do otherwise anyway.

(The rest of this email is merely pondering the future, so not about this patch in particular.)

But if we had a new, more general property for the range reported to monitors via infoframes, then it would be worth to re-visit the design. The HDR properties only set the infoframe and expect userspace to make sure that the pixels actually correspond to what the infoframes tell the monitor. One can't do HDR tone mapping automatically in the kernel, so in that sense the HDR property behaviour is obvious. But which behaviour would fit range property or others better, I'm not sure.

Generally there seems to be two approaches to designing KMS properties:

• Let userspace describe what data it has and what data should be sent to a monitor, and let the kernel driver magically come up with a conversion.

• Only userspace understands how the pixel data is encoded, and programs the transformations (DEGAMMA/CTM/GAMMA etc.) such, that the result is what a monitor expects based on e.g. infoframes.

Why not both?

Because "both" means you have overlapping sets of properties that might contradict each other. Or then you need a switch between the two models.

This patchset is thought to control what's happening "on the cable", so if the input data is in a different format, the kernel has to convert it.

Right, if that is the desired semantics.

That's not how the HDR property works. Kernel does not convert there. The HDR property only sets infoframes that the monitor interprets.

Maybe in the future there could be an additional set of "input-" properties. aka "input bpc", "input color format", and "input color range". With an additional constraint that if "input-" property == "active-" property the kernel is not allowed to do any conversion regarding this aspect, giving userspace full control if wanted.

If by "input" you mean "the result from userspace provided content going through the configured KMS pixel pipeline", then yes. But it's hard to put it into words accurately.

The FB could contain whatever which userspace then programs DEGAMMA and CTM to produce what would be the "input" pixels for example.

This is getting closer to the "abstract KMS pipeline" idea which has been predicted to fall apart in the email thread linked below.

...

Doing the former requires policy in the kernel. If there is a specification that uniquely defines what the conversion is, this is good. But if not or if there are artistic decisions to be made, like with HDR tone mapping, then it doesn't work.

OTOH, the former approach allows the driver to use any and all hardware features it has to realize the conversion, perhaps taking advantage of even fixed-function hardware blocks. The latter approach is much harder to map to hardware features.

This dilemma has been discussed in length in https://lists.freedesktop.org/archives/dri-devel/2021-June/311689.html

Thanks, pq

On Fri, 18 Jun 2021 11:11:16 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

This commit implements the "Broadcast RGB" drm property for the AMD GPU driver.

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 22 ++++++++++++++----- .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++ 2 files changed, 21 insertions(+), 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 9ffd2f9d3d75..c5dbf948a47a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5252,7 +5252,8 @@ get_aspect_ratio(const struct drm_display_mode *mode_in) }

static enum dc_color_space -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,

•        enum drm_mode_color_range preferred_color_range)
  

{ enum dc_color_space color_space = COLOR_SPACE_SRGB;

@@ -5267,13 +5268,17 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) * respectively */ if (dc_crtc_timing->pix_clk_100hz > 270300) {

• 	if (dc_crtc_timing->flags.Y_ONLY)
  

• 	if (dc_crtc_timing->flags.Y_ONLY
  

• 			|| preferred_color_range ==
  

• 				DRM_MODE_COLOR_RANGE_LIMITED_16_235)
  	color_space =
  		COLOR_SPACE_YCBCR709_LIMITED;
  else
  	color_space = COLOR_SPACE_YCBCR709;
  

Hi,

does this mean that amdgpu would be using a property named "Broadcast RGB" to control the range of YCbCr too?

That is surprising. If this is truly wanted, then the documentation of "Broadcast RGB" must say that it applies to YCbCr too.

Does amdgpu do the same as intel wrt. to the question about whose responsibility it is to make the pixels at the connector to match the set range?

Thanks, pq

} else {

• 	if (dc_crtc_timing->flags.Y_ONLY)
  

• 	if (dc_crtc_timing->flags.Y_ONLY
  

• 			|| preferred_color_range ==
  

• 				DRM_MODE_COLOR_RANGE_LIMITED_16_235)
  	color_space =
  		COLOR_SPACE_YCBCR601_LIMITED;
  else
  

@@ -5283,7 +5288,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) } break; case PIXEL_ENCODING_RGB:

• color_space = COLOR_SPACE_SRGB;
  

• if (preferred_color_range == DRM_MODE_COLOR_RANGE_LIMITED_16_235)
  

• 	color_space = COLOR_SPACE_SRGB_LIMITED;
  

• else
  

• 	color_space = COLOR_SPACE_SRGB;
  

  break;

  default:

@@ -5429,7 +5437,10 @@ static void fill_stream_properties_from_drm_display_mode(

timing_out->aspect_ratio = get_aspect_ratio(mode_in);

• stream->output_color_space = get_output_color_space(timing_out);

• stream->output_color_space = get_output_color_space(timing_out,

• 					    connector_state ?
  

• 					    connector_state->preferred_color_range :
  

• 					    DRM_MODE_COLOR_RANGE_UNSET);
  
  

  stream->out_transfer_func->type = TF_TYPE_PREDEFINED; stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;

@@ -7780,6 +7791,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16); drm_connector_attach_preferred_color_format_property(&aconnector->base); drm_connector_attach_active_color_format_property(&aconnector->base);

• drm_connector_attach_preferred_color_range_property(&aconnector->base);
  

  drm_connector_attach_active_color_range_property(&aconnector->base); }

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index 2563788ba95a..80e1389fd0ec 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 @@ -421,6 +421,10 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr, if (connector->active_color_format_property) drm_connector_attach_active_color_format_property(&aconnector->base);

• connector->preferred_color_range_property = master->base.preferred_color_range_property;
• if (connector->preferred_color_range_property)

• drm_connector_attach_preferred_color_range_property(&aconnector->base);
  

• connector->active_color_range_property = master->base.active_color_range_property; if (connector->active_color_range_property) drm_connector_attach_active_color_range_property(&aconnector->base);

On Tue, 22 Jun 2021 11:28:57 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

Am 22.06.21 um 09:29 schrieb Pekka Paalanen:

...

On Fri, 18 Jun 2021 11:11:16 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

...

This commit implements the "Broadcast RGB" drm property for the AMD GPU driver.

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 22 ++++++++++++++----- .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++ 2 files changed, 21 insertions(+), 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 9ffd2f9d3d75..c5dbf948a47a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5252,7 +5252,8 @@ get_aspect_ratio(const struct drm_display_mode *mode_in) }

static enum dc_color_space -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,

•        enum drm_mode_color_range preferred_color_range)
  

{ enum dc_color_space color_space = COLOR_SPACE_SRGB;

@@ -5267,13 +5268,17 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) * respectively */ if (dc_crtc_timing->pix_clk_100hz > 270300) {

• 	if (dc_crtc_timing->flags.Y_ONLY)
  

• 	if (dc_crtc_timing->flags.Y_ONLY
  

• 			|| preferred_color_range ==
  

• 				DRM_MODE_COLOR_RANGE_LIMITED_16_235)
  	color_space =
  		COLOR_SPACE_YCBCR709_LIMITED;
  else
  	color_space = COLOR_SPACE_YCBCR709;  
  

Hi,

does this mean that amdgpu would be using a property named "Broadcast RGB" to control the range of YCbCr too?

Yes, because I avoided creating a new property, but I'm not really happy with it either.

Possibility 1: Use "Broadcast RGB" for Y'CbCr too and clarify in documentation     - still confusing name     - limited does not mean something a little bit different for Y'CbCr and not strictly 16-235: https://www.kernel.org/doc/html/v5.12/userspace-api/media/v4l/colorspaces-de... , but name of option is given by preexisting property

Possibility 2: Deprecate "Broadcast RGB" and a a more neutral sounding "preferred color range", with the more neutral sounding "limited" option instead of "Limited 16:235"     - What's the relation between the 2? pq mentioned on the amdgpu gitlab that there is a posibility for userspace to have only the new or the old one shown

It's just an idea that we could decide to expose only one or the other property. It would need to be engineered in code, go through the UAPI validation with userspace etc. I'm not aware of this being done before exactly like this, but DRM client caps exist.

- Alternatively ignore "Broadcast RGB" when "preferred color range" is set and have them coexist?

Determining "is set" means we would need "unset" value for "preferred color range". But there is no notion of who set it. If some KMS client decides to set it, then it will likely remain set, even if you next start another KMS client who does not use this property - it would just confuse users when "Broadcast RGB" silently stopped working while it still exists.

So I don't think this is a good solution.

When considering a new property, what I wrote just earlier fit here: https://lists.freedesktop.org/archives/dri-devel/2021-June/312248.html

There are more questions that just what does the limited range actually mean.

...

That is surprising. If this is truly wanted, then the documentation of "Broadcast RGB" must say that it applies to YCbCr too.

Does amdgpu do the same as intel wrt. to the question about whose responsibility it is to make the pixels at the connector to match the set range?

I guess the kernel driver does the conversion, but i have to check for both.

For Intel I did not change the behavior of Boradcast RGB, but i think it's not clearly specified in the docs where the conversion happens.

Right, at the very least the current behaviour needs to be documented before enrolling this property to any more drivers, so that those drivers can then be reviewed to work the same way.

You notice I didn't actually answer your question 1 or 2. I don't know. Going with 1 is easy compared to 2, even if the names are awkward but it technically shouldn't cause any problems. 2 may or may not be better, and until we have answers to which design is better, it's maybe best to leave option 2 alone?

Thanks, pq