[PATCH v2] drm/connector: hdmi: Add a 'link bpc' property

[PATCH v2] drm/connector: hdmi: Add a 'link bpc' property

[Marius Vlad]

From: Derek Foreman <derek.foreman@collabora.com>

Add a way to know the actual bpc of a running link.

Drivers might change the current bpc link value due to changes in mode
line or refresh rates. For example when enabling VRR the underlying
hardware might not be able sustain the same bandwidth for a particular
mode line, and it might attempt to lower the bpc. Another example can be
found when switching the color output format, part of YUV420 fallback.

This means we might be displaying a stale bpc value although it was
modified for different reasons -- like a refresh rate or an output
color format.

Introduces a new property 'link bpc' that user-space can use to get the
current bpc value of a running link while in the same time allow
user-space to set-up bpc using 'max bpc' property.

An implementation for Weston [1] and a simple test for i-g-t [2] have been added.

Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>

[1] https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850
[2] https://lists.freedesktop.org/archives/igt-dev/2025-October/097061.html

---

v1: 
- https://lore.kernel.org/dri-devel/20250801101750.1726-1-marius.vlad@collabora.com/T/#u

v2: 
- replace return with EBUSY if connector already exists (Dmitry)
- add i-g-t test and an implementation for Weston (Dmitry)
- re-wording patch description (Jani)
    

 drivers/gpu/drm/drm_atomic_uapi.c |  5 +++++
 drivers/gpu/drm/drm_connector.c   | 25 +++++++++++++++++++++++++
 include/drm/drm_connector.h       |  8 ++++++++
 3 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 85dbdaa4a2e2..15c5ad7ddfb5 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -776,6 +776,9 @@ 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->link_bpc_property) {
+		drm_dbg_kms(dev, "only drivers can set link bpc property. Use max bpc instead\n");
+		return -EINVAL;
 	} else if (property == connector->privacy_screen_sw_state_property) {
 		state->privacy_screen_sw_state = val;
 	} else if (property == connector->broadcast_rgb_property) {
@@ -861,6 +864,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->link_bpc_property) {
+		*val = state->hdmi.output_bpc;
 	} else if (property == connector->privacy_screen_sw_state_property) {
 		*val = state->privacy_screen_sw_state;
 	} else if (property == connector->broadcast_rgb_property) {
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 272d6254ea47..7cc99cd16e20 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -542,6 +542,27 @@ int drmm_connector_init(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drmm_connector_init);
 
+static int
+drm_connector_attach_link_bpc_property(struct drm_connector *connector,
+				       int max)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	if (connector->link_bpc_property)
+		return -EBUSY;
+
+	prop = drm_property_create_range(dev, 0, "link bpc", 8, max);
+	if (!prop)
+		return -ENOMEM;
+
+	connector->link_bpc_property = prop;
+
+	drm_object_attach_property(&connector->base, prop, max);
+
+	return 0;
+}
+
 /**
  * drmm_connector_hdmi_init - Init a preallocated HDMI connector
  * @dev: DRM device
@@ -618,6 +639,10 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
 	drm_connector_attach_max_bpc_property(connector, 8, max_bpc);
 	connector->max_bpc = max_bpc;
 
+	ret = drm_connector_attach_link_bpc_property(connector, max_bpc);
+	if (ret)
+		return ret;
+
 	if (max_bpc > 8)
 		drm_connector_attach_hdr_output_metadata_property(connector);
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 8f34f4b8183d..4a50198aa7c0 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2079,6 +2079,14 @@ struct drm_connector {
 	 */
 	struct drm_property *max_bpc_property;
 
+	/**
+	 * @link_bpc_property: Current connector link bpc set by the driver
+	 *
+	 * This property can be used to retrieve the current link bpc from
+	 * connector_state::hdmi:output_bpc
+	 */
+	struct drm_property *link_bpc_property;
+
 	/** @privacy_screen: drm_privacy_screen for this connector, or NULL. */
 	struct drm_privacy_screen *privacy_screen;
 
-- 
2.47.2

Re: [PATCH v2] drm/connector: hdmi: Add a 'link bpc' property

[Ville Syrjälä]

On Mon, Oct 06, 2025 at 11:30:43AM +0300, Marius Vlad wrote:
> From: Derek Foreman <derek.foreman@collabora.com>
> 
> Add a way to know the actual bpc of a running link.
> 
> Drivers might change the current bpc link value due to changes in mode
> line or refresh rates. For example when enabling VRR the underlying
> hardware might not be able sustain the same bandwidth for a particular
> mode line, and it might attempt to lower the bpc.

Not sure what this VRR stuff is trying to say. Enabling VRR doesn't
itself change anything about the link bandwidth.

> Another example can be
> found when switching the color output format, part of YUV420 fallback.
> 
> This means we might be displaying a stale bpc value although it was
> modified for different reasons -- like a refresh rate or an output
> color format.
> 
> Introduces a new property 'link bpc' that user-space can use to get the
> current bpc value of a running link while in the same time allow
> user-space to set-up bpc using 'max bpc' property.
> 
> An implementation for Weston [1] and a simple test for i-g-t [2] have been added.
> 
> Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> 
> [1] https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850
> [2] https://lists.freedesktop.org/archives/igt-dev/2025-October/097061.html
> 
> ---
> 
> v1: 
> - https://lore.kernel.org/dri-devel/20250801101750.1726-1-marius.vlad@collabora.com/T/#u
> 
> v2: 
> - replace return with EBUSY if connector already exists (Dmitry)
> - add i-g-t test and an implementation for Weston (Dmitry)
> - re-wording patch description (Jani)
>     
> 
>  drivers/gpu/drm/drm_atomic_uapi.c |  5 +++++
>  drivers/gpu/drm/drm_connector.c   | 25 +++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  8 ++++++++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 85dbdaa4a2e2..15c5ad7ddfb5 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -776,6 +776,9 @@ 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->link_bpc_property) {
> +		drm_dbg_kms(dev, "only drivers can set link bpc property. Use max bpc instead\n");
> +		return -EINVAL;

Is there a particular reason this isn't just an immutable prop?

>  	} else if (property == connector->privacy_screen_sw_state_property) {
>  		state->privacy_screen_sw_state = val;
>  	} else if (property == connector->broadcast_rgb_property) {
> @@ -861,6 +864,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->link_bpc_property) {
> +		*val = state->hdmi.output_bpc;

Assuming hdmi here doesn't seem good. What about all the other
connector types?

>  	} else if (property == connector->privacy_screen_sw_state_property) {
>  		*val = state->privacy_screen_sw_state;
>  	} else if (property == connector->broadcast_rgb_property) {
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 272d6254ea47..7cc99cd16e20 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -542,6 +542,27 @@ int drmm_connector_init(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drmm_connector_init);
>  
> +static int
> +drm_connector_attach_link_bpc_property(struct drm_connector *connector,
> +				       int max)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	if (connector->link_bpc_property)
> +		return -EBUSY;
> +
> +	prop = drm_property_create_range(dev, 0, "link bpc", 8, max);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	connector->link_bpc_property = prop;
> +
> +	drm_object_attach_property(&connector->base, prop, max);
> +
> +	return 0;
> +}
> +
>  /**
>   * drmm_connector_hdmi_init - Init a preallocated HDMI connector
>   * @dev: DRM device
> @@ -618,6 +639,10 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  	drm_connector_attach_max_bpc_property(connector, 8, max_bpc);
>  	connector->max_bpc = max_bpc;
>  
> +	ret = drm_connector_attach_link_bpc_property(connector, max_bpc);
> +	if (ret)
> +		return ret;
> +
>  	if (max_bpc > 8)
>  		drm_connector_attach_hdr_output_metadata_property(connector);
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 8f34f4b8183d..4a50198aa7c0 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -2079,6 +2079,14 @@ struct drm_connector {
>  	 */
>  	struct drm_property *max_bpc_property;
>  
> +	/**
> +	 * @link_bpc_property: Current connector link bpc set by the driver
> +	 *
> +	 * This property can be used to retrieve the current link bpc from
> +	 * connector_state::hdmi:output_bpc
> +	 */
> +	struct drm_property *link_bpc_property;
> +
>  	/** @privacy_screen: drm_privacy_screen for this connector, or NULL. */
>  	struct drm_privacy_screen *privacy_screen;
>  
> -- 
> 2.47.2

-- 
Ville Syrjälä
Intel

Re: [PATCH v2] drm/connector: hdmi: Add a 'link bpc' property

[Marius Vlad]

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

Hi Ville,
On Mon, Oct 06, 2025 at 12:23:31PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 06, 2025 at 11:30:43AM +0300, Marius Vlad wrote:
> > From: Derek Foreman <derek.foreman@collabora.com>
> > 
> > Add a way to know the actual bpc of a running link.
> > 
> > Drivers might change the current bpc link value due to changes in mode
> > line or refresh rates. For example when enabling VRR the underlying
> > hardware might not be able sustain the same bandwidth for a particular
> > mode line, and it might attempt to lower the bpc.
> 
> Not sure what this VRR stuff is trying to say. Enabling VRR doesn't
> itself change anything about the link bandwidth.
> 
Any time a modeset happens which involves setting up 'max bpc' might
result in downgrading bpc in an attempt find an optimal output.

VRR by itself won't do that, neither other possible components or
blocks, but it might have an affect on it, like a modeset that requires
a higher refresh rate which can not be done with the currently set bpc.

Does this feel like it is more to your liking in terms of explaining the
VRR implication or should I just drop VRR entirely?

> > Another example can be
> > found when switching the color output format, part of YUV420 fallback.
> > 
> > This means we might be displaying a stale bpc value although it was
> > modified for different reasons -- like a refresh rate or an output
> > color format.
> > 
> > Introduces a new property 'link bpc' that user-space can use to get the
> > current bpc value of a running link while in the same time allow
> > user-space to set-up bpc using 'max bpc' property.
> > 
> > An implementation for Weston [1] and a simple test for i-g-t [2] have been added.
> > 
> > Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > 
> > [1] https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850
> > [2] https://lists.freedesktop.org/archives/igt-dev/2025-October/097061.html
> > 
> > ---
> > 
> > v1: 
> > - https://lore.kernel.org/dri-devel/20250801101750.1726-1-marius.vlad@collabora.com/T/#u
> > 
> > v2: 
> > - replace return with EBUSY if connector already exists (Dmitry)
> > - add i-g-t test and an implementation for Weston (Dmitry)
> > - re-wording patch description (Jani)
> >     
> > 
> >  drivers/gpu/drm/drm_atomic_uapi.c |  5 +++++
> >  drivers/gpu/drm/drm_connector.c   | 25 +++++++++++++++++++++++++
> >  include/drm/drm_connector.h       |  8 ++++++++
> >  3 files changed, 38 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 85dbdaa4a2e2..15c5ad7ddfb5 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -776,6 +776,9 @@ 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->link_bpc_property) {
> > +		drm_dbg_kms(dev, "only drivers can set link bpc property. Use max bpc instead\n");
> > +		return -EINVAL;
> 
> Is there a particular reason this isn't just an immutable prop?
Did tried passing DRM_MODE_PROP_IMMUTABLE here, but DRM UAPI will not go through 
drm_atomic_connector_get_property() hence dropping the flag (it goes
through __drm_object_property_get_prop_value() from what I can tell) and
with that I don't see the value being updated.
> 
> >  	} else if (property == connector->privacy_screen_sw_state_property) {
> >  		state->privacy_screen_sw_state = val;
> >  	} else if (property == connector->broadcast_rgb_property) {
> > @@ -861,6 +864,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->link_bpc_property) {
> > +		*val = state->hdmi.output_bpc;
> 
> Assuming hdmi here doesn't seem good. What about all the other
> connector types?
Right, Jani raised this as well. I don't have a good answer here really.
I'm using what I have in the tree at the moment.

On one side this would only be for HDMI type of connectors, and on
another side only drivers that actually use the helpers would gain
access to the property.

When adding support for this Maxime even mentioned that i915/vc4 was using a
similar algorithm (https://patchwork.freedesktop.org/patch/595684/).
Itself this patch doesn't even touch that but I gather it does raises a
few eyebrows as this is strictly for HDMI. I imagined there might be
reason for just doing this for HDMI but tbh I haven't really followed up
on that.

Do I get that you'd like see this happening for other types of connectors? How
would that go?

Would following a similar path we have now in the tree for to the
broadcast_rgb be something you'd see as reasonable? I see that i915
hooks its own get_property (intel_digital_connector_atomic_get_property()) 
which I wasn't aware until now.

> 
> >  	} else if (property == connector->privacy_screen_sw_state_property) {
> >  		*val = state->privacy_screen_sw_state;
> >  	} else if (property == connector->broadcast_rgb_property) {
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 272d6254ea47..7cc99cd16e20 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -542,6 +542,27 @@ int drmm_connector_init(struct drm_device *dev,
> >  }
> >  EXPORT_SYMBOL(drmm_connector_init);
> >  
> > +static int
> > +drm_connector_attach_link_bpc_property(struct drm_connector *connector,
> > +				       int max)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_property *prop;
> > +
> > +	if (connector->link_bpc_property)
> > +		return -EBUSY;
> > +
> > +	prop = drm_property_create_range(dev, 0, "link bpc", 8, max);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +
> > +	connector->link_bpc_property = prop;
> > +
> > +	drm_object_attach_property(&connector->base, prop, max);
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * drmm_connector_hdmi_init - Init a preallocated HDMI connector
> >   * @dev: DRM device
> > @@ -618,6 +639,10 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> >  	drm_connector_attach_max_bpc_property(connector, 8, max_bpc);
> >  	connector->max_bpc = max_bpc;
> >  
> > +	ret = drm_connector_attach_link_bpc_property(connector, max_bpc);
> > +	if (ret)
> > +		return ret;
> > +
> >  	if (max_bpc > 8)
> >  		drm_connector_attach_hdr_output_metadata_property(connector);
> >  
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 8f34f4b8183d..4a50198aa7c0 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -2079,6 +2079,14 @@ struct drm_connector {
> >  	 */
> >  	struct drm_property *max_bpc_property;
> >  
> > +	/**
> > +	 * @link_bpc_property: Current connector link bpc set by the driver
> > +	 *
> > +	 * This property can be used to retrieve the current link bpc from
> > +	 * connector_state::hdmi:output_bpc
> > +	 */
> > +	struct drm_property *link_bpc_property;
> > +
> >  	/** @privacy_screen: drm_privacy_screen for this connector, or NULL. */
> >  	struct drm_privacy_screen *privacy_screen;
> >  
> > -- 
> > 2.47.2
> 
> -- 
> Ville Syrjälä
> Intel

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

Re: [PATCH v2] drm/connector: hdmi: Add a 'link bpc' property

[Ville Syrjälä]

On Mon, Oct 06, 2025 at 06:03:20PM +0300, Marius Vlad wrote:
> Hi Ville,
> On Mon, Oct 06, 2025 at 12:23:31PM +0300, Ville Syrjälä wrote:
> > On Mon, Oct 06, 2025 at 11:30:43AM +0300, Marius Vlad wrote:
> > > From: Derek Foreman <derek.foreman@collabora.com>
> > > 
> > > Add a way to know the actual bpc of a running link.
> > > 
> > > Drivers might change the current bpc link value due to changes in mode
> > > line or refresh rates. For example when enabling VRR the underlying
> > > hardware might not be able sustain the same bandwidth for a particular
> > > mode line, and it might attempt to lower the bpc.
> > 
> > Not sure what this VRR stuff is trying to say. Enabling VRR doesn't
> > itself change anything about the link bandwidth.
> > 
> Any time a modeset happens which involves setting up 'max bpc' might
> result in downgrading bpc in an attempt find an optimal output.
> 
> VRR by itself won't do that, neither other possible components or
> blocks, but it might have an affect on it, like a modeset that requires
> a higher refresh rate which can not be done with the currently set bpc.
> 
> Does this feel like it is more to your liking in terms of explaining the
> VRR implication or should I just drop VRR entirely?

I think mentioning VRR is just confusing. The point is simply that link
bandwidth limits (or in fact other internal hardware limits) may require
the link color depth to be reduced.

And BTW this reported bpc value may not even tell you anything about
the effective color depth achieved by the entire pipeline. There maybe
be parts of the pipeline prior to the output link that have a lower
color depth. On Intel hardware I think we could technically keep the
output link running at eg. 12bpc even if there's a 6bpc restriction
upstream of it. And at that point the discussion would probably turn
to dithering, but I won't open that can of worms here, other than
saying that we it very poorly.

> 
> > > Another example can be
> > > found when switching the color output format, part of YUV420 fallback.
> > > 
> > > This means we might be displaying a stale bpc value although it was
> > > modified for different reasons -- like a refresh rate or an output
> > > color format.
> > > 
> > > Introduces a new property 'link bpc' that user-space can use to get the
> > > current bpc value of a running link while in the same time allow
> > > user-space to set-up bpc using 'max bpc' property.
> > > 
> > > An implementation for Weston [1] and a simple test for i-g-t [2] have been added.
> > > 
> > > Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> > > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > > 
> > > [1] https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850
> > > [2] https://lists.freedesktop.org/archives/igt-dev/2025-October/097061.html
> > > 
> > > ---
> > > 
> > > v1: 
> > > - https://lore.kernel.org/dri-devel/20250801101750.1726-1-marius.vlad@collabora.com/T/#u
> > > 
> > > v2: 
> > > - replace return with EBUSY if connector already exists (Dmitry)
> > > - add i-g-t test and an implementation for Weston (Dmitry)
> > > - re-wording patch description (Jani)
> > >     
> > > 
> > >  drivers/gpu/drm/drm_atomic_uapi.c |  5 +++++
> > >  drivers/gpu/drm/drm_connector.c   | 25 +++++++++++++++++++++++++
> > >  include/drm/drm_connector.h       |  8 ++++++++
> > >  3 files changed, 38 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index 85dbdaa4a2e2..15c5ad7ddfb5 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -776,6 +776,9 @@ 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->link_bpc_property) {
> > > +		drm_dbg_kms(dev, "only drivers can set link bpc property. Use max bpc instead\n");
> > > +		return -EINVAL;
> > 
> > Is there a particular reason this isn't just an immutable prop?
> Did tried passing DRM_MODE_PROP_IMMUTABLE here, but DRM UAPI will not go through 
> drm_atomic_connector_get_property() hence dropping the flag (it goes
> through __drm_object_property_get_prop_value() from what I can tell) and
> with that I don't see the value being updated.

It will have to be updated by calling the appropriate function.

> > 
> > >  	} else if (property == connector->privacy_screen_sw_state_property) {
> > >  		state->privacy_screen_sw_state = val;
> > >  	} else if (property == connector->broadcast_rgb_property) {
> > > @@ -861,6 +864,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->link_bpc_property) {
> > > +		*val = state->hdmi.output_bpc;
> > 
> > Assuming hdmi here doesn't seem good. What about all the other
> > connector types?
> Right, Jani raised this as well. I don't have a good answer here really.
> I'm using what I have in the tree at the moment.
> 
> On one side this would only be for HDMI type of connectors, and on
> another side only drivers that actually use the helpers would gain
> access to the property.
> 
> When adding support for this Maxime even mentioned that i915/vc4 was using a
> similar algorithm (https://patchwork.freedesktop.org/patch/595684/).
> Itself this patch doesn't even touch that but I gather it does raises a
> few eyebrows as this is strictly for HDMI. I imagined there might be
> reason for just doing this for HDMI but tbh I haven't really followed up
> on that.

The link bpc is much more fluid on DP than on HDMI. So I'd say
DP is more important than HDMI.

Though as mentioned the link bpc doesn't actually tell you anything
about the effecive bpc of the whole pipeline, so whether it's
actually useful for anything real is debatable.

> 
> Do I get that you'd like see this happening for other types of connectors? How
> would that go?
> 
> Would following a similar path we have now in the tree for to the
> broadcast_rgb be something you'd see as reasonable? I see that i915
> hooks its own get_property (intel_digital_connector_atomic_get_property()) 
> which I wasn't aware until now.

I think it should just be an immutable property since you can't
set it anyway. The "immutable" name is pretty bad in fact. What
that flag actually means is the value can't be set by userspace,
but can still change dynamically.

> 
> > 
> > >  	} else if (property == connector->privacy_screen_sw_state_property) {
> > >  		*val = state->privacy_screen_sw_state;
> > >  	} else if (property == connector->broadcast_rgb_property) {
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 272d6254ea47..7cc99cd16e20 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -542,6 +542,27 @@ int drmm_connector_init(struct drm_device *dev,
> > >  }
> > >  EXPORT_SYMBOL(drmm_connector_init);
> > >  
> > > +static int
> > > +drm_connector_attach_link_bpc_property(struct drm_connector *connector,
> > > +				       int max)
> > > +{
> > > +	struct drm_device *dev = connector->dev;
> > > +	struct drm_property *prop;
> > > +
> > > +	if (connector->link_bpc_property)
> > > +		return -EBUSY;
> > > +
> > > +	prop = drm_property_create_range(dev, 0, "link bpc", 8, max);
> > > +	if (!prop)
> > > +		return -ENOMEM;
> > > +
> > > +	connector->link_bpc_property = prop;
> > > +
> > > +	drm_object_attach_property(&connector->base, prop, max);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * drmm_connector_hdmi_init - Init a preallocated HDMI connector
> > >   * @dev: DRM device
> > > @@ -618,6 +639,10 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> > >  	drm_connector_attach_max_bpc_property(connector, 8, max_bpc);
> > >  	connector->max_bpc = max_bpc;
> > >  
> > > +	ret = drm_connector_attach_link_bpc_property(connector, max_bpc);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	if (max_bpc > 8)
> > >  		drm_connector_attach_hdr_output_metadata_property(connector);
> > >  
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 8f34f4b8183d..4a50198aa7c0 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -2079,6 +2079,14 @@ struct drm_connector {
> > >  	 */
> > >  	struct drm_property *max_bpc_property;
> > >  
> > > +	/**
> > > +	 * @link_bpc_property: Current connector link bpc set by the driver
> > > +	 *
> > > +	 * This property can be used to retrieve the current link bpc from
> > > +	 * connector_state::hdmi:output_bpc
> > > +	 */
> > > +	struct drm_property *link_bpc_property;
> > > +
> > >  	/** @privacy_screen: drm_privacy_screen for this connector, or NULL. */
> > >  	struct drm_privacy_screen *privacy_screen;
> > >  
> > > -- 
> > > 2.47.2
> > 
> > -- 
> > Ville Syrjälä
> > Intel



-- 
Ville Syrjälä
Intel