On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> wrote: > On Wed, 2018-05-16 at 11:08 +0300, Jani Nikula wrote: >> I think the patch is now the way it should be. We should not change >> our interpretation based on the value. > > Is it correct to infer, from your response, that VBT values are not > always set based on hardware capability as documented in bspec? Correct. I think it was the good intention of the VBT change to only allow values that map to valid hardware values, but I think it has caused much more trouble than it has helped. First, the change was not universally tied to a VBT version, and I've seen VBTs in the wild with version >= 209 that still use multiples of 100 us. (And the spec is still in contradiction with itself.) Second, regardless of mapping or multiples of 100 us we need to verify our input. Because I've seen VBTs in the wild that are bogus regardless of how they're supposed to be interpreted. Third, we already had the code in place to map multiples of 100 us to hardware. We'll need to keep that practically forever. And now we need to have *another* mapping. Fourth, the multiples of 100 us does not require *any* spec change when hardware changes to support different values. The new mapping requires changes throughout the stack that looks at the values. (Basically I object to any VBT specification that says anything platform dependent. It should be generic.) Now, the patch at hand uses the best guesses we can make to translate whatever the VBT has to microseconds, in intel_bios.c. That part does not care about hardware capability. For validity, it only looks at what we think is according to VBT spec. It should be hardware agnostic, except for the IS_GEN9_BC() thing, which should probably include gen10+ too. The code in intel_psr.c gets the microseconds as input, and maps that to hardware capability as best we can, erring towards longer delays. Two different abstractions for two different things. One to abstract VBT, another to abstract hardware. This is in line with the direction I've tried to take intel_bios.c and VBT parsing and the VBT spec (the little I've had influence for that) for the longest time. We must not let the VBT abstractions to leak into the driver code. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
