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

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

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@linux.intel.com>
> Cc: Lukasz Laguna <lukasz.laguna@intel.com>
> Cc: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> Signed-off-by: Jakub Kolakowski <jakub1.kolakowski@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(...) ?

> +{
> +	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 */