[PATCH i-g-t 2/5] lib/igt_pm: Fix and standardize IGT PM library documentation

Riana Tauro riana.tauro at intel.com
Tue May 14 06:49:03 UTC 2024


Hi Rodrigo

On 5/14/2024 12:25 AM, Rodrigo Vivi wrote:
> A revamp to get some kind of standard in the various exported
> functions in this library.
> 
> Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>   lib/igt_pm.c | 148 +++++++++++++++++++++++++--------------------------
>   1 file changed, 73 insertions(+), 75 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 928b72685..2c91aeb33 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -454,7 +454,7 @@ static void __igt_pm_restore_sata_link_power_management(void)
>   /**
>    * igt_pm_enable_sata_link_power_management:
>    *
> - * Enable the min_power policy for SATA link power management.
> + * Enables the min_power policy for SATA link power management.
>    * Without this we cannot reach deep runtime power states.
>    */
>   void igt_pm_enable_sata_link_power_management(void)
> @@ -469,7 +469,7 @@ void igt_pm_enable_sata_link_power_management(void)
>   /**
>    * igt_pm_restore_sata_link_power_management:
>    *
> - * Restore the link power management policies to the values
> + * Restores the link power management policies to the values
>    * prior to enabling min_power.
>    *
>    * Caveat: If the system supports hotplugging and hotplugging takes
> @@ -566,8 +566,7 @@ static void __igt_pm_runtime_exit_handler(int sig)
>    * Sets up the runtime PM helper functions and enables runtime PM. To speed up
>    * tests the autosuspend delay is set to 0.
>    *
> - * Returns:
> - * True if runtime pm is available, false otherwise.
> + * Return: True if runtime pm is available, false otherwise.
>    */
>   bool igt_setup_runtime_pm(int device)
>   {
> @@ -658,7 +657,7 @@ bool igt_setup_runtime_pm(int device)
>   /**
>    * igt_disable_runtime_pm:
>    *
> - * Disable the runtime pm for i915 device.
> + * Disables the runtime pm for i915 device.
>    * igt_disable_runtime_pm assumes that igt_setup_runtime_pm has already
>    * called to save runtime autosuspend and control attributes.
>    */
> @@ -683,11 +682,6 @@ void igt_disable_runtime_pm(void)
>   	close(fd);
>   }
>   
> -/**
> - * igt_get_runtime_pm_status:
> - *
> - * Returns: The current runtime PM status.
> - */
>   static enum igt_runtime_pm_status __igt_get_runtime_pm_status(int fd)
>   {
>   	ssize_t n_read;
> @@ -711,6 +705,11 @@ static enum igt_runtime_pm_status __igt_get_runtime_pm_status(int fd)
>   	return IGT_RUNTIME_PM_STATUS_UNKNOWN;
>   }
>   
> +/**
> + * igt_get_runtime_pm_status:
> + *
> + * Return: The current runtime PM status.
> + */
>   enum igt_runtime_pm_status igt_get_runtime_pm_status(void)
>   {
>   	enum igt_runtime_pm_status status;
> @@ -728,12 +727,6 @@ enum igt_runtime_pm_status igt_get_runtime_pm_status(void)
>   	return status;
>   }
>   
> -/**
> - * _pm_status_name
> - * @status: runtime PM status to stringify
> - *
> - * Returns: The current runtime PM status as a string
> - */
>   static const char *_pm_status_name(enum igt_runtime_pm_status status)
>   {
>   	switch (status) {
> @@ -757,9 +750,8 @@ static const char *_pm_status_name(enum igt_runtime_pm_status status)
>    * Waits until for the driver to switch to into the desired runtime PM status,
>    * with a 10 second timeout.
>    *
> - * Returns:
> - * True if the desired runtime PM status was attained, false if the operation
> - * timed out.
> + * Return: True if the desired runtime PM status was attained, false if the
> + *         operation timed out.
>    */
>   bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
>   {
> @@ -791,15 +783,14 @@ static const char *yesno(bool x)
>   }
>   
>   /**
> - * dmc_loaded:
> - * @debugfs: fd to the debugfs dir.
> + * igt_pm_dmc_loaded:
> + * @debugfs: FD to the debugfs directory
>    *
>    * Check whether DMC FW is loaded or not. DMC FW is require for few Display C
>    * states like DC5 and DC6. FW does the Context Save and Restore during Display
>    * C States entry and exit.
>    *
> - * Returns:
> - * True if DMC FW is loaded otherwise false.
> + * Return: True if DMC FW is loaded otherwise false.
>    */
>   bool igt_pm_dmc_loaded(int debugfs)
>   {
> @@ -821,11 +812,11 @@ bool igt_pm_dmc_loaded(int debugfs)
>   
>   /**
>    * igt_pm_pc8_plus_residencies_enabled:
> - * @msr_fd: fd to /dev/cpu/0/msr
> + * @msr_fd: FD to /dev/cpu/0/msr
> + *
>    * Check whether BIOS has disabled the PC8 package deeper state.
>    *
> - * Returns:
> - * True if PC8+ package deeper state enabled on machine otherwise false.
> + * Return: True if PC8+ package deeper state enabled on machine otherwise false.
>    */
>   bool igt_pm_pc8_plus_residencies_enabled(int msr_fd)
>   {
> @@ -847,10 +838,10 @@ bool igt_pm_pc8_plus_residencies_enabled(int msr_fd)
>    * i915_output_is_lpsp_capable:
>    * @drm_fd: fd to drm device
>    * @output: igt output for which lpsp capability need to be evaluated
> - * Check lpsp capability for a given output.
>    *
> - * Returns:
> - * True if given output is lpsp capable otherwise false.
> + * Checks LPSP capability for a given output.
> + *
> + * Return: True if given output is LPSP  capable otherwise false.
                                            ^ extra space has to be removed
>    */
>   bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
>   {
> @@ -887,14 +878,13 @@ static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
>   
>   /**
>    * igt_pm_get_pcie_acpihp_slot:
> - * @pci_dev: pci bridge device.
> - * Get pci bridge acpi hotplug slot number, if bridge's ACPI firmware_node
> + * @pci_dev: PCI bridge device struct
> + *
> + * Gets PCI bridge acpi hotplug slot number, if bridge's ACPI firmware_node
>    * handle supports _SUN method.
>    *
> - * Returns:
> - * PCIe bridge Slot number.
> - * Returns -ENOENT number in case firmware_node/sun is not supported by the
> - * bridge.
> + * Return: PCIe bridge Slot number or -ENOENT number in case firmware_node/sun
> + *         is not supported by the bridge.
>    */
>   int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev)
>   {
> @@ -928,11 +918,11 @@ int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev)
>   
>   /**
>    * igt_pm_acpi_d3cold_supported:
> - * @pci_dev: root port pci_dev.
> - * Check ACPI D3Cold support.
> + * @pci_dev: Root port PCI device struct
> + *
> + * Checks ACPI D3Cold support.
>    *
> - * Returns:
> - * True if ACPI D3Cold supported otherwise false.
> + * Return: True if ACPI D3Cold supported otherwise false.
>    */
>   bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev)
>   {
> @@ -958,11 +948,11 @@ bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev)
>   
>   /**
>    * igt_pm_get_acpi_real_d_state:
> - * @pci_dev: root port pci_dev.
> - * Get ACPI D state for a given root port.
> + * @pci_dev: Root port PCI device struct
>    *
> - * Returns:
> - * igt_acpi_d_state state.
> + * Gets ACPI D state for a given root port.
> + *
> + * Return: igt_acpi_d_state state.
>    */
>   enum igt_acpi_d_state
>   igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev)
> @@ -1155,12 +1145,9 @@ igt_pm_setup_pci_card_power_attrs(struct pci_device *pci_dev, bool save_attrs, i
>   
>   /**
>    * igt_pm_get_autosuspend_delay:
> - * @pci_dev: pci_dev.
> - *
> - * Get pci_dev autosuspend delay value from pci sysfs.
> + * @pci_dev: PCI device struct
>    *
> - * Returns:
> - * autosuspend_delay_ms.
> + * Return: The autosuspend delay time in miliseconds.
>    */
>   int igt_pm_get_autosuspend_delay(struct pci_device *pci_dev)
>   {
> @@ -1177,10 +1164,10 @@ int igt_pm_get_autosuspend_delay(struct pci_device *pci_dev)
>   
>   /**
>    * igt_pm_set_autosuspend_delay:
> - * @pci_dev: pci_dev.
> - * @delay_ms: autosuspend delay in ms.
> + * @pci_dev: PCI device struct
> + * @delay_ms: Autosuspend delay in miliseconds.
>    *
> - * Set pci_dev autosuspend delay value through pci sysfs.
> + * Sets the autosuspend delay value for the PCI device through.
>    */
>   void igt_pm_set_autosuspend_delay(struct pci_device *pci_dev, int delay_ms)
>   {
> @@ -1201,26 +1188,28 @@ void igt_pm_set_autosuspend_delay(struct pci_device *pci_dev, int delay_ms)
>   
>   /**
>    * igt_pm_enable_pci_card_runtime_pm:
> - * @root: root port pci_dev.
> - * @i915: i915 pci_dev.
> - * Enable runtime PM for all PCI endpoints devices for a given root port by
> + * @root: Root port PCI device struct
> + * @gfx: PCI device struct of graphics device
> + *
> + * Enables runtime PM for all PCI endpoints devices for a given root port by
>    * setting power/control attr to "auto" and setting autosuspend_delay_ms
>    * to zero.
>    */
>   void igt_pm_enable_pci_card_runtime_pm(struct pci_device *root,
> -				       struct pci_device *i915)
> +				       struct pci_device *gfx)
>   {
>   	int delay = -1;
>   
> -	if (i915)
> -		delay = igt_pm_get_autosuspend_delay(i915);
> +	if (gfx)
> +		delay = igt_pm_get_autosuspend_delay(gfx);
>   
>   	igt_pm_setup_pci_card_power_attrs(root, false, delay);
>   }
>   
>   /**
>    * igt_pm_setup_pci_card_runtime_pm:
> - * @pci_dev: root port pci_dev.
> + * @pci_dev: Root port PCI device struct
> + *
>    * Setup runtime PM for all PCI endpoints devices for a given root port by
>    * enabling runtime suspend and setting autosuspend_delay_ms to zero.
>    * It also saves and restore power control attribute for all PCI endpoints
> @@ -1234,11 +1223,10 @@ void igt_pm_setup_pci_card_runtime_pm(struct pci_device *pci_dev)
>   
>   /**
>    * igt_pm_get_d3cold_allowed:
> - * @pci_slot_name: slot name of the pci device
> - * @value: value to be read into
> + * @pci_slot_name: Slot name of the PCI device
> + * @value: Value to be read into
>    *
> - * Reads the value of d3cold_allowed attribute
> - * of the pci device
> + * Reads the value of d3cold_allowed attribute of the PCI device.
>    */
>   void igt_pm_get_d3cold_allowed(const char *pci_slot_name, uint32_t *value)
>   {
> @@ -1258,10 +1246,10 @@ void igt_pm_get_d3cold_allowed(const char *pci_slot_name, uint32_t *value)
>   
>   /**
>    * igt_pm_set_d3cold_allowed:
> - * @pci_slot_name: slot name of pci device
> - * @value: value to be written
> + * @pci_slot_name: Slot name of PCI device
> + * @value: Value to be written
>    *
> - * writes the value to d3cold_allowed attribute of pci device
> + * Writes the value to d3cold_allowed attribute of PCI device.
>    */
>   void igt_pm_set_d3cold_allowed(const char *pci_slot_name, uint32_t value)
>   {
> @@ -1294,7 +1282,8 @@ igt_pm_restore_power_attr(struct pci_device *pci_dev, const char *attr, char *va
>   
>   /**
>    * igt_pm_restore_pci_card_runtime_pm:
> - * Restore control and autosuspend_delay_ms power attribute for all
> + *
> + * Restores control and autosuspend_delay_ms power attribute for all
s/attribute/attributes

With the above fixes
Looks good to me
Reviewed-by: Riana Tauro <riana.tauro at intel.com>


>    * PCI endpoints devices under gfx root port, which were saved earlier
>    * by igt_pm_setup_pci_card_runtime_pm().
>    */
> @@ -1342,8 +1331,9 @@ static void igt_pm_print_pci_dev_runtime_status(struct pci_device *pci_dev)
>   
>   /**
>    * igt_pm_print_pci_card_runtime_status:
> - * @pci_dev: root port pci_dev.
> - * Print runtime suspend status for all PCI endpoints devices for a given
> + * @pci_dev: Root port PCI device struct
> + *
> + * Prints runtime suspend status for all PCI endpoints devices for a given
>    * root port.
>    */
>   void igt_pm_print_pci_card_runtime_status(void)
> @@ -1361,8 +1351,9 @@ void igt_pm_print_pci_card_runtime_status(void)
>   /**
>    * i915_is_slpc_enabled_gt:
>    * @drm_fd: DRM file descriptor
> - * @gt: GT id
> - * Check if SLPC is enabled on a GT
> + * @gt: GT ID
> + *
> + * Return: True if SLPC is enabled on a given @gt.
>    */
>   bool i915_is_slpc_enabled_gt(int drm_fd, int gt)
>   {
> @@ -1387,13 +1378,20 @@ bool i915_is_slpc_enabled_gt(int drm_fd, int gt)
>   /**
>    * i915_is_slpc_enabled:
>    * @drm_fd: DRM file descriptor
> - * Check if SLPC is enabled for the device
> + *
> + * Return: True if SLPC is enabled on the device.
>    */
>   bool i915_is_slpc_enabled(int drm_fd)
>   {
>   	return i915_is_slpc_enabled_gt(drm_fd, 0);
>   }
>   
> +/**
> + * igt_pm_get_runtime_suspended_time:
> + * @pci_dev: PCI device struct
> + *
> + * Return: The total time that the device has been suspended.
> + */
>   int igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev)
>   {
>   	char time_str[64];
> @@ -1438,9 +1436,9 @@ int igt_pm_get_runtime_active_time(struct pci_device *pci_dev)
>   
>   /**
>    * igt_pm_get_runtime_usage:
> - * @pci_dev: pci device
> + * @pci_dev: PCI device struct
>    *
> - * Reports the runtime PM usage count of a device.
> + * Return: The runtime PM usage count of a device.
>    */
>   int igt_pm_get_runtime_usage(struct pci_device *pci_dev)
>   {
> @@ -1455,12 +1453,12 @@ int igt_pm_get_runtime_usage(struct pci_device *pci_dev)
>   }
>   
>   /**
> - * igt_pm_ignore_slpc_efficient_freq
> + * igt_pm_ignore_slpc_efficient_freq:
>    * @i915: open i915 drm file descriptor
>    * @gtfd: open gt sysfs fd
>    * @val: value to set
>    *
> - * Ignores/un-ignores SLPC efficient frequency
> + * Ignores/un-ignores SLPC efficient frequency.
>    */
>   void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val)
>   {


More information about the Intel-xe mailing list