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

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Friday, April 1, 2022 7:40 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>; Dixit, Ashutosh
> <ashutosh.dixit@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 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 6:26 PM
> >> To: Gupta, Anshuman <anshuman.gupta@intel.com>; Dixit, Ashutosh
> >> <ashutosh.dixit@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 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?
> > Benefit is there to debug CI runtime suspend failures , we need to know the
> culprit child blocking i915 runtime PM.
> > runtime_active_kids just revels the count , it doesn't reveal the culprit children.
> 
> I understand. But how is that problem or the information specific to i915? Why
> should this be added to i915 instead of runtime pm infra?
> Surely this is not even a new problem; how do others currently figure this
> information out?
> 
> So I'm not going to block this if you all think this is a good idea. But the point is,
> the first solution should not be to add some i915 specific stuff when a more
> generic solution might exist or be preferred.
Hi Rafael,
Could you please provide your input,  about generic interface to dump the active children of a device.
Thanks,
Anshuman Gupta.
> 
> 
> BR,
> Jani.
> 
> 
> 
> 
> > Thanks,
> > Anshuman.
> >>
> >> 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
> 
> --
> Jani Nikula, Intel Open Source Graphics Center