drm/i915/psr: Adds psrwake options for all platforms

Commit Message

vathsala nagaraju June 12, 2018, 5:19 a.m. UTC

From: Vathsala Nagaraju <[e-mail]>

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 <[e-mail]>
Cc: Rodrigo Vivi <[e-mail]>
Cc: Puthikorn Voravootivat <[e-mail]>
Cc: Dhinakaran Pandiyan <[e-mail]>

Signed-off-by: Vathsala Nagaraju <[e-mail]>
---
 drivers/gpu/drm/i915/intel_bios.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jani Nikula June 12, 2018, 9 a.m. UTC | #1

On Tue, 12 Jun 2018, vathsala nagaraju <[e-mail]> wrote:
> From: Vathsala Nagaraju <[e-mail]>
>
> 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 <[e-mail]>
> Cc: Rodrigo Vivi <[e-mail]>
> Cc: Puthikorn Voravootivat <[e-mail]>
> Cc: Dhinakaran Pandiyan <[e-mail]>
>
> Signed-off-by: Vathsala Nagaraju <[e-mail]>
> ---
>  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.

Please tell anyone who asks, and also those who don't, that *all* of the
VBT changes should be based on the *version*, and *none* of them should
be based on the *platform*.

BR,
Jani.

>  		switch (psr_table->tp1_wakeup_time) {
>  		case 0:
>  			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;

vathsala nagaraju June 13, 2018, 6:30 a.m. UTC | #2

On 6/12/2018 2:30 PM, Jani Nikula wrote:
> On Tue, 12 Jun 2018, vathsala nagaraju <[e-mail]> wrote:
>> From: Vathsala Nagaraju <[e-mail]>
>>
>> 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 <[e-mail]>
>> Cc: Rodrigo Vivi <[e-mail]>
>> Cc: Puthikorn Voravootivat <[e-mail]>
>> Cc: Dhinakaran Pandiyan <[e-mail]>
>>
>> Signed-off-by: Vathsala Nagaraju <[e-mail]>
>> ---
>>   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?

> Please tell anyone who asks, and also those who don't, that *all* of the
> VBT changes should be based on the *version*, and *none* of them should
> be based on the *platform*.
>
> BR,
> Jani.
>
>>   		switch (psr_table->tp1_wakeup_time) {
>>   		case 0:
>>   			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;

Jani Nikula June 13, 2018, 6:41 a.m. UTC | #3

On Wed, 13 Jun 2018, "Nagaraju, Vathsala" <[e-mail]> wrote:
> On 6/12/2018 2:30 PM, Jani Nikula wrote:
>> On Tue, 12 Jun 2018, vathsala nagaraju <[e-mail]> wrote:
>>> From: Vathsala Nagaraju <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Cc: Rodrigo Vivi <[e-mail]>
>>> Cc: Puthikorn Voravootivat <[e-mail]>
>>> Cc: Dhinakaran Pandiyan <[e-mail]>
>>>
>>> Signed-off-by: Vathsala Nagaraju <[e-mail]>
>>> ---
>>>   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.

BR,
Jani.


>
>> Please tell anyone who asks, and also those who don't, that *all* of the
>> VBT changes should be based on the *version*, and *none* of them should
>> be based on the *platform*.
>>
>> BR,
>> Jani.
>>
>>>   		switch (psr_table->tp1_wakeup_time) {
>>>   		case 0:
>>>   			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
>

Ville Syrjälä June 13, 2018, 10:42 a.m. UTC | #4

On Tue, Jun 12, 2018 at 10:49:09AM +0530, vathsala nagaraju wrote:
> From: Vathsala Nagaraju <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Rodrigo Vivi <[e-mail]>
> Cc: Puthikorn Voravootivat <[e-mail]>
> Cc: Dhinakaran Pandiyan <[e-mail]>
> 
> Signed-off-by: Vathsala Nagaraju <[e-mail]>
> ---
>  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))) {

That doesn't match your commit message.

>  		switch (psr_table->tp1_wakeup_time) {
>  		case 0:
>  			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Dhinakaran Pandiyan June 13, 2018, 5:32 p.m. UTC | #5

On Wed, 2018-06-13 at 09:41 +0300, Jani Nikula wrote:
> On Wed, 13 Jun 2018, "Nagaraju, Vathsala" <[e-mail]
> m> wrote:
> > 
> > On 6/12/2018 2:30 PM, Jani Nikula wrote:
> > > 
> > > On Tue, 12 Jun 2018, vathsala nagaraju <vathsala.nagaraju@intel.c
> > > om> wrote:
> > > > 
> > > > From: Vathsala Nagaraju <[e-mail]>
> > > > 
> > > > 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 <[e-mail]>
> > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > Cc: Puthikorn Voravootivat <[e-mail]>
> > > > Cc: Dhinakaran Pandiyan <[e-mail]>
> > > > 
> > > > Signed-off-by: Vathsala Nagaraju <[e-mail]>
> > > > ---
> > > >   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.



> BR,
> Jani.
> 
> 
> > 
> > 
> > > 
> > > Please tell anyone who asks, and also those who don't, that *all*
> > > of the
> > > VBT changes should be based on the *version*, and *none* of them
> > > should
> > > be based on the *platform*.
> > > 
> > > BR,
> > > Jani.
> > > 
> > > > 
> > > >   		switch (psr_table->tp1_wakeup_time) {
> > > >   		case 0:
> > > >   			dev_priv->vbt.psr.tp1_wakeup_time_us
> > > > = 500;

Dhinakaran Pandiyan June 13, 2018, 5:40 p.m. UTC | #6

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@intel.
> > co
> > m> wrote:
> > > 
> > > 
> > > On 6/12/2018 2:30 PM, Jani Nikula wrote:
> > > > 
> > > > 
> > > > On Tue, 12 Jun 2018, vathsala nagaraju <vathsala.nagaraju@intel
> > > > .c
> > > > om> wrote:
> > > > > 
> > > > > 
> > > > > From: Vathsala Nagaraju <[e-mail]>
> > > > > 
> > > > > 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 <[e-mail]>
> > > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > > Cc: Puthikorn Voravootivat <[e-mail]>
> > > > > Cc: Dhinakaran Pandiyan <[e-mail]>
> > > > > 
> > > > > Signed-off-by: Vathsala Nagaraju <[e-mail]
> > > > > >
> > > > > ---
> > > > >   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.

vathsala nagaraju June 14, 2018, 6:29 a.m. UTC | #7

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@intel.
>>> co
>>> m> wrote:
>>>>
>>>> On 6/12/2018 2:30 PM, Jani Nikula wrote:
>>>>>
>>>>> On Tue, 12 Jun 2018, vathsala nagaraju <vathsala.nagaraju@intel
>>>>> .c
>>>>> om> wrote:
>>>>>>
>>>>>> From: Vathsala Nagaraju <[e-mail]>
>>>>>>
>>>>>> 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 <[e-mail]>
>>>>>> Cc: Rodrigo Vivi <[e-mail]>
>>>>>> Cc: Puthikorn Voravootivat <[e-mail]>
>>>>>> Cc: Dhinakaran Pandiyan <[e-mail]>
>>>>>>
>>>>>> Signed-off-by: Vathsala Nagaraju <[e-mail]
>>>>>> ---
>>>>>>    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.

vathsala nagaraju June 14, 2018, 7:38 a.m. UTC | #8

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@intel.
>>> co
>>> m> wrote:
>>>>
>>>> On 6/12/2018 2:30 PM, Jani Nikula wrote:
>>>>>
>>>>> On Tue, 12 Jun 2018, vathsala nagaraju <vathsala.nagaraju@intel
>>>>> .c
>>>>> om> wrote:
>>>>>>
>>>>>> From: Vathsala Nagaraju <[e-mail]>
>>>>>>
>>>>>> 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 <[e-mail]>
>>>>>> Cc: Rodrigo Vivi <[e-mail]>
>>>>>> Cc: Puthikorn Voravootivat <[e-mail]>
>>>>>> Cc: Dhinakaran Pandiyan <[e-mail]>
>>>>>>
>>>>>> Signed-off-by: Vathsala Nagaraju <[e-mail]
>>>>>> ---
>>>>>>    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

vathsala nagaraju June 14, 2018, 7:46 a.m. UTC | #9

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@intel.
>>> co
>>> m> wrote:
>>>>
>>>> On 6/12/2018 2:30 PM, Jani Nikula wrote:
>>>>>
>>>>> On Tue, 12 Jun 2018, vathsala nagaraju <vathsala.nagaraju@intel
>>>>> .c
>>>>> om> wrote:
>>>>>>
>>>>>> From: Vathsala Nagaraju <[e-mail]>
>>>>>>
>>>>>> 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 <[e-mail]>
>>>>>> Cc: Rodrigo Vivi <[e-mail]>
>>>>>> Cc: Puthikorn Voravootivat <[e-mail]>
>>>>>> Cc: Dhinakaran Pandiyan <[e-mail]>
>>>>>>
>>>>>> Signed-off-by: Vathsala Nagaraju <[e-mail]
>>>>>> ---
>>>>>>    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.
I did a copy paste of the table provided by vbt team, will edit for skylake
Since VBT team cannot confirm version for SKL ,so skipping for skylake.

Dhinakaran Pandiyan June 14, 2018, 4 p.m. UTC | #10

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 <[e-mail]>

> > > > > > > 

> > > > > > > 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 <[e-mail]>

> > > > > > > Cc: Rodrigo Vivi <[e-mail]>

> > > > > > > Cc: Puthikorn Voravootivat <[e-mail]>

> > > > > > > Cc: Dhinakaran Pandiyan <[e-mail]>

> > > > > > > 

> > > > > > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel

> > > > > > > .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

Rodrigo Vivi June 14, 2018, 4:48 p.m. UTC | #11

On Thu, Jun 14, 2018 at 09:00:15AM -0700, Pandiyan, Dhinakaran wrote:
> 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 <[e-mail]>
> > > > > > > > 
> > > > > > > > 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 <[e-mail]>
> > > > > > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > > > > > Cc: Puthikorn Voravootivat <[e-mail]>
> > > > > > > > Cc: Dhinakaran Pandiyan <[e-mail]>
> > > > > > > > 
> > > > > > > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel
> > > > > > > > .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,

:(
it seems we have to live with it

> the best we
> can do is make sure things work in most cases.

I agree.

> 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) {

+1

> /* 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
> 
> 
> 
> 
>

vathsala nagaraju June 14, 2018, 4:56 p.m. UTC | #12

+ 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.
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 <[e-mail]>; Nikula, Jani <[e-mail]>; Nagaraju, Vathsala <[e-mail]>
Cc: [e-mail]; [e-mail]
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 <[e-mail]>

> > > > > > > 

> > > > > > > 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 <[e-mail]>

> > > > > > > Cc: Rodrigo Vivi <[e-mail]>

> > > > > > > Cc: Puthikorn Voravootivat <[e-mail]>

> > > > > > > Cc: Dhinakaran Pandiyan <[e-mail]>

> > > > > > > 

> > > > > > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel 

> > > > > > > .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

Dhinakaran Pandiyan June 14, 2018, 5:28 p.m. UTC | #13

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?


> 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 <[e-mail]>; Nikula, Jani <jani.nikula
> @intel.com>; Nagaraju, Vathsala <[e-mail]>
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]>
> > > > > > > > 
> > > > > > > > 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 <[e-mail]>
> > > > > > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > > > > > Cc: Puthikorn Voravootivat <[e-mail]>
> > > > > > > > Cc: Dhinakaran Pandiyan <[e-mail]>
> > > > > > > > 
> > > > > > > > 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
> 
> 
> 
> 
>

Jani Nikula June 15, 2018, 8:10 a.m. UTC | #14

On Thu, 14 Jun 2018, Dhinakaran Pandiyan <[e-mail]> 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))

*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 <[e-mail]>; Nikula, Jani <jani.nikula
>> @intel.com>; Nagaraju, Vathsala <[e-mail]>
>> Cc: [e-mail]; [e-mail]
>> 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 <[e-mail]>
>> > > > > > > > 
>> > > > > > > > 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 <[e-mail]>
>> > > > > > > > Cc: Rodrigo Vivi <[e-mail]>
>> > > > > > > > Cc: Puthikorn Voravootivat <[e-mail]>
>> > > > > > > > Cc: Dhinakaran Pandiyan <[e-mail]>
>> > > > > > > > 
>> > > > > > > > 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
>> 
>> 
>> 
>> 
>>

Dhinakaran Pandiyan June 15, 2018, 6:14 p.m. UTC | #15

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 <[e-mail]>; Nikula, Jani
> > > <jani.nikula
> > > @intel.com>; Nagaraju, Vathsala <[e-mail]>
> > > Cc: [e-mail]; [e-mail]
> > > 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 <[e-mail]
> > > > > > > > > > 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 <[e-mail]>
> > > > > > > > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > > > > > > > Cc: Puthikorn Voravootivat <[e-mail]>
> > > > > > > > > > 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
> > > 
> > > 
> > > 
> > > 
> > >

Patch

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))) {
 		switch (psr_table->tp1_wakeup_time) {
 		case 0:
 			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;