Attention is currently required from: Furquan Shaikh, Maulik V Vaghela, Tim Wawrzynczak, Subrata Banik, Meera Ravindranath, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56303 )
Change subject: drivers/intel/gma: Refactor IGD opregion 2.1 support code ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Thanks for sharing your feedback. I shall get back with the improvement if any with respect to runtime vs static calculation of version from timing standpoint.
The compiler should be able to optimise the current approach so that `opregion_get_version()` returns a constant value, without doing any operations.
Irrespective of that don't you think this CL fixes few problem upfront that we are expected to face with new opregion spec uprev
- Adding new check always whenever new revision got introduced ?
/* Function to get the IGD Opregion version */ static struct opregion_version opregion_get_version(void) {
if (CONFIG(INTEL_GMA_OPREGION_3_0))
return (struct opregion_version) { .major = 3, .minor = 0 };if (CONFIG(INTEL_GMA_OPREGION_2_1)) return (struct opregion_version) { .major = 2, .minor = 1 };
return (struct opregion_version) { .major = 2, .minor = 0 };
}
Compared to in this approach adding a Kconfig without any addition checks in opregion_get_version.
I want to try something once CB:52758 lands: require that platforms explicitly select which OpRegion version they want to use. The way I want to do this will also ensure that `opregion_get_version()` is updated when adding support for another OpRegion version.
- None of opregion_get_version and uses_relative_vbt_addr functions are honoring revision field, as opregion version is combination of those 3 fields (.major, .minor and .revision), won't we miss to detect a correct version (i know i read all revision as zero) but isn't it better to adhere to entire 32bit value if possible ?
I don't think there will be any future incompatibilities with this check.