Re: [PATCH] drm/i915/psr: vbt change for psr

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Thu, 2018-05-17 at 11:02 +0300, Jani Nikula wrote:
> On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c
> om> 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.

Thanks for the detailed explanation of the problem and the idea behind
the design.

-DK


> 
> BR,
> Jani.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx