Re: [PATCH] drm/i915/psr: Adds psrwake options for all platforms

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

On Fri, 2018-06-15 at 11:10 +0300, Jani Nikula wrote:
> On Thu, 14 Jun 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c
> om> wrote:
> > 
> > On Thu, 2018-06-14 at 16:56 +0000, Nagaraju, Vathsala wrote:
> > > 
> > > + Ashutosh(VBT team)   + maulik
> > > 
> > > 209 is confirmed version on kbl both by vbt team (Maulik) and
> > > google,
> > > so we had used it.
> > > 
> > > DK's suggestion is 
> > > if ((bdb->version >= 203 && IS_GEN9_BC(dev_priv)) ||
> > > IS_GEMINILAKE(dev_priv) ||  INTEL_GEN(dev_priv) >= 10) {
> > > /* new mapping */
> > > 
> > > As per Ashutosh, 203 Is not the right version, 205 is fine , but
> > > user
> > > can still provide decimal value for SKL.
> >  
> > I am confused, why does the commit message mention 203 then?
> Whatever the version, I want that to be required always. i.e.
> 
> 	if (version >= N && (bunch of other conditions))

Using the version number as a guard makes sense but this means we need
a version number that's applicable to all platforms.

Vathsala,
Can you please confirm if all platforms that you want to use the new
mapping for have version numbers >= 205?

If that's the case, it should be easy to change the check to

if (version >= 205 && (IS_GEN9_BC() || IS_GEMINILAKE() || INTEL_GEN()
>= 10))


> 
> *not*
> 
> 	if (version >= N || (bunch of other conditions))
> 
> BR,
> Jani.
> 
> > 
> > 
> > 
> > > 
> > > Jani/Rodrigo, should we use 205 for SKL or  drop SKL from the new
> > > mapping?
> > > 
> > > -----Original Message-----
> > > From: Pandiyan, Dhinakaran 
> > > Sent: Thursday, June 14, 2018 9:30 PM
> > > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Nikula, Jani
> > > <jani.nikula
> > > @intel.com>; Nagaraju, Vathsala <vathsala.nagaraju@intel.com>
> > > Cc: puthik@chromium.org; intel-gfx@lists.freedesktop.org
> > > Subject: Re: [PATCH] drm/i915/psr: Adds psrwake options for all
> > > platforms
> > > 
> > > On Thu, 2018-06-14 at 11:59 +0530, Nagaraju, Vathsala wrote:
> > > > 
> > > > 
> > > > 
> > > > On 6/13/2018 11:10 PM, Dhinakaran Pandiyan wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On Wed, 2018-06-13 at 10:32 -0700, Dhinakaran Pandiyan wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Wed, 2018-06-13 at 09:41 +0300, Jani Nikula wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Wed, 13 Jun 2018, "Nagaraju, Vathsala"
> > > > > > > <vathsala.nagaraju@
> > > > > > > in 
> > > > > > > tel.
> > > > > > > co
> > > > > > > m> wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 6/12/2018 2:30 PM, Jani Nikula wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On Tue, 12 Jun 2018, vathsala nagaraju
> > > > > > > > > <vathsala.nagaraju
> > > > > > > > > @i 
> > > > > > > > > ntel .c
> > > > > > > > > om> wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > From: Vathsala Nagaraju <vathsala.nagaraju@intel.co
> > > > > > > > > > m>
> > > > > > > > > > 
> > > > > > > > > > Adds new psrwake options defined in the below
> > > > > > > > > > table.
> > > > > > > > > > Platform	PSR wake options vbt version
> > > > > > > > > > KBL/CFL/WHL	All
> > > > > > > > > > SKL		All PV releases (Check for 203+
> > > > > > > > > > might
> > > > > > > > > > help
> > > > > > > > > > but cannot be foolproof)
> > > > > > > > > > BXT		Uses old interpretation.
> > > > > > > > > > CNL/ICL+	All
> > > > > > > > > > GLK		All
> > > > > > > > > > 
> > > > > > > > > > For SKL, we will continue to use older
> > > > > > > > > > interpretation
> > > > > > > > > > for 
> > > > > > > > > > the above reason.
> > > > > > > > > > 
> > > > > > > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > > > > Cc: Puthikorn Voravootivat <puthik@chromium.org>
> > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.
> > > > > > > > > > com>
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju
> > > > > > > > > > @int
> > > > > > > > > > el 
> > > > > > > > > > .com
> > > > > > > > > > ---
> > > > > > > > > >    drivers/gpu/drm/i915/intel_bios.c | 3 ++-
> > > > > > > > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > > > > b/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > > > > index 465dff4..010ff68 100644
> > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > > > > > > > > > @@ -710,7 +710,8 @@ static int 
> > > > > > > > > > intel_bios_ssc_frequency(struct drm_i915_private 
> > > > > > > > > > *dev_priv,
> > > > > > > > > >    	 * New psr options 0=500us, 1=100us,
> > > > > > > > > > 2=2500us, 3=0us
> > > > > > > > > >    	 * Old decimal value is wake up time in
> > > > > > > > > > multiples of
> > > > > > > > > > 100 us.
> > > > > > > > > >    	 */
> > > > > > > > > > -	if (bdb->version >= 209 &&
> > > > > > > > > > IS_GEN9_BC(dev_priv))
> > > > > > > > > > {
> > > > > > > > > > +	if ((INTEL_GEN(dev_priv) >= 10) ||
> > > > > > > > > > +	    (IS_GEN9_BC(dev_priv) &&
> > > > > > > > > > !IS_SKYLAKE(dev_priv)))
> > > > > > > > > > {
> > > > > > > > > Please keep the version check.
> > > > > > > > Sure. For SKL , shall we use older interpretation for
> > > > > > > > all
> > > > > > > > bdb 
> > > > > > > > version as vbt team cannot confirm bdb version for SKL?
> > > > > > > I guess.
> > > > > > > 
> > > > > > Why not change the version check to >= 203, if that's what
> > > > > > PV
> > > > > > releases
> > > > > > had as per your commit message? With the current code,
> > > > > > Linux
> > > > > > and
> > > > > > Windows set 500 us and 2.5 ms respectively on my laptop.
> > > > > Said laptop is a SKL with bdb version 205.
> > > > + ashutosh(VBT team)
> > > > Since VBT team cannot confirm version for SKL ,so skipped for
> > > > skylake.
> > > > I did a copy paste of the table provided by vbt team, will edit
> > > > for
> > > > skylake.
> > > > 
> > > We are not going to get this right for all combinations, the best
> > > we
> > > can do is make sure things work in most cases. I prefer to err on
> > > the
> > > side of using the new mapping because when translated
> > > incorrectly, 3
> > > out of 4 values lead to >= intended training time. Given the fact
> > > that
> > > SKL PV releases also used the new mapping, I suggest you do this
> > > 
> > > if ((bdb->version >= 203 && IS_GEN9_BC(dev_priv)) ||
> > > IS_GEMINILAKE(dev_priv) ||  INTEL_GEN(dev_priv) >= 10) {
> > > /* new mapping */
> > > 
> > > }
> > > 
> > > I don't know what the basis for the current check for version 209
> > > is,
> > > other than it was the version on the KBL you tested.
> > > 
> > > -DK
> > > 
> > > 
> > > 
> > > 
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx