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

[Cristian Ciocaltea]

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.

Moreover, we would end up with some redundancy, as most of these input fields,
after validation, would be stored (altered or not) directly in
drm_connector/drm_connector_hdmi structs.

Hence, I think we are still better off sticking with
drmm_connector_hdmi_init_with_caps() for now.

> I see the
> following options:
> 
> 1. Continue with drmm_connector_hdmi_init_with_caps() after removing the caps
>    parameter, and then drop drmm_connector_hdmi_init() after the migration.
> 
> 2. Keep the existing (unaltered) drmm_connector_hdmi_init(), which would be
>    slightly inconsistent: supported_formats and max_bpc would still be passed 
>    as arguments, while the rest would go through drm_hdmi_connector_funcs.
> 
> 3. A variation of option 1: additionally rename
>    drmm_connector_hdmi_init_with_caps() to drmm_connector_hdmi_init() after the
>    migration.
> 
> Thanks,
> Cristian