[PATCH] drm/i915: Fix subslice configuration on Gen9LP

[PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

According to the documentation, when programming the subslice count power-
gating configuration register, the value to be written into it on Gen9LP
should actually in the format of:

  1 slice  = 0x001
  2 slices = 0x010
  3 slices = 0x100

And not the popcount of the enabled subslice mask as on other platforms.

So on Gen9LP platforms we have been programming 0x11 into those bits, but
the documentation does not explain what would that achieve. Could it be
that we enable only two subslice on three sub-slice parts? Or hardware
simply ignores it and sticks with the maximum configuration?

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Bspec: 12247
---
Could this actually be true or I am severely misreading the docs? It does
not sound plausible to me this would have been missed all this time..

How to test in what configuration do these parts run before and after this
patch?
---
 drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 36050f085071..cdfa962a1975 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
 	}
 
 	if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
+		u8 val;
+
+		val = hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
+
+		if (IS_GEN9_LP(dev_priv))
+			val = BIT(val - 1);
+
 		rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
-		rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]) <<
-			GEN8_RPCS_SS_CNT_SHIFT;
+		rpcs |= val << GEN8_RPCS_SS_CNT_SHIFT;
 		rpcs |= GEN8_RPCS_ENABLE;
 	}
 
-- 
2.17.1

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

Re: [PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Tvrtko Ursulin]

On 22/08/18 15:29, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> According to the documentation, when programming the subslice count power-
> gating configuration register, the value to be written into it on Gen9LP
> should actually in the format of:
> 
>    1 slice  = 0x001
>    2 slices = 0x010
>    3 slices = 0x100
> 
> And not the popcount of the enabled subslice mask as on other platforms.
> 
> So on Gen9LP platforms we have been programming 0x11 into those bits, but
> the documentation does not explain what would that achieve. Could it be
> that we enable only two subslice on three sub-slice parts? Or hardware
> simply ignores it and sticks with the maximum configuration?
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Bspec: 12247

If it does turn out to be broken and this the correct fix then:

Fixes: 0cea6502bf9c ("drm/i915: Request full SSEU enablement on Gen9")

?

At that time code looked like:

+       if (INTEL_INFO(dev)->has_subslice_pg) {
+               rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
+               rpcs |= INTEL_INFO(dev)->subslice_per_slice <<
+                       GEN8_RPCS_SS_CNT_SHIFT;
+               rpcs |= GEN8_RPCS_ENABLE;
+       }

So would had already most likely been broken.

Tvrtko


> ---
> Could this actually be true or I am severely misreading the docs? It does
> not sound plausible to me this would have been missed all this time..
> 
> How to test in what configuration do these parts run before and after this
> patch?
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 36050f085071..cdfa962a1975 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
>   	}
>   
>   	if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
> +		u8 val;
> +
> +		val = hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
> +
> +		if (IS_GEN9_LP(dev_priv))
> +			val = BIT(val - 1);
> +
>   		rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
> -		rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]) <<
> -			GEN8_RPCS_SS_CNT_SHIFT;
> +		rpcs |= val << GEN8_RPCS_SS_CNT_SHIFT;
>   		rpcs |= GEN8_RPCS_ENABLE;
>   	}
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Fix subslice configuration on Gen9LP

[Patchwork]

== Series Details ==

Series: drm/i915: Fix subslice configuration on Gen9LP
URL   : https://patchwork.freedesktop.org/series/48566/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4696 -> Patchwork_9991 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48566/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9991:

  === IGT changes ===

    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-FAIL -> DMESG-WARN

    
== Known issues ==

  Here are the changes found in Patchwork_9991 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         PASS -> DMESG-FAIL (fdo#107174)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)
      fi-cnl-psr:         PASS -> DMESG-WARN (fdo#104951)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      {fi-skl-iommu}:     DMESG-FAIL (fdo#107174, fdo#106560) -> PASS
      fi-kbl-7567u:       DMESG-FAIL (fdo#106947, fdo#106560) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      {fi-byt-clapper}:   FAIL (fdo#107362, fdo#103191) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      {fi-cfl-8109u}:     INCOMPLETE (fdo#106070) -> PASS

    
    ==== Warnings ====

    {igt@pm_rpm@module-reload}:
      fi-cnl-psr:         FAIL -> WARN (fdo#107602)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602


== Participating hosts (54 -> 48) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-glk-j4005 


== Build changes ==

    * Linux: CI_DRM_4696 -> Patchwork_9991

  CI_DRM_4696: ced152c46fc90f7c1ac8963850d64c9f1ce652d6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4608: 94ebd21177feedf03e8f6dd1e73dca1a6ec7a0ac @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9991: 75e3f1910f813b2dff2ae2794d9e84e9f832188c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

75e3f1910f81 drm/i915: Fix subslice configuration on Gen9LP

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9991/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Lionel Landwerlin]

On 22/08/2018 15:29, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> According to the documentation, when programming the subslice count power-
> gating configuration register, the value to be written into it on Gen9LP
> should actually in the format of:
>
>    1 slice  = 0x001
>    2 slices = 0x010
>    3 slices = 0x100


s/slice/subslice/


Also 0b001 etc... Not hexadecimal.


>
> And not the popcount of the enabled subslice mask as on other platforms.
>
> So on Gen9LP platforms we have been programming 0x11 into those bits, but
> the documentation does not explain what would that achieve. Could it be
> that we enable only two subslice on three sub-slice parts? Or hardware
> simply ignores it and sticks with the maximum configuration?
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Bspec: 12247
> ---
> Could this actually be true or I am severely misreading the docs? It does
> not sound plausible to me this would have been missed all this time..
>
> How to test in what configuration do these parts run before and after this
> patch?
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 36050f085071..cdfa962a1975 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
>   	}
>   
>   	if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
> +		u8 val;
> +
> +		val = hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
> +
> +		if (IS_GEN9_LP(dev_priv))
> +			val = BIT(val - 1);


Hmm... Are you breaking the 2 subslices setting here then?

(2 subslices = 0b10 which should be equal to hweight8(subslice_mask) if 
I'm thinking right)


> +
>   		rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
> -		rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]) <<
> -			GEN8_RPCS_SS_CNT_SHIFT;
> +		rpcs |= val << GEN8_RPCS_SS_CNT_SHIFT;
>   		rpcs |= GEN8_RPCS_ENABLE;
>   	}
>   


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

Re: [PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Tvrtko Ursulin]

On 22/08/2018 16:08, Lionel Landwerlin wrote:
> On 22/08/2018 15:29, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> According to the documentation, when programming the subslice count 
>> power-
>> gating configuration register, the value to be written into it on Gen9LP
>> should actually in the format of:
>>
>>    1 slice  = 0x001
>>    2 slices = 0x010
>>    3 slices = 0x100
> 
> 
> s/slice/subslice/
> 
> 
> Also 0b001 etc... Not hexadecimal.

Oops, you're right.

>>
>> And not the popcount of the enabled subslice mask as on other platforms.
>>
>> So on Gen9LP platforms we have been programming 0x11 into those bits, but
>> the documentation does not explain what would that achieve. Could it be
>> that we enable only two subslice on three sub-slice parts? Or hardware
>> simply ignores it and sticks with the maximum configuration?
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Bspec: 12247
>> ---
>> Could this actually be true or I am severely misreading the docs? It does
>> not sound plausible to me this would have been missed all this time..
>>
>> How to test in what configuration do these parts run before and after 
>> this
>> patch?
>> ---
>>   drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> index 36050f085071..cdfa962a1975 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>       }
>>       if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>> +        u8 val;
>> +
>> +        val = hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
>> +
>> +        if (IS_GEN9_LP(dev_priv))
>> +            val = BIT(val - 1);
> 
> 
> Hmm... Are you breaking the 2 subslices setting here then?
> 
> (2 subslices = 0b10 which should be equal to hweight8(subslice_mask) if 
> I'm thinking right)

No and yes, I think.

subslice_mask = 0b011 => hweight = 2 => BIT(2 - 1) = BIT(1) = 0b010 into 
the register

In the same way, all together:

subslice_mask = 0b001 => hweight = 1 => BIT(0) = 0b001
subslice_mask = 0b011 => hweight = 2 => BIT(1) = 0b010
subslice_mask = 0b111 => hweight = 3 => BIT(2) = 0b100

Have I made a mistake somewhere?

Regards,

Tvrtko

> 
>> +
>>           rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
>> -        rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]) <<
>> -            GEN8_RPCS_SS_CNT_SHIFT;
>> +        rpcs |= val << GEN8_RPCS_SS_CNT_SHIFT;
>>           rpcs |= GEN8_RPCS_ENABLE;
>>       }
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Lionel Landwerlin]

On 22/08/2018 16:17, Tvrtko Ursulin wrote:
>
> On 22/08/2018 16:08, Lionel Landwerlin wrote:
>> On 22/08/2018 15:29, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> According to the documentation, when programming the subslice count 
>>> power-
>>> gating configuration register, the value to be written into it on 
>>> Gen9LP
>>> should actually in the format of:
>>>
>>>    1 slice  = 0x001
>>>    2 slices = 0x010
>>>    3 slices = 0x100
>>
>>
>> s/slice/subslice/
>>
>>
>> Also 0b001 etc... Not hexadecimal.
>
> Oops, you're right.
>
>>>
>>> And not the popcount of the enabled subslice mask as on other 
>>> platforms.
>>>
>>> So on Gen9LP platforms we have been programming 0x11 into those 
>>> bits, but
>>> the documentation does not explain what would that achieve. Could it be
>>> that we enable only two subslice on three sub-slice parts? Or hardware
>>> simply ignores it and sticks with the maximum configuration?
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> Bspec: 12247
>>> ---
>>> Could this actually be true or I am severely misreading the docs? It 
>>> does
>>> not sound plausible to me this would have been missed all this time..
>>>
>>> How to test in what configuration do these parts run before and 
>>> after this
>>> patch?
>>> ---
>>>   drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 36050f085071..cdfa962a1975 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>>       }
>>>       if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>>> +        u8 val;
>>> +
>>> +        val = hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
>>> +
>>> +        if (IS_GEN9_LP(dev_priv))
>>> +            val = BIT(val - 1);
>>
>>
>> Hmm... Are you breaking the 2 subslices setting here then?
>>
>> (2 subslices = 0b10 which should be equal to hweight8(subslice_mask) 
>> if I'm thinking right)
>
> No and yes, I think.
>
> subslice_mask = 0b011 => hweight = 2 => BIT(2 - 1) = BIT(1) = 0b010 
> into the register
>
> In the same way, all together:
>
> subslice_mask = 0b001 => hweight = 1 => BIT(0) = 0b001
> subslice_mask = 0b011 => hweight = 2 => BIT(1) = 0b010
> subslice_mask = 0b111 => hweight = 3 => BIT(2) = 0b100
>
> Have I made a mistake somewhere?


Ah, yes! You're right :)

My eyes got tricked, thanks for finding this out.


With the comment fixed :


Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>


>
> Regards,
>
> Tvrtko
>
>>
>>> +
>>>           rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
>>> -        rpcs |= 
>>> hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]) <<
>>> -            GEN8_RPCS_SS_CNT_SHIFT;
>>> +        rpcs |= val << GEN8_RPCS_SS_CNT_SHIFT;
>>>           rpcs |= GEN8_RPCS_ENABLE;
>>>       }
>>
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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

Re: [PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Tvrtko Ursulin]

On 22/08/2018 16:22, Lionel Landwerlin wrote:
> On 22/08/2018 16:17, Tvrtko Ursulin wrote:
>>
>> On 22/08/2018 16:08, Lionel Landwerlin wrote:
>>> On 22/08/2018 15:29, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> According to the documentation, when programming the subslice count 
>>>> power-
>>>> gating configuration register, the value to be written into it on 
>>>> Gen9LP
>>>> should actually in the format of:
>>>>
>>>>    1 slice  = 0x001
>>>>    2 slices = 0x010
>>>>    3 slices = 0x100
>>>
>>>
>>> s/slice/subslice/
>>>
>>>
>>> Also 0b001 etc... Not hexadecimal.
>>
>> Oops, you're right.
>>
>>>>
>>>> And not the popcount of the enabled subslice mask as on other 
>>>> platforms.
>>>>
>>>> So on Gen9LP platforms we have been programming 0x11 into those 
>>>> bits, but
>>>> the documentation does not explain what would that achieve. Could it be
>>>> that we enable only two subslice on three sub-slice parts? Or hardware
>>>> simply ignores it and sticks with the maximum configuration?
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>> Bspec: 12247
>>>> ---
>>>> Could this actually be true or I am severely misreading the docs? It 
>>>> does
>>>> not sound plausible to me this would have been missed all this time..
>>>>
>>>> How to test in what configuration do these parts run before and 
>>>> after this
>>>> patch?
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>>> index 36050f085071..cdfa962a1975 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>> @@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>>>       }
>>>>       if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>>>> +        u8 val;
>>>> +
>>>> +        val = hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
>>>> +
>>>> +        if (IS_GEN9_LP(dev_priv))
>>>> +            val = BIT(val - 1);
>>>
>>>
>>> Hmm... Are you breaking the 2 subslices setting here then?
>>>
>>> (2 subslices = 0b10 which should be equal to hweight8(subslice_mask) 
>>> if I'm thinking right)
>>
>> No and yes, I think.
>>
>> subslice_mask = 0b011 => hweight = 2 => BIT(2 - 1) = BIT(1) = 0b010 
>> into the register
>>
>> In the same way, all together:
>>
>> subslice_mask = 0b001 => hweight = 1 => BIT(0) = 0b001
>> subslice_mask = 0b011 => hweight = 2 => BIT(1) = 0b010
>> subslice_mask = 0b111 => hweight = 3 => BIT(2) = 0b100
>>
>> Have I made a mistake somewhere?
> 
> 
> Ah, yes! You're right :)
> 
> My eyes got tricked, thanks for finding this out.

At least half of the credit goes to you for linking to 12247 in scope of 
one different thread!

> 
> With the comment fixed :
> 
> 
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

Thanks!

Any ideas on how to test this? I'd like to commit message to be more 
precise - have we been running with one slice too few? Or hardware 
ignores the undocumented bit combination? Or even, is the documentation 
perhaps incorrect?!

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Lionel Landwerlin]

On 22/08/2018 16:27, Tvrtko Ursulin wrote:
>
> On 22/08/2018 16:22, Lionel Landwerlin wrote:
>> On 22/08/2018 16:17, Tvrtko Ursulin wrote:
>>>
>>> On 22/08/2018 16:08, Lionel Landwerlin wrote:
>>>> On 22/08/2018 15:29, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> According to the documentation, when programming the subslice 
>>>>> count power-
>>>>> gating configuration register, the value to be written into it on 
>>>>> Gen9LP
>>>>> should actually in the format of:
>>>>>
>>>>>    1 slice  = 0x001
>>>>>    2 slices = 0x010
>>>>>    3 slices = 0x100
>>>>
>>>>
>>>> s/slice/subslice/
>>>>
>>>>
>>>> Also 0b001 etc... Not hexadecimal.
>>>
>>> Oops, you're right.
>>>
>>>>>
>>>>> And not the popcount of the enabled subslice mask as on other 
>>>>> platforms.
>>>>>
>>>>> So on Gen9LP platforms we have been programming 0x11 into those 
>>>>> bits, but
>>>>> the documentation does not explain what would that achieve. Could 
>>>>> it be
>>>>> that we enable only two subslice on three sub-slice parts? Or 
>>>>> hardware
>>>>> simply ignores it and sticks with the maximum configuration?
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>>> Bspec: 12247
>>>>> ---
>>>>> Could this actually be true or I am severely misreading the docs? 
>>>>> It does
>>>>> not sound plausible to me this would have been missed all this time..
>>>>>
>>>>> How to test in what configuration do these parts run before and 
>>>>> after this
>>>>> patch?
>>>>> ---
>>>>>   drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
>>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>>>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>>>> index 36050f085071..cdfa962a1975 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>>> @@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>>>>       }
>>>>>       if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>>>>> +        u8 val;
>>>>> +
>>>>> +        val = hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
>>>>> +
>>>>> +        if (IS_GEN9_LP(dev_priv))
>>>>> +            val = BIT(val - 1);
>>>>
>>>>
>>>> Hmm... Are you breaking the 2 subslices setting here then?
>>>>
>>>> (2 subslices = 0b10 which should be equal to 
>>>> hweight8(subslice_mask) if I'm thinking right)
>>>
>>> No and yes, I think.
>>>
>>> subslice_mask = 0b011 => hweight = 2 => BIT(2 - 1) = BIT(1) = 0b010 
>>> into the register
>>>
>>> In the same way, all together:
>>>
>>> subslice_mask = 0b001 => hweight = 1 => BIT(0) = 0b001
>>> subslice_mask = 0b011 => hweight = 2 => BIT(1) = 0b010
>>> subslice_mask = 0b111 => hweight = 3 => BIT(2) = 0b100
>>>
>>> Have I made a mistake somewhere?
>>
>>
>> Ah, yes! You're right :)
>>
>> My eyes got tricked, thanks for finding this out.
>
> At least half of the credit goes to you for linking to 12247 in scope 
> of one different thread!
>
>>
>> With the comment fixed :
>>
>>
>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>
> Thanks!
>
> Any ideas on how to test this? I'd like to commit message to be more 
> precise - have we been running with one slice too few? Or hardware 
> ignores the undocumented bit combination? Or even, is the 
> documentation perhaps incorrect?!


Yeah the tests that I wrote to test the API you're picking up use a 
MI_PREDICATE where you can make a command (let's say 
MI_STORE_REGISTER_MEM) conditional on the number of slice.

You can use that in a user batch an verify that memory has or hasn't 
been written to.

That's only before Gen10 though.


I'm looking at whether something that is passed onto the EUs could map 
back to the physical location.

Not sure if there is something...


-

Lionel


>
> Regards,
>
> Tvrtko
>

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

✓ Fi.CI.IGT: success for drm/i915: Fix subslice configuration on Gen9LP

[Patchwork]

== Series Details ==

Series: drm/i915: Fix subslice configuration on Gen9LP
URL   : https://patchwork.freedesktop.org/series/48566/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4696_full -> Patchwork_9991_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9991_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9991_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9991_full:

  === IGT changes ===

    ==== Warnings ====

    igt@kms_cursor_legacy@cursorb-vs-flipa-toggle:
      shard-hsw:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9991_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_isolation@vcs1-s3:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@gem_exec_await@wide-contexts:
      shard-glk:          PASS -> FAIL (fdo#105900)
      shard-kbl:          PASS -> FAIL (fdo#105900)

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-kbl:          NOTRUN -> FAIL (fdo#106641)

    
    ==== Possible fixes ====

    igt@drv_suspend@shrink:
      shard-glk:          FAIL (fdo#106886) -> PASS

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS +1

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS +1

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#102887, fdo#105363) -> PASS

    igt@perf@polling:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4696 -> Patchwork_9991

  CI_DRM_4696: ced152c46fc90f7c1ac8963850d64c9f1ce652d6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4608: 94ebd21177feedf03e8f6dd1e73dca1a6ec7a0ac @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9991: 75e3f1910f813b2dff2ae2794d9e84e9f832188c @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9991/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Tvrtko Ursulin]

On 22/08/2018 16:47, Lionel Landwerlin wrote:
> On 22/08/2018 16:27, Tvrtko Ursulin wrote:
>>
>> On 22/08/2018 16:22, Lionel Landwerlin wrote:
>>> On 22/08/2018 16:17, Tvrtko Ursulin wrote:
>>>>
>>>> On 22/08/2018 16:08, Lionel Landwerlin wrote:
>>>>> On 22/08/2018 15:29, Tvrtko Ursulin wrote:
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>
>>>>>> According to the documentation, when programming the subslice 
>>>>>> count power-
>>>>>> gating configuration register, the value to be written into it on 
>>>>>> Gen9LP
>>>>>> should actually in the format of:
>>>>>>
>>>>>>    1 slice  = 0x001
>>>>>>    2 slices = 0x010
>>>>>>    3 slices = 0x100
>>>>>
>>>>>
>>>>> s/slice/subslice/
>>>>>
>>>>>
>>>>> Also 0b001 etc... Not hexadecimal.
>>>>
>>>> Oops, you're right.
>>>>
>>>>>>
>>>>>> And not the popcount of the enabled subslice mask as on other 
>>>>>> platforms.
>>>>>>
>>>>>> So on Gen9LP platforms we have been programming 0x11 into those 
>>>>>> bits, but
>>>>>> the documentation does not explain what would that achieve. Could 
>>>>>> it be
>>>>>> that we enable only two subslice on three sub-slice parts? Or 
>>>>>> hardware
>>>>>> simply ignores it and sticks with the maximum configuration?
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>>>> Bspec: 12247
>>>>>> ---
>>>>>> Could this actually be true or I am severely misreading the docs? 
>>>>>> It does
>>>>>> not sound plausible to me this would have been missed all this time..
>>>>>>
>>>>>> How to test in what configuration do these parts run before and 
>>>>>> after this
>>>>>> patch?
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
>>>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>>>>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>>>>> index 36050f085071..cdfa962a1975 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>>>> @@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>>>>>       }
>>>>>>       if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>>>>>> +        u8 val;
>>>>>> +
>>>>>> +        val = hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
>>>>>> +
>>>>>> +        if (IS_GEN9_LP(dev_priv))
>>>>>> +            val = BIT(val - 1);
>>>>>
>>>>>
>>>>> Hmm... Are you breaking the 2 subslices setting here then?
>>>>>
>>>>> (2 subslices = 0b10 which should be equal to 
>>>>> hweight8(subslice_mask) if I'm thinking right)
>>>>
>>>> No and yes, I think.
>>>>
>>>> subslice_mask = 0b011 => hweight = 2 => BIT(2 - 1) = BIT(1) = 0b010 
>>>> into the register
>>>>
>>>> In the same way, all together:
>>>>
>>>> subslice_mask = 0b001 => hweight = 1 => BIT(0) = 0b001
>>>> subslice_mask = 0b011 => hweight = 2 => BIT(1) = 0b010
>>>> subslice_mask = 0b111 => hweight = 3 => BIT(2) = 0b100
>>>>
>>>> Have I made a mistake somewhere?
>>>
>>>
>>> Ah, yes! You're right :)
>>>
>>> My eyes got tricked, thanks for finding this out.
>>
>> At least half of the credit goes to you for linking to 12247 in scope 
>> of one different thread!
>>
>>>
>>> With the comment fixed :
>>>
>>>
>>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>
>> Thanks!
>>
>> Any ideas on how to test this? I'd like to commit message to be more 
>> precise - have we been running with one slice too few? Or hardware 
>> ignores the undocumented bit combination? Or even, is the 
>> documentation perhaps incorrect?!
> 
> 
> Yeah the tests that I wrote to test the API you're picking up use a 
> MI_PREDICATE where you can make a command (let's say 
> MI_STORE_REGISTER_MEM) conditional on the number of slice.
> 
> You can use that in a user batch an verify that memory has or hasn't 
> been written to.
> 
> That's only before Gen10 though.
> 
> 
> I'm looking at whether something that is passed onto the EUs could map 
> back to the physical location.
> 
> Not sure if there is something...

I've sent an exploratory IGT based on your MI_SET_PREDICATE code to CI. 
Although that only works on the basis of slices, and here doubt is about 
subslices. But at least it should log subslice configuration so we'll 
see if the incorrect programming survived in the register, or on 
read-back we get a different value.

Otherwise I was thinking if in some Mesa test farm you have an 
appropriate BXT/GLK and could notice a performance difference before and 
after this patch?

But again, sounds so unbelievable we would be running with one disabled 
slice all this time..

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix subslice configuration on Gen9LP

[Tvrtko Ursulin]

On 23/08/2018 09:59, Tvrtko Ursulin wrote:
> On 22/08/2018 16:47, Lionel Landwerlin wrote:
>> On 22/08/2018 16:27, Tvrtko Ursulin wrote:
>>>
>>> On 22/08/2018 16:22, Lionel Landwerlin wrote:
>>>> On 22/08/2018 16:17, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 22/08/2018 16:08, Lionel Landwerlin wrote:
>>>>>> On 22/08/2018 15:29, Tvrtko Ursulin wrote:
>>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>
>>>>>>> According to the documentation, when programming the subslice 
>>>>>>> count power-
>>>>>>> gating configuration register, the value to be written into it on 
>>>>>>> Gen9LP
>>>>>>> should actually in the format of:
>>>>>>>
>>>>>>>    1 slice  = 0x001
>>>>>>>    2 slices = 0x010
>>>>>>>    3 slices = 0x100
>>>>>>
>>>>>>
>>>>>> s/slice/subslice/
>>>>>>
>>>>>>
>>>>>> Also 0b001 etc... Not hexadecimal.
>>>>>
>>>>> Oops, you're right.
>>>>>
>>>>>>>
>>>>>>> And not the popcount of the enabled subslice mask as on other 
>>>>>>> platforms.
>>>>>>>
>>>>>>> So on Gen9LP platforms we have been programming 0x11 into those 
>>>>>>> bits, but
>>>>>>> the documentation does not explain what would that achieve. Could 
>>>>>>> it be
>>>>>>> that we enable only two subslice on three sub-slice parts? Or 
>>>>>>> hardware
>>>>>>> simply ignores it and sticks with the maximum configuration?
>>>>>>>
>>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>>>>> Bspec: 12247
>>>>>>> ---
>>>>>>> Could this actually be true or I am severely misreading the docs? 
>>>>>>> It does
>>>>>>> not sound plausible to me this would have been missed all this 
>>>>>>> time..
>>>>>>>
>>>>>>> How to test in what configuration do these parts run before and 
>>>>>>> after this
>>>>>>> patch?
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++++--
>>>>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>>>>>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>>>>>> index 36050f085071..cdfa962a1975 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>>>>> @@ -2508,9 +2508,15 @@ make_rpcs(struct drm_i915_private *dev_priv)
>>>>>>>       }
>>>>>>>       if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
>>>>>>> +        u8 val;
>>>>>>> +
>>>>>>> +        val = 
>>>>>>> hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
>>>>>>> +
>>>>>>> +        if (IS_GEN9_LP(dev_priv))
>>>>>>> +            val = BIT(val - 1);
>>>>>>
>>>>>>
>>>>>> Hmm... Are you breaking the 2 subslices setting here then?
>>>>>>
>>>>>> (2 subslices = 0b10 which should be equal to 
>>>>>> hweight8(subslice_mask) if I'm thinking right)
>>>>>
>>>>> No and yes, I think.
>>>>>
>>>>> subslice_mask = 0b011 => hweight = 2 => BIT(2 - 1) = BIT(1) = 0b010 
>>>>> into the register
>>>>>
>>>>> In the same way, all together:
>>>>>
>>>>> subslice_mask = 0b001 => hweight = 1 => BIT(0) = 0b001
>>>>> subslice_mask = 0b011 => hweight = 2 => BIT(1) = 0b010
>>>>> subslice_mask = 0b111 => hweight = 3 => BIT(2) = 0b100
>>>>>
>>>>> Have I made a mistake somewhere?
>>>>
>>>>
>>>> Ah, yes! You're right :)
>>>>
>>>> My eyes got tricked, thanks for finding this out.
>>>
>>> At least half of the credit goes to you for linking to 12247 in scope 
>>> of one different thread!
>>>
>>>>
>>>> With the comment fixed :
>>>>
>>>>
>>>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>
>>> Thanks!
>>>
>>> Any ideas on how to test this? I'd like to commit message to be more 
>>> precise - have we been running with one slice too few? Or hardware 
>>> ignores the undocumented bit combination? Or even, is the 
>>> documentation perhaps incorrect?!
>>
>>
>> Yeah the tests that I wrote to test the API you're picking up use a 
>> MI_PREDICATE where you can make a command (let's say 
>> MI_STORE_REGISTER_MEM) conditional on the number of slice.
>>
>> You can use that in a user batch an verify that memory has or hasn't 
>> been written to.
>>
>> That's only before Gen10 though.
>>
>>
>> I'm looking at whether something that is passed onto the EUs could map 
>> back to the physical location.
>>
>> Not sure if there is something...
> 
> I've sent an exploratory IGT based on your MI_SET_PREDICATE code to CI. 
> Although that only works on the basis of slices, and here doubt is about 
> subslices. But at least it should log subslice configuration so we'll 
> see if the incorrect programming survived in the register, or on 
> read-back we get a different value.
> 
> Otherwise I was thinking if in some Mesa test farm you have an 
> appropriate BXT/GLK and could notice a performance difference before and 
> after this patch?
> 
> But again, sounds so unbelievable we would be running with one disabled 
> slice all this time..

Results are finally back 
(https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_2831/fi-glk-dsi/igt@drv_selftest@live_contexts.html 
- starts at timestamp 457.627031 in dmesg) and empirically it seems that 
BXT/GLK handles the SScount identically to big core SKUs.

In other words there is no switching penalty between SScount 0b11 and 
0b100, whilst there is between different configs, implying that 
undocumented 0b11 correctly selected three subslices, equally as the 
documented 0b100.

Small concern is that switching performance between 0b100<->0b10 and 
0b11<->0b10 seems potentially greater than the margin of error which I 
can't explain other than maybe my test being imperfect, or noise is 
greater than I think.

I will raise a note against bspec to see if we can clarify that via that 
route.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx