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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 01 Apr 2022, "Gupta, Anshuman" <anshuman.gupta@intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@linux.intel.com>
>> Sent: Friday, April 1, 2022 5:31 PM
>> To: Dixit, Ashutosh <ashutosh.dixit@intel.com>; Gupta, Anshuman
>> <anshuman.gupta@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; Wilson, Chris P <chris.p.wilson@intel.com>;
>> Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915/debugfs: Dump i915 children runtime
>> status
>> 
>> On Tue, 29 Mar 2022, "Dixit, Ashutosh" <ashutosh.dixit@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.

My point is, the patch at hand adds runtime pm status printing that
isn't specific to drm or i915 into i915 debugfs. Why?

What is the reason we should take on the burden of maintaining this
while the right place for it might be in runtime pm code, benefiting
other drivers in addition to ours?

BR,
Jani.


> 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

-- 
Jani Nikula, Intel Open Source Graphics Center