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

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 05 Mar 2024, "Chauhan, Shekhar" <shekhar.chauhan@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@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.

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

-- 
Jani Nikula, Intel