Bug 103484 - SSEU status is all 0 on BXT
Summary: SSEU status is all 0 on BXT
Status: NEW
Alias: None
Product: DRI
Classification: Unclassified
Component: DRM/Intel (show other bugs)
Version: DRI git
Hardware: Other All
: medium normal
Assignee: Intel GFX Bugs mailing list
QA Contact: Intel GFX Bugs mailing list
URL:
Whiteboard: Triaged, ReadyForDev
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-27 15:00 UTC by Lionel Landwerlin
Modified: 2018-09-20 15:06 UTC (History)
4 users (show)

See Also:
i915 platform: BXT, CNL
i915 features: display/Other


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lionel Landwerlin 2017-10-27 15:00:42 UTC
I have a BXT/APL 2x6 PCIID: 0x5a85

cat /sys/kernel/debug/dri/0/i915_sseu_status
SSEU Device Info
  Available Slice Mask: 0001
  Available Slice Total: 1
  Available Subslice Total: 2
  Available Slice0 Subslice Mask: 0006
  Available EU Total: 12
  Available EU Per Subslice: 6
  Has Pooled EU: no
  Has Slice Power Gating: no
  Has Subslice Power Gating: yes
  Has EU Power Gating: yes
SSEU Device Status
  Enabled Slice Mask: 0000
  Enabled Slice Total: 0
  Enabled Subslice Total: 0
  Enabled EU Total: 0
  Enabled EU Per Subslice: 0

It's not really critical.
I couldn't find anything in the documentation that says we should read the power gating in a different way from other gen9 platforms.
I also verified we actually program the slice/subslice power gating, so it seems like we should get the ack.
Comment 1 Chris Wilson 2017-10-27 15:33:42 UTC
8086:5a85 snap!

ickle@broxton:~$ sudo cat /sys/kernel/debug/dri/0/i915_sseu_status
SSEU Device Info
  Available Slice Mask: 0001
  Available Slice Total: 1
  Available Subslice Total: 2
  Available Subslice Mask: 0006
  Available Subslice Per Slice: 2
  Available EU Total: 12
  Available EU Per Subslice: 6
  Has Pooled EU: no
  Has Slice Power Gating: no
  Has Subslice Power Gating: yes
  Has EU Power Gating: yes
SSEU Device Status
  Enabled Slice Mask: 0000
  Enabled Slice Total: 0
  Enabled Subslice Total: 0
  Enabled Subslice Mask: 0000
  Enabled Subslice Per Slice: 0
  Enabled EU Total: 0
  Enabled EU Per Subslice: 0
Comment 2 Chris Wilson 2017-10-27 15:39:04 UTC
Oh hum:

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index daf7a16a4ee2..53f01a87673e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4587,6 +4587,7 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
        memset(&sseu, 0, sizeof(sseu));
 
        intel_runtime_pm_get(dev_priv);
+       intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
        if (IS_CHERRYVIEW(dev_priv)) {
                cherryview_sseu_device_status(dev_priv, &sseu);
@@ -4596,6 +4597,7 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
                gen9_sseu_device_status(dev_priv, &sseu);
        }
 
+       intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
        intel_runtime_pm_put(dev_priv);
 
        i915_print_sseu_info(m, false, &sseu);

So while the registers may not need fw for read; the device does need to be awake for the eu to be ungated.
Comment 3 Chris Wilson 2017-10-27 15:39:36 UTC
I think this is probably intentional: so you can sample current state without an observational bias?
Comment 4 Lionel Landwerlin 2017-10-27 15:41:29 UTC
Funny that it isn't required on SKL (GT4).
Comment 5 Lionel Landwerlin 2017-10-27 15:46:15 UTC
Ahaha :)
Repeating the cat manually under gnome-shell, sometimes you get 0, sometimes the actual values :)
Comment 6 Lionel Landwerlin 2017-10-27 15:47:07 UTC
It might be a testimony that we're not doing power management very well on some platforms...
Comment 7 Chris Wilson 2017-10-27 16:07:31 UTC
(In reply to Lionel Landwerlin from comment #6)
> It might be a testimony that we're not doing power management very well on
> some platforms...

Or that those registers are meant to be fw, and we haven't hooked up bxt correctly?

Hmm.

#define GEN9_SLICE_PGCTL_ACK(slice)     _MMIO(0x804c + (slice)*0x4)

static const struct intel_forcewake_range __gen9_fw_ranges[] = {
...
        GEN_FW_RANGE(0x8000, 0x812f, FORCEWAKE_BLITTER),

so the register read does require fw. So

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 3d91d8a28d9f..56445b06a68c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -812,7 +812,7 @@ static const struct intel_forcewake_range __gen9_fw_ranges[] = {
        GEN_FW_RANGE(0x3000, 0x3fff, FORCEWAKE_RENDER),
        GEN_FW_RANGE(0x4000, 0x51ff, FORCEWAKE_BLITTER),
        GEN_FW_RANGE(0x5200, 0x7fff, FORCEWAKE_RENDER),
-       GEN_FW_RANGE(0x8000, 0x812f, FORCEWAKE_BLITTER),
+       GEN_FW_RANGE(0x8000, 0x812f, FORCEWAKE_BLITTER | FORCEWAKE_RENDER),
        GEN_FW_RANGE(0x8130, 0x813f, FORCEWAKE_MEDIA),
        GEN_FW_RANGE(0x8140, 0x815f, FORCEWAKE_RENDER),
        GEN_FW_RANGE(0x8160, 0x82ff, FORCEWAKE_BLITTER),

is also sufficient. I'm starting to suspect the domain is wrong here.
Comment 8 Lionel Landwerlin 2017-10-31 17:23:50 UTC
I think Rodrigo mentioned this problem applies to CNL as well.
Comment 9 Tvrtko Ursulin 2017-11-09 09:20:31 UTC
(In reply to Lionel Landwerlin from comment #4)
> Funny that it isn't required on SKL (GT4).

Daniele points out this would be due WaRsDisableCoarsePowerGating on GT3/4.

He also double-checked the docs and how the other OS does it and we are apprently fully compliant.

So it may just be we are the first to notice the issue.
Comment 10 Jari Tahvanainen 2017-12-12 10:48:53 UTC
(In reply to Lionel Landwerlin from comment #8)
> I think Rodrigo mentioned this problem applies to CNL as well.

I added CNL to i915 Platforms list, please remove if not valid for CNL.
Comment 11 Lionel Landwerlin 2018-02-19 14:39:38 UTC
I suspect this problem exists on KBL as well : https://intel-gfx-ci.01.org/tree/drm-tip/kasan_6/fi-kbl-7560u/igt@pm_sseu@full-enable.html
Comment 12 Rodrigo Vivi 2018-02-28 22:21:00 UTC
Oh,,, I didn't even considered this a bug, but an expected behaviour to see what EUs were really powered.

So, or we assume this is expected behaviour and close the bug or we go with the forcewake that Chris proposed.

What is the downside of reading the actual slices status?
Comment 13 Lionel Landwerlin 2018-02-28 23:12:02 UTC
Most of the register that give us the power status of slices/subslices/EUs are named "... Acknowledge".
So it sounds like they report the last programmed status.

The register setting the slice/subslice/EU powergating state (0x20c8), is only programmable through the RCS. So it would make sense that we need to forcewake render to be able to read a sensible status.

That would explain why we see 0s on pretty much all platforms.
Comment 14 Jani Saarinen 2018-03-29 07:10:42 UTC
First of all. Sorry about spam.
This is mass update for our bugs. 

Sorry if you feel this annoying but with this trying to understand if bug still valid or not.
If bug investigation still in progress, please ignore this and I apologize!

If you think this is not anymore valid, please comment to the bug that can be closed.
If you haven't tested with our latest pre-upstream tree(drm-tip), can you do that also to see if issue is valid there still and if you cannot see issue there, please comment to the bug.
Comment 15 Jani Saarinen 2018-04-20 14:42:17 UTC
Lionel, is this still issue?
Comment 16 Lionel Landwerlin 2018-04-20 16:44:50 UTC
I didn't see a fix go into i915 (I could have missed it).
But regardless I don't think reading the register directly is a good idea.

We have some work in the pipe to enable powergating per context and that I'm sure will lead to different result depending on when the register are read.

Should we have a kernel context emit a few MI_STORE_REGISTER_MEM and read back the result? Or is that overkill?
Comment 17 Jani Saarinen 2018-04-20 20:20:28 UTC
Chris, Ville, any help here?
Comment 18 Jani Saarinen 2018-04-27 12:33:14 UTC
ping
Comment 19 Jani Saarinen 2018-05-04 09:58:04 UTC
Jani, Joonas, any advice from you on this?
Comment 20 Lionel Landwerlin 2018-06-21 11:37:31 UTC
Tentative fix : https://patchwork.freedesktop.org/series/45161/
Comment 21 Lionel Landwerlin 2018-06-21 13:49:06 UTC
Chris suggests we move the igt test as a selftest in i915 for 3 reasons :

1. ack registers are priveleged so must be read from kernel
2. need to read the value from command streamer because we need HW awake + a context scheduled (ack register values reflect the programmed value in context image)
3. there is some ongoing work to change powergating configuration dynamically and that will likely rely on heuristic in i915, which means userspace can probably only guess the value in the ack registers
Comment 22 Lionel Landwerlin 2018-06-21 13:49:56 UTC
(In reply to Lionel Landwerlin from comment #21)
> Chris suggests we move the igt test as a selftest in i915 for 3 reasons :
> 
> 1. ack registers are priveleged so must be read from kernel
> 2. need to read the value from command streamer because we need HW awake + a
> context scheduled (ack register values reflect the programmed value in
> context image)
> 3. there is some ongoing work to change powergating configuration
> dynamically and that will likely rely on heuristic in i915, which means
> userspace can probably only guess the value in the ack registers

I should add that the selftest should only verify the initial value programmed is correct.
Comment 23 Lakshmi 2018-09-07 07:36:40 UTC
Lionel, Is this issue still open?
Comment 24 Lionel Landwerlin 2018-09-07 09:29:10 UTC
(In reply to Lakshmi from comment #23)
> Lionel, Is this issue still open?

Yep, unfortunately I didn't get to write a selftest.
Comment 25 Lionel Landwerlin 2018-09-11 12:40:49 UTC
(In reply to Lionel Landwerlin from comment #24)
> (In reply to Lakshmi from comment #23)
> > Lionel, Is this issue still open?
> 
> Yep, unfortunately I didn't get to write a selftest.

I was trying to hint that someone working on i915 could do the work ;)


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.