[PATCH 0/4] Add "activ bpc" drm property and use it in AMD and Intel driver

[PATCH 0/4] Add "activ bpc" drm property and use it in AMD and Intel driver

[Werner Sembach]

I started work on my proposal for better color handling in Linux display
drivers: https://lkml.org/lkml/2021/5/12/764

Since the first read-only property is now implemented for amdgpu and i915 I
wanted to collect some feedback, since the other two read-only properties will
be quite similar, I hope.

I have already commited the first patch in this series independently as it fixes
a function already in use.

The actuall update of the values is implemented in patch three and four in the
atomic_commit_tail() function of amdgpu and i915 respectifly. It does get
updated more often than needed with the current approach, but without harm since
just the same value is written again. A check if the update is required would be
the same amount of computation. Let me know if you know a better place to put
the update than attomic_commit_tail().

[PATCH 1/4] drm/amd/display: Add missing cases convert_dc_color_depth_into_bpc

[Werner Sembach]

convert_dc_color_depth_into_bpc() that converts the enum dc_color_depth to an
integer had the casses for COLOR_DEPTH_999 and COLOR_DEPTH_111111 missing.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
 1 file changed, 4 insertions(+)

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 389eff96fcf6..6e82889271d5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6515,6 +6515,10 @@ static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_de
 			return 14;
 		case COLOR_DEPTH_161616:
 			return 16;
+		case COLOR_DEPTH_999:
+			return 9;
+		case COLOR_DEPTH_111111:
+			return 11;
 		default:
 			break;
 		}
-- 
2.25.1

[PATCH 2/4] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

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

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
 drivers/gpu/drm/drm_connector.c   | 40 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 15 ++++++++++++
 3 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 268bb69c2e2f..7ae4e40936b5 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -873,6 +873,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->active_bpc_property) {
+		*val = state->active_bpc;
 	} 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 7631f76e7f34..5f42a5be5822 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1195,6 +1195,13 @@ 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 is used by userspace check the bit depth
+ *	actually applied by the GPU driver after evaluation all hardware
+ *	capabilities and max bpc. Drivers to use the function
+ *	drm_connector_attach_active_bpc_property() to create and attach the
+ *	property to the connector during initialization.
+ *
  * Connectors also have one standardized atomic property:
  *
  * CRTC_ID:
@@ -2150,6 +2157,39 @@ 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;
+
+	prop = connector->active_bpc_property;
+	if (!prop) {
+		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
+		if (!prop)
+			return -ENOMEM;
+
+		connector->active_bpc_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, 0);
+	connector->state->active_bpc = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1922b278ffad..c58cba2b6afe 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -781,6 +781,13 @@ struct drm_connector_state {
 	 */
 	u8 max_bpc;
 
+	/**
+	 * @active_bpc: Read only property set by the GPU driver to the actually
+	 * applied bit depth of the pixels after evaluating all hardware
+	 * limitations.
+	 */
+	u8 active_bpc;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1380,6 +1387,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)
@@ -1698,6 +1711,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);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.25.1

Re: [PATCH 2/4] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Ville Syrjälä]

On Fri, Jun 04, 2021 at 07:17:21PM +0200, Werner Sembach 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 available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 40 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 15 ++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..7ae4e40936b5 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -873,6 +873,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->active_bpc_property) {
> +		*val = state->active_bpc;
>  	} 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 7631f76e7f34..5f42a5be5822 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1195,6 +1195,13 @@ 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 is used by userspace check the bit depth
> + *	actually applied by the GPU driver after evaluation all hardware
> + *	capabilities and max bpc. Drivers to use the function
> + *	drm_connector_attach_active_bpc_property() to create and attach the
> + *	property to the connector during initialization.
> + *
>   * Connectors also have one standardized atomic property:
>   *
>   * CRTC_ID:
> @@ -2150,6 +2157,39 @@ 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;
> +
> +	prop = connector->active_bpc_property;
> +	if (!prop) {
> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);

Should be immutable.

Also wondering what the semantics of this should be when eg. DSC
is active?

> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_bpc_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_bpc = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1922b278ffad..c58cba2b6afe 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -781,6 +781,13 @@ struct drm_connector_state {
>  	 */
>  	u8 max_bpc;
>  
> +	/**
> +	 * @active_bpc: Read only property set by the GPU driver to the actually
> +	 * applied bit depth of the pixels after evaluating all hardware
> +	 * limitations.
> +	 */
> +	u8 active_bpc;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1380,6 +1387,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)
> @@ -1698,6 +1711,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);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/4] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Werner Sembach]

Am 04.06.21 um 19:26 schrieb Ville Syrjälä:
> On Fri, Jun 04, 2021 at 07:17:21PM +0200, Werner Sembach 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 available.
>>
>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>> ---
>>   drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>>   drivers/gpu/drm/drm_connector.c   | 40 +++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h       | 15 ++++++++++++
>>   3 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 268bb69c2e2f..7ae4e40936b5 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -873,6 +873,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->active_bpc_property) {
>> +		*val = state->active_bpc;
>>   	} 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 7631f76e7f34..5f42a5be5822 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1195,6 +1195,13 @@ 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 is used by userspace check the bit depth
>> + *	actually applied by the GPU driver after evaluation all hardware
>> + *	capabilities and max bpc. Drivers to use the function
>> + *	drm_connector_attach_active_bpc_property() to create and attach the
>> + *	property to the connector during initialization.
>> + *
>>    * Connectors also have one standardized atomic property:
>>    *
>>    * CRTC_ID:
>> @@ -2150,6 +2157,39 @@ 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;
>> +
>> +	prop = connector->active_bpc_property;
>> +	if (!prop) {
>> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> Should be immutable.
Yes. I didn't know if there is a way to do this (or just don't define a 
set-function), but I think I found the define for this.
>
> Also wondering what the semantics of this should be when eg. DSC
> is active?
I'm unfamiliar how the inner workings of DSC (I guess Display Stream 
Compression?) are. But doesn't it also have color depth?

The active bpc should be what the GPU tells the display to actually show 
the user when he looks at just one pixel.

So dithering computed on the host should not be included (aka when the 
gpu sends a premade picture to the screen and tells it the lesser pbc), 
while FRC dithering computed in the display firmware should be included 
(since the GPU can't really tell the difference between FRC displays and 
True 10-Bit displays, can't it?)
>
>> +		if (!prop)
>> +			return -ENOMEM;
>> +
>> +		connector->active_bpc_property = prop;
>> +	}
>> +
>> +	drm_object_attach_property(&connector->base, prop, 0);
>> +	connector->state->active_bpc = 0;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
>> +
>>   /**
>>    * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>>    * capable property for a connector
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 1922b278ffad..c58cba2b6afe 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -781,6 +781,13 @@ struct drm_connector_state {
>>   	 */
>>   	u8 max_bpc;
>>   
>> +	/**
>> +	 * @active_bpc: Read only property set by the GPU driver to the actually
>> +	 * applied bit depth of the pixels after evaluating all hardware
>> +	 * limitations.
>> +	 */
>> +	u8 active_bpc;
>> +
>>   	/**
>>   	 * @hdr_output_metadata:
>>   	 * DRM blob property for HDR output metadata
>> @@ -1380,6 +1387,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)
>> @@ -1698,6 +1711,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);
>>   
>>   /**
>>    * struct drm_tile_group - Tile group metadata
>> -- 
>> 2.25.1

Re: [PATCH 2/4] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Ville Syrjälä]

On Mon, Jun 07, 2021 at 08:46:47AM +0200, Werner Sembach wrote:
> 
> Am 04.06.21 um 19:26 schrieb Ville Syrjälä:
> > On Fri, Jun 04, 2021 at 07:17:21PM +0200, Werner Sembach 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 available.
> >>
> >> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> >> ---
> >>   drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
> >>   drivers/gpu/drm/drm_connector.c   | 40 +++++++++++++++++++++++++++++++
> >>   include/drm/drm_connector.h       | 15 ++++++++++++
> >>   3 files changed, 57 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index 268bb69c2e2f..7ae4e40936b5 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -873,6 +873,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->active_bpc_property) {
> >> +		*val = state->active_bpc;
> >>   	} 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 7631f76e7f34..5f42a5be5822 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -1195,6 +1195,13 @@ 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 is used by userspace check the bit depth
> >> + *	actually applied by the GPU driver after evaluation all hardware
> >> + *	capabilities and max bpc. Drivers to use the function
> >> + *	drm_connector_attach_active_bpc_property() to create and attach the
> >> + *	property to the connector during initialization.
> >> + *
> >>    * Connectors also have one standardized atomic property:
> >>    *
> >>    * CRTC_ID:
> >> @@ -2150,6 +2157,39 @@ 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;
> >> +
> >> +	prop = connector->active_bpc_property;
> >> +	if (!prop) {
> >> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> > Should be immutable.
> Yes. I didn't know if there is a way to do this (or just don't define a 
> set-function), but I think I found the define for this.
> >
> > Also wondering what the semantics of this should be when eg. DSC
> > is active?
> I'm unfamiliar how the inner workings of DSC (I guess Display Stream 
> Compression?) are. But doesn't it also have color depth?

Some number of bits go into the compressor, smaller/equal number
of bits come out. Not sure if the choice of the input bpc affects
what the sink does when decompressing or not.

> 
> The active bpc should be what the GPU tells the display to actually show 
> the user when he looks at just one pixel.
> 
> So dithering computed on the host should not be included (aka when the 
> gpu sends a premade picture to the screen and tells it the lesser pbc), 
> while FRC dithering computed in the display firmware should be included 
> (since the GPU can't really tell the difference between FRC displays and 
> True 10-Bit displays, can't it?)

Not sure. Haven't checked if there's something in some standard for
that.

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/4] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4076 bytes --]

Hi,

On Fri, Jun 04, 2021 at 07:17:21PM +0200, Werner Sembach 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.

Just a heads up, we'll need an open source project that has accepted it
before merging it.

See https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements

> 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 available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 40 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 15 ++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..7ae4e40936b5 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -873,6 +873,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->active_bpc_property) {
> +		*val = state->active_bpc;
>  	} 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 7631f76e7f34..5f42a5be5822 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1195,6 +1195,13 @@ 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 is used by userspace check the bit depth
> + *	actually applied by the GPU driver after evaluation all hardware

                                ^ display

Depending on the system, the display component might have a GPU attached
or not, and the GPU might have a display component or not.

> + *	capabilities and max bpc. Drivers to use the function
> + *	drm_connector_attach_active_bpc_property() to create and attach the
> + *	property to the connector during initialization.
> + *
>   * Connectors also have one standardized atomic property:
>   *
>   * CRTC_ID:
> @@ -2150,6 +2157,39 @@ 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;
> +
> +	prop = connector->active_bpc_property;
> +	if (!prop) {
> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_bpc_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_bpc = 0;

I guess we want to default to 8?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/4] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Werner Sembach]

Am 07.06.21 um 09:40 schrieb Maxime Ripard:
> Hi,
>
> On Fri, Jun 04, 2021 at 07:17:21PM +0200, Werner Sembach 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.
> Just a heads up, we'll need an open source project that has accepted it
> before merging it.
>
> See https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
Yes, I know.
>
>> 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 available.
>>
>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>> ---
>>   drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>>   drivers/gpu/drm/drm_connector.c   | 40 +++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h       | 15 ++++++++++++
>>   3 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 268bb69c2e2f..7ae4e40936b5 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -873,6 +873,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->active_bpc_property) {
>> +		*val = state->active_bpc;
>>   	} 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 7631f76e7f34..5f42a5be5822 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1195,6 +1195,13 @@ 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 is used by userspace check the bit depth
>> + *	actually applied by the GPU driver after evaluation all hardware
>                                  ^ display
>
> Depending on the system, the display component might have a GPU attached
> or not, and the GPU might have a display component or not.
Ok, I try to bee more clear in the wording
>
>> + *	capabilities and max bpc. Drivers to use the function
>> + *	drm_connector_attach_active_bpc_property() to create and attach the
>> + *	property to the connector during initialization.
>> + *
>>    * Connectors also have one standardized atomic property:
>>    *
>>    * CRTC_ID:
>> @@ -2150,6 +2157,39 @@ 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;
>> +
>> +	prop = connector->active_bpc_property;
>> +	if (!prop) {
>> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
>> +		if (!prop)
>> +			return -ENOMEM;
>> +
>> +		connector->active_bpc_property = prop;
>> +	}
>> +
>> +	drm_object_attach_property(&connector->base, prop, 0);
>> +	connector->state->active_bpc = 0;
> I guess we want to default to 8?
>
> Maxime
The default state should only be active when no display is connected or 
the display is off, so I thought 0 makes more sense, as no active 
display = no active bpc.

Re: [PATCH 2/4] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 6121 bytes --]

On Fri,  4 Jun 2021 19:17:21 +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 available.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>  drivers/gpu/drm/drm_connector.c   | 40 +++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       | 15 ++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 268bb69c2e2f..7ae4e40936b5 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -873,6 +873,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->active_bpc_property) {
> +		*val = state->active_bpc;
>  	} 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 7631f76e7f34..5f42a5be5822 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1195,6 +1195,13 @@ 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 is used by userspace check the bit depth
> + *	actually applied by the GPU driver after evaluation all hardware
> + *	capabilities and max bpc. Drivers to use the function
> + *	drm_connector_attach_active_bpc_property() to create and attach the
> + *	property to the connector during initialization.
> + *

Hi Werner,

the idea looks good to me, but the above doc could be a little more
fluent. May I suggest something like:

	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.

There should also be something said about dithering done by the display
engine (not monitor), but I'm not sure how that should be worded. It
may also depend on if and how userspace can know about dithering. So if
a dithering related property is added later, maybe add a note here too
in that patch.


Thanks,
pq


>   * Connectors also have one standardized atomic property:
>   *
>   * CRTC_ID:
> @@ -2150,6 +2157,39 @@ 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;
> +
> +	prop = connector->active_bpc_property;
> +	if (!prop) {
> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->active_bpc_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, 0);
> +	connector->state->active_bpc = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
> +
>  /**
>   * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>   * capable property for a connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1922b278ffad..c58cba2b6afe 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -781,6 +781,13 @@ struct drm_connector_state {
>  	 */
>  	u8 max_bpc;
>  
> +	/**
> +	 * @active_bpc: Read only property set by the GPU driver to the actually
> +	 * applied bit depth of the pixels after evaluating all hardware
> +	 * limitations.
> +	 */
> +	u8 active_bpc;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1380,6 +1387,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)
> @@ -1698,6 +1711,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);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Werner Sembach]

[-- Attachment #1: Type: text/plain, Size: 6584 bytes --]


Am 07.06.21 um 09:52 schrieb Pekka Paalanen:
> On Fri,  4 Jun 2021 19:17:21 +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 available.
>>
>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>> ---
>>   drivers/gpu/drm/drm_atomic_uapi.c |  2 ++
>>   drivers/gpu/drm/drm_connector.c   | 40 +++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h       | 15 ++++++++++++
>>   3 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 268bb69c2e2f..7ae4e40936b5 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -873,6 +873,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->active_bpc_property) {
>> +		*val = state->active_bpc;
>>   	} 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 7631f76e7f34..5f42a5be5822 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1195,6 +1195,13 @@ 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 is used by userspace check the bit depth
>> + *	actually applied by the GPU driver after evaluation all hardware
>> + *	capabilities and max bpc. Drivers to use the function
>> + *	drm_connector_attach_active_bpc_property() to create and attach the
>> + *	property to the connector during initialization.
>> + *
> Hi Werner,
>
> the idea looks good to me, but the above doc could be a little more
> fluent. May I suggest something like:
>
> 	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.
>
> There should also be something said about dithering done by the display
> engine (not monitor), but I'm not sure how that should be worded. It
> may also depend on if and how userspace can know about dithering. So if
> a dithering related property is added later, maybe add a note here too
> in that patch.
For this, and the DSC that Ville Syrjälä mentioned, I guess "the same 
behavior as max bpc" would be the most intuitive. But I don't know what 
that is and it's also not mentioned in the documentation.
>
>
> Thanks,
> pq
>
>
>>    * Connectors also have one standardized atomic property:
>>    *
>>    * CRTC_ID:
>> @@ -2150,6 +2157,39 @@ 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;
>> +
>> +	prop = connector->active_bpc_property;
>> +	if (!prop) {
>> +		prop = drm_property_create_range(dev, 0, "active bpc", min, max);
>> +		if (!prop)
>> +			return -ENOMEM;
>> +
>> +		connector->active_bpc_property = prop;
>> +	}
>> +
>> +	drm_object_attach_property(&connector->base, prop, 0);
>> +	connector->state->active_bpc = 0;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_connector_attach_active_bpc_property);
>> +
>>   /**
>>    * drm_connector_set_vrr_capable_property - sets the variable refresh rate
>>    * capable property for a connector
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 1922b278ffad..c58cba2b6afe 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -781,6 +781,13 @@ struct drm_connector_state {
>>   	 */
>>   	u8 max_bpc;
>>   
>> +	/**
>> +	 * @active_bpc: Read only property set by the GPU driver to the actually
>> +	 * applied bit depth of the pixels after evaluating all hardware
>> +	 * limitations.
>> +	 */
>> +	u8 active_bpc;
>> +
>>   	/**
>>   	 * @hdr_output_metadata:
>>   	 * DRM blob property for HDR output metadata
>> @@ -1380,6 +1387,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)
>> @@ -1698,6 +1711,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);
>>   
>>   /**
>>    * struct drm_tile_group - Tile group metadata
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[-- Attachment #2: Type: text/html, Size: 7429 bytes --]

[PATCH 3/4] drm/amd/display: Add handling for new "active bpc" property

[Werner Sembach]

This commits implements the "active bpc" drm property for the AMD GPU driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 18 +++++++++++++++++-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c    |  4 +++-
 2 files changed, 20 insertions(+), 2 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 6e82889271d5..52a057c585b1 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7519,8 +7519,10 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				adev->mode_info.underscan_vborder_property,
 				0);
 
-	if (!aconnector->mst_port)
+	if (!aconnector->mst_port) {
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+	}
 
 	/* This defaults to the max in the range, but we want 8bpc for non-edp. */
 	aconnector->base.state->max_bpc = (connector_type == DRM_MODE_CONNECTOR_eDP) ? 16 : 8;
@@ -8890,6 +8892,20 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		mutex_unlock(&dm->dc_lock);
 	}
 
+	/* 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;
+		if (crtc) {
+			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+			if (dm_new_crtc_state->stream)
+				new_con_state->active_bpc = convert_dc_color_depth_into_bpc(
+					dm_new_crtc_state->stream->timing.display_color_depth);
+		}
+		else
+			new_con_state->active_bpc = 0;
+	}
+
 	/* Count number of newly disabled CRTCs for dropping PM refs later. */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
 				      new_crtc_state, i) {
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 9b221db526dc..2a8dc6b2c6c7 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
@@ -397,8 +397,10 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	}
 
 	connector->max_bpc_property = master->base.max_bpc_property;
-	if (connector->max_bpc_property)
+	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 8, 16);
+		drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
+	}
 
 	connector->vrr_capable_property = master->base.vrr_capable_property;
 	if (connector->vrr_capable_property)
-- 
2.25.1

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

[Werner Sembach]

This commits implements the "active bpc" drm property for the Intel GPU driver.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 13 +++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
 drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
 drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
 4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 64e9107d70f7..f7898d9d7438 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10164,6 +10164,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
 	struct intel_crtc *crtc;
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
 	u64 put_domains[I915_MAX_PIPES] = {};
 	intel_wakeref_t wakeref = 0;
 	int i;
@@ -10324,6 +10326,17 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	}
 	intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
 
+	/* Extract information from crtc to communicate it to userspace as connector properties */
+	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
+		struct drm_crtc *crtc = new_conn_state->crtc;
+		if (crtc) {
+			new_crtc_state = to_intel_crtc_state(drm_atomic_get_new_crtc_state(&state->base, crtc));
+			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
+		}
+		else
+			new_conn_state->active_bpc = 0;
+	}
+
 	/*
 	 * Defer the cleanup of the old state to a separate worker to not
 	 * impede the current task (userspace for blocking modesets) that
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 642c60f3d9b1..67826ba976ed 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4671,10 +4671,14 @@ 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_bpc_property(connector, 6, 10);
+	}
+	else if (DISPLAY_VER(dev_priv) >= 5) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_bpc_property(connector, 6, 12);
+	}
 
 	/* Register HDMI colorspace for case of lspcon */
 	if (intel_bios_is_lspcon_present(dev_priv, port)) {
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 2daa3f67791e..5a1869dc2210 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -844,8 +844,10 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	 */
 	connector->max_bpc_property =
 		intel_dp->attached_connector->base.max_bpc_property;
-	if (connector->max_bpc_property)
+	if (connector->max_bpc_property) {
 		drm_connector_attach_max_bpc_property(connector, 6, 12);
+		drm_connector_attach_active_bpc_property(connector, 6, 12);
+	}
 
 	return connector;
 
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index d69f0a6dc26d..8af78b27b6ce 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2463,8 +2463,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 		drm_object_attach_property(&connector->base,
 			connector->dev->mode_config.hdr_output_metadata_property, 0);
 
-	if (!HAS_GMCH(dev_priv))
+	if (!HAS_GMCH(dev_priv)) {
 		drm_connector_attach_max_bpc_property(connector, 8, 12);
+		drm_connector_attach_active_bpc_property(connector, 8, 12);
+	}
 }
 
 /*
-- 
2.25.1

Re: [PATCH 4/4] drm/i915/display: Add handling for new "active bpc" property

[Ville Syrjälä]

On Fri, Jun 04, 2021 at 07:17:23PM +0200, Werner Sembach wrote:
> This commits implements the "active bpc" drm property for the Intel GPU driver.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
>  drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
>  4 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 64e9107d70f7..f7898d9d7438 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10164,6 +10164,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
>  	struct intel_crtc *crtc;
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
>  	u64 put_domains[I915_MAX_PIPES] = {};
>  	intel_wakeref_t wakeref = 0;
>  	int i;
> @@ -10324,6 +10326,17 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	}
>  	intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
>  
> +	/* Extract information from crtc to communicate it to userspace as connector properties */
> +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
> +		struct drm_crtc *crtc = new_conn_state->crtc;
> +		if (crtc) {
> +			new_crtc_state = to_intel_crtc_state(drm_atomic_get_new_crtc_state(&state->base, crtc));

intel_atomic_get_new_crtc_state()

> +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> +		}
> +		else
> +			new_conn_state->active_bpc = 0;
> +	}

This also seems too late. I think the whole thing should be
done somewhere around the normal swap_state() stuff.

> +
>  	/*
>  	 * Defer the cleanup of the old state to a separate worker to not
>  	 * impede the current task (userspace for blocking modesets) that
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 642c60f3d9b1..67826ba976ed 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4671,10 +4671,14 @@ 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_bpc_property(connector, 6, 10);
> +	}
> +	else if (DISPLAY_VER(dev_priv) >= 5) {
>  		drm_connector_attach_max_bpc_property(connector, 6, 12);
> +		drm_connector_attach_active_bpc_property(connector, 6, 12);
> +	}
>  
>  	/* Register HDMI colorspace for case of lspcon */
>  	if (intel_bios_is_lspcon_present(dev_priv, port)) {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 2daa3f67791e..5a1869dc2210 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -844,8 +844,10 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	 */
>  	connector->max_bpc_property =
>  		intel_dp->attached_connector->base.max_bpc_property;
> -	if (connector->max_bpc_property)
> +	if (connector->max_bpc_property) {
>  		drm_connector_attach_max_bpc_property(connector, 6, 12);
> +		drm_connector_attach_active_bpc_property(connector, 6, 12);
> +	}
>  
>  	return connector;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index d69f0a6dc26d..8af78b27b6ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2463,8 +2463,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
>  		drm_object_attach_property(&connector->base,
>  			connector->dev->mode_config.hdr_output_metadata_property, 0);
>  
> -	if (!HAS_GMCH(dev_priv))
> +	if (!HAS_GMCH(dev_priv)) {
>  		drm_connector_attach_max_bpc_property(connector, 8, 12);
> +		drm_connector_attach_active_bpc_property(connector, 8, 12);
> +	}
>  }
>  
>  /*
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 4/4] drm/i915/display: Add handling for new "active bpc" property

[Werner Sembach]

Am 04.06.21 um 19:30 schrieb Ville Syrjälä:
> On Fri, Jun 04, 2021 at 07:17:23PM +0200, Werner Sembach wrote:
>> This commits implements the "active bpc" drm property for the Intel GPU driver.
>>
>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 13 +++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
>>   drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
>>   drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
>>   4 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 64e9107d70f7..f7898d9d7438 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -10164,6 +10164,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>>   	struct drm_i915_private *dev_priv = to_i915(dev);
>>   	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
>>   	struct intel_crtc *crtc;
>> +	struct drm_connector *connector;
>> +	struct drm_connector_state *new_conn_state;
>>   	u64 put_domains[I915_MAX_PIPES] = {};
>>   	intel_wakeref_t wakeref = 0;
>>   	int i;
>> @@ -10324,6 +10326,17 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>>   	}
>>   	intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
>>   
>> +	/* Extract information from crtc to communicate it to userspace as connector properties */
>> +	for_each_new_connector_in_state(&state->base, connector, new_conn_state, i) {
>> +		struct drm_crtc *crtc = new_conn_state->crtc;
>> +		if (crtc) {
>> +			new_crtc_state = to_intel_crtc_state(drm_atomic_get_new_crtc_state(&state->base, crtc));
> intel_atomic_get_new_crtc_state()
Thanks, will use that.
>
>> +			new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
>> +		}
>> +		else
>> +			new_conn_state->active_bpc = 0;
>> +	}
> This also seems too late. I think the whole thing should be
> done somewhere around the normal swap_state() stuff.
Ok, will look into it.
>
>> +
>>   	/*
>>   	 * Defer the cleanup of the old state to a separate worker to not
>>   	 * impede the current task (userspace for blocking modesets) that
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 642c60f3d9b1..67826ba976ed 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -4671,10 +4671,14 @@ 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_bpc_property(connector, 6, 10);
>> +	}
>> +	else if (DISPLAY_VER(dev_priv) >= 5) {
>>   		drm_connector_attach_max_bpc_property(connector, 6, 12);
>> +		drm_connector_attach_active_bpc_property(connector, 6, 12);
>> +	}
>>   
>>   	/* Register HDMI colorspace for case of lspcon */
>>   	if (intel_bios_is_lspcon_present(dev_priv, port)) {
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> index 2daa3f67791e..5a1869dc2210 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> @@ -844,8 +844,10 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>>   	 */
>>   	connector->max_bpc_property =
>>   		intel_dp->attached_connector->base.max_bpc_property;
>> -	if (connector->max_bpc_property)
>> +	if (connector->max_bpc_property) {
>>   		drm_connector_attach_max_bpc_property(connector, 6, 12);
>> +		drm_connector_attach_active_bpc_property(connector, 6, 12);
>> +	}
>>   
>>   	return connector;
>>   
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> index d69f0a6dc26d..8af78b27b6ce 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> @@ -2463,8 +2463,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
>>   		drm_object_attach_property(&connector->base,
>>   			connector->dev->mode_config.hdr_output_metadata_property, 0);
>>   
>> -	if (!HAS_GMCH(dev_priv))
>> +	if (!HAS_GMCH(dev_priv)) {
>>   		drm_connector_attach_max_bpc_property(connector, 8, 12);
>> +		drm_connector_attach_active_bpc_property(connector, 8, 12);
>> +	}
>>   }
>>   
>>   /*
>> -- 
>> 2.25.1

Re: [PATCH 4/4] drm/i915/display: Add handling for new "active bpc" property

[Werner Sembach]

Am 07.06.21 um 08:47 schrieb Werner Sembach:
>
> Am 04.06.21 um 19:30 schrieb Ville Syrjälä:
>> On Fri, Jun 04, 2021 at 07:17:23PM +0200, Werner Sembach wrote:
>>> This commits implements the "active bpc" drm property for the Intel 
>>> GPU driver.
>>>
>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_display.c | 13 +++++++++++++
>>>   drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
>>>   drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
>>>   drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
>>>   4 files changed, 25 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
>>> b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 64e9107d70f7..f7898d9d7438 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -10164,6 +10164,8 @@ static void intel_atomic_commit_tail(struct 
>>> intel_atomic_state *state)
>>>       struct drm_i915_private *dev_priv = to_i915(dev);
>>>       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
>>>       struct intel_crtc *crtc;
>>> +    struct drm_connector *connector;
>>> +    struct drm_connector_state *new_conn_state;
>>>       u64 put_domains[I915_MAX_PIPES] = {};
>>>       intel_wakeref_t wakeref = 0;
>>>       int i;
>>> @@ -10324,6 +10326,17 @@ static void intel_atomic_commit_tail(struct 
>>> intel_atomic_state *state)
>>>       }
>>>       intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
>>>   +    /* Extract information from crtc to communicate it to 
>>> userspace as connector properties */
>>> +    for_each_new_connector_in_state(&state->base, connector, 
>>> new_conn_state, i) {
>>> +        struct drm_crtc *crtc = new_conn_state->crtc;
>>> +        if (crtc) {
>>> +            new_crtc_state = 
>>> to_intel_crtc_state(drm_atomic_get_new_crtc_state(&state->base, crtc));
>> intel_atomic_get_new_crtc_state()
> Thanks, will use that.
>>
>>> + new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
>>> +        }
>>> +        else
>>> +            new_conn_state->active_bpc = 0;
>>> +    }
>> This also seems too late. I think the whole thing should be
>> done somewhere around the normal swap_state() stuff.
> Ok, will look into it.
So I tried to put it in intel_atomic_commit() after 
drm_atomic_helper_swap_state() and before 
INIT_WORK(&state->base.commit_work, intel_atomic_commit_work) (which 
creates a worker for intel_atomic_commit_tail), but somewhere in 
between, the connector_state seems to change: The bpc written with the 
for_each_new_connector_in_state() loop, gets discarded.
>>
>>> +
>>>       /*
>>>        * Defer the cleanup of the old state to a separate worker to not
>>>        * impede the current task (userspace for blocking modesets) that
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
>>> b/drivers/gpu/drm/i915/display/intel_dp.c
>>> index 642c60f3d9b1..67826ba976ed 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>> @@ -4671,10 +4671,14 @@ 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_bpc_property(connector, 6, 10);
>>> +    }
>>> +    else if (DISPLAY_VER(dev_priv) >= 5) {
>>>           drm_connector_attach_max_bpc_property(connector, 6, 12);
>>> +        drm_connector_attach_active_bpc_property(connector, 6, 12);
>>> +    }
>>>         /* Register HDMI colorspace for case of lspcon */
>>>       if (intel_bios_is_lspcon_present(dev_priv, port)) {
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c 
>>> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>> index 2daa3f67791e..5a1869dc2210 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>> @@ -844,8 +844,10 @@ static struct drm_connector 
>>> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>>>        */
>>>       connector->max_bpc_property =
>>> intel_dp->attached_connector->base.max_bpc_property;
>>> -    if (connector->max_bpc_property)
>>> +    if (connector->max_bpc_property) {
>>>           drm_connector_attach_max_bpc_property(connector, 6, 12);
>>> +        drm_connector_attach_active_bpc_property(connector, 6, 12);
>>> +    }
>>>         return connector;
>>>   diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
>>> b/drivers/gpu/drm/i915/display/intel_hdmi.c
>>> index d69f0a6dc26d..8af78b27b6ce 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
>>> @@ -2463,8 +2463,10 @@ intel_hdmi_add_properties(struct intel_hdmi 
>>> *intel_hdmi, struct drm_connector *c
>>>           drm_object_attach_property(&connector->base,
>>> connector->dev->mode_config.hdr_output_metadata_property, 0);
>>>   -    if (!HAS_GMCH(dev_priv))
>>> +    if (!HAS_GMCH(dev_priv)) {
>>>           drm_connector_attach_max_bpc_property(connector, 8, 12);
>>> +        drm_connector_attach_active_bpc_property(connector, 8, 12);
>>> +    }
>>>   }
>>>     /*
>>> -- 
>>> 2.25.1
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] drm/i915/display: Add handling for new "active bpc" property

[Werner Sembach]

Am 07.06.21 um 22:33 schrieb Werner Sembach:
> Am 07.06.21 um 08:47 schrieb Werner Sembach:
>>
>> Am 04.06.21 um 19:30 schrieb Ville Syrjälä:
>>> On Fri, Jun 04, 2021 at 07:17:23PM +0200, Werner Sembach wrote:
>>>> This commits implements the "active bpc" drm property for the Intel 
>>>> GPU driver.
>>>>
>>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/display/intel_display.c | 13 +++++++++++++
>>>>   drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
>>>>   drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
>>>>   drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
>>>>   4 files changed, 25 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
>>>> b/drivers/gpu/drm/i915/display/intel_display.c
>>>> index 64e9107d70f7..f7898d9d7438 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>> @@ -10164,6 +10164,8 @@ static void intel_atomic_commit_tail(struct 
>>>> intel_atomic_state *state)
>>>>       struct drm_i915_private *dev_priv = to_i915(dev);
>>>>       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
>>>>       struct intel_crtc *crtc;
>>>> +    struct drm_connector *connector;
>>>> +    struct drm_connector_state *new_conn_state;
>>>>       u64 put_domains[I915_MAX_PIPES] = {};
>>>>       intel_wakeref_t wakeref = 0;
>>>>       int i;
>>>> @@ -10324,6 +10326,17 @@ static void 
>>>> intel_atomic_commit_tail(struct intel_atomic_state *state)
>>>>       }
>>>>       intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
>>>>   +    /* Extract information from crtc to communicate it to 
>>>> userspace as connector properties */
>>>> +    for_each_new_connector_in_state(&state->base, connector, 
>>>> new_conn_state, i) {
>>>> +        struct drm_crtc *crtc = new_conn_state->crtc;
>>>> +        if (crtc) {
>>>> +            new_crtc_state = 
>>>> to_intel_crtc_state(drm_atomic_get_new_crtc_state(&state->base, 
>>>> crtc));
>>> intel_atomic_get_new_crtc_state()
>> Thanks, will use that.
>>>
>>>> + new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
>>>> +        }
>>>> +        else
>>>> +            new_conn_state->active_bpc = 0;
>>>> +    }
>>> This also seems too late. I think the whole thing should be
>>> done somewhere around the normal swap_state() stuff.
>> Ok, will look into it.
> So I tried to put it in intel_atomic_commit() after 
> drm_atomic_helper_swap_state() and before 
> INIT_WORK(&state->base.commit_work, intel_atomic_commit_work) (which 
> creates a worker for intel_atomic_commit_tail), but somewhere in 
> between, the connector_state seems to change: The bpc written with the 
> for_each_new_connector_in_state() loop, gets discarded.

This was not the problem. Setting the drm property immutable made it 
(also?) immutable from the driver context, which I didn't test separately.

Removed the immutable again and moved the loop.

>>>
>>>> +
>>>>       /*
>>>>        * Defer the cleanup of the old state to a separate worker to 
>>>> not
>>>>        * impede the current task (userspace for blocking modesets) 
>>>> that
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
>>>> b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> index 642c60f3d9b1..67826ba976ed 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> @@ -4671,10 +4671,14 @@ 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_bpc_property(connector, 6, 10);
>>>> +    }
>>>> +    else if (DISPLAY_VER(dev_priv) >= 5) {
>>>>           drm_connector_attach_max_bpc_property(connector, 6, 12);
>>>> +        drm_connector_attach_active_bpc_property(connector, 6, 12);
>>>> +    }
>>>>         /* Register HDMI colorspace for case of lspcon */
>>>>       if (intel_bios_is_lspcon_present(dev_priv, port)) {
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c 
>>>> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>> index 2daa3f67791e..5a1869dc2210 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>> @@ -844,8 +844,10 @@ static struct drm_connector 
>>>> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>>>>        */
>>>>       connector->max_bpc_property =
>>>> intel_dp->attached_connector->base.max_bpc_property;
>>>> -    if (connector->max_bpc_property)
>>>> +    if (connector->max_bpc_property) {
>>>>           drm_connector_attach_max_bpc_property(connector, 6, 12);
>>>> +        drm_connector_attach_active_bpc_property(connector, 6, 12);
>>>> +    }
>>>>         return connector;
>>>>   diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
>>>> b/drivers/gpu/drm/i915/display/intel_hdmi.c
>>>> index d69f0a6dc26d..8af78b27b6ce 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
>>>> @@ -2463,8 +2463,10 @@ intel_hdmi_add_properties(struct intel_hdmi 
>>>> *intel_hdmi, struct drm_connector *c
>>>> drm_object_attach_property(&connector->base,
>>>> connector->dev->mode_config.hdr_output_metadata_property, 0);
>>>>   -    if (!HAS_GMCH(dev_priv))
>>>> +    if (!HAS_GMCH(dev_priv)) {
>>>>           drm_connector_attach_max_bpc_property(connector, 8, 12);
>>>> +        drm_connector_attach_active_bpc_property(connector, 8, 12);
>>>> +    }
>>>>   }
>>>>     /*
>>>> -- 
>>>> 2.25.1
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] drm/i915/display: Add handling for new "active bpc" property

[Ville Syrjälä]

On Tue, Jun 08, 2021 at 07:19:31PM +0200, Werner Sembach wrote:
> 
> Am 07.06.21 um 22:33 schrieb Werner Sembach:
> > Am 07.06.21 um 08:47 schrieb Werner Sembach:
> >>
> >> Am 04.06.21 um 19:30 schrieb Ville Syrjälä:
> >>> On Fri, Jun 04, 2021 at 07:17:23PM +0200, Werner Sembach wrote:
> >>>> This commits implements the "active bpc" drm property for the Intel 
> >>>> GPU driver.
> >>>>
> >>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> >>>> ---
> >>>>   drivers/gpu/drm/i915/display/intel_display.c | 13 +++++++++++++
> >>>>   drivers/gpu/drm/i915/display/intel_dp.c      |  8 ++++++--
> >>>>   drivers/gpu/drm/i915/display/intel_dp_mst.c  |  4 +++-
> >>>>   drivers/gpu/drm/i915/display/intel_hdmi.c    |  4 +++-
> >>>>   4 files changed, 25 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> >>>> b/drivers/gpu/drm/i915/display/intel_display.c
> >>>> index 64e9107d70f7..f7898d9d7438 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >>>> @@ -10164,6 +10164,8 @@ static void intel_atomic_commit_tail(struct 
> >>>> intel_atomic_state *state)
> >>>>       struct drm_i915_private *dev_priv = to_i915(dev);
> >>>>       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> >>>>       struct intel_crtc *crtc;
> >>>> +    struct drm_connector *connector;
> >>>> +    struct drm_connector_state *new_conn_state;
> >>>>       u64 put_domains[I915_MAX_PIPES] = {};
> >>>>       intel_wakeref_t wakeref = 0;
> >>>>       int i;
> >>>> @@ -10324,6 +10326,17 @@ static void 
> >>>> intel_atomic_commit_tail(struct intel_atomic_state *state)
> >>>>       }
> >>>>       intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
> >>>>   +    /* Extract information from crtc to communicate it to 
> >>>> userspace as connector properties */
> >>>> +    for_each_new_connector_in_state(&state->base, connector, 
> >>>> new_conn_state, i) {
> >>>> +        struct drm_crtc *crtc = new_conn_state->crtc;
> >>>> +        if (crtc) {
> >>>> +            new_crtc_state = 
> >>>> to_intel_crtc_state(drm_atomic_get_new_crtc_state(&state->base, 
> >>>> crtc));
> >>> intel_atomic_get_new_crtc_state()
> >> Thanks, will use that.
> >>>
> >>>> + new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> >>>> +        }
> >>>> +        else
> >>>> +            new_conn_state->active_bpc = 0;
> >>>> +    }
> >>> This also seems too late. I think the whole thing should be
> >>> done somewhere around the normal swap_state() stuff.
> >> Ok, will look into it.
> > So I tried to put it in intel_atomic_commit() after 
> > drm_atomic_helper_swap_state() and before 
> > INIT_WORK(&state->base.commit_work, intel_atomic_commit_work) (which 
> > creates a worker for intel_atomic_commit_tail), but somewhere in 
> > between, the connector_state seems to change: The bpc written with the 
> > for_each_new_connector_in_state() loop, gets discarded.
> 
> This was not the problem. Setting the drm property immutable made it 
> (also?) immutable from the driver context, which I didn't test separately.
> 
> Removed the immutable again and moved the loop.

Immutable props are special. See __drm_object_property_get_value().

-- 
Ville Syrjälä
Intel