[PATCH i-g-t 1/2] lib/igt_pm: Introduce helper to check for PM capability

[Kamil Konieczny]

Hi Bernatowicz,,
On 2025-03-13 at 11:30:01 +0100, Bernatowicz, Marcin wrote:
> 
> 
> On 3/11/2025 6:01 PM, Jakub Kolakowski wrote:
> > Add power management capability checking helper. With this helper one
> > can make sure the capability is present on device under test before
> > proceeding with any actions related to PM.
> 
> Maybe mention that it's PCI Power Managment capability, something like:
> "Introduce igt_pm_has_pm_capability(), a helper function to check whether a
> PCI device supports the PCI Power Management (PM) capability. ..."
> 
> This is mainly aimed to check
> > for availability of D-states but overall it checks for the PCI
> > capability in general.
> 
> I would drop this statement, the helper does not explicitly verify support
> for specific D-states (to check for specific D-state availability, the Power
> Management Control/Status Register (PMCSR) must be examined). Also, the
> phrase "checks for the PCI capability in general" is vague.
> 
> > 
> > Cc: Adam Miszczak <adam.miszczak at linux.intel.com>
> > Cc: Lukasz Laguna <lukasz.laguna at intel.com>
> > Cc: Marcin Bernatowicz <marcin.bernatowicz at linux.intel.com>
> > Signed-off-by: Jakub Kolakowski <jakub1.kolakowski at intel.com>
> > ---
> >   lib/igt_pm.c | 20 ++++++++++++++++++++
> >   lib/igt_pm.h |  1 +
> >   2 files changed, 21 insertions(+)
> > 
> > diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> > index 1a5d9c42b..21e3137ab 100644
> > --- a/lib/igt_pm.c
> > +++ b/lib/igt_pm.c
> > @@ -45,6 +45,7 @@
> >   #include "igt_pm.h"
> >   #include "igt_aux.h"
> >   #include "igt_sysfs.h"
> > +#include "igt_pci.h"
> >   /**
> >    * SECTION:igt_pm
> > @@ -81,6 +82,7 @@ enum {
> >   #define MAX_POLICY_STRLEN	strlen(MAX_PERFORMANCE_STR)
> >   /* Root Port bus can have max 32 dev and each dev can have max 8 func */
> >   #define MAX_PCI_DEVICES		256
> > +#define PCI_PM_CAP_ID 0x01
> >   int8_t *__sata_pm_policies;
> >   int __scsi_host_cnt;
> > @@ -1470,3 +1472,21 @@ void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val)
> >   	igt_require(igt_sysfs_has_attr(gtfd, "slpc_ignore_eff_freq"));
> >   	igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val);
> >   }
> > +
> > +
> > +/**
> > + * igt_is_pm_supported:
> > + * @dev: pci device
> > + *
> > + * Returns: True if power management capability is present, otherwise false
> 
> Indicate it's PCI capability: "Returns: true if the device has PCI Power
> Management capability, false otherwise."
> 
> > + */
> > +bool igt_is_pm_supported(struct pci_device *pci_dev)
> 
> Shouldn't the function name start with igt_pm_ ?
> 
> Maybe igt_pm_has_pm_capability(...) ?

Or one of two (latter looks better imo):

igt_pm_has_pci_pm_capability()

igt_has_pci_pm_capability()

Regards,
Kamil

> 
> > +{
> > +	int offset;
> > +
> > +	offset = find_pci_cap_offset(pci_dev, PCI_PM_CAP_ID);
> > +	if (offset > 0)
> > +		return true;
> > +
> > +	return false;
> 
> return (offset > 0);
> 
> > +}
> > \ No newline at end of file
> > diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> > index 6b428f53e..befb61f3c 100644
> > --- a/lib/igt_pm.h
> > +++ b/lib/igt_pm.h
> > @@ -97,5 +97,6 @@ uint64_t igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev);
> >   uint64_t igt_pm_get_runtime_active_time(struct pci_device *pci_dev);
> >   int igt_pm_get_runtime_usage(struct pci_device *pci_dev);
> >   void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val);
> > +bool igt_is_pm_supported(struct pci_device *pci_dev);
> >   #endif /* IGT_PM_H */
>