On 7/16/26 4:44 PM, Maxime Ripard 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;
>> ...
>> }
>
> No, but you don't have to do that either.
>
> You have three cases:
> - Rpi < 4 has max_bpc = 8, HDMI 1.4
> - RPI 4 HDMI 0 and RPI 5 has max_bpc = 12, HDMI 2.0
> - RPI 4 HDMI 1 has max_bpc = 12, HDMI 1.4
>
> Just create three different structures, and put a pointer to the right
> one in the vc4_hdmi_variant structure. There's no need to be dynamic
> there.
Ack, will do!
Thanks,
Cristian
