[Intel-gfx] [PATCH] drm/i915/debugfs: Dump i915 children runtime status

[Gupta, Anshuman]

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Friday, April 1, 2022 5:31 PM
> To: Dixit, Ashutosh <ashutosh.dixit at intel.com>; Gupta, Anshuman
> <anshuman.gupta at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Wilson, Chris P <chris.p.wilson at intel.com>;
> Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/debugfs: Dump i915 children runtime
> status
> 
> On Tue, 29 Mar 2022, "Dixit, Ashutosh" <ashutosh.dixit at intel.com> wrote:
> > On Mon, 28 Mar 2022 03:22:27 -0700, Anshuman Gupta wrote:
> >>
> >> +#ifdef CONFIG_PM
> >> +static int i915_runtime_dump_child_status(struct device *dev, void
> >> +*data) {
> >> +	struct seq_file *m = data;
> >> +	const char *rpm_status;
> >> +
> >> +	/* Early return if runtime_pm is disabled */
> >> +	if (dev->power.disable_depth)
> >> +		return 0;
> >> +
> >> +	switch (dev->power.runtime_status) {
> >> +	case RPM_SUSPENDED:
> >> +		rpm_status = "suspended";
> >> +		break;
> >> +	case RPM_SUSPENDING:
> >> +		rpm_status = "suspending";
> >> +		break;
> >> +	case RPM_RESUMING:
> >> +		rpm_status = "resuming";
> >> +		break;
> >> +	case RPM_ACTIVE:
> >> +		rpm_status = "active";
> >> +		break;
> >> +	default:
> >> +		rpm_status = "unknown";
> >> +	}
> >> +
> >> +	seq_printf(m, "\t%s %s: Runtime status: %s\n", dev_driver_string(dev),
> >> +		   dev_name(dev), rpm_status);
> >> +
> >> +	return 0;
> >> +}
> >> +#endif
> >
> > Maybe a nit, but perhaps defining a const array is better than having
> > a switch statement? Similar to what is done in rtpm_status_str(). The
> > function itself is very similar to rtpm_status_str() so can probably
> > benefit from that similarity. Can perhaps even be nearly identical to
> > rtpm_status_str() (since that is static in the genpd (generic power
> > domain) code).
> >
> > See also 2bd5306a8764 ("PM / Domains: add debugfs listing of struct
> > generic_pm_domain-s"), though I am not sure if genpd's are applicable
> > in our case and certainly look way out of scope for now. Thanks.
> 
> See also /sys/devices/i915/power/runtime_status and
> /sys/devices/i915/power/runtime_active_kids.
> 
> Kinda feels like the info should be made available there?
runtime_active_kids we are already printing by dev_priv->drm.dev->power.child_count.
About runtime_status , we already prints usage count and pci device power state, IMO that is sufficient for debug ?
If it is really needed , I will add dev->power.runtime_status in next revision.
Thanks,
Anshuman Gupta.




> 
> BR,
> Jani.
> 
> >
> >> +
> >>  static int i915_runtime_pm_status(struct seq_file *m, void *unused)
> >>  {
> >>	struct drm_i915_private *dev_priv = node_to_i915(m->private);  @@
> >>-500,6 +534,10 @@ static int i915_runtime_pm_status(struct seq_file
> >>*m, void *unused)
> >>  #ifdef CONFIG_PM
> >>	seq_printf(m, "Usage count: %d\n",
> >>		   atomic_read(&dev_priv->drm.dev->power.usage_count));
> >> +	seq_printf(m, "Runtime active children: %d\n",
> >> +		   atomic_read(&dev_priv->drm.dev->power.child_count));
> >> +	device_for_each_child(&pdev->dev, m,
> >> +i915_runtime_dump_child_status);
> >> +
> >>  #else
> >>	seq_printf(m, "Device Power Management (CONFIG_PM) disabled\n");
> >>  #endif
> >> --
> >> 2.26.2
> >>
> 
> --
> Jani Nikula, Intel Open Source Graphics Center