| Message ID | 20240226175854.287871-3-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] drm/i915/bios: bump expected child device size | expand |
On Mon, Feb 26, 2024 at 07:58:54PM +0200, 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); Should we make that !=40 perhaps? Anyways, series is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > + if (version > 256) > + return -ENOENT; > + else if (version >= 256) > + 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); > -- > 2.39.2
On Thu, 29 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Feb 26, 2024 at 07:58:54PM +0200, 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); > > Should we make that !=40 perhaps? Yeah, but let's get this going first. > Anyways, series is > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks, pushed to din. BR, Jani.
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? > + 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);
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);
On 3/5/2024 16:43, Jani Nikula wrote: > 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. 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);
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) + 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);
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(-)