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