Bug 103484

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

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.

I think this is probably intentional: so you can sample current state without an observational bias?

Funny that it isn't required on SKL (GT4).

Ahaha :)
Repeating the cat manually under gnome-shell, sometimes you get 0, sometimes the actual values :)

It might be a testimony that we're not doing power management very well on some platforms...

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

I think Rodrigo mentioned this problem applies to CNL as well.

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

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

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

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?

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.

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.

Lionel, is this still issue?

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?

Chris, Ville, any help here?

ping

Jani, Joonas, any advice from you on this?

Tentative fix : https://patchwork.freedesktop.org/series/45161/

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

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

Lionel, Is this issue still open?

(In reply to Lakshmi from comment #23)
> Lionel, Is this issue still open?

Yep, unfortunately I didn't get to write a selftest.

(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 ;)

#assessment

Issue seems to be still valid, nothing to add, just meet SLA, investigate and fix eventually...

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/drm/intel/issues/60.