[PATCH v2 3/3] drm/i915/bios: abstract child device expected size

[Chauhan, Shekhar]

On 3/5/2024 16:43, Jani Nikula wrote:
> On Tue, 05 Mar 2024, "Chauhan, Shekhar" <shekhar.chauhan at intel.com> wrote:
>> On 2/26/2024 23:28, Jani Nikula wrote:
>>> Add a function to return the expected child device size. Flip the if
>>> ladder around and use the same versions as in documentation to make it
>>> easier to verify. Return an error for unknown versions. No functional
>>> changes.
>>>
>>> v2: Move BUILD_BUG_ON() next to the expected sizes
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_bios.c | 40 ++++++++++++++---------
>>>    1 file changed, 24 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>>> index c0f41bd1f946..343726de9aa7 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>>> @@ -2699,27 +2699,35 @@ static void parse_ddi_ports(struct drm_i915_private *i915)
>>>    		print_ddi_port(devdata);
>>>    }
>>>    
>>> +static int child_device_expected_size(u16 version)
>>> +{
>>> +	BUILD_BUG_ON(sizeof(struct child_device_config) < 40);
>>> +
>>> +	if (version > 256)
>>> +		return -ENOENT;
>>> +	else if (version >= 256)
>> Correct me if I'm wrong, but isn't version >= 256, a bit cryptic after
>> the first check?
>> Would it be wise to make it version > 256, return -ENOENT and if version
>> == 256, return 40?
> It may look so right now, but consider these future cases:
>
> - VBT version gets bumped, and we get the info that, say, version 270
>    still has size 40. What needs to be changed?
>
> - VBT version gets bumped, and we get the info that, say, version 271
>    has size 41. What needs to be changed?
>
> Note that VBT versions above are pure examples, and don't reflect the
> spec in any way.
>
> We know right now that versions >= 256 will have size 40. We don't want
> to express that in a way that requires us to modify it in the
> future. This is the difference to the old if ladder.
Understood. Thanks.
>
> Indeed, we could already bump the first if to
>
> 	if (version > 257)
>
> because we now know version 257 has size 40.
>
> BR,
> Jani.
>
>
>>> +		return 40;
>>> +	else if (version >= 216)
>>> +		return 39;
>>> +	else if (version >= 196)
>>> +		return 38;
>>> +	else if (version >= 195)
>>> +		return 37;
>>> +	else if (version >= 111)
>>> +		return LEGACY_CHILD_DEVICE_CONFIG_SIZE;
>>> +	else if (version >= 106)
>>> +		return 27;
>>> +	else
>>> +		return 22;
>>> +}
>>> +
>>>    static bool child_device_size_valid(struct drm_i915_private *i915, int size)
>>>    {
>>>    	int expected_size;
>>>    
>>> -	if (i915->display.vbt.version < 106) {
>>> -		expected_size = 22;
>>> -	} else if (i915->display.vbt.version < 111) {
>>> -		expected_size = 27;
>>> -	} else if (i915->display.vbt.version < 195) {
>>> -		expected_size = LEGACY_CHILD_DEVICE_CONFIG_SIZE;
>>> -	} else if (i915->display.vbt.version == 195) {
>>> -		expected_size = 37;
>>> -	} else if (i915->display.vbt.version <= 215) {
>>> -		expected_size = 38;
>>> -	} else if (i915->display.vbt.version <= 255) {
>>> -		expected_size = 39;
>>> -	} else if (i915->display.vbt.version <= 256) {
>>> -		expected_size = 40;
>>> -	} else {
>>> +	expected_size = child_device_expected_size(i915->display.vbt.version);
>>> +	if (expected_size < 0) {
>>>    		expected_size = sizeof(struct child_device_config);
>>> -		BUILD_BUG_ON(sizeof(struct child_device_config) < 40);
>>>    		drm_dbg(&i915->drm,
>>>    			"Expected child device config size for VBT version %u not known; assuming %d\n",
>>>    			i915->display.vbt.version, expected_size);

-- 
-shekhar