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);
...
}
