Attention is currently required from: Furquan Shaikh, Maulik V Vaghela, Tim Wawrzynczak, Angel Pons, Meera Ravindranath, Patrick Rudolph. Subrata Banik 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.
Sure, will wait for your code change.
- 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.
You are mostly right as i read these two are critical but wanted to ensure we are taking complete version into account. Hopefully we don't run into any issue.
Thanks for having good discussion.