| Message ID | 1464854549-24361-1-git-send-email-david.weinehall@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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; }
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(+)