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

[Marius Vlad <marius.vlad@collabora.com>]

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

Hi Dmitry,
On Fri, Aug 01, 2025 at 05:09:06PM +0300, Dmitry Baryshkov wrote:
> On Fri, Aug 01, 2025 at 01:17:50PM +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. 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.
> > 
> > This patch introduces a new property 'link bpc' that user-space can
> > use to get the current bpc value of a running link. In the same
> > time this would allow user-space set up bpc using 'max_bpc' property.
> 
> Could you please point out the userspace implementation which uses this
> property?
I'll be adding a MR for Weston for retriving this property. It will compare
it with 'max bpc' and inform the users that we've noticed a link change.
> 
> > 
> > Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  5 +++++
> >  drivers/gpu/drm/drm_connector.c   | 26 ++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       |  8 ++++++++
> >  3 files changed, 39 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index ecc73d52bfae..3a2ffb957ade 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..7ed27aec0ccc 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -542,6 +542,28 @@ 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 min, int max)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_property *prop;
> > +
> > +	prop = connector->link_bpc_property;
> > +	if (prop)
> > +		return 0;
> 
> Shouldn't it be:
> 
> if (connector->link_bpc_property)
> 	return -EBUSY;
Yeah.
> 
> > +
> > +	prop = drm_property_create_range(dev, 0, "link bpc", min, 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 +640,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, 8, max_bpc);
> > +	if (ret)
> > +		return ret;
> 
> Is there a code which sets this property?
Hmm, the idea is/was that userspace will just read out this property and
compare with the 'max bpc' one. In my mind it should be immutable. Is
that what you're implying here?
> 
> > +
> >  	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
> > 
> 
> -- 
> With best wishes
> Dmitry

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