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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: dhinakaran.pandiyan@intel.com,
	vathsala nagaraju <vathsala.nagaraju@intel.com>,
	rodrigo.vivi@intel.com, intel-gfx@lists.freedesktop.org
Cc: Puthikorn Voravootivat <puthik@chromium.org>,
	Maulik V Vaghela <maulik.v.vaghela@intel.com>
Subject: Re: [PATCH] drm/i915/psr: vbt change for psr
Date: Thu, 17 May 2018 11:02:58 +0300	[thread overview]
Message-ID: <87muwyzp3h.fsf@intel.com> (raw)
In-Reply-To: <1526492658.17473.134.camel@intel.com>

On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx