[PATCH] drm/i915/debugfs: Missing intel_runtime_pm_*

[PATCH] drm/i915/debugfs: Missing intel_runtime_pm_*

[David Weinehall]

i915_sseu_status() was missing intel_runtime_pm_{get,put},
meaning that in some cases access to HW would be attempted
while the device is suspended.

Testcase: igt/pm_rpm/debugfs-read
Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e4f2c55d9697..694b0d394c7b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -5290,11 +5290,14 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct sseu_dev_status stat;
 
 	if (INTEL_INFO(dev)->gen < 8)
 		return -ENODEV;
 
+	intel_runtime_pm_get(dev_priv);
+
 	seq_puts(m, "SSEU Device Info\n");
 	seq_printf(m, "  Available Slice Total: %u\n",
 		   INTEL_INFO(dev)->slice_total);
@@ -5333,6 +5336,8 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
 	seq_printf(m, "  Enabled EU Per Subslice: %u\n",
 		   stat.eu_per_subslice);
 
+	intel_runtime_pm_put(dev_priv);
+
 	return 0;
 }
 
-- 
2.8.1

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

Re: [PATCH] drm/i915/debugfs: Missing intel_runtime_pm_*

[Chris Wilson]

On Thu, Jun 02, 2016 at 11:02:29AM +0300, David Weinehall wrote:
> i915_sseu_status() was missing intel_runtime_pm_{get,put},
> meaning that in some cases access to HW would be attempted
> while the device is suspended.
> 
> Testcase: igt/pm_rpm/debugfs-read
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e4f2c55d9697..694b0d394c7b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -5290,11 +5290,14 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
>  {
>  	struct drm_info_node *node = (struct drm_info_node *) m->private;
>  	struct drm_device *dev = node->minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct sseu_dev_status stat;
>  
>  	if (INTEL_INFO(dev)->gen < 8)

whilst here: INTEL_GEN(dev_priv) and may as well fix any other dev in
the function.

>  		return -ENODEV;
>  
> +	intel_runtime_pm_get(dev_priv);

I was wondering if instead we wanted a 

...sw dump...
if (intel_runtime_pm_get_if_noidle(dev_priv)) {
	...hw dump...
	intel_runtime_pm_put();
} else {
	seq_printf(m, "Device powered down, not probing current HW configuration.\n");
}

As far as the basic fix goes,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/debugfs: Missing intel_runtime_pm_*

[David Weinehall]

On Thu, Jun 02, 2016 at 09:15:00AM +0100, Chris Wilson wrote:
> On Thu, Jun 02, 2016 at 11:02:29AM +0300, David Weinehall wrote:
> > i915_sseu_status() was missing intel_runtime_pm_{get,put},
> > meaning that in some cases access to HW would be attempted
> > while the device is suspended.
> > 
> > Testcase: igt/pm_rpm/debugfs-read
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index e4f2c55d9697..694b0d394c7b 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -5290,11 +5290,14 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
> >  {
> >  	struct drm_info_node *node = (struct drm_info_node *) m->private;
> >  	struct drm_device *dev = node->minor->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct sseu_dev_status stat;
> >  
> >  	if (INTEL_INFO(dev)->gen < 8)
> 
> whilst here: INTEL_GEN(dev_priv) and may as well fix any other dev in
> the function.
> 
> >  		return -ENODEV;
> >  
> > +	intel_runtime_pm_get(dev_priv);
> 
> I was wondering if instead we wanted a 
> 
> ...sw dump...
> if (intel_runtime_pm_get_if_noidle(dev_priv)) {
> 	...hw dump...
> 	intel_runtime_pm_put();
> } else {
> 	seq_printf(m, "Device powered down, not probing current HW configuration.\n");
> }

There are a lot of cases in the file with similar behaviour.
Maybe a separate patch to clear them all up at once would make sense.
The same goes for dev -> dev_priv (one patch for each of these two
transitions).


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

Re: [PATCH] drm/i915/debugfs: Missing intel_runtime_pm_*

[Chris Wilson]

On Thu, Jun 02, 2016 at 11:31:03AM +0300, David Weinehall wrote:
> On Thu, Jun 02, 2016 at 09:15:00AM +0100, Chris Wilson wrote:
> > On Thu, Jun 02, 2016 at 11:02:29AM +0300, David Weinehall wrote:
> > > i915_sseu_status() was missing intel_runtime_pm_{get,put},
> > > meaning that in some cases access to HW would be attempted
> > > while the device is suspended.
> > > 
> > > Testcase: igt/pm_rpm/debugfs-read
> > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index e4f2c55d9697..694b0d394c7b 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -5290,11 +5290,14 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
> > >  {
> > >  	struct drm_info_node *node = (struct drm_info_node *) m->private;
> > >  	struct drm_device *dev = node->minor->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;

Oh, this definitely should be to_i915(dev)

> > >  	struct sseu_dev_status stat;
> > >  
> > >  	if (INTEL_INFO(dev)->gen < 8)
> > 
> > whilst here: INTEL_GEN(dev_priv) and may as well fix any other dev in
> > the function.
> > 
> > >  		return -ENODEV;
> > >  
> > > +	intel_runtime_pm_get(dev_priv);
> > 
> > I was wondering if instead we wanted a 
> > 
> > ...sw dump...
> > if (intel_runtime_pm_get_if_noidle(dev_priv)) {
> > 	...hw dump...
> > 	intel_runtime_pm_put();
> > } else {
> > 	seq_printf(m, "Device powered down, not probing current HW configuration.\n");
> > }
> 
> There are a lot of cases in the file with similar behaviour.
> Maybe a separate patch to clear them all up at once would make sense.
> The same goes for dev -> dev_priv (one patch for each of these two
> transitions).

There are schools here:

1) big patches doing mechanical change
2) small patches fixing up surrounding code as you drive by


Both have their advantages (small patches -> less conflict, more likely
to tidy up surrouding code, big patches -> more chance of conflict, more
or less needs sed/coccinelle, artifacts from mechanical changes).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/debugfs: Missing intel_runtime_pm_*

[David Weinehall]

i915_sseu_status() was missing intel_runtime_pm_{get,put},
meaning that in some cases access to HW would be attempted
while the device is suspended.

v2: Rewrite to use dev_priv and to_i915() instead,
    as suggested by Chris Wilson

Testcase: igt/pm_rpm/debugfs-read
Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 62 +++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e4f2c55d9697..884ece0e2a5e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -5164,10 +5164,9 @@ struct sseu_dev_status {
 	unsigned int eu_per_subslice;
 };
 
-static void cherryview_sseu_device_status(struct drm_device *dev,
+static void cherryview_sseu_device_status(struct drm_i915_private *dev_priv,
 					  struct sseu_dev_status *stat)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ss_max = 2;
 	int ss;
 	u32 sig1[ss_max], sig2[ss_max];
@@ -5196,16 +5195,15 @@ static void cherryview_sseu_device_status(struct drm_device *dev,
 	stat->subslice_total = stat->subslice_per_slice;
 }
 
-static void gen9_sseu_device_status(struct drm_device *dev,
+static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
 				    struct sseu_dev_status *stat)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	int s_max = 3, ss_max = 4;
 	int s, ss;
 	u32 s_reg[s_max], eu_reg[2*s_max], eu_mask[2];
 
 	/* BXT has a single slice and at most 3 subslices. */
-	if (IS_BROXTON(dev)) {
+	if (IS_BROXTON(dev_priv)) {
 		s_max = 1;
 		ss_max = 3;
 	}
@@ -5234,18 +5232,18 @@ static void gen9_sseu_device_status(struct drm_device *dev,
 
 		stat->slice_total++;
 
-		if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
-			ss_cnt = INTEL_INFO(dev)->subslice_per_slice;
+		if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
+			ss_cnt = INTEL_INFO(dev_priv)->subslice_per_slice;
 
 		for (ss = 0; ss < ss_max; ss++) {
 			unsigned int eu_cnt;
 
-			if (IS_BROXTON(dev) &&
+			if (IS_BROXTON(dev_priv) &&
 			    !(s_reg[s] & (GEN9_PGCTL_SS_ACK(ss))))
 				/* skip disabled subslice */
 				continue;
 
-			if (IS_BROXTON(dev))
+			if (IS_BROXTON(dev_priv))
 				ss_cnt++;
 
 			eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
@@ -5261,25 +5259,25 @@ static void gen9_sseu_device_status(struct drm_device *dev,
 	}
 }
 
-static void broadwell_sseu_device_status(struct drm_device *dev,
+static void broadwell_sseu_device_status(struct drm_i915_private *dev_priv,
 					 struct sseu_dev_status *stat)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	int s;
 	u32 slice_info = I915_READ(GEN8_GT_SLICE_INFO);
 
 	stat->slice_total = hweight32(slice_info & GEN8_LSLICESTAT_MASK);
 
 	if (stat->slice_total) {
-		stat->subslice_per_slice = INTEL_INFO(dev)->subslice_per_slice;
+		stat->subslice_per_slice =
+				INTEL_INFO(dev_priv)->subslice_per_slice;
 		stat->subslice_total = stat->slice_total *
 				       stat->subslice_per_slice;
-		stat->eu_per_subslice = INTEL_INFO(dev)->eu_per_subslice;
+		stat->eu_per_subslice = INTEL_INFO(dev_priv)->eu_per_subslice;
 		stat->eu_total = stat->eu_per_subslice * stat->subslice_total;
 
 		/* subtract fused off EU(s) from enabled slice(s) */
 		for (s = 0; s < stat->slice_total; s++) {
-			u8 subslice_7eu = INTEL_INFO(dev)->subslice_7eu[s];
+			u8 subslice_7eu = INTEL_INFO(dev_priv)->subslice_7eu[s];
 
 			stat->eu_total -= hweight8(subslice_7eu);
 		}
@@ -5289,38 +5287,40 @@ static void broadwell_sseu_device_status(struct drm_device *dev,
 static int i915_sseu_status(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
-	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = to_i915(node->minor->dev);
 	struct sseu_dev_status stat;
 
-	if (INTEL_INFO(dev)->gen < 8)
+	if (INTEL_GEN(dev_priv) < 8)
 		return -ENODEV;
 
+	intel_runtime_pm_get(dev_priv);
+
 	seq_puts(m, "SSEU Device Info\n");
 	seq_printf(m, "  Available Slice Total: %u\n",
-		   INTEL_INFO(dev)->slice_total);
+		   INTEL_INFO(dev_priv)->slice_total);
 	seq_printf(m, "  Available Subslice Total: %u\n",
-		   INTEL_INFO(dev)->subslice_total);
+		   INTEL_INFO(dev_priv)->subslice_total);
 	seq_printf(m, "  Available Subslice Per Slice: %u\n",
-		   INTEL_INFO(dev)->subslice_per_slice);
+		   INTEL_INFO(dev_priv)->subslice_per_slice);
 	seq_printf(m, "  Available EU Total: %u\n",
-		   INTEL_INFO(dev)->eu_total);
+		   INTEL_INFO(dev_priv)->eu_total);
 	seq_printf(m, "  Available EU Per Subslice: %u\n",
-		   INTEL_INFO(dev)->eu_per_subslice);
+		   INTEL_INFO(dev_priv)->eu_per_subslice);
 	seq_printf(m, "  Has Slice Power Gating: %s\n",
-		   yesno(INTEL_INFO(dev)->has_slice_pg));
+		   yesno(INTEL_INFO(dev_priv)->has_slice_pg));
 	seq_printf(m, "  Has Subslice Power Gating: %s\n",
-		   yesno(INTEL_INFO(dev)->has_subslice_pg));
+		   yesno(INTEL_INFO(dev_priv)->has_subslice_pg));
 	seq_printf(m, "  Has EU Power Gating: %s\n",
-		   yesno(INTEL_INFO(dev)->has_eu_pg));
+		   yesno(INTEL_INFO(dev_priv)->has_eu_pg));
 
 	seq_puts(m, "SSEU Device Status\n");
 	memset(&stat, 0, sizeof(stat));
-	if (IS_CHERRYVIEW(dev)) {
-		cherryview_sseu_device_status(dev, &stat);
-	} else if (IS_BROADWELL(dev)) {
-		broadwell_sseu_device_status(dev, &stat);
-	} else if (INTEL_INFO(dev)->gen >= 9) {
-		gen9_sseu_device_status(dev, &stat);
+	if (IS_CHERRYVIEW(dev_priv)) {
+		cherryview_sseu_device_status(dev_priv, &stat);
+	} else if (IS_BROADWELL(dev_priv)) {
+		broadwell_sseu_device_status(dev_priv, &stat);
+	} else if (INTEL_INFO(dev_priv)->gen >= 9) {
+		gen9_sseu_device_status(dev_priv, &stat);
 	}
 	seq_printf(m, "  Enabled Slice Total: %u\n",
 		   stat.slice_total);
@@ -5333,6 +5333,8 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
 	seq_printf(m, "  Enabled EU Per Subslice: %u\n",
 		   stat.eu_per_subslice);
 
+	intel_runtime_pm_put(dev_priv);
+
 	return 0;
 }
 
-- 
2.8.1

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

✗ Ro.CI.BAT: warning for drm/i915/debugfs: Missing intel_runtime_pm_* (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/debugfs: Missing intel_runtime_pm_* (rev2)
URL   : https://patchwork.freedesktop.org/series/8140/
State : warning

== Summary ==

Series 8140v2 drm/i915/debugfs: Missing intel_runtime_pm_*
http://patchwork.freedesktop.org/api/1.0/series/8140/revisions/2/mbox

Test core_auth:
        Subgroup basic-auth:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_busy:
        Subgroup basic-parallel-vebox:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup basic-vebox:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_exec_basic:
        Subgroup gtt-blt:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_exec_flush:
        Subgroup basic-uc-pro-default:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup basic-uc-set-default:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
        Subgroup basic-wb-pro-default:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_mmap_gtt:
        Subgroup basic-write-gtt:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
        Subgroup basic-write-gtt-no-prefault:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup basic-vebox:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_tiled_pread_basic:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test kms_addfb_basic:
        Subgroup addfb25-bad-modifier:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup addfb25-framebuffer-vs-set-tiling:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup bad-pitch-128:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup bad-pitch-256:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
        Subgroup bad-pitch-32:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
        Subgroup basic-y-tiled:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup unused-modifier:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
        Subgroup unused-offsets:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> SKIP       (fi-skl-i5-6260u)

fi-hsw-i7-4770k  total:209  pass:190  dwarn:0   dfail:0   fail:0   skip:19 
fi-skl-i5-6260u  total:209  pass:197  dwarn:0   dfail:0   fail:0   skip:12 
fi-skl-i7-6700k  total:209  pass:184  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:209  pass:170  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:102  pass:93   dwarn:0   dfail:0   fail:0   skip:8  
ro-bdw-i7-5600u  total:102  pass:75   dwarn:0   dfail:0   fail:0   skip:26 
ro-bsw-n3050     total:209  pass:168  dwarn:0   dfail:0   fail:2   skip:39 
ro-hsw-i3-4010u  total:209  pass:186  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:102  pass:82   dwarn:0   dfail:0   fail:0   skip:19 
ro-ilk1-i5-650   total:204  pass:146  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:102  pass:75   dwarn:0   dfail:0   fail:0   skip:26 
ro-skl-i7-6700hq total:204  pass:172  dwarn:11  dfail:0   fail:0   skip:21 
ro-snb-i7-2620M  total:102  pass:72   dwarn:0   dfail:0   fail:0   skip:29 
fi-bdw-i7-5557u failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot
ro-ivb2-i7-3770 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1082/

976092d drm-intel-nightly: 2016y-06m-02d-15h-42m-07s UTC integration manifest
bbd2511 drm/i915/debugfs: Missing intel_runtime_pm_*

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