Re: [PATCH v8 02/39] drm/connector: Add caps-based HDMI connector init helper

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

On 7/22/26 9:46 AM, Dmitry Baryshkov wrote:
> On Wed, Jul 15, 2026 at 01:34:19PM +0300, Cristian Ciocaltea wrote:
>> On 7/15/26 11:50 AM, Maxime Ripard wrote:
>>> On Fri, Jul 10, 2026 at 01:27:37PM +0300, Cristian Ciocaltea wrote:
>>>> On 7/8/26 1:11 PM, Cristian Ciocaltea wrote:
>>>>> Hi Maxime,
>>>>>
>>>>> On 7/7/26 7:10 PM, Maxime Ripard wrote:
>>>>>> On Fri, Jul 03, 2026 at 10:31:55PM +0300, Cristian Ciocaltea wrote:
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> Thanks for your quick review!
>>>>>>>
>>>>>>> On 7/3/26 5:05 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Thu, Jul 02, 2026 at 05:46:15PM +0300, Cristian Ciocaltea wrote:
>>>>>>>>> In preparation for adding HDMI 2.x source capabilities, introduce struct
>>>>>>>>> drm_connector_hdmi_caps and a new drmm_connector_hdmi_init_with_caps()
>>>>>>>>> helper.
>>>>>>>>>
>>>>>>>>> The existing drmm_connector_hdmi_init() helper currently takes
>>>>>>>>> individual capability arguments such as supported_formats and max_bpc.
>>>>>>>>> Adding more HDMI-specific arguments to that function would not scale
>>>>>>>>> well, so move those values into a dedicated capabilities structure and
>>>>>>>>> implement the existing helper as a wrapper around the new caps-based
>>>>>>>>> interface.
>>>>>>>>
>>>>>>>> I think, it was an intention of Maxime: make sure that every driver is
>>>>>>>> forced to provide some values here. With the struct-based init it is
>>>>>>>> easy to overlook or to ommit a value.
>>>>>>>
>>>>>>> Agreed that the struct-based init loses the compile-time guarantee that every
>>>>>>> argument is explicitly provided - that's a real downside.  
>>>>>>>
>>>>>>> I'd argue it's recoverable, though: the init helper validates the mandatory
>>>>>>> fields, so a driver that omits a required value gets rejected at init time
>>>>>>> rather than silently misconfigured.  The "you must provide sane values" property
>>>>>>> is expected to be preserved, just enforced at runtime instead of by the
>>>>>>> compiler. 
>>>>>>
>>>>>> Yeah, I don't think we can win with C here. Rust might, but we're
>>>>>> probably a long way from that.
>>>>>>
>>>>>>> The main motivation for the struct is scalability/maintainability as we add HDMI
>>>>>>> 2.x capabilities: new fields go into the struct rather than growing the helper's
>>>>>>> argument list, so existing callers don't need churny signature updates on every
>>>>>>> extension.
>>>>>>>
>>>>>>> FWIW, in the previous revision we discussed addressing the concern with a
>>>>>>> callback instead.  Sadly, I had to discard that approach, as it proved not
>>>>>>> flexible enough, e.g. drm_bridge_connector_init() computes caps dynamically, and
>>>>>>> would have required either stateful callbacks, or storing redundant/temporary
>>>>>>> cap data in driver-private structures just to satisfy the callback.
>>>>>>
>>>>>> I just realized something reviewing your patch: we don't necessarily
>>>>>> need an extra argument or a callback, we can just put these fields into
>>>>>> drm_hdmi_connector_funcs directly, and then validate them in init.
>>>>>
>>>>> If I understand correctly, we should drop the drm_connector_hdmi_caps struct
>>>>> introduced by this patch and move all its fields into drm_hdmi_connector_funcs.
>>>>>
>>>>> In that case, how should we proceed with drmm_connector_hdmi_init()? 
>>>>
>>>> Actually, this brings us to the callback issue: we cannot compute caps
>>>> dynamically, as it only works with static data, since funcs is supposed to be
>>>> immutable.
>>>
>>> Does it? The core and helpers must consider it immutable but it doesn't
>>> have to. drm_bridge_connector for example could totally allocate it and
>>> dynamically create it based on the bridge capabilities.
>>
>> If we take the VC4 case, is it fine to drop the const from the static
>> drm_connector_hdmi_funcs to allow dynamically setting up supported_hdmi_ver and
>> max_bpc in vc4_hdmi_connector_init()?
>>
>> static struct drm_connector_hdmi_funcs vc4_hdmi_hdmi_connector_funcs = {
>> 	.tmds_char_rate_valid	 = vc4_hdmi_connector_clock_valid,
>> 	...
>> }
>>
>> static int vc4_hdmi_connector_init() 
>> {
>> 	...
>>     
>> 	if (vc4_hdmi->variant->supports_hdr)
>> 		vc4_hdmi_hdmi_connector_funcs.max_bpc = 12;
>>
>> 	if (vc4_hdmi->variant->max_pixel_clock >= HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ)
>> 		vc4_hdmi_hdmi_connector_funcs.supported_hdmi_ver = HDMI_VERSION_2_0;
>> 	else if (vc4_hdmi->variant->max_pixel_clock >= HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ)
>> 		vc4_hdmi_hdmi_connector_funcs.supported_hdmi_ver = HDMI_VERSION_1_3;
>> 	...
>> }
> 
> I'm sorry for the late response, I was OoO. Why do we want to put data
> into the funcs part? My suggestion would be to put the data into the
> drm_connector_hdmi itself.
> 
> 
> Setting connector->hdmi.supported_hdmi_ver = HDMI_VERSION_1_4; is more
> idiomatic than passing it through the funcs.
On 7/22/26 9:46 AM, Dmitry Baryshkov wrote:
> On Wed, Jul 15, 2026 at 01:34:19PM +0300, Cristian Ciocaltea wrote:
>> On 7/15/26 11:50 AM, Maxime Ripard wrote:
>>> On Fri, Jul 10, 2026 at 01:27:37PM +0300, Cristian Ciocaltea wrote:
>>>> On 7/8/26 1:11 PM, Cristian Ciocaltea wrote:
>>>>> Hi Maxime,
>>>>>
>>>>> On 7/7/26 7:10 PM, Maxime Ripard wrote:
>>>>>> On Fri, Jul 03, 2026 at 10:31:55PM +0300, Cristian Ciocaltea wrote:
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> Thanks for your quick review!
>>>>>>>
>>>>>>> On 7/3/26 5:05 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Thu, Jul 02, 2026 at 05:46:15PM +0300, Cristian Ciocaltea wrote:
>>>>>>>>> In preparation for adding HDMI 2.x source capabilities, introduce struct
>>>>>>>>> drm_connector_hdmi_caps and a new drmm_connector_hdmi_init_with_caps()
>>>>>>>>> helper.
>>>>>>>>>
>>>>>>>>> The existing drmm_connector_hdmi_init() helper currently takes
>>>>>>>>> individual capability arguments such as supported_formats and max_bpc.
>>>>>>>>> Adding more HDMI-specific arguments to that function would not scale
>>>>>>>>> well, so move those values into a dedicated capabilities structure and
>>>>>>>>> implement the existing helper as a wrapper around the new caps-based
>>>>>>>>> interface.
>>>>>>>>
>>>>>>>> I think, it was an intention of Maxime: make sure that every driver is
>>>>>>>> forced to provide some values here. With the struct-based init it is
>>>>>>>> easy to overlook or to ommit a value.
>>>>>>>
>>>>>>> Agreed that the struct-based init loses the compile-time guarantee that every
>>>>>>> argument is explicitly provided - that's a real downside.  
>>>>>>>
>>>>>>> I'd argue it's recoverable, though: the init helper validates the mandatory
>>>>>>> fields, so a driver that omits a required value gets rejected at init time
>>>>>>> rather than silently misconfigured.  The "you must provide sane values" property
>>>>>>> is expected to be preserved, just enforced at runtime instead of by the
>>>>>>> compiler. 
>>>>>>
>>>>>> Yeah, I don't think we can win with C here. Rust might, but we're
>>>>>> probably a long way from that.
>>>>>>
>>>>>>> The main motivation for the struct is scalability/maintainability as we add HDMI
>>>>>>> 2.x capabilities: new fields go into the struct rather than growing the helper's
>>>>>>> argument list, so existing callers don't need churny signature updates on every
>>>>>>> extension.
>>>>>>>
>>>>>>> FWIW, in the previous revision we discussed addressing the concern with a
>>>>>>> callback instead.  Sadly, I had to discard that approach, as it proved not
>>>>>>> flexible enough, e.g. drm_bridge_connector_init() computes caps dynamically, and
>>>>>>> would have required either stateful callbacks, or storing redundant/temporary
>>>>>>> cap data in driver-private structures just to satisfy the callback.
>>>>>>
>>>>>> I just realized something reviewing your patch: we don't necessarily
>>>>>> need an extra argument or a callback, we can just put these fields into
>>>>>> drm_hdmi_connector_funcs directly, and then validate them in init.
>>>>>
>>>>> If I understand correctly, we should drop the drm_connector_hdmi_caps struct
>>>>> introduced by this patch and move all its fields into drm_hdmi_connector_funcs.
>>>>>
>>>>> In that case, how should we proceed with drmm_connector_hdmi_init()? 
>>>>
>>>> Actually, this brings us to the callback issue: we cannot compute caps
>>>> dynamically, as it only works with static data, since funcs is supposed to be
>>>> immutable.
>>>
>>> Does it? The core and helpers must consider it immutable but it doesn't
>>> have to. drm_bridge_connector for example could totally allocate it and
>>> dynamically create it based on the bridge capabilities.
>>
>> If we take the VC4 case, is it fine to drop the const from the static
>> drm_connector_hdmi_funcs to allow dynamically setting up supported_hdmi_ver and
>> max_bpc in vc4_hdmi_connector_init()?
>>
>> static struct drm_connector_hdmi_funcs vc4_hdmi_hdmi_connector_funcs = {
>> 	.tmds_char_rate_valid	 = vc4_hdmi_connector_clock_valid,
>> 	...
>> }
>>
>> static int vc4_hdmi_connector_init() 
>> {
>> 	...
>>     
>> 	if (vc4_hdmi->variant->supports_hdr)
>> 		vc4_hdmi_hdmi_connector_funcs.max_bpc = 12;
>>
>> 	if (vc4_hdmi->variant->max_pixel_clock >= HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ)
>> 		vc4_hdmi_hdmi_connector_funcs.supported_hdmi_ver = HDMI_VERSION_2_0;
>> 	else if (vc4_hdmi->variant->max_pixel_clock >= HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ)
>> 		vc4_hdmi_hdmi_connector_funcs.supported_hdmi_ver = HDMI_VERSION_1_3;
>> 	...
>> }
> 
> I'm sorry for the late response, I was OoO. Why do we want to put data
> into the funcs part? My suggestion would be to put the data into the
> drm_connector_hdmi itself.
> 
> 
> Setting connector->hdmi.supported_hdmi_ver = HDMI_VERSION_1_4; is more
> idiomatic than passing it through the funcs.

I've already done the conversion so that vendor, product, supported_formats and
max_bpc values previously passed as arguments are now provided through
drm_connector_hdmi_funcs, along with the additional supported_hdmi_ver and
supported_tmds_char_rate fields.

In most cases it wasn't necessary to pass this data dynamically (with the
exception of the bridge connector and some kunit tests), so it was just a matter
of extending the immutable hdmi_funcs structs.

I'll send v9 a bit later today so we can discuss directly on the code changes.

FWIW, in the VC4 case, I followed Maxime's suggestion and introduced three 
hdmi_funcs instances (+ a macro to avoid duplication) and assigned them to the
corresponding vc4_hdmi_variant entries:

#define VC4_HDMI_CONNECTOR_FUNCS_COMMON						\
	.vendor			 = "Broadcom",					\
	.product		 = "Videocore",					\
	.supported_formats	 = BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444) |	\
				   BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR422) |	\
				   BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR444),	\
	.tmds_char_rate_valid	 = vc4_hdmi_connector_clock_valid,		\
	.avi = {								\
		.clear_infoframe = vc4_hdmi_clear_avi_infoframe,		\
		.write_infoframe = vc4_hdmi_write_avi_infoframe,		\
	},									\
	.hdmi = {								\
		.clear_infoframe = vc4_hdmi_clear_hdmi_infoframe,		\
		.write_infoframe = vc4_hdmi_write_hdmi_infoframe,		\
	},									\
	...

static const struct drm_connector_hdmi_funcs vc4_hdmi_connector_funcs_hdmi14 = {
	VC4_HDMI_CONNECTOR_FUNCS_COMMON,
	.max_bpc		= 12,
	.supported_hdmi_ver	= HDMI_VERSION_1_4,
};

static const struct drm_connector_hdmi_funcs vc4_hdmi_connector_funcs_hdmi20 = {
	VC4_HDMI_CONNECTOR_FUNCS_COMMON,
	.max_bpc		= 12,
	.supported_hdmi_ver	= HDMI_VERSION_2_0,
	.scrambler_enable	= vc4_hdmi_scrambler_enable,
	.scrambler_disable	= vc4_hdmi_scrambler_disable,
};


static const struct vc4_hdmi_variant bcm2712_hdmi0_variant = {
	...
	.hp_detect		= vc5_hdmi_hp_detect,
	.hdmi_funcs		= &vc4_hdmi_connector_funcs_hdmi20,
};

vc4_hdmi_connector_init()
{
	...
	ret = drmm_connector_hdmi_init(dev, connector,
				       &vc4_hdmi_connector_funcs,
				       vc4_hdmi->variant->hdmi_funcs,
				       DRM_MODE_CONNECTOR_HDMIA,
				       vc4_hdmi->ddc);
	...
}