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:
Sorry, but I don't think it's an improvement.
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.
Irrespective of that don't you think this CL fixes few problem upfront that we are expected to face with new opregion spec uprev
1. 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.
2. 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 ?
-
Subrata Banik (Code Review)