[PATCH v4 00/11] VRAM Self Refresh

[PATCH v4 00/11] VRAM Self Refresh

[Badal Nilawar]

Changes in v4:
  - Resolved build warnings

Changes in v3:
 PCIe ACPI Patches:
  - dropped the notifier block code and added patch to allow only one Aux
    power limit request per root port (Rafael J. Wysocki)
  - Addressed Review comments (Rafael J. Wysocki, Bjorn Helgaas)

 Xe Pacthes:
  - Addressed Review comments (Bjorn Helgaas)


Anshuman Gupta (6):
  PCI/ACPI: Add D3cold Aux Power Limit_DSM method
  PCI/ACPI: Add PERST# Assertion Delay _DSM method
  drm/xe/vrsr: Detect VRSR Capability
  drm/xe/vrsr: Refactor d3cold.allowed to a enum
  drm/xe/pm: D3Cold target state
  drm/xe/vrsr: Enable VRSR

Badal Nilawar (5):
  PCI/ACPI: Per root port allow one Aux power limit request
  drm/xe/vrsr: Introduce flag has_vrsr
  drm/xe/vrsr: Initialize VRSR feature
  drm/xe/vrsr: Enable VRSR on default VGA boot device
  drm/xe/vrsr: Introduce a debugfs node named vrsr_capable

 drivers/acpi/scan.c                     |   1 +
 drivers/gpu/drm/xe/display/xe_display.c |  28 +++-
 drivers/gpu/drm/xe/display/xe_display.h |   2 +
 drivers/gpu/drm/xe/regs/xe_regs.h       |   3 +
 drivers/gpu/drm/xe/xe_debugfs.c         |  20 +++
 drivers/gpu/drm/xe/xe_device_types.h    |  12 +-
 drivers/gpu/drm/xe/xe_pci.c             |  13 +-
 drivers/gpu/drm/xe/xe_pcode_api.h       |   7 +
 drivers/gpu/drm/xe/xe_pm.c              | 206 +++++++++++++++++++++---
 drivers/gpu/drm/xe/xe_pm.h              |   9 +-
 drivers/pci/pci-acpi.c                  | 167 +++++++++++++++++++
 include/acpi/acpi_bus.h                 |   2 +
 include/linux/pci-acpi.h                |  16 +-
 13 files changed, 456 insertions(+), 30 deletions(-)

-- 
2.34.1

[PATCH v4 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Badal Nilawar]

From: Anshuman Gupta <anshuman.gupta@intel.com>

Implement _DSM method 0Ah according to PCI firmware specifications,
section 4.6.10 Rev 3.3., to request auxilary power needed for the
device when in D3Cold.

Note that this implementation assumes only a single device below the
Downstream Port will request for Aux Power Limit under a given
Root Port because it does not track and aggregate requests
from all child devices below the Downstream Port as required
by Section 4.6.10 Rev 3.3.

One possible mitigation would be only allowing only first PCIe
Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.

Signed-off-by: Varun Gupta <varun.gupta@intel.com>
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
V2(Bjorn/Rafael):
  - Call acpi_dsm_check() to find method 0Ah supported
  - Return retry interval to caller
V3(Kuppuswamy)
  - Add NULL check for retry interval
---
 drivers/pci/pci-acpi.c   | 87 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |  8 ++++
 2 files changed, 95 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index af370628e583..87f30910a5f1 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1421,6 +1421,93 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
 	ACPI_FREE(obj);
 }
 
+/**
+ * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3Cold
+ * @dev: PCI device instance
+ * @requested_power: Requested auxiliary power in milliwatts
+ * @retry_interval: Retry interval returned by platform to retry auxiliary
+ *                  power request
+ *
+ * This function sends a request to the host BIOS via root port ACPI _DSM Function 0Ah
+ * for the auxiliary power needed by the PCI device when it is in D3Cold.
+ * It checks and evaluates the _DSM (Device Specific Method) to request the auxiliary
+ * power and handles the response accordingly.
+ *
+ * This function shall be only called by 1st non-bridge Endpoint driver
+ * on Function 0. For a Multi-Function Device, the driver for Function 0 is
+ * required to report an aggregate power requirement covering all
+ * functions contained within the device.
+ *
+ * Return: Returns 0 on success and errno on failure.
+ */
+int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
+				      u32 *retry_interval)
+{
+	union acpi_object in_obj = {
+		.integer.type = ACPI_TYPE_INTEGER,
+		.integer.value = requested_power,
+	};
+
+	union acpi_object *out_obj;
+	acpi_handle handle;
+	int result, ret = -EINVAL;
+
+	if (!dev || !retry_interval)
+		return -EINVAL;
+
+	handle = ACPI_HANDLE(&dev->dev);
+	if (!handle)
+		return -EINVAL;
+
+	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_D3COLD_AUX_POWER_LIMIT)) {
+		pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_D3COLD_AUX_POWER_LIMIT);
+		return -ENODEV;
+	}
+
+	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
+					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
+					  &in_obj, ACPI_TYPE_INTEGER);
+	if (!out_obj)
+		return -EINVAL;
+
+	result = out_obj->integer.value;
+	if (retry_interval)
+		*retry_interval = 0;
+
+	switch (result) {
+	case 0x0:
+		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
+			requested_power);
+		break;
+	case 0x1:
+		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
+			 requested_power);
+		ret = 0;
+		break;
+	case 0x2:
+		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
+		ret = -EBUSY;
+		break;
+	default:
+		if (result >= 0x11 && result <= 0x1F) {
+			if (retry_interval) {
+				*retry_interval = result & 0xF;
+				pci_warn(dev, "D3cold Aux Power request needs retry interval: %u seconds\n",
+					 *retry_interval);
+				ret = -EAGAIN;
+			}
+		} else {
+			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
+				result);
+		}
+		break;
+	}
+
+	ACPI_FREE(out_obj);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
+
 static void pci_acpi_set_external_facing(struct pci_dev *dev)
 {
 	u8 val;
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 078225b514d4..6079306ad754 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -121,6 +121,7 @@ extern const guid_t pci_acpi_dsm_guid;
 #define DSM_PCI_DEVICE_NAME			0x07
 #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
 #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
+#define DSM_PCI_D3COLD_AUX_POWER_LIMIT		0x0A
 
 #ifdef CONFIG_PCIE_EDR
 void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
@@ -132,10 +133,17 @@ static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
 
 int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
 void pci_acpi_clear_companion_lookup_hook(void);
+int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
+				      u32 *retry_interval);
 
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
+static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
+						    u32 *retry_interval)
+{
+	return -EOPNOTSUPP;
+}
 #endif	/* CONFIG_ACPI */
 
 #endif	/* _PCI_ACPI_H_ */
-- 
2.34.1

Re: [PATCH v4 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Sathyanarayanan Kuppuswamy]

On 5/29/25 4:16 AM, Badal Nilawar wrote:
> From: Anshuman Gupta <anshuman.gupta@intel.com>
>
> Implement _DSM method 0Ah according to PCI firmware specifications,
> section 4.6.10 Rev 3.3., to request auxilary power needed for the
> device when in D3Cold.
>
> Note that this implementation assumes only a single device below the
> Downstream Port will request for Aux Power Limit under a given
> Root Port because it does not track and aggregate requests
> from all child devices below the Downstream Port as required
> by Section 4.6.10 Rev 3.3.
>
> One possible mitigation would be only allowing only first PCIe
> Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.
>
> Signed-off-by: Varun Gupta <varun.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>

If it is Co-developed by above people, you need to add Co-developed-by tag followed
by Signed-off-by tag.

> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
> V2(Bjorn/Rafael):
>    - Call acpi_dsm_check() to find method 0Ah supported
>    - Return retry interval to caller
> V3(Kuppuswamy)
>    - Add NULL check for retry interval
> ---
>   drivers/pci/pci-acpi.c   | 87 ++++++++++++++++++++++++++++++++++++++++
>   include/linux/pci-acpi.h |  8 ++++
>   2 files changed, 95 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index af370628e583..87f30910a5f1 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1421,6 +1421,93 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>   	ACPI_FREE(obj);
>   }
>   
> +/**
> + * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3Cold
> + * @dev: PCI device instance
> + * @requested_power: Requested auxiliary power in milliwatts
> + * @retry_interval: Retry interval returned by platform to retry auxiliary
> + *                  power request
> + *
> + * This function sends a request to the host BIOS via root port ACPI _DSM Function 0Ah
> + * for the auxiliary power needed by the PCI device when it is in D3Cold.
> + * It checks and evaluates the _DSM (Device Specific Method) to request the auxiliary
> + * power and handles the response accordingly.
> + *
> + * This function shall be only called by 1st non-bridge Endpoint driver
> + * on Function 0. For a Multi-Function Device, the driver for Function 0 is
> + * required to report an aggregate power requirement covering all
> + * functions contained within the device.
> + *
> + * Return: Returns 0 on success and errno on failure.
> + */
> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +				      u32 *retry_interval)
> +{
> +	union acpi_object in_obj = {
> +		.integer.type = ACPI_TYPE_INTEGER,
> +		.integer.value = requested_power,
> +	};
> +
> +	union acpi_object *out_obj;
> +	acpi_handle handle;
> +	int result, ret = -EINVAL;
> +
> +	if (!dev || !retry_interval)
> +		return -EINVAL;
> +
> +	handle = ACPI_HANDLE(&dev->dev);
> +	if (!handle)
> +		return -EINVAL;
> +
> +	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_D3COLD_AUX_POWER_LIMIT)) {
> +		pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_D3COLD_AUX_POWER_LIMIT);
> +		return -ENODEV;
> +	}
> +
> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
> +					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
> +					  &in_obj, ACPI_TYPE_INTEGER);
> +	if (!out_obj)
> +		return -EINVAL;
> +
> +	result = out_obj->integer.value;
> +	if (retry_interval)

Since you already do NULL check at the top, you don't need above check.

> +		*retry_interval = 0;
> +
> +	switch (result) {
> +	case 0x0:
> +		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
> +			requested_power);
> +		break;
> +	case 0x1:
> +		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
> +			 requested_power);
> +		ret = 0;
> +		break;
> +	case 0x2:
> +		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
> +		ret = -EBUSY;
> +		break;
> +	default:
> +		if (result >= 0x11 && result <= 0x1F) {
> +			if (retry_interval) {

Same as above.

> +				*retry_interval = result & 0xF;
> +				pci_warn(dev, "D3cold Aux Power request needs retry interval: %u seconds\n",
> +					 *retry_interval);
> +				ret = -EAGAIN;
> +			}
> +		} else {
> +			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
> +				result);
> +		}
> +		break;
> +	}
> +
> +	ACPI_FREE(out_obj);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
> +
>   static void pci_acpi_set_external_facing(struct pci_dev *dev)
>   {
>   	u8 val;
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 078225b514d4..6079306ad754 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -121,6 +121,7 @@ extern const guid_t pci_acpi_dsm_guid;
>   #define DSM_PCI_DEVICE_NAME			0x07
>   #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
>   #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
> +#define DSM_PCI_D3COLD_AUX_POWER_LIMIT		0x0A
>   
>   #ifdef CONFIG_PCIE_EDR
>   void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
> @@ -132,10 +133,17 @@ static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
>   
>   int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
>   void pci_acpi_clear_companion_lookup_hook(void);
> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +				      u32 *retry_interval);
>   
>   #else	/* CONFIG_ACPI */
>   static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> +static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +						    u32 *retry_interval)
> +{
> +	return -EOPNOTSUPP;
> +}
>   #endif	/* CONFIG_ACPI */
>   
>   #endif	/* _PCI_ACPI_H_ */

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Nilawar, Badal]

Apologies for the delay in response.

On 30-05-2025 03:06, Sathyanarayanan Kuppuswamy wrote:
>
> On 5/29/25 4:16 AM, Badal Nilawar wrote:
>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>
>> Implement _DSM method 0Ah according to PCI firmware specifications,
>> section 4.6.10 Rev 3.3., to request auxilary power needed for the
>> device when in D3Cold.
>>
>> Note that this implementation assumes only a single device below the
>> Downstream Port will request for Aux Power Limit under a given
>> Root Port because it does not track and aggregate requests
>> from all child devices below the Downstream Port as required
>> by Section 4.6.10 Rev 3.3.
>>
>> One possible mitigation would be only allowing only first PCIe
>> Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.
>>
>> Signed-off-by: Varun Gupta <varun.gupta@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>
> If it is Co-developed by above people, you need to add Co-developed-by 
> tag followed
> by Signed-off-by tag.

Sure I will add Co-developed by.

>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>> V2(Bjorn/Rafael):
>>    - Call acpi_dsm_check() to find method 0Ah supported
>>    - Return retry interval to caller
>> V3(Kuppuswamy)
>>    - Add NULL check for retry interval
>> ---
>>   drivers/pci/pci-acpi.c   | 87 ++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pci-acpi.h |  8 ++++
>>   2 files changed, 95 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index af370628e583..87f30910a5f1 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1421,6 +1421,93 @@ static void pci_acpi_optimize_delay(struct 
>> pci_dev *pdev,
>>       ACPI_FREE(obj);
>>   }
>>   +/**
>> + * pci_acpi_request_d3cold_aux_power - Request aux power while 
>> device is in D3Cold
>> + * @dev: PCI device instance
>> + * @requested_power: Requested auxiliary power in milliwatts
>> + * @retry_interval: Retry interval returned by platform to retry 
>> auxiliary
>> + *                  power request
>> + *
>> + * This function sends a request to the host BIOS via root port ACPI 
>> _DSM Function 0Ah
>> + * for the auxiliary power needed by the PCI device when it is in 
>> D3Cold.
>> + * It checks and evaluates the _DSM (Device Specific Method) to 
>> request the auxiliary
>> + * power and handles the response accordingly.
>> + *
>> + * This function shall be only called by 1st non-bridge Endpoint driver
>> + * on Function 0. For a Multi-Function Device, the driver for 
>> Function 0 is
>> + * required to report an aggregate power requirement covering all
>> + * functions contained within the device.
>> + *
>> + * Return: Returns 0 on success and errno on failure.
>> + */
>> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 
>> requested_power,
>> +                      u32 *retry_interval)
>> +{
>> +    union acpi_object in_obj = {
>> +        .integer.type = ACPI_TYPE_INTEGER,
>> +        .integer.value = requested_power,
>> +    };
>> +
>> +    union acpi_object *out_obj;
>> +    acpi_handle handle;
>> +    int result, ret = -EINVAL;
>> +
>> +    if (!dev || !retry_interval)
>> +        return -EINVAL;
>> +
>> +    handle = ACPI_HANDLE(&dev->dev);
>> +    if (!handle)
>> +        return -EINVAL;
>> +
>> +    if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << 
>> DSM_PCI_D3COLD_AUX_POWER_LIMIT)) {
>> +        pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", 
>> DSM_PCI_D3COLD_AUX_POWER_LIMIT);
>> +        return -ENODEV;
>> +    }
>> +
>> +    out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>> +                      DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>> +                      &in_obj, ACPI_TYPE_INTEGER);
>> +    if (!out_obj)
>> +        return -EINVAL;
>> +
>> +    result = out_obj->integer.value;
>> +    if (retry_interval)
>
> Since you already do NULL check at the top, you don't need above check.

Sure, I will fix it.

>
>> +        *retry_interval = 0;
>> +
>> +    switch (result) {
>> +    case 0x0:
>> +        pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>> +            requested_power);
>> +        break;
>> +    case 0x1:
>> +        pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>> +             requested_power);
>> +        ret = 0;
>> +        break;
>> +    case 0x2:
>> +        pci_info(dev, "D3cold Aux Power: Main power won't be 
>> removed\n");
>> +        ret = -EBUSY;
>> +        break;
>> +    default:
>> +        if (result >= 0x11 && result <= 0x1F) {
>> +            if (retry_interval) {
>
> Same as above.

Ok

Thanks,
Badal

>
>> +                *retry_interval = result & 0xF;
>> +                pci_warn(dev, "D3cold Aux Power request needs retry 
>> interval: %u seconds\n",
>> +                     *retry_interval);
>> +                ret = -EAGAIN;
>> +            }
>> +        } else {
>> +            pci_err(dev, "D3cold Aux Power: Reserved or unsupported 
>> response: 0x%x\n",
>> +                result);
>> +        }
>> +        break;
>> +    }
>> +
>> +    ACPI_FREE(out_obj);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>> +
>>   static void pci_acpi_set_external_facing(struct pci_dev *dev)
>>   {
>>       u8 val;
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 078225b514d4..6079306ad754 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -121,6 +121,7 @@ extern const guid_t pci_acpi_dsm_guid;
>>   #define DSM_PCI_DEVICE_NAME            0x07
>>   #define DSM_PCI_POWER_ON_RESET_DELAY        0x08
>>   #define DSM_PCI_DEVICE_READINESS_DURATIONS    0x09
>> +#define DSM_PCI_D3COLD_AUX_POWER_LIMIT        0x0A
>>     #ifdef CONFIG_PCIE_EDR
>>   void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
>> @@ -132,10 +133,17 @@ static inline void 
>> pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
>>     int pci_acpi_set_companion_lookup_hook(struct acpi_device 
>> *(*func)(struct pci_dev *));
>>   void pci_acpi_clear_companion_lookup_hook(void);
>> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 
>> requested_power,
>> +                      u32 *retry_interval);
>>     #else    /* CONFIG_ACPI */
>>   static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> +static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev 
>> *dev, u32 requested_power,
>> +                            u32 *retry_interval)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>>   #endif    /* CONFIG_ACPI */
>>     #endif    /* _PCI_ACPI_H_ */
>

Re: [PATCH v4 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Rafael J. Wysocki]

On Thu, May 29, 2025 at 1:14 PM Badal Nilawar <badal.nilawar@intel.com> wrote:
>
> From: Anshuman Gupta <anshuman.gupta@intel.com>
>
> Implement _DSM method 0Ah according to PCI firmware specifications,
> section 4.6.10 Rev 3.3., to request auxilary power needed for the
> device when in D3Cold.
>
> Note that this implementation assumes only a single device below the
> Downstream Port will request for Aux Power Limit under a given
> Root Port because it does not track and aggregate requests
> from all child devices below the Downstream Port as required
> by Section 4.6.10 Rev 3.3.
>
> One possible mitigation would be only allowing only first PCIe
> Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.
>
> Signed-off-by: Varun Gupta <varun.gupta@intel.com>

What's this S-o-b for?

> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
> V2(Bjorn/Rafael):
>   - Call acpi_dsm_check() to find method 0Ah supported
>   - Return retry interval to caller
> V3(Kuppuswamy)
>   - Add NULL check for retry interval
> ---
>  drivers/pci/pci-acpi.c   | 87 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h |  8 ++++
>  2 files changed, 95 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index af370628e583..87f30910a5f1 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1421,6 +1421,93 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>         ACPI_FREE(obj);
>  }
>
> +/**
> + * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3Cold
> + * @dev: PCI device instance
> + * @requested_power: Requested auxiliary power in milliwatts
> + * @retry_interval: Retry interval returned by platform to retry auxiliary
> + *                  power request
> + *
> + * This function sends a request to the host BIOS via root port ACPI _DSM Function 0Ah
> + * for the auxiliary power needed by the PCI device when it is in D3Cold.
> + * It checks and evaluates the _DSM (Device Specific Method) to request the auxiliary
> + * power and handles the response accordingly.
> + *
> + * This function shall be only called by 1st non-bridge Endpoint driver
> + * on Function 0. For a Multi-Function Device, the driver for Function 0 is
> + * required to report an aggregate power requirement covering all
> + * functions contained within the device.
> + *
> + * Return: Returns 0 on success and errno on failure.
> + */
> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +                                     u32 *retry_interval)
> +{
> +       union acpi_object in_obj = {
> +               .integer.type = ACPI_TYPE_INTEGER,
> +               .integer.value = requested_power,
> +       };
> +
> +       union acpi_object *out_obj;
> +       acpi_handle handle;
> +       int result, ret = -EINVAL;
> +
> +       if (!dev || !retry_interval)
> +               return -EINVAL;
> +
> +       handle = ACPI_HANDLE(&dev->dev);
> +       if (!handle)
> +               return -EINVAL;
> +
> +       if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_D3COLD_AUX_POWER_LIMIT)) {
> +               pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_D3COLD_AUX_POWER_LIMIT);
> +               return -ENODEV;
> +       }
> +
> +       out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
> +                                         DSM_PCI_D3COLD_AUX_POWER_LIMIT,
> +                                         &in_obj, ACPI_TYPE_INTEGER);
> +       if (!out_obj)
> +               return -EINVAL;
> +
> +       result = out_obj->integer.value;
> +       if (retry_interval)
> +               *retry_interval = 0;
> +
> +       switch (result) {
> +       case 0x0:

It would be better to use an enum for the possible return values.

> +               pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
> +                       requested_power);
> +               break;
> +       case 0x1:
> +               pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
> +                        requested_power);
> +               ret = 0;
> +               break;
> +       case 0x2:
> +               pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
> +               ret = -EBUSY;
> +               break;
> +       default:
> +               if (result >= 0x11 && result <= 0x1F) {

if (!(result & ~0x1F))

I think, and it would be better to use a symbol for this mask (and below too).

> +                       if (retry_interval) {

This has been checked already and is guaranteed to be nonzero at this point.

> +                               *retry_interval = result & 0xF;
> +                               pci_warn(dev, "D3cold Aux Power request needs retry interval: %u seconds\n",
> +                                        *retry_interval);
> +                               ret = -EAGAIN;
> +                       }
> +               } else {
> +                       pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
> +                               result);
> +               }
> +               break;
> +       }
> +
> +       ACPI_FREE(out_obj);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
> +
>  static void pci_acpi_set_external_facing(struct pci_dev *dev)
>  {
>         u8 val;
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 078225b514d4..6079306ad754 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -121,6 +121,7 @@ extern const guid_t pci_acpi_dsm_guid;
>  #define DSM_PCI_DEVICE_NAME                    0x07
>  #define DSM_PCI_POWER_ON_RESET_DELAY           0x08
>  #define DSM_PCI_DEVICE_READINESS_DURATIONS     0x09
> +#define DSM_PCI_D3COLD_AUX_POWER_LIMIT         0x0A
>
>  #ifdef CONFIG_PCIE_EDR
>  void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
> @@ -132,10 +133,17 @@ static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
>
>  int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
>  void pci_acpi_clear_companion_lookup_hook(void);
> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +                                     u32 *retry_interval);
>
>  #else  /* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> +static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +                                                   u32 *retry_interval)
> +{
> +       return -EOPNOTSUPP;
> +}
>  #endif /* CONFIG_ACPI */
>
>  #endif /* _PCI_ACPI_H_ */
> --

Re: [PATCH v4 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Nilawar, Badal]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

Apologies for delay in response.

On 02-07-2025 16:38, Rafael J. Wysocki wrote:
> On Thu, May 29, 2025 at 1:14 PM Badal Nilawar<badal.nilawar@intel.com> wrote:
>> From: Anshuman Gupta<anshuman.gupta@intel.com>
>>
>> Implement _DSM method 0Ah according to PCI firmware specifications,
>> section 4.6.10 Rev 3.3., to request auxilary power needed for the
>> device when in D3Cold.
>>
>> Note that this implementation assumes only a single device below the
>> Downstream Port will request for Aux Power Limit under a given
>> Root Port because it does not track and aggregate requests
>> from all child devices below the Downstream Port as required
>> by Section 4.6.10 Rev 3.3.
>>
>> One possible mitigation would be only allowing only first PCIe
>> Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.
>>
>> Signed-off-by: Varun Gupta<varun.gupta@intel.com>
> What's this S-o-b for?

Varun was handling this patch from Anshuman earlier. Anshuman can you 
please confirm whether the contribution
from Varun was significant and needed to be co-developed or can this 
S-o-b be safely dropped in future versions.

Thanks,
Badal

>
>> Signed-off-by: Badal Nilawar<badal.nilawar@intel.com>
>> Signed-off-by: Anshuman Gupta<anshuman.gupta@intel.com>

[-- Attachment #2: Type: text/html, Size: 2766 bytes --]

Re: [PATCH v4 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Nilawar, Badal]

On 02-07-2025 16:38, Rafael J. Wysocki wrote:
> On Thu, May 29, 2025 at 1:14 PM Badal Nilawar <badal.nilawar@intel.com> wrote:
>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>
>> Implement _DSM method 0Ah according to PCI firmware specifications,
>> section 4.6.10 Rev 3.3., to request auxilary power needed for the
>> device when in D3Cold.
>>
>> Note that this implementation assumes only a single device below the
>> Downstream Port will request for Aux Power Limit under a given
>> Root Port because it does not track and aggregate requests
>> from all child devices below the Downstream Port as required
>> by Section 4.6.10 Rev 3.3.
>>
>> One possible mitigation would be only allowing only first PCIe
>> Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.
>>
>> Signed-off-by: Varun Gupta <varun.gupta@intel.com>
> What's this S-o-b for?
>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>> V2(Bjorn/Rafael):
>>    - Call acpi_dsm_check() to find method 0Ah supported
>>    - Return retry interval to caller
>> V3(Kuppuswamy)
>>    - Add NULL check for retry interval
>> ---
>>   drivers/pci/pci-acpi.c   | 87 ++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pci-acpi.h |  8 ++++
>>   2 files changed, 95 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index af370628e583..87f30910a5f1 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1421,6 +1421,93 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>>          ACPI_FREE(obj);
>>   }
>>
>> +/**
>> + * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3Cold
>> + * @dev: PCI device instance
>> + * @requested_power: Requested auxiliary power in milliwatts
>> + * @retry_interval: Retry interval returned by platform to retry auxiliary
>> + *                  power request
>> + *
>> + * This function sends a request to the host BIOS via root port ACPI _DSM Function 0Ah
>> + * for the auxiliary power needed by the PCI device when it is in D3Cold.
>> + * It checks and evaluates the _DSM (Device Specific Method) to request the auxiliary
>> + * power and handles the response accordingly.
>> + *
>> + * This function shall be only called by 1st non-bridge Endpoint driver
>> + * on Function 0. For a Multi-Function Device, the driver for Function 0 is
>> + * required to report an aggregate power requirement covering all
>> + * functions contained within the device.
>> + *
>> + * Return: Returns 0 on success and errno on failure.
>> + */
>> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>> +                                     u32 *retry_interval)
>> +{
>> +       union acpi_object in_obj = {
>> +               .integer.type = ACPI_TYPE_INTEGER,
>> +               .integer.value = requested_power,
>> +       };
>> +
>> +       union acpi_object *out_obj;
>> +       acpi_handle handle;
>> +       int result, ret = -EINVAL;
>> +
>> +       if (!dev || !retry_interval)
>> +               return -EINVAL;
>> +
>> +       handle = ACPI_HANDLE(&dev->dev);
>> +       if (!handle)
>> +               return -EINVAL;
>> +
>> +       if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_D3COLD_AUX_POWER_LIMIT)) {
>> +               pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_D3COLD_AUX_POWER_LIMIT);
>> +               return -ENODEV;
>> +       }
>> +
>> +       out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>> +                                         DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>> +                                         &in_obj, ACPI_TYPE_INTEGER);
>> +       if (!out_obj)
>> +               return -EINVAL;
>> +
>> +       result = out_obj->integer.value;
>> +       if (retry_interval)
>> +               *retry_interval = 0;
>> +
>> +       switch (result) {
>> +       case 0x0:
> It would be better to use an enum for the possible return values.

Sure I will use enum, something like

typedef enum {
     AUX_PWR_REQ_DENIED               = 0x0,
     AUX_PWR_REQ_GRANTED              = 0x1,
     AUX_PWR_REQ_NO_MAIN_PWR_REMOVAL  = 0x2
} AUX_PWR_REQ_STATUS;

>
>> +               pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>> +                       requested_power);
>> +               break;
>> +       case 0x1:
>> +               pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>> +                        requested_power);
>> +               ret = 0;
>> +               break;
>> +       case 0x2:
>> +               pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
>> +               ret = -EBUSY;
>> +               break;
>> +       default:
>> +               if (result >= 0x11 && result <= 0x1F) {
> if (!(result & ~0x1F))

This will pick result = 0x10 as well, that's why checking the range 0x11 
- 0x1F.

> I think, and it would be better to use a symbol for this mask (and below too).
I will add an enum for the retry interval range — something like 
AUX_PWR_REQ_RETRY_INT_MIN and AUX_PWR_REQ_RETRY_INT_MAX for 0x11 and 0x1F.

>
>> +                       if (retry_interval) {
> This has been checked already and is guaranteed to be nonzero at this point.

Sure.

Thanks,
Badal

>
>> +                               *retry_interval = result & 0xF;
>> +                               pci_warn(dev, "D3cold Aux Power request needs retry interval: %u seconds\n",
>> +                                        *retry_interval);
>> +                               ret = -EAGAIN;
>> +                       }
>> +               } else {
>> +                       pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>> +                               result);
>> +               }
>> +               break;
>> +       }
>> +
>> +       ACPI_FREE(out_obj);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>> +
>>   static void pci_acpi_set_external_facing(struct pci_dev *dev)
>>   {
>>          u8 val;
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 078225b514d4..6079306ad754 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -121,6 +121,7 @@ extern const guid_t pci_acpi_dsm_guid;
>>   #define DSM_PCI_DEVICE_NAME                    0x07
>>   #define DSM_PCI_POWER_ON_RESET_DELAY           0x08
>>   #define DSM_PCI_DEVICE_READINESS_DURATIONS     0x09
>> +#define DSM_PCI_D3COLD_AUX_POWER_LIMIT         0x0A
>>
>>   #ifdef CONFIG_PCIE_EDR
>>   void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
>> @@ -132,10 +133,17 @@ static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
>>
>>   int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
>>   void pci_acpi_clear_companion_lookup_hook(void);
>> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>> +                                     u32 *retry_interval);
>>
>>   #else  /* CONFIG_ACPI */
>>   static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> +static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>> +                                                   u32 *retry_interval)
>> +{
>> +       return -EOPNOTSUPP;
>> +}
>>   #endif /* CONFIG_ACPI */
>>
>>   #endif /* _PCI_ACPI_H_ */
>> --

Re: [PATCH v4 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Bjorn Helgaas]

On Thu, May 29, 2025 at 04:46:44PM +0530, Badal Nilawar wrote:
> From: Anshuman Gupta <anshuman.gupta@intel.com>
> 
> Implement _DSM method 0Ah according to PCI firmware specifications,
> section 4.6.10 Rev 3.3., to request auxilary power needed for the
> device when in D3Cold.

Cite as "PCI Firmware r3.3, sec 4.6.10"
> 
> Note that this implementation assumes only a single device below the
> Downstream Port will request for Aux Power Limit under a given
> Root Port because it does not track and aggregate requests
> from all child devices below the Downstream Port as required
> by Section 4.6.10 Rev 3.3.

Wrap to fill 75 columns.  Update citation as above.

> One possible mitigation would be only allowing only first PCIe
> Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.

I don't quite understand how this mitigation would address the lack of
aggregation or how the mitigation could be enforced.  Maybe just drop
this?

s/call_DSM/call _DSM/

> Signed-off-by: Varun Gupta <varun.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
> V2(Bjorn/Rafael):
>   - Call acpi_dsm_check() to find method 0Ah supported
>   - Return retry interval to caller
> V3(Kuppuswamy)
>   - Add NULL check for retry interval
> ---
>  drivers/pci/pci-acpi.c   | 87 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h |  8 ++++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index af370628e583..87f30910a5f1 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1421,6 +1421,93 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>  	ACPI_FREE(obj);
>  }
>  
> +/**
> + * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3Cold
> + * @dev: PCI device instance
> + * @requested_power: Requested auxiliary power in milliwatts
> + * @retry_interval: Retry interval returned by platform to retry auxiliary
> + *                  power request
> + *
> + * This function sends a request to the host BIOS via root port ACPI _DSM Function 0Ah
> + * for the auxiliary power needed by the PCI device when it is in D3Cold.
> + * It checks and evaluates the _DSM (Device Specific Method) to request the auxiliary
> + * power and handles the response accordingly.

s/This functions sends a request/Request auxiliary power .../ (imperative mood)
s/the host BIOS/platform firmware/ (non-x86 firmware may not be BIOS)
s/D3Cold/D3cold/ (twice) to match other usage
s/root port/Root Port/ to match spec usage
s/It checks and evaluates the/Evaluate the/
s/and handles/and handle/

It's not quite clear from this description, but the code assumes this
is called for a Root Port.  I don't think that's quite the right
approach because the spec only says this _DSM function is implemented
in the scope of a Downstream Port.  That *could* be a Root Port, but
it doesn't have to be; it could be a Switch Downstream Port.

The caller shouldn't have to traverse up the tree, checking whether
this _DSM function is implemented at each level.  So I think the
driver should call this with *its* device, and
pci_acpi_request_d3cold_aux_power() should walk up the tree looking
for this _DSM function.

> + * This function shall be only called by 1st non-bridge Endpoint driver
> + * on Function 0. For a Multi-Function Device, the driver for Function 0 is
> + * required to report an aggregate power requirement covering all
> + * functions contained within the device.

This last paragraph covers two separate issues:

  1) Sec 4.6.10 requires driver for function 0 to aggregate power
     requirements for all functions in a multi-function device.  We
     can't enforce that the driver, e.g., xe, does this, but it seems
     like this is a hint that we should only evaluate this _DSM for
     function 0 of a multi-function device.  So it seems like we
     should return an error if called for a function other than 0.

  2) The "Location" part of sec 4.6.10 has a separate restriction
     that system software, i.e., the PCI core, should aggregate
     requests from child devices, which we don't do so far.

     If drivers call pci_acpi_request_d3cold_aux_power() for their
     devices (not the Root Port), there might be a way to do this
     aggregation by tracking the sum of all requests at the Downstream
     Port where we find this _DSM function implemented.  But I don't
     know whether it's worth trying to implement this now because it
     seems complicated.

I don't know what "1st non-bridge Endpoint driver" means.  How does a
driver writer know whether the driver qualifies?  Maybe just keep the
part about this only being supported for function 0 and note that we
don't support the aggregation across multiple devices?

Wrap this all to fit in 80 columns like the rest of the file.

> + * Return: Returns 0 on success and errno on failure.
> + */
> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +				      u32 *retry_interval)

Name the parameter "requested_power_mw" or even just "requested_mw" so
we know the units.  Also in header file.

> +{
> +	union acpi_object in_obj = {
> +		.integer.type = ACPI_TYPE_INTEGER,
> +		.integer.value = requested_power,
> +	};
> +
> +	union acpi_object *out_obj;
> +	acpi_handle handle;
> +	int result, ret = -EINVAL;
> +
> +	if (!dev || !retry_interval)
> +		return -EINVAL;

I think it's reasonable to allow retry_interval to be NULL if the
caller doesn't want to bother with retries.

> +	handle = ACPI_HANDLE(&dev->dev);
> +	if (!handle)
> +		return -EINVAL;
> +
> +	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_D3COLD_AUX_POWER_LIMIT)) {
> +		pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_D3COLD_AUX_POWER_LIMIT);
> +		return -ENODEV;
> +	}
> +
> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
> +					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
> +					  &in_obj, ACPI_TYPE_INTEGER);
> +	if (!out_obj)
> +		return -EINVAL;
> +
> +	result = out_obj->integer.value;
> +	if (retry_interval)
> +		*retry_interval = 0;
> +
> +	switch (result) {
> +	case 0x0:
> +		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
> +			requested_power);
> +		break;
> +	case 0x1:
> +		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
> +			 requested_power);
> +		ret = 0;
> +		break;
> +	case 0x2:
> +		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
> +		ret = -EBUSY;
> +		break;
> +	default:
> +		if (result >= 0x11 && result <= 0x1F) {
> +			if (retry_interval) {
> +				*retry_interval = result & 0xF;
> +				pci_warn(dev, "D3cold Aux Power request needs retry interval: %u seconds\n",
> +					 *retry_interval);

Seems like pci_info() to me; the user can't do anything about this,
nothing is really wrong, and the message shouldn't prompt a bug
report.

> +				ret = -EAGAIN;
> +			}
> +		} else {
> +			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
> +				result);
> +		}
> +		break;
> +	}
> +
> +	ACPI_FREE(out_obj);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
> +
>  static void pci_acpi_set_external_facing(struct pci_dev *dev)
>  {
>  	u8 val;
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 078225b514d4..6079306ad754 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -121,6 +121,7 @@ extern const guid_t pci_acpi_dsm_guid;
>  #define DSM_PCI_DEVICE_NAME			0x07
>  #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
>  #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
> +#define DSM_PCI_D3COLD_AUX_POWER_LIMIT		0x0A
>  
>  #ifdef CONFIG_PCIE_EDR
>  void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
> @@ -132,10 +133,17 @@ static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
>  
>  int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
>  void pci_acpi_clear_companion_lookup_hook(void);
> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +				      u32 *retry_interval);
>  
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> +static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +						    u32 *retry_interval)

Wrap this to fit in 80 columns like the rest of the file.

> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif	/* CONFIG_ACPI */
>  
>  #endif	/* _PCI_ACPI_H_ */
> -- 
> 2.34.1
> 

Re: [PATCH v4 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Nilawar, Badal]

On 05-09-2025 00:00, Bjorn Helgaas wrote:
> On Thu, May 29, 2025 at 04:46:44PM +0530, Badal Nilawar wrote:
>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>
>> Implement _DSM method 0Ah according to PCI firmware specifications,
>> section 4.6.10 Rev 3.3., to request auxilary power needed for the
>> device when in D3Cold.
> Cite as "PCI Firmware r3.3, sec 4.6.10"
Ok.
>> Note that this implementation assumes only a single device below the
>> Downstream Port will request for Aux Power Limit under a given
>> Root Port because it does not track and aggregate requests
>> from all child devices below the Downstream Port as required
>> by Section 4.6.10 Rev 3.3.
> Wrap to fill 75 columns.  Update citation as above.
Ok.
>
>> One possible mitigation would be only allowing only first PCIe
>> Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.
> I don't quite understand how this mitigation would address the lack of
> aggregation or how the mitigation could be enforced.  Maybe just drop
> this?
Ok. In patch 2 this is being mitigated by allowing one request per root 
port.
>
> s/call_DSM/call _DSM/
Ok
>
>> Signed-off-by: Varun Gupta <varun.gupta@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>> V2(Bjorn/Rafael):
>>    - Call acpi_dsm_check() to find method 0Ah supported
>>    - Return retry interval to caller
>> V3(Kuppuswamy)
>>    - Add NULL check for retry interval
>> ---
>>   drivers/pci/pci-acpi.c   | 87 ++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pci-acpi.h |  8 ++++
>>   2 files changed, 95 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index af370628e583..87f30910a5f1 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1421,6 +1421,93 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>>   	ACPI_FREE(obj);
>>   }
>>   
>> +/**
>> + * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3Cold
>> + * @dev: PCI device instance
>> + * @requested_power: Requested auxiliary power in milliwatts
>> + * @retry_interval: Retry interval returned by platform to retry auxiliary
>> + *                  power request
>> + *
>> + * This function sends a request to the host BIOS via root port ACPI _DSM Function 0Ah
>> + * for the auxiliary power needed by the PCI device when it is in D3Cold.
>> + * It checks and evaluates the _DSM (Device Specific Method) to request the auxiliary
>> + * power and handles the response accordingly.
> s/This functions sends a request/Request auxiliary power .../ (imperative mood)
> s/the host BIOS/platform firmware/ (non-x86 firmware may not be BIOS)
> s/D3Cold/D3cold/ (twice) to match other usage
> s/root port/Root Port/ to match spec usage
> s/It checks and evaluates the/Evaluate the/
> s/and handles/and handle/
Ok
>
> It's not quite clear from this description, but the code assumes this
> is called for a Root Port.  I don't think that's quite the right
> approach because the spec only says this _DSM function is implemented
> in the scope of a Downstream Port.  That *could* be a Root Port, but
> it doesn't have to be; it could be a Switch Downstream Port.
Agree, I will rephrase with "Root Port/Switch Downstream Port"
>
> The caller shouldn't have to traverse up the tree, checking whether
> this _DSM function is implemented at each level.  So I think the
> driver should call this with *its* device, and
> pci_acpi_request_d3cold_aux_power() should walk up the tree looking
> for this _DSM function.
Agree, will handle this.
>
>> + * This function shall be only called by 1st non-bridge Endpoint driver
>> + * on Function 0. For a Multi-Function Device, the driver for Function 0 is
>> + * required to report an aggregate power requirement covering all
>> + * functions contained within the device.
> This last paragraph covers two separate issues:
>
>    1) Sec 4.6.10 requires driver for function 0 to aggregate power
>       requirements for all functions in a multi-function device.  We
>       can't enforce that the driver, e.g., xe, does this, but it seems
>       like this is a hint that we should only evaluate this _DSM for
>       function 0 of a multi-function device.  So it seems like we
>       should return an error if called for a function other than 0.
Fine, I will return error for function other than 0.
>
>    2) The "Location" part of sec 4.6.10 has a separate restriction
>       that system software, i.e., the PCI core, should aggregate
>       requests from child devices, which we don't do so far.
>
>       If drivers call pci_acpi_request_d3cold_aux_power() for their
>       devices (not the Root Port), there might be a way to do this
>       aggregation by tracking the sum of all requests at the Downstream
>       Port where we find this _DSM function implemented.  But I don't
>       know whether it's worth trying to implement this now because it
>       seems complicated.
Aggregation is complicated and not required for current use case.
>
> I don't know what "1st non-bridge Endpoint driver" means.  How does a
> driver writer know whether the driver qualifies?  Maybe just keep the
> part about this only being supported for function 0 and note that we
> don't support the aggregation across multiple devices?
Ok.
>
> Wrap this all to fit in 80 columns like the rest of the file.
Now it is allowed to fit in 100 columns but I will fit it in 80 columns 
to maintain consistency with rest of the file.
>
>> + * Return: Returns 0 on success and errno on failure.
>> + */
>> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>> +				      u32 *retry_interval)
> Name the parameter "requested_power_mw" or even just "requested_mw" so
> we know the units.  Also in header file.
Ok
>
>> +{
>> +	union acpi_object in_obj = {
>> +		.integer.type = ACPI_TYPE_INTEGER,
>> +		.integer.value = requested_power,
>> +	};
>> +
>> +	union acpi_object *out_obj;
>> +	acpi_handle handle;
>> +	int result, ret = -EINVAL;
>> +
>> +	if (!dev || !retry_interval)
>> +		return -EINVAL;
> I think it's reasonable to allow retry_interval to be NULL if the
> caller doesn't want to bother with retries.
Ok
>
>> +	handle = ACPI_HANDLE(&dev->dev);
>> +	if (!handle)
>> +		return -EINVAL;
>> +
>> +	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_D3COLD_AUX_POWER_LIMIT)) {
>> +		pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_D3COLD_AUX_POWER_LIMIT);
>> +		return -ENODEV;
>> +	}
>> +
>> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>> +					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>> +					  &in_obj, ACPI_TYPE_INTEGER);
>> +	if (!out_obj)
>> +		return -EINVAL;
>> +
>> +	result = out_obj->integer.value;
>> +	if (retry_interval)
>> +		*retry_interval = 0;
>> +
>> +	switch (result) {
>> +	case 0x0:
>> +		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>> +			requested_power);
>> +		break;
>> +	case 0x1:
>> +		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>> +			 requested_power);
>> +		ret = 0;
>> +		break;
>> +	case 0x2:
>> +		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
>> +		ret = -EBUSY;
>> +		break;
>> +	default:
>> +		if (result >= 0x11 && result <= 0x1F) {
>> +			if (retry_interval) {
>> +				*retry_interval = result & 0xF;
>> +				pci_warn(dev, "D3cold Aux Power request needs retry interval: %u seconds\n",
>> +					 *retry_interval);
> Seems like pci_info() to me; the user can't do anything about this,
> nothing is really wrong, and the message shouldn't prompt a bug
> report.
Ok.
>
>> +				ret = -EAGAIN;
>> +			}
>> +		} else {
>> +			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>> +				result);
>> +		}
>> +		break;
>> +	}
>> +
>> +	ACPI_FREE(out_obj);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>> +
>>   static void pci_acpi_set_external_facing(struct pci_dev *dev)
>>   {
>>   	u8 val;
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 078225b514d4..6079306ad754 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -121,6 +121,7 @@ extern const guid_t pci_acpi_dsm_guid;
>>   #define DSM_PCI_DEVICE_NAME			0x07
>>   #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
>>   #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
>> +#define DSM_PCI_D3COLD_AUX_POWER_LIMIT		0x0A
>>   
>>   #ifdef CONFIG_PCIE_EDR
>>   void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
>> @@ -132,10 +133,17 @@ static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
>>   
>>   int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
>>   void pci_acpi_clear_companion_lookup_hook(void);
>> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>> +				      u32 *retry_interval);
>>   
>>   #else	/* CONFIG_ACPI */
>>   static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> +static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>> +						    u32 *retry_interval)
> Wrap this to fit in 80 columns like the rest of the file.

Ok.

Thanks,
Badal

>
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>>   #endif	/* CONFIG_ACPI */
>>   
>>   #endif	/* _PCI_ACPI_H_ */
>> -- 
>> 2.34.1
>>

[PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Badal Nilawar]

For given root port allow one Aux power limit request.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 drivers/acpi/scan.c     |  1 +
 drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
 include/acpi/acpi_bus.h |  2 ++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index fb1fe9f3b1a3..9ae7be9db01a 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
 	INIT_LIST_HEAD(&device->physical_node_list);
 	INIT_LIST_HEAD(&device->del_list);
 	mutex_init(&device->physical_node_lock);
+	mutex_init(&device->power.aux_pwr_lock);
 
 	mutex_lock(&acpi_device_lock);
 
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 87f30910a5f1..d33efba4ca94 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
 	union acpi_object *out_obj;
 	acpi_handle handle;
 	int result, ret = -EINVAL;
+	struct acpi_device *adev;
 
 	if (!dev || !retry_interval)
 		return -EINVAL;
@@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
 		return -ENODEV;
 	}
 
+	adev = ACPI_COMPANION(&dev->dev);
+	if (!adev)
+		return -EINVAL;
+
+	mutex_lock(&adev->power.aux_pwr_lock);
+
+	/* Check if aux power already granted */
+	if (adev->power.aux_power_limit) {
+		pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
+			 adev->power.aux_power_limit);
+		mutex_unlock(&adev->power.aux_pwr_lock);
+		return -EPERM;
+	}
+
 	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
 					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
 					  &in_obj, ACPI_TYPE_INTEGER);
-	if (!out_obj)
+	if (!out_obj) {
+		mutex_unlock(&adev->power.aux_pwr_lock);
 		return -EINVAL;
+	}
 
 	result = out_obj->integer.value;
 	if (retry_interval)
@@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
 	case 0x0:
 		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
 			requested_power);
+		adev->power.aux_power_limit = 0;
 		break;
 	case 0x1:
 		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
 			 requested_power);
+		adev->power.aux_power_limit = requested_power;
 		ret = 0;
 		break;
 	case 0x2:
 		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
+		adev->power.aux_power_limit = 0;
 		ret = -EBUSY;
 		break;
 	default:
@@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
 			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
 				result);
 		}
+		adev->power.aux_power_limit = 0;
 		break;
 	}
 
+	mutex_unlock(&adev->power.aux_pwr_lock);
+
 	ACPI_FREE(out_obj);
 	return ret;
 }
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index aad1a95e6863..c4ce3d84be00 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -294,6 +294,8 @@ struct acpi_device_power {
 	struct acpi_device_power_flags flags;
 	struct acpi_device_power_state states[ACPI_D_STATE_COUNT];	/* Power states (D0-D3Cold) */
 	u8 state_for_enumeration; /* Deepest power state for enumeration */
+	u32 aux_power_limit;		/* aux power limit granted by bios */
+	struct mutex aux_pwr_lock;	/* prevent concurrent aux power limit requests */
 };
 
 struct acpi_dep_data {
-- 
2.34.1

Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Sathyanarayanan Kuppuswamy]

On 5/29/25 4:16 AM, Badal Nilawar wrote:
> For given root port allow one Aux power limit request.
>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>   drivers/acpi/scan.c     |  1 +
>   drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
>   include/acpi/acpi_bus.h |  2 ++
>   3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index fb1fe9f3b1a3..9ae7be9db01a 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
>   	INIT_LIST_HEAD(&device->physical_node_list);
>   	INIT_LIST_HEAD(&device->del_list);
>   	mutex_init(&device->physical_node_lock);
> +	mutex_init(&device->power.aux_pwr_lock);
>   
>   	mutex_lock(&acpi_device_lock);
>   
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 87f30910a5f1..d33efba4ca94 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>   	union acpi_object *out_obj;
>   	acpi_handle handle;
>   	int result, ret = -EINVAL;
> +	struct acpi_device *adev;
>   
>   	if (!dev || !retry_interval)
>   		return -EINVAL;
> @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>   		return -ENODEV;
>   	}
>   
> +	adev = ACPI_COMPANION(&dev->dev);
> +	if (!adev)
> +		return -EINVAL;
> +
> +	mutex_lock(&adev->power.aux_pwr_lock);
> +
> +	/* Check if aux power already granted */
> +	if (adev->power.aux_power_limit) {
> +		pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
> +			 adev->power.aux_power_limit);
> +		mutex_unlock(&adev->power.aux_pwr_lock);
> +		return -EPERM;
> +	}
> +
>   	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>   					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>   					  &in_obj, ACPI_TYPE_INTEGER);
> -	if (!out_obj)
> +	if (!out_obj) {
> +		mutex_unlock(&adev->power.aux_pwr_lock);
>   		return -EINVAL;
> +	}
>   
>   	result = out_obj->integer.value;
>   	if (retry_interval)
> @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>   	case 0x0:
>   		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>   			requested_power);
> +		adev->power.aux_power_limit = 0;
>   		break;
>   	case 0x1:
>   		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>   			 requested_power);
> +		adev->power.aux_power_limit = requested_power;
>   		ret = 0;
>   		break;
>   	case 0x2:
>   		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
> +		adev->power.aux_power_limit = 0;
>   		ret = -EBUSY;
>   		break;
>   	default:
> @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>   			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>   				result);
>   		}
> +		adev->power.aux_power_limit = 0;
>   		break;
>   	}
>   
> +	mutex_unlock(&adev->power.aux_pwr_lock);
> +
>   	ACPI_FREE(out_obj);
>   	return ret;
>   }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index aad1a95e6863..c4ce3d84be00 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -294,6 +294,8 @@ struct acpi_device_power {
>   	struct acpi_device_power_flags flags;
>   	struct acpi_device_power_state states[ACPI_D_STATE_COUNT];	/* Power states (D0-D3Cold) */
>   	u8 state_for_enumeration; /* Deepest power state for enumeration */
> +	u32 aux_power_limit;		/* aux power limit granted by bios */
> +	struct mutex aux_pwr_lock;	/* prevent concurrent aux power limit requests */


Do you need a new lock ? Is it possible to reuse existing mutex like device_lock()?


>   };
>   
>   struct acpi_dep_data {

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Rafael J. Wysocki]

On Thu, May 29, 2025 at 11:41 PM Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
> On 5/29/25 4:16 AM, Badal Nilawar wrote:
> > For given root port allow one Aux power limit request.
> >
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > ---
> >   drivers/acpi/scan.c     |  1 +
> >   drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
> >   include/acpi/acpi_bus.h |  2 ++
> >   3 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index fb1fe9f3b1a3..9ae7be9db01a 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
> >       INIT_LIST_HEAD(&device->physical_node_list);
> >       INIT_LIST_HEAD(&device->del_list);
> >       mutex_init(&device->physical_node_lock);
> > +     mutex_init(&device->power.aux_pwr_lock);
> >
> >       mutex_lock(&acpi_device_lock);
> >
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 87f30910a5f1..d33efba4ca94 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> >       union acpi_object *out_obj;
> >       acpi_handle handle;
> >       int result, ret = -EINVAL;
> > +     struct acpi_device *adev;
> >
> >       if (!dev || !retry_interval)
> >               return -EINVAL;
> > @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> >               return -ENODEV;
> >       }
> >
> > +     adev = ACPI_COMPANION(&dev->dev);
> > +     if (!adev)
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&adev->power.aux_pwr_lock);
> > +
> > +     /* Check if aux power already granted */
> > +     if (adev->power.aux_power_limit) {
> > +             pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
> > +                      adev->power.aux_power_limit);
> > +             mutex_unlock(&adev->power.aux_pwr_lock);
> > +             return -EPERM;
> > +     }
> > +
> >       out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
> >                                         DSM_PCI_D3COLD_AUX_POWER_LIMIT,
> >                                         &in_obj, ACPI_TYPE_INTEGER);
> > -     if (!out_obj)
> > +     if (!out_obj) {
> > +             mutex_unlock(&adev->power.aux_pwr_lock);
> >               return -EINVAL;
> > +     }
> >
> >       result = out_obj->integer.value;
> >       if (retry_interval)
> > @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> >       case 0x0:
> >               pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
> >                       requested_power);
> > +             adev->power.aux_power_limit = 0;
> >               break;
> >       case 0x1:
> >               pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
> >                        requested_power);
> > +             adev->power.aux_power_limit = requested_power;
> >               ret = 0;
> >               break;
> >       case 0x2:
> >               pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
> > +             adev->power.aux_power_limit = 0;
> >               ret = -EBUSY;
> >               break;
> >       default:
> > @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> >                       pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
> >                               result);
> >               }
> > +             adev->power.aux_power_limit = 0;
> >               break;
> >       }
> >
> > +     mutex_unlock(&adev->power.aux_pwr_lock);
> > +
> >       ACPI_FREE(out_obj);
> >       return ret;
> >   }
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index aad1a95e6863..c4ce3d84be00 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -294,6 +294,8 @@ struct acpi_device_power {
> >       struct acpi_device_power_flags flags;
> >       struct acpi_device_power_state states[ACPI_D_STATE_COUNT];      /* Power states (D0-D3Cold) */
> >       u8 state_for_enumeration; /* Deepest power state for enumeration */
> > +     u32 aux_power_limit;            /* aux power limit granted by bios */
> > +     struct mutex aux_pwr_lock;      /* prevent concurrent aux power limit requests */
>
>
> Do you need a new lock ?

Yes.

> Is it possible to reuse existing mutex like device_lock()?

No.

Doing such things results in code where nobody knows what the given
lock scope is.

Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Sathyanarayanan Kuppuswamy]

On 7/2/25 4:11 AM, Rafael J. Wysocki wrote:
> On Thu, May 29, 2025 at 11:41 PM Sathyanarayanan Kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> On 5/29/25 4:16 AM, Badal Nilawar wrote:
>>> For given root port allow one Aux power limit request.
>>>
>>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>> ---
>>>    drivers/acpi/scan.c     |  1 +
>>>    drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
>>>    include/acpi/acpi_bus.h |  2 ++
>>>    3 files changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>> index fb1fe9f3b1a3..9ae7be9db01a 100644
>>> --- a/drivers/acpi/scan.c
>>> +++ b/drivers/acpi/scan.c
>>> @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
>>>        INIT_LIST_HEAD(&device->physical_node_list);
>>>        INIT_LIST_HEAD(&device->del_list);
>>>        mutex_init(&device->physical_node_lock);
>>> +     mutex_init(&device->power.aux_pwr_lock);
>>>
>>>        mutex_lock(&acpi_device_lock);
>>>
>>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>>> index 87f30910a5f1..d33efba4ca94 100644
>>> --- a/drivers/pci/pci-acpi.c
>>> +++ b/drivers/pci/pci-acpi.c
>>> @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>>        union acpi_object *out_obj;
>>>        acpi_handle handle;
>>>        int result, ret = -EINVAL;
>>> +     struct acpi_device *adev;
>>>
>>>        if (!dev || !retry_interval)
>>>                return -EINVAL;
>>> @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>>                return -ENODEV;
>>>        }
>>>
>>> +     adev = ACPI_COMPANION(&dev->dev);
>>> +     if (!adev)
>>> +             return -EINVAL;
>>> +
>>> +     mutex_lock(&adev->power.aux_pwr_lock);
>>> +
>>> +     /* Check if aux power already granted */
>>> +     if (adev->power.aux_power_limit) {
>>> +             pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
>>> +                      adev->power.aux_power_limit);
>>> +             mutex_unlock(&adev->power.aux_pwr_lock);
>>> +             return -EPERM;
>>> +     }
>>> +
>>>        out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>>>                                          DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>>>                                          &in_obj, ACPI_TYPE_INTEGER);
>>> -     if (!out_obj)
>>> +     if (!out_obj) {
>>> +             mutex_unlock(&adev->power.aux_pwr_lock);
>>>                return -EINVAL;
>>> +     }
>>>
>>>        result = out_obj->integer.value;
>>>        if (retry_interval)
>>> @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>>        case 0x0:
>>>                pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>>>                        requested_power);
>>> +             adev->power.aux_power_limit = 0;
>>>                break;
>>>        case 0x1:
>>>                pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>>>                         requested_power);
>>> +             adev->power.aux_power_limit = requested_power;
>>>                ret = 0;
>>>                break;
>>>        case 0x2:
>>>                pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
>>> +             adev->power.aux_power_limit = 0;
>>>                ret = -EBUSY;
>>>                break;
>>>        default:
>>> @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>>                        pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>>>                                result);
>>>                }
>>> +             adev->power.aux_power_limit = 0;
>>>                break;
>>>        }
>>>
>>> +     mutex_unlock(&adev->power.aux_pwr_lock);
>>> +
>>>        ACPI_FREE(out_obj);
>>>        return ret;
>>>    }
>>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>>> index aad1a95e6863..c4ce3d84be00 100644
>>> --- a/include/acpi/acpi_bus.h
>>> +++ b/include/acpi/acpi_bus.h
>>> @@ -294,6 +294,8 @@ struct acpi_device_power {
>>>        struct acpi_device_power_flags flags;
>>>        struct acpi_device_power_state states[ACPI_D_STATE_COUNT];      /* Power states (D0-D3Cold) */
>>>        u8 state_for_enumeration; /* Deepest power state for enumeration */
>>> +     u32 aux_power_limit;            /* aux power limit granted by bios */
>>> +     struct mutex aux_pwr_lock;      /* prevent concurrent aux power limit requests */
>>
>> Do you need a new lock ?
> Yes.
>
>> Is it possible to reuse existing mutex like device_lock()?
> No.
>
> Doing such things results in code where nobody knows what the given
> lock scope is.

Got it. Thanks for the clarification.


-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Rafael J. Wysocki]

On Thu, May 29, 2025 at 1:14 PM Badal Nilawar <badal.nilawar@intel.com> wrote:
>
> For given root port allow one Aux power limit request.
>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>  drivers/acpi/scan.c     |  1 +
>  drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
>  include/acpi/acpi_bus.h |  2 ++
>  3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index fb1fe9f3b1a3..9ae7be9db01a 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
>         INIT_LIST_HEAD(&device->physical_node_list);
>         INIT_LIST_HEAD(&device->del_list);
>         mutex_init(&device->physical_node_lock);
> +       mutex_init(&device->power.aux_pwr_lock);
>
>         mutex_lock(&acpi_device_lock);
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 87f30910a5f1..d33efba4ca94 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>         union acpi_object *out_obj;
>         acpi_handle handle;
>         int result, ret = -EINVAL;
> +       struct acpi_device *adev;
>
>         if (!dev || !retry_interval)
>                 return -EINVAL;
> @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>                 return -ENODEV;
>         }
>
> +       adev = ACPI_COMPANION(&dev->dev);
> +       if (!adev)
> +               return -EINVAL;
> +
> +       mutex_lock(&adev->power.aux_pwr_lock);

Use a mutex locking guard for this new lock, please.

> +
> +       /* Check if aux power already granted */
> +       if (adev->power.aux_power_limit) {
> +               pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
> +                        adev->power.aux_power_limit);
> +               mutex_unlock(&adev->power.aux_pwr_lock);
> +               return -EPERM;

Maybe -EALREADY?

> +       }
> +
>         out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>                                           DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>                                           &in_obj, ACPI_TYPE_INTEGER);
> -       if (!out_obj)
> +       if (!out_obj) {
> +               mutex_unlock(&adev->power.aux_pwr_lock);
>                 return -EINVAL;
> +       }
>
>         result = out_obj->integer.value;
>         if (retry_interval)
> @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>         case 0x0:
>                 pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>                         requested_power);
> +               adev->power.aux_power_limit = 0;
>                 break;
>         case 0x1:
>                 pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>                          requested_power);
> +               adev->power.aux_power_limit = requested_power;
>                 ret = 0;
>                 break;
>         case 0x2:
>                 pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
> +               adev->power.aux_power_limit = 0;
>                 ret = -EBUSY;
>                 break;
>         default:
> @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>                         pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>                                 result);
>                 }
> +               adev->power.aux_power_limit = 0;
>                 break;
>         }
>
> +       mutex_unlock(&adev->power.aux_pwr_lock);
> +
>         ACPI_FREE(out_obj);
>         return ret;
>  }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index aad1a95e6863..c4ce3d84be00 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -294,6 +294,8 @@ struct acpi_device_power {
>         struct acpi_device_power_flags flags;
>         struct acpi_device_power_state states[ACPI_D_STATE_COUNT];      /* Power states (D0-D3Cold) */
>         u8 state_for_enumeration; /* Deepest power state for enumeration */
> +       u32 aux_power_limit;            /* aux power limit granted by bios */
> +       struct mutex aux_pwr_lock;      /* prevent concurrent aux power limit requests */
>  };
>
>  struct acpi_dep_data {
> --
> 2.34.1
>

Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Nilawar, Badal]

On 02-07-2025 16:51, Rafael J. Wysocki wrote:
> On Thu, May 29, 2025 at 1:14 PM Badal Nilawar <badal.nilawar@intel.com> wrote:
>> For given root port allow one Aux power limit request.
>>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   drivers/acpi/scan.c     |  1 +
>>   drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
>>   include/acpi/acpi_bus.h |  2 ++
>>   3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index fb1fe9f3b1a3..9ae7be9db01a 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
>>          INIT_LIST_HEAD(&device->physical_node_list);
>>          INIT_LIST_HEAD(&device->del_list);
>>          mutex_init(&device->physical_node_lock);
>> +       mutex_init(&device->power.aux_pwr_lock);
>>
>>          mutex_lock(&acpi_device_lock);
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 87f30910a5f1..d33efba4ca94 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>          union acpi_object *out_obj;
>>          acpi_handle handle;
>>          int result, ret = -EINVAL;
>> +       struct acpi_device *adev;
>>
>>          if (!dev || !retry_interval)
>>                  return -EINVAL;
>> @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>                  return -ENODEV;
>>          }
>>
>> +       adev = ACPI_COMPANION(&dev->dev);
>> +       if (!adev)
>> +               return -EINVAL;
>> +
>> +       mutex_lock(&adev->power.aux_pwr_lock);
> Use a mutex locking guard for this new lock, please.
Sure.
>
>> +
>> +       /* Check if aux power already granted */
>> +       if (adev->power.aux_power_limit) {
>> +               pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
>> +                        adev->power.aux_power_limit);
>> +               mutex_unlock(&adev->power.aux_pwr_lock);
>> +               return -EPERM;
> Maybe -EALREADY?

 From definition it is for operation in progress. Not sure if it 
appropriate for "allow once" scenario.

#define EALREADY        114     /* Operation already in progress */

Thanks,
Badal

>
>> +       }
>> +
>>          out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>>                                            DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>>                                            &in_obj, ACPI_TYPE_INTEGER);
>> -       if (!out_obj)
>> +       if (!out_obj) {
>> +               mutex_unlock(&adev->power.aux_pwr_lock);
>>                  return -EINVAL;
>> +       }
>>
>>          result = out_obj->integer.value;
>>          if (retry_interval)
>> @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>          case 0x0:
>>                  pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>>                          requested_power);
>> +               adev->power.aux_power_limit = 0;
>>                  break;
>>          case 0x1:
>>                  pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>>                           requested_power);
>> +               adev->power.aux_power_limit = requested_power;
>>                  ret = 0;
>>                  break;
>>          case 0x2:
>>                  pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
>> +               adev->power.aux_power_limit = 0;
>>                  ret = -EBUSY;
>>                  break;
>>          default:
>> @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>                          pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>>                                  result);
>>                  }
>> +               adev->power.aux_power_limit = 0;
>>                  break;
>>          }
>>
>> +       mutex_unlock(&adev->power.aux_pwr_lock);
>> +
>>          ACPI_FREE(out_obj);
>>          return ret;
>>   }
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index aad1a95e6863..c4ce3d84be00 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -294,6 +294,8 @@ struct acpi_device_power {
>>          struct acpi_device_power_flags flags;
>>          struct acpi_device_power_state states[ACPI_D_STATE_COUNT];      /* Power states (D0-D3Cold) */
>>          u8 state_for_enumeration; /* Deepest power state for enumeration */
>> +       u32 aux_power_limit;            /* aux power limit granted by bios */
>> +       struct mutex aux_pwr_lock;      /* prevent concurrent aux power limit requests */
>>   };
>>
>>   struct acpi_dep_data {
>> --
>> 2.34.1
>>

Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Ilpo Järvinen]

On Thu, 29 May 2025, Badal Nilawar wrote:

> For given root port allow one Aux power limit request.
> 
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>  drivers/acpi/scan.c     |  1 +
>  drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
>  include/acpi/acpi_bus.h |  2 ++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index fb1fe9f3b1a3..9ae7be9db01a 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
>  	INIT_LIST_HEAD(&device->physical_node_list);
>  	INIT_LIST_HEAD(&device->del_list);
>  	mutex_init(&device->physical_node_lock);
> +	mutex_init(&device->power.aux_pwr_lock);

Hi,

mutex_destroy() also exists but I don't find any added by this patch?
I think the pre-existing mutex might also have this same problem.

>  
>  	mutex_lock(&acpi_device_lock);
>  
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 87f30910a5f1..d33efba4ca94 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  	union acpi_object *out_obj;
>  	acpi_handle handle;
>  	int result, ret = -EINVAL;
> +	struct acpi_device *adev;
>  
>  	if (!dev || !retry_interval)
>  		return -EINVAL;
> @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  		return -ENODEV;
>  	}
>  
> +	adev = ACPI_COMPANION(&dev->dev);
> +	if (!adev)
> +		return -EINVAL;
> +
> +	mutex_lock(&adev->power.aux_pwr_lock);
> +
> +	/* Check if aux power already granted */
> +	if (adev->power.aux_power_limit) {
> +		pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
> +			 adev->power.aux_power_limit);
> +		mutex_unlock(&adev->power.aux_pwr_lock);
> +		return -EPERM;
> +	}
> +
>  	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>  					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>  					  &in_obj, ACPI_TYPE_INTEGER);
> -	if (!out_obj)
> +	if (!out_obj) {
> +		mutex_unlock(&adev->power.aux_pwr_lock);
>  		return -EINVAL;
> +	}
>  
>  	result = out_obj->integer.value;
>  	if (retry_interval)
> @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  	case 0x0:
>  		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>  			requested_power);
> +		adev->power.aux_power_limit = 0;
>  		break;
>  	case 0x1:
>  		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>  			 requested_power);
> +		adev->power.aux_power_limit = requested_power;
>  		ret = 0;
>  		break;
>  	case 0x2:
>  		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
> +		adev->power.aux_power_limit = 0;
>  		ret = -EBUSY;
>  		break;
>  	default:
> @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>  				result);
>  		}
> +		adev->power.aux_power_limit = 0;
>  		break;
>  	}
>  
> +	mutex_unlock(&adev->power.aux_pwr_lock);
> +
>  	ACPI_FREE(out_obj);
>  	return ret;
>  }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index aad1a95e6863..c4ce3d84be00 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -294,6 +294,8 @@ struct acpi_device_power {
>  	struct acpi_device_power_flags flags;
>  	struct acpi_device_power_state states[ACPI_D_STATE_COUNT];	/* Power states (D0-D3Cold) */
>  	u8 state_for_enumeration; /* Deepest power state for enumeration */
> +	u32 aux_power_limit;		/* aux power limit granted by bios */
> +	struct mutex aux_pwr_lock;	/* prevent concurrent aux power limit requests */
>  };
>  
>  struct acpi_dep_data {
> 

-- 
 i.

Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Nilawar, Badal]

On 02-07-2025 16:58, Ilpo Järvinen wrote:
> On Thu, 29 May 2025, Badal Nilawar wrote:
>
>> For given root port allow one Aux power limit request.
>>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   drivers/acpi/scan.c     |  1 +
>>   drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
>>   include/acpi/acpi_bus.h |  2 ++
>>   3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index fb1fe9f3b1a3..9ae7be9db01a 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
>>   	INIT_LIST_HEAD(&device->physical_node_list);
>>   	INIT_LIST_HEAD(&device->del_list);
>>   	mutex_init(&device->physical_node_lock);
>> +	mutex_init(&device->power.aux_pwr_lock);
> Hi,
>
> mutex_destroy() also exists but I don't find any added by this patch?
> I think the pre-existing mutex might also have this same problem.

I will add mutex_destroy in acpi_device_release().

Thanks,
Badal

>
>>   
>>   	mutex_lock(&acpi_device_lock);
>>   
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 87f30910a5f1..d33efba4ca94 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   	union acpi_object *out_obj;
>>   	acpi_handle handle;
>>   	int result, ret = -EINVAL;
>> +	struct acpi_device *adev;
>>   
>>   	if (!dev || !retry_interval)
>>   		return -EINVAL;
>> @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   		return -ENODEV;
>>   	}
>>   
>> +	adev = ACPI_COMPANION(&dev->dev);
>> +	if (!adev)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&adev->power.aux_pwr_lock);
>> +
>> +	/* Check if aux power already granted */
>> +	if (adev->power.aux_power_limit) {
>> +		pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
>> +			 adev->power.aux_power_limit);
>> +		mutex_unlock(&adev->power.aux_pwr_lock);
>> +		return -EPERM;
>> +	}
>> +
>>   	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>>   					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>>   					  &in_obj, ACPI_TYPE_INTEGER);
>> -	if (!out_obj)
>> +	if (!out_obj) {
>> +		mutex_unlock(&adev->power.aux_pwr_lock);
>>   		return -EINVAL;
>> +	}
>>   
>>   	result = out_obj->integer.value;
>>   	if (retry_interval)
>> @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   	case 0x0:
>>   		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>>   			requested_power);
>> +		adev->power.aux_power_limit = 0;
>>   		break;
>>   	case 0x1:
>>   		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>>   			 requested_power);
>> +		adev->power.aux_power_limit = requested_power;
>>   		ret = 0;
>>   		break;
>>   	case 0x2:
>>   		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
>> +		adev->power.aux_power_limit = 0;
>>   		ret = -EBUSY;
>>   		break;
>>   	default:
>> @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>>   				result);
>>   		}
>> +		adev->power.aux_power_limit = 0;
>>   		break;
>>   	}
>>   
>> +	mutex_unlock(&adev->power.aux_pwr_lock);
>> +
>>   	ACPI_FREE(out_obj);
>>   	return ret;
>>   }
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index aad1a95e6863..c4ce3d84be00 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -294,6 +294,8 @@ struct acpi_device_power {
>>   	struct acpi_device_power_flags flags;
>>   	struct acpi_device_power_state states[ACPI_D_STATE_COUNT];	/* Power states (D0-D3Cold) */
>>   	u8 state_for_enumeration; /* Deepest power state for enumeration */
>> +	u32 aux_power_limit;		/* aux power limit granted by bios */
>> +	struct mutex aux_pwr_lock;	/* prevent concurrent aux power limit requests */
>>   };
>>   
>>   struct acpi_dep_data {
>>

Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Bjorn Helgaas]

On Thu, May 29, 2025 at 04:46:45PM +0530, Badal Nilawar wrote:
> For given root port allow one Aux power limit request.

Please include the reason why we do this.  I don't think we need it.

PCI Firmware r3.3, sec 4.6.10 says this _DSM function can be invoked
multiple times with different requests.

We might need a mutex just to avoid concurrent evaluations of this
_DSM function by different drivers; I'm not sure whether there's a
requirement to avoid that or whether the ACPI core already enforces
something like that.  But that would be a separate thing from
aux_power_limit.

> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>  drivers/acpi/scan.c     |  1 +
>  drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
>  include/acpi/acpi_bus.h |  2 ++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index fb1fe9f3b1a3..9ae7be9db01a 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
>  	INIT_LIST_HEAD(&device->physical_node_list);
>  	INIT_LIST_HEAD(&device->del_list);
>  	mutex_init(&device->physical_node_lock);
> +	mutex_init(&device->power.aux_pwr_lock);
>  
>  	mutex_lock(&acpi_device_lock);
>  
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 87f30910a5f1..d33efba4ca94 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  	union acpi_object *out_obj;
>  	acpi_handle handle;
>  	int result, ret = -EINVAL;
> +	struct acpi_device *adev;
>  
>  	if (!dev || !retry_interval)
>  		return -EINVAL;
> @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  		return -ENODEV;
>  	}
>  
> +	adev = ACPI_COMPANION(&dev->dev);
> +	if (!adev)
> +		return -EINVAL;
> +
> +	mutex_lock(&adev->power.aux_pwr_lock);
> +
> +	/* Check if aux power already granted */
> +	if (adev->power.aux_power_limit) {
> +		pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
> +			 adev->power.aux_power_limit);
> +		mutex_unlock(&adev->power.aux_pwr_lock);
> +		return -EPERM;
> +	}
> +
>  	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>  					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>  					  &in_obj, ACPI_TYPE_INTEGER);
> -	if (!out_obj)
> +	if (!out_obj) {
> +		mutex_unlock(&adev->power.aux_pwr_lock);
>  		return -EINVAL;
> +	}
>  
>  	result = out_obj->integer.value;
>  	if (retry_interval)
> @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  	case 0x0:
>  		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>  			requested_power);
> +		adev->power.aux_power_limit = 0;
>  		break;
>  	case 0x1:
>  		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>  			 requested_power);
> +		adev->power.aux_power_limit = requested_power;
>  		ret = 0;
>  		break;
>  	case 0x2:
>  		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
> +		adev->power.aux_power_limit = 0;
>  		ret = -EBUSY;
>  		break;
>  	default:
> @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>  				result);
>  		}
> +		adev->power.aux_power_limit = 0;
>  		break;
>  	}
>  
> +	mutex_unlock(&adev->power.aux_pwr_lock);
> +
>  	ACPI_FREE(out_obj);
>  	return ret;
>  }
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index aad1a95e6863..c4ce3d84be00 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -294,6 +294,8 @@ struct acpi_device_power {
>  	struct acpi_device_power_flags flags;
>  	struct acpi_device_power_state states[ACPI_D_STATE_COUNT];	/* Power states (D0-D3Cold) */
>  	u8 state_for_enumeration; /* Deepest power state for enumeration */
> +	u32 aux_power_limit;		/* aux power limit granted by bios */
> +	struct mutex aux_pwr_lock;	/* prevent concurrent aux power limit requests */
>  };
>  
>  struct acpi_dep_data {
> -- 
> 2.34.1
> 

Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

[Nilawar, Badal]

[-- Attachment #1: Type: text/plain, Size: 4803 bytes --]


On 05-09-2025 00:06, Bjorn Helgaas wrote:
> On Thu, May 29, 2025 at 04:46:45PM +0530, Badal Nilawar wrote:
>> For given root port allow one Aux power limit request.
> Please include the reason why we do this.  I don't think we need it.

  This is added as aggregation is not supported. I will add the reason.

>
> PCI Firmware r3.3, sec 4.6.10 says this _DSM function can be invoked
> multiple times with different requests.
>
> We might need a mutex just to avoid concurrent evaluations of this
> _DSM function by different drivers; I'm not sure whether there's a
> requirement to avoid that or whether the ACPI core already enforces
> something like that.  But that would be a separate thing from
> aux_power_limit.

As discussed in patch1 as driver's pci_dev  will be passed to 
pci_acpi_request_d3cold_aux_power(), will save that |pci_dev| in |struct 
acpi_device_power| to support multiple |_DSM| invocations by the same 
driver.

Thanks,
Badal

>
>> Cc: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>> Cc: Anshuman Gupta<anshuman.gupta@intel.com>
>> Signed-off-by: Badal Nilawar<badal.nilawar@intel.com>
>> ---
>>   drivers/acpi/scan.c     |  1 +
>>   drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
>>   include/acpi/acpi_bus.h |  2 ++
>>   3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index fb1fe9f3b1a3..9ae7be9db01a 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
>>   	INIT_LIST_HEAD(&device->physical_node_list);
>>   	INIT_LIST_HEAD(&device->del_list);
>>   	mutex_init(&device->physical_node_lock);
>> +	mutex_init(&device->power.aux_pwr_lock);
>>   
>>   	mutex_lock(&acpi_device_lock);
>>   
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 87f30910a5f1..d33efba4ca94 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   	union acpi_object *out_obj;
>>   	acpi_handle handle;
>>   	int result, ret = -EINVAL;
>> +	struct acpi_device *adev;
>>   
>>   	if (!dev || !retry_interval)
>>   		return -EINVAL;
>> @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   		return -ENODEV;
>>   	}
>>   
>> +	adev = ACPI_COMPANION(&dev->dev);
>> +	if (!adev)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&adev->power.aux_pwr_lock);
>> +
>> +	/* Check if aux power already granted */
>> +	if (adev->power.aux_power_limit) {
>> +		pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
>> +			 adev->power.aux_power_limit);
>> +		mutex_unlock(&adev->power.aux_pwr_lock);
>> +		return -EPERM;
>> +	}
>> +
>>   	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>>   					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>>   					  &in_obj, ACPI_TYPE_INTEGER);
>> -	if (!out_obj)
>> +	if (!out_obj) {
>> +		mutex_unlock(&adev->power.aux_pwr_lock);
>>   		return -EINVAL;
>> +	}
>>   
>>   	result = out_obj->integer.value;
>>   	if (retry_interval)
>> @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   	case 0x0:
>>   		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>>   			requested_power);
>> +		adev->power.aux_power_limit = 0;
>>   		break;
>>   	case 0x1:
>>   		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>>   			 requested_power);
>> +		adev->power.aux_power_limit = requested_power;
>>   		ret = 0;
>>   		break;
>>   	case 0x2:
>>   		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
>> +		adev->power.aux_power_limit = 0;
>>   		ret = -EBUSY;
>>   		break;
>>   	default:
>> @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>>   				result);
>>   		}
>> +		adev->power.aux_power_limit = 0;
>>   		break;
>>   	}
>>   
>> +	mutex_unlock(&adev->power.aux_pwr_lock);
>> +
>>   	ACPI_FREE(out_obj);
>>   	return ret;
>>   }
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index aad1a95e6863..c4ce3d84be00 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -294,6 +294,8 @@ struct acpi_device_power {
>>   	struct acpi_device_power_flags flags;
>>   	struct acpi_device_power_state states[ACPI_D_STATE_COUNT];	/* Power states (D0-D3Cold) */
>>   	u8 state_for_enumeration; /* Deepest power state for enumeration */
>> +	u32 aux_power_limit;		/* aux power limit granted by bios */
>> +	struct mutex aux_pwr_lock;	/* prevent concurrent aux power limit requests */
>>   };
>>   
>>   struct acpi_dep_data {
>> -- 
>> 2.34.1
>>

[-- Attachment #2: Type: text/html, Size: 5837 bytes --]

[PATCH v4 03/11] PCI/ACPI: Add PERST# Assertion Delay _DSM method

[Badal Nilawar]

From: Anshuman Gupta <anshuman.gupta@intel.com>

Implement _DSM Method 0Bh as per PCI firmware specs
section 4.6.11 Rev 3.3.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 drivers/pci/pci-acpi.c   | 57 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |  8 +++++-
 2 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index d33efba4ca94..88044491feaa 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1531,6 +1531,63 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
 }
 EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
 
+/**
+ * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM
+ * @dev: PCI device instance
+ * @delay_us: Requested delay_us
+ *
+ * This function sends a request to the host BIOS via ACPI _DSM to grant the
+ * required PERST# delay for the specified PCI device. It evaluates the _DSM
+ * to request the PERST# delay and handles the response accordingly.
+ *
+ * Return: returns 0 on success and errno on failure.
+ */
+int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
+{
+	union acpi_object in_obj = {
+		.integer.type = ACPI_TYPE_INTEGER,
+		.integer.value = delay_us,
+	};
+
+	union acpi_object *out_obj;
+	acpi_handle handle;
+	int result, ret = -EINVAL;
+
+	if (!dev)
+		return -EINVAL;
+
+	handle = ACPI_HANDLE(&dev->dev);
+	if (!handle)
+		return -EINVAL;
+
+	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_PERST_ASSERTION_DELAY)) {
+		pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_PERST_ASSERTION_DELAY);
+		return -ENODEV;
+	}
+
+	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
+					  DSM_PCI_PERST_ASSERTION_DELAY,
+					  &in_obj, ACPI_TYPE_INTEGER);
+	if (!out_obj)
+		return -EINVAL;
+
+	result = out_obj->integer.value;
+
+	if (result == delay_us) {
+		pci_info(dev, "PERST# Assertion Delay set to %u microseconds\n", delay_us);
+		ret = 0;
+	} else if (result == 0) {
+		pci_warn(dev, "PERST# Assertion Delay request failed, no previous valid request\n");
+	} else {
+		pci_warn(dev, "PERST# Assertion Delay request failed, Previous valid delay: %u microseconds\n",
+			 result);
+	}
+
+	ACPI_FREE(out_obj);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_acpi_add_perst_assertion_delay);
+
 static void pci_acpi_set_external_facing(struct pci_dev *dev)
 {
 	u8 val;
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 6079306ad754..e53d4893cf56 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -122,6 +122,7 @@ extern const guid_t pci_acpi_dsm_guid;
 #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
 #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
 #define DSM_PCI_D3COLD_AUX_POWER_LIMIT		0x0A
+#define DSM_PCI_PERST_ASSERTION_DELAY		0x0B
 
 #ifdef CONFIG_PCIE_EDR
 void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
@@ -135,7 +136,7 @@ int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_de
 void pci_acpi_clear_companion_lookup_hook(void);
 int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
 				      u32 *retry_interval);
-
+int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us);
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
@@ -144,6 +145,11 @@ static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 req
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
+{
+	return -EOPNOTSUPP;
+}
 #endif	/* CONFIG_ACPI */
 
 #endif	/* _PCI_ACPI_H_ */
-- 
2.34.1

Re: [PATCH v4 03/11] PCI/ACPI: Add PERST# Assertion Delay _DSM method

[Sathyanarayanan Kuppuswamy]

On 5/29/25 4:16 AM, Badal Nilawar wrote:
> From: Anshuman Gupta <anshuman.gupta@intel.com>
>
> Implement _DSM Method 0Bh as per PCI firmware specs
> section 4.6.11 Rev 3.3.
>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>   drivers/pci/pci-acpi.c   | 57 ++++++++++++++++++++++++++++++++++++++++
>   include/linux/pci-acpi.h |  8 +++++-
>   2 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index d33efba4ca94..88044491feaa 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1531,6 +1531,63 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>   }
>   EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>   
> +/**
> + * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM
> + * @dev: PCI device instance
> + * @delay_us: Requested delay_us
> + *
> + * This function sends a request to the host BIOS via ACPI _DSM to grant the
> + * required PERST# delay for the specified PCI device. It evaluates the _DSM
> + * to request the PERST# delay and handles the response accordingly.
> + *
> + * Return: returns 0 on success and errno on failure.
> + */
> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
> +{
> +	union acpi_object in_obj = {
> +		.integer.type = ACPI_TYPE_INTEGER,
> +		.integer.value = delay_us,
> +	};
> +
> +	union acpi_object *out_obj;
> +	acpi_handle handle;
> +	int result, ret = -EINVAL;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	handle = ACPI_HANDLE(&dev->dev);
> +	if (!handle)
> +		return -EINVAL;
> +
> +	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_PERST_ASSERTION_DELAY)) {
> +		pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_PERST_ASSERTION_DELAY);
> +		return -ENODEV;
> +	}
> +
> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
> +					  DSM_PCI_PERST_ASSERTION_DELAY,
> +					  &in_obj, ACPI_TYPE_INTEGER);
> +	if (!out_obj)
> +		return -EINVAL;
> +
> +	result = out_obj->integer.value;
> +
> +	if (result == delay_us) {
> +		pci_info(dev, "PERST# Assertion Delay set to %u microseconds\n", delay_us);
> +		ret = 0;

I think above is a debug message. If it is set properly, why would you want to know
the details?

> +	} else if (result == 0) {
> +		pci_warn(dev, "PERST# Assertion Delay request failed, no previous valid request\n");
> +	} else {
> +		pci_warn(dev, "PERST# Assertion Delay request failed, Previous valid delay: %u microseconds\n",
> +			 result);
> +	}

May be you don't need to elaborate the error details. Will following work?

pci_warn(dev, "PERST# Assertion Delay request failed, result:%u micro seconds\n", result);

> +
> +	ACPI_FREE(out_obj);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_add_perst_assertion_delay);
> +
>   static void pci_acpi_set_external_facing(struct pci_dev *dev)
>   {
>   	u8 val;
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 6079306ad754..e53d4893cf56 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -122,6 +122,7 @@ extern const guid_t pci_acpi_dsm_guid;
>   #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
>   #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
>   #define DSM_PCI_D3COLD_AUX_POWER_LIMIT		0x0A
> +#define DSM_PCI_PERST_ASSERTION_DELAY		0x0B
>   
>   #ifdef CONFIG_PCIE_EDR
>   void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
> @@ -135,7 +136,7 @@ int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_de
>   void pci_acpi_clear_companion_lookup_hook(void);
>   int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>   				      u32 *retry_interval);
> -
> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us);
>   #else	/* CONFIG_ACPI */
>   static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> @@ -144,6 +145,11 @@ static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 req
>   {
>   	return -EOPNOTSUPP;
>   }
> +
> +static inline int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
> +{
> +	return -EOPNOTSUPP;
> +}
>   #endif	/* CONFIG_ACPI */
>   
>   #endif	/* _PCI_ACPI_H_ */

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 03/11] PCI/ACPI: Add PERST# Assertion Delay _DSM method

[Rafael J. Wysocki]

On Thu, May 29, 2025 at 11:57 PM Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
> On 5/29/25 4:16 AM, Badal Nilawar wrote:
> > From: Anshuman Gupta <anshuman.gupta@intel.com>
> >
> > Implement _DSM Method 0Bh as per PCI firmware specs
> > section 4.6.11 Rev 3.3.
> >
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > ---
> >   drivers/pci/pci-acpi.c   | 57 ++++++++++++++++++++++++++++++++++++++++
> >   include/linux/pci-acpi.h |  8 +++++-
> >   2 files changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index d33efba4ca94..88044491feaa 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -1531,6 +1531,63 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> >   }
> >   EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
> >
> > +/**
> > + * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM
> > + * @dev: PCI device instance
> > + * @delay_us: Requested delay_us
> > + *
> > + * This function sends a request to the host BIOS via ACPI _DSM to grant the
> > + * required PERST# delay for the specified PCI device. It evaluates the _DSM
> > + * to request the PERST# delay and handles the response accordingly.
> > + *
> > + * Return: returns 0 on success and errno on failure.
> > + */
> > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
> > +{
> > +     union acpi_object in_obj = {
> > +             .integer.type = ACPI_TYPE_INTEGER,
> > +             .integer.value = delay_us,
> > +     };
> > +
> > +     union acpi_object *out_obj;
> > +     acpi_handle handle;
> > +     int result, ret = -EINVAL;
> > +
> > +     if (!dev)
> > +             return -EINVAL;
> > +
> > +     handle = ACPI_HANDLE(&dev->dev);
> > +     if (!handle)
> > +             return -EINVAL;
> > +
> > +     if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_PERST_ASSERTION_DELAY)) {
> > +             pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_PERST_ASSERTION_DELAY);
> > +             return -ENODEV;
> > +     }
> > +
> > +     out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
> > +                                       DSM_PCI_PERST_ASSERTION_DELAY,
> > +                                       &in_obj, ACPI_TYPE_INTEGER);
> > +     if (!out_obj)
> > +             return -EINVAL;
> > +
> > +     result = out_obj->integer.value;
> > +
> > +     if (result == delay_us) {
> > +             pci_info(dev, "PERST# Assertion Delay set to %u microseconds\n", delay_us);
> > +             ret = 0;
>
> I think above is a debug message. If it is set properly, why would you want to know
> the details?
>
> > +     } else if (result == 0) {
> > +             pci_warn(dev, "PERST# Assertion Delay request failed, no previous valid request\n");
> > +     } else {
> > +             pci_warn(dev, "PERST# Assertion Delay request failed, Previous valid delay: %u microseconds\n",
> > +                      result);
> > +     }
>
> May be you don't need to elaborate the error details. Will following work?
>
> pci_warn(dev, "PERST# Assertion Delay request failed, result:%u micro seconds\n", result);

Or even

pci_info(dev, "PERST# Assertion Delay request failed, using %u ms
delay\n", result);

And it doesn't really need to be pci_warn().

>
> > +
> > +     ACPI_FREE(out_obj);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_acpi_add_perst_assertion_delay);
> > +
> >   static void pci_acpi_set_external_facing(struct pci_dev *dev)
> >   {
> >       u8 val;
> > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > index 6079306ad754..e53d4893cf56 100644
> > --- a/include/linux/pci-acpi.h
> > +++ b/include/linux/pci-acpi.h
> > @@ -122,6 +122,7 @@ extern const guid_t pci_acpi_dsm_guid;
> >   #define DSM_PCI_POWER_ON_RESET_DELAY                0x08
> >   #define DSM_PCI_DEVICE_READINESS_DURATIONS  0x09
> >   #define DSM_PCI_D3COLD_AUX_POWER_LIMIT              0x0A
> > +#define DSM_PCI_PERST_ASSERTION_DELAY                0x0B
> >
> >   #ifdef CONFIG_PCIE_EDR
> >   void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
> > @@ -135,7 +136,7 @@ int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_de
> >   void pci_acpi_clear_companion_lookup_hook(void);
> >   int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> >                                     u32 *retry_interval);
> > -
> > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us);
> >   #else       /* CONFIG_ACPI */
> >   static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> >   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> > @@ -144,6 +145,11 @@ static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 req
> >   {
> >       return -EOPNOTSUPP;
> >   }
> > +
> > +static inline int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
> > +{
> > +     return -EOPNOTSUPP;
> > +}
> >   #endif      /* CONFIG_ACPI */
> >
> >   #endif      /* _PCI_ACPI_H_ */
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>

Re: [PATCH v4 03/11] PCI/ACPI: Add PERST# Assertion Delay _DSM method

[Nilawar, Badal]

On 02-07-2025 16:55, Rafael J. Wysocki wrote:
> On Thu, May 29, 2025 at 11:57 PM Sathyanarayanan Kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> On 5/29/25 4:16 AM, Badal Nilawar wrote:
>>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>>
>>> Implement _DSM Method 0Bh as per PCI firmware specs
>>> section 4.6.11 Rev 3.3.
>>>
>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>> ---
>>>    drivers/pci/pci-acpi.c   | 57 ++++++++++++++++++++++++++++++++++++++++
>>>    include/linux/pci-acpi.h |  8 +++++-
>>>    2 files changed, 64 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>>> index d33efba4ca94..88044491feaa 100644
>>> --- a/drivers/pci/pci-acpi.c
>>> +++ b/drivers/pci/pci-acpi.c
>>> @@ -1531,6 +1531,63 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>>    }
>>>    EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>>>
>>> +/**
>>> + * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM
>>> + * @dev: PCI device instance
>>> + * @delay_us: Requested delay_us
>>> + *
>>> + * This function sends a request to the host BIOS via ACPI _DSM to grant the
>>> + * required PERST# delay for the specified PCI device. It evaluates the _DSM
>>> + * to request the PERST# delay and handles the response accordingly.
>>> + *
>>> + * Return: returns 0 on success and errno on failure.
>>> + */
>>> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
>>> +{
>>> +     union acpi_object in_obj = {
>>> +             .integer.type = ACPI_TYPE_INTEGER,
>>> +             .integer.value = delay_us,
>>> +     };
>>> +
>>> +     union acpi_object *out_obj;
>>> +     acpi_handle handle;
>>> +     int result, ret = -EINVAL;
>>> +
>>> +     if (!dev)
>>> +             return -EINVAL;
>>> +
>>> +     handle = ACPI_HANDLE(&dev->dev);
>>> +     if (!handle)
>>> +             return -EINVAL;
>>> +
>>> +     if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_PERST_ASSERTION_DELAY)) {
>>> +             pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_PERST_ASSERTION_DELAY);
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>>> +                                       DSM_PCI_PERST_ASSERTION_DELAY,
>>> +                                       &in_obj, ACPI_TYPE_INTEGER);
>>> +     if (!out_obj)
>>> +             return -EINVAL;
>>> +
>>> +     result = out_obj->integer.value;
>>> +
>>> +     if (result == delay_us) {
>>> +             pci_info(dev, "PERST# Assertion Delay set to %u microseconds\n", delay_us);
>>> +             ret = 0;
>> I think above is a debug message. If it is set properly, why would you want to know
>> the details?
>>
>>> +     } else if (result == 0) {
>>> +             pci_warn(dev, "PERST# Assertion Delay request failed, no previous valid request\n");
>>> +     } else {
>>> +             pci_warn(dev, "PERST# Assertion Delay request failed, Previous valid delay: %u microseconds\n",
>>> +                      result);
>>> +     }
>> May be you don't need to elaborate the error details. Will following work?
>>
>> pci_warn(dev, "PERST# Assertion Delay request failed, result:%u micro seconds\n", result);
> Or even
>
> pci_info(dev, "PERST# Assertion Delay request failed, using %u ms
> delay\n", result);
>
> And it doesn't really need to be pci_warn().

Make sense. I will incorporate above suggestion in next rev.

Thanks,
Badal

>
>>> +
>>> +     ACPI_FREE(out_obj);
>>> +     return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pci_acpi_add_perst_assertion_delay);
>>> +
>>>    static void pci_acpi_set_external_facing(struct pci_dev *dev)
>>>    {
>>>        u8 val;
>>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>>> index 6079306ad754..e53d4893cf56 100644
>>> --- a/include/linux/pci-acpi.h
>>> +++ b/include/linux/pci-acpi.h
>>> @@ -122,6 +122,7 @@ extern const guid_t pci_acpi_dsm_guid;
>>>    #define DSM_PCI_POWER_ON_RESET_DELAY                0x08
>>>    #define DSM_PCI_DEVICE_READINESS_DURATIONS  0x09
>>>    #define DSM_PCI_D3COLD_AUX_POWER_LIMIT              0x0A
>>> +#define DSM_PCI_PERST_ASSERTION_DELAY                0x0B
>>>
>>>    #ifdef CONFIG_PCIE_EDR
>>>    void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
>>> @@ -135,7 +136,7 @@ int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_de
>>>    void pci_acpi_clear_companion_lookup_hook(void);
>>>    int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>>                                      u32 *retry_interval);
>>> -
>>> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us);
>>>    #else       /* CONFIG_ACPI */
>>>    static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>>    static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>>> @@ -144,6 +145,11 @@ static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 req
>>>    {
>>>        return -EOPNOTSUPP;
>>>    }
>>> +
>>> +static inline int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
>>> +{
>>> +     return -EOPNOTSUPP;
>>> +}
>>>    #endif      /* CONFIG_ACPI */
>>>
>>>    #endif      /* _PCI_ACPI_H_ */
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
>>

Re: [PATCH v4 03/11] PCI/ACPI: Add PERST# Assertion Delay _DSM method

[Bjorn Helgaas]

On Thu, May 29, 2025 at 04:46:46PM +0530, Badal Nilawar wrote:
> From: Anshuman Gupta <anshuman.gupta@intel.com>
> 
> Implement _DSM Method 0Bh as per PCI firmware specs
> section 4.6.11 Rev 3.3.

Update citation as for the other _DSM function.

> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>  drivers/pci/pci-acpi.c   | 57 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h |  8 +++++-
>  2 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index d33efba4ca94..88044491feaa 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1531,6 +1531,63 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  }
>  EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>  
> +/**
> + * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM
> + * @dev: PCI device instance
> + * @delay_us: Requested delay_us
> + *
> + * This function sends a request to the host BIOS via ACPI _DSM to grant the
> + * required PERST# delay for the specified PCI device. It evaluates the _DSM
> + * to request the PERST# delay and handles the response accordingly.

Reword in imperative mood.

Like pci_acpi_request_d3cold_aux_power(), I think the driver should
call this with its device, not the Root Port.

> + * Return: returns 0 on success and errno on failure.
> + */
> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
> +{
> +	union acpi_object in_obj = {
> +		.integer.type = ACPI_TYPE_INTEGER,
> +		.integer.value = delay_us,
> +	};
> +
> +	union acpi_object *out_obj;
> +	acpi_handle handle;
> +	int result, ret = -EINVAL;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	handle = ACPI_HANDLE(&dev->dev);
> +	if (!handle)
> +		return -EINVAL;
> +
> +	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_PERST_ASSERTION_DELAY)) {
> +		pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_PERST_ASSERTION_DELAY);
> +		return -ENODEV;
> +	}
> +
> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
> +					  DSM_PCI_PERST_ASSERTION_DELAY,
> +					  &in_obj, ACPI_TYPE_INTEGER);
> +	if (!out_obj)
> +		return -EINVAL;
> +
> +	result = out_obj->integer.value;
> +
> +	if (result == delay_us) {
> +		pci_info(dev, "PERST# Assertion Delay set to %u microseconds\n", delay_us);
> +		ret = 0;
> +	} else if (result == 0) {
> +		pci_warn(dev, "PERST# Assertion Delay request failed, no previous valid request\n");
> +	} else {
> +		pci_warn(dev, "PERST# Assertion Delay request failed, Previous valid delay: %u microseconds\n",
> +			 result);
> +	}
> +
> +	ACPI_FREE(out_obj);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_add_perst_assertion_delay);
> +
>  static void pci_acpi_set_external_facing(struct pci_dev *dev)
>  {
>  	u8 val;
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 6079306ad754..e53d4893cf56 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -122,6 +122,7 @@ extern const guid_t pci_acpi_dsm_guid;
>  #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
>  #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
>  #define DSM_PCI_D3COLD_AUX_POWER_LIMIT		0x0A
> +#define DSM_PCI_PERST_ASSERTION_DELAY		0x0B
>  
>  #ifdef CONFIG_PCIE_EDR
>  void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
> @@ -135,7 +136,7 @@ int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_de
>  void pci_acpi_clear_companion_lookup_hook(void);
>  int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>  				      u32 *retry_interval);
> -
> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us);
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> @@ -144,6 +145,11 @@ static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 req
>  {
>  	return -EOPNOTSUPP;
>  }
> +
> +static inline int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)

Wrap to fit in 80 columns like the rest of the file.

> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif	/* CONFIG_ACPI */
>  
>  #endif	/* _PCI_ACPI_H_ */
> -- 
> 2.34.1
> 

Re: [PATCH v4 03/11] PCI/ACPI: Add PERST# Assertion Delay _DSM method

[Nilawar, Badal]

On 05-09-2025 00:14, Bjorn Helgaas wrote:
> On Thu, May 29, 2025 at 04:46:46PM +0530, Badal Nilawar wrote:
>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>
>> Implement _DSM Method 0Bh as per PCI firmware specs
>> section 4.6.11 Rev 3.3.
> Update citation as for the other _DSM function.
Sure.
>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   drivers/pci/pci-acpi.c   | 57 ++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pci-acpi.h |  8 +++++-
>>   2 files changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index d33efba4ca94..88044491feaa 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1531,6 +1531,63 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   }
>>   EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>>   
>> +/**
>> + * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM
>> + * @dev: PCI device instance
>> + * @delay_us: Requested delay_us
>> + *
>> + * This function sends a request to the host BIOS via ACPI _DSM to grant the
>> + * required PERST# delay for the specified PCI device. It evaluates the _DSM
>> + * to request the PERST# delay and handles the response accordingly.
> Reword in imperative mood.
>
> Like pci_acpi_request_d3cold_aux_power(), I think the driver should
> call this with its device, not the Root Port.
Ok.
>
>> + * Return: returns 0 on success and errno on failure.
>> + */
>> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
>> +{
>> +	union acpi_object in_obj = {
>> +		.integer.type = ACPI_TYPE_INTEGER,
>> +		.integer.value = delay_us,
>> +	};
>> +
>> +	union acpi_object *out_obj;
>> +	acpi_handle handle;
>> +	int result, ret = -EINVAL;
>> +
>> +	if (!dev)
>> +		return -EINVAL;
>> +
>> +	handle = ACPI_HANDLE(&dev->dev);
>> +	if (!handle)
>> +		return -EINVAL;
>> +
>> +	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_PERST_ASSERTION_DELAY)) {
>> +		pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_PERST_ASSERTION_DELAY);
>> +		return -ENODEV;
>> +	}
>> +
>> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>> +					  DSM_PCI_PERST_ASSERTION_DELAY,
>> +					  &in_obj, ACPI_TYPE_INTEGER);
>> +	if (!out_obj)
>> +		return -EINVAL;
>> +
>> +	result = out_obj->integer.value;
>> +
>> +	if (result == delay_us) {
>> +		pci_info(dev, "PERST# Assertion Delay set to %u microseconds\n", delay_us);
>> +		ret = 0;
>> +	} else if (result == 0) {
>> +		pci_warn(dev, "PERST# Assertion Delay request failed, no previous valid request\n");
>> +	} else {
>> +		pci_warn(dev, "PERST# Assertion Delay request failed, Previous valid delay: %u microseconds\n",
>> +			 result);
>> +	}
>> +
>> +	ACPI_FREE(out_obj);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_acpi_add_perst_assertion_delay);
>> +
>>   static void pci_acpi_set_external_facing(struct pci_dev *dev)
>>   {
>>   	u8 val;
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 6079306ad754..e53d4893cf56 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -122,6 +122,7 @@ extern const guid_t pci_acpi_dsm_guid;
>>   #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
>>   #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
>>   #define DSM_PCI_D3COLD_AUX_POWER_LIMIT		0x0A
>> +#define DSM_PCI_PERST_ASSERTION_DELAY		0x0B
>>   
>>   #ifdef CONFIG_PCIE_EDR
>>   void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
>> @@ -135,7 +136,7 @@ int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_de
>>   void pci_acpi_clear_companion_lookup_hook(void);
>>   int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   				      u32 *retry_interval);
>> -
>> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us);
>>   #else	/* CONFIG_ACPI */
>>   static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> @@ -144,6 +145,11 @@ static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 req
>>   {
>>   	return -EOPNOTSUPP;
>>   }
>> +
>> +static inline int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
> Wrap to fit in 80 columns like the rest of the file.

Ok.

Thanks,
Badal

>> +{
>> +	return -EOPNOTSUPP;
>> +}
>>   #endif	/* CONFIG_ACPI */
>>   
>>   #endif	/* _PCI_ACPI_H_ */
>> -- 
>> 2.34.1
>>

[PATCH v4 04/11] drm/xe/vrsr: Introduce flag has_vrsr

[Badal Nilawar]

Introduce flag has_vrsr to determine if platform supports VRSR feature

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 drivers/gpu/drm/xe/xe_device_types.h | 2 ++
 drivers/gpu/drm/xe/xe_pci.c          | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index f81be293b260..e2749ed2a61f 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -334,6 +334,8 @@ struct xe_device {
 		u8 has_sriov:1;
 		/** @info.has_usm: Device has unified shared memory support */
 		u8 has_usm:1;
+		/** @info.has_vrsr: Has capability to enter into VRAM self refresh */
+		u8 has_vrsr:1;
 		/** @info.has_64bit_timestamp: Device supports 64-bit timestamps */
 		u8 has_64bit_timestamp:1;
 		/** @info.is_dgfx: is discrete device */
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 024175cfe61e..46a99d6ef1a5 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -68,6 +68,7 @@ struct xe_device_desc {
 	u8 has_llc:1;
 	u8 has_pxp:1;
 	u8 has_sriov:1;
+	u8 has_vrsr:1;
 	u8 needs_scratch:1;
 	u8 skip_guc_pc:1;
 	u8 skip_mtcfg:1;
@@ -342,6 +343,7 @@ static const struct xe_device_desc bmg_desc = {
 	.dma_mask_size = 46,
 	.has_display = true,
 	.has_fan_control = true,
+	.has_vrsr = true,
 	.has_heci_cscfi = 1,
 	.needs_scratch = true,
 };
@@ -589,6 +591,7 @@ static int xe_info_init_early(struct xe_device *xe,
 	xe->info.has_llc = desc->has_llc;
 	xe->info.has_pxp = desc->has_pxp;
 	xe->info.has_sriov = desc->has_sriov;
+	xe->info.has_vrsr = desc->has_vrsr;
 	xe->info.skip_guc_pc = desc->skip_guc_pc;
 	xe->info.skip_mtcfg = desc->skip_mtcfg;
 	xe->info.skip_pcode = desc->skip_pcode;
-- 
2.34.1

Re: [v4,04/11] drm/xe/vrsr: Introduce flag has_vrsr

[Poosa, Karthik]

On 29-05-2025 16:46, Badal Nilawar wrote:
> Introduce flag has_vrsr to determine if platform supports VRSR feature
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_device_types.h | 2 ++
>   drivers/gpu/drm/xe/xe_pci.c          | 3 +++
>   2 files changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index f81be293b260..e2749ed2a61f 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -334,6 +334,8 @@ struct xe_device {
>   		u8 has_sriov:1;
>   		/** @info.has_usm: Device has unified shared memory support */
>   		u8 has_usm:1;
> +		/** @info.has_vrsr: Has capability to enter into VRAM self refresh */
> +		u8 has_vrsr:1;
>   		/** @info.has_64bit_timestamp: Device supports 64-bit timestamps */
>   		u8 has_64bit_timestamp:1;
>   		/** @info.is_dgfx: is discrete device */
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 024175cfe61e..46a99d6ef1a5 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -68,6 +68,7 @@ struct xe_device_desc {
>   	u8 has_llc:1;
>   	u8 has_pxp:1;
>   	u8 has_sriov:1;
> +	u8 has_vrsr:1;
>   	u8 needs_scratch:1;
>   	u8 skip_guc_pc:1;
>   	u8 skip_mtcfg:1;
> @@ -342,6 +343,7 @@ static const struct xe_device_desc bmg_desc = {
>   	.dma_mask_size = 46,
>   	.has_display = true,
>   	.has_fan_control = true,
> +	.has_vrsr = true,
>   	.has_heci_cscfi = 1,
>   	.needs_scratch = true,
>   };
> @@ -589,6 +591,7 @@ static int xe_info_init_early(struct xe_device *xe,
>   	xe->info.has_llc = desc->has_llc;
>   	xe->info.has_pxp = desc->has_pxp;
>   	xe->info.has_sriov = desc->has_sriov;
> +	xe->info.has_vrsr = desc->has_vrsr;
>   	xe->info.skip_guc_pc = desc->skip_guc_pc;
>   	xe->info.skip_mtcfg = desc->skip_mtcfg;
>   	xe->info.skip_pcode = desc->skip_pcode;

LGTM.

Reviewed-by: Karthik Poosa <karthik.poosa@intel.com>

[PATCH v4 05/11] drm/xe/vrsr: Detect VRSR Capability

[Badal Nilawar]

From: Anshuman Gupta <anshuman.gupta@intel.com>

Detect VRAM Self Refresh(vrsr) Capability.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/xe/regs/xe_regs.h    |  3 +++
 drivers/gpu/drm/xe/xe_device_types.h |  4 ++++
 drivers/gpu/drm/xe/xe_pm.c           | 26 ++++++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/xe/regs/xe_regs.h b/drivers/gpu/drm/xe/regs/xe_regs.h
index 3abb17d2ca33..4db486fb310a 100644
--- a/drivers/gpu/drm/xe/regs/xe_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_regs.h
@@ -53,6 +53,9 @@
 #define MTL_MPE_FREQUENCY			XE_REG(0x13802c)
 #define   MTL_RPE_MASK				REG_GENMASK(8, 0)
 
+#define VRAM_SR_CAPABILITY                         XE_REG(0x138144)
+#define   VRAM_SR_SUPPORTED                        REG_BIT(0)
+
 #define VF_CAP_REG				XE_REG(0x1901f8, XE_REG_OPTION_VF)
 #define   VF_CAP				REG_BIT(0)
 
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index e2749ed2a61f..3a15b3a252fd 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -512,6 +512,9 @@ struct xe_device {
 		/** @d3cold.allowed: Indicates if d3cold is a valid device state */
 		bool allowed;
 
+		/** @d3cold.vrsr_capable: Indicates if d3cold VRAM Self Refresh is supported */
+		bool vrsr_capable;
+
 		/**
 		 * @d3cold.vram_threshold:
 		 *
@@ -522,6 +525,7 @@ struct xe_device {
 		 * Default threshold value is 300mb.
 		 */
 		u32 vram_threshold;
+
 		/** @d3cold.lock: protect vram_threshold */
 		struct mutex lock;
 	} d3cold;
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index 693866def183..c9395e62d21d 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -13,13 +13,16 @@
 #include <drm/ttm/ttm_placement.h>
 
 #include "display/xe_display.h"
+#include "regs/xe_regs.h"
 #include "xe_bo.h"
 #include "xe_bo_evict.h"
 #include "xe_device.h"
+#include "xe_force_wake.h"
 #include "xe_ggtt.h"
 #include "xe_gt.h"
 #include "xe_guc.h"
 #include "xe_irq.h"
+#include "xe_mmio.h"
 #include "xe_pcode.h"
 #include "xe_pxp.h"
 #include "xe_trace.h"
@@ -235,6 +238,28 @@ static bool xe_pm_pci_d3cold_capable(struct xe_device *xe)
 	return true;
 }
 
+static bool xe_pm_vrsr_capable(struct xe_device *xe)
+{
+	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
+	unsigned int fw_ref;
+	struct xe_gt *gt;
+	u32 val;
+
+	gt = xe_root_mmio_gt(xe);
+
+	if (!xe->info.probe_display)
+		return false;
+
+	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
+	if (!fw_ref)
+		return false;
+
+	val = xe_mmio_read32(mmio, VRAM_SR_CAPABILITY);
+	xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
+
+	return val & VRAM_SR_SUPPORTED;
+}
+
 static void xe_pm_runtime_init(struct xe_device *xe)
 {
 	struct device *dev = xe->drm.dev;
@@ -349,6 +374,7 @@ int xe_pm_init(struct xe_device *xe)
 		err = xe_pm_set_vram_threshold(xe, vram_threshold);
 		if (err)
 			goto err_unregister;
+		xe->d3cold.vrsr_capable = xe_pm_vrsr_capable(xe);
 	}
 
 	xe_pm_runtime_init(xe);
-- 
2.34.1

[PATCH v4 06/11] drm/xe/vrsr: Initialize VRSR feature

[Badal Nilawar]

Add the API xe_pm_vrsr_enable to initialize the VRSR feature,
requesting AUX power limit and PERST# assertion delay.

V2: Add retry mechanism while requesting AUX power limit

Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/xe/xe_device_types.h |   1 +
 drivers/gpu/drm/xe/xe_pcode_api.h    |   7 ++
 drivers/gpu/drm/xe/xe_pm.c           | 105 ++++++++++++++++++++++++++-
 drivers/gpu/drm/xe/xe_pm.h           |   1 +
 4 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 3a15b3a252fd..5f9a1a358468 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -7,6 +7,7 @@
 #define _XE_DEVICE_TYPES_H_
 
 #include <linux/pci.h>
+#include <linux/pci-acpi.h>
 
 #include <drm/drm_device.h>
 #include <drm/drm_file.h>
diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
index 127d4d26c4cf..f54ef03799d4 100644
--- a/drivers/gpu/drm/xe/xe_pcode_api.h
+++ b/drivers/gpu/drm/xe/xe_pcode_api.h
@@ -43,6 +43,13 @@
 #define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
 #define	    POWER_SETUP_I1_DATA_MASK		REG_GENMASK(15, 0)
 
+#define	  PCODE_D3_VRAM_SELF_REFRESH	0x71
+#define	    PCODE_D3_VRSR_SC_DISABLE	0x0
+#define	    PCODE_D3_VRSR_SC_ENABLE	0x1
+#define     PCODE_D3_VRSR_SC_AUX_PL_AND_PERST_DELAY	0x2
+#define	    POWER_D3_VRSR_PERST_MASK	REG_GENMASK(31, 16)
+#define	    POWER_D3_VRSR_AUX_PL_MASK	REG_GENMASK(15, 0)
+
 #define   PCODE_FREQUENCY_CONFIG		0x6e
 /* Frequency Config Sub Commands (param1) */
 #define     PCODE_MBOX_FC_SC_READ_FUSED_P0	0x0
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index c9395e62d21d..278f2eeeaab6 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -5,6 +5,7 @@
 
 #include "xe_pm.h"
 
+#include <linux/delay.h>
 #include <linux/fault-inject.h>
 #include <linux/pm_runtime.h>
 #include <linux/suspend.h>
@@ -23,6 +24,7 @@
 #include "xe_guc.h"
 #include "xe_irq.h"
 #include "xe_mmio.h"
+#include "xe_pcode_api.h"
 #include "xe_pcode.h"
 #include "xe_pxp.h"
 #include "xe_trace.h"
@@ -260,6 +262,107 @@ static bool xe_pm_vrsr_capable(struct xe_device *xe)
 	return val & VRAM_SR_SUPPORTED;
 }
 
+static int pci_acpi_aux_power_setup(struct xe_device *xe)
+{
+	struct xe_tile *root_tile = xe_device_get_root_tile(xe);
+	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
+	struct pci_dev *root_pdev;
+	int ret;
+	u32 uval;
+	u32 aux_pwr_limit;
+	u32 retry_interval;
+	u32 perst_delay;
+
+	root_pdev = pcie_find_root_port(pdev);
+	if (!root_pdev)
+		return -EINVAL;
+
+	ret = xe_pcode_read(root_tile, PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
+			    PCODE_D3_VRSR_SC_AUX_PL_AND_PERST_DELAY, 0),
+			    &uval, NULL);
+
+	if (ret)
+		return ret;
+
+	aux_pwr_limit = REG_FIELD_GET(POWER_D3_VRSR_AUX_PL_MASK, uval);
+	perst_delay = REG_FIELD_GET(POWER_D3_VRSR_PERST_MASK, uval);
+
+	drm_dbg(&xe->drm, "Aux Power limit = %d mW\n", aux_pwr_limit);
+	drm_dbg(&xe->drm, "PERST# Assertion delay = %d microseconds\n", perst_delay);
+
+retry:
+	ret = pci_acpi_request_d3cold_aux_power(root_pdev, aux_pwr_limit, &retry_interval);
+
+	if (ret == -EAGAIN) {
+		drm_warn(&xe->drm, "D3cold Aux Power request needs retry interval: %d seconds\n",
+			 retry_interval);
+		msleep(retry_interval * 1000);
+		goto retry;
+	}
+
+	if (ret)
+		return ret;
+
+	ret = pci_acpi_add_perst_assertion_delay(root_pdev, perst_delay);
+
+	return ret;
+}
+
+static void xe_pm_vrsr_init(struct xe_device *xe)
+{
+	int ret;
+
+	/* Check if platform support D3Cold VRSR */
+	if (!xe->info.has_vrsr)
+		return;
+
+	if (!xe_pm_vrsr_capable(xe))
+		return;
+
+	/*
+	 * If the VRSR initialization fails, the device will proceed with the regular
+	 * D3Cold flow
+	 */
+	ret = pci_acpi_aux_power_setup(xe);
+	if (ret) {
+		drm_info(&xe->drm, "VRSR capable: No\n");
+		return;
+	}
+
+	xe->d3cold.vrsr_capable = true;
+	drm_info(&xe->drm, "VRSR capable: Yes\n");
+}
+
+/**
+ * xe_pm_vrsr_enable - Enable VRAM self refresh
+ * @xe: The xe device.
+ * @enable: true: Enable, false: Disable
+ *
+ * This function enables the VRSR feature in D3Cold path.
+ *
+ * Return: It returns 0 on success and errno on failure.
+ */
+int xe_pm_vrsr_enable(struct xe_device *xe, bool enable)
+{
+	struct xe_tile *root_tile = xe_device_get_root_tile(xe);
+	int ret;
+	u32 uval = 0;
+
+	if (!xe->d3cold.vrsr_capable)
+		return -ENXIO;
+
+	drm_dbg(&xe->drm, "%s VRSR\n", enable ? "Enabling" : "Disabling");
+
+	if (enable)
+		ret = xe_pcode_write(root_tile, PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
+				     PCODE_D3_VRSR_SC_ENABLE, 0), uval);
+	else
+		ret = xe_pcode_write(root_tile, PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
+				     PCODE_D3_VRSR_SC_DISABLE, 0), uval);
+
+	return ret;
+}
+
 static void xe_pm_runtime_init(struct xe_device *xe)
 {
 	struct device *dev = xe->drm.dev;
@@ -374,7 +477,7 @@ int xe_pm_init(struct xe_device *xe)
 		err = xe_pm_set_vram_threshold(xe, vram_threshold);
 		if (err)
 			goto err_unregister;
-		xe->d3cold.vrsr_capable = xe_pm_vrsr_capable(xe);
+		xe_pm_vrsr_init(xe);
 	}
 
 	xe_pm_runtime_init(xe);
diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
index 59678b310e55..ba550281b130 100644
--- a/drivers/gpu/drm/xe/xe_pm.h
+++ b/drivers/gpu/drm/xe/xe_pm.h
@@ -35,4 +35,5 @@ bool xe_rpm_reclaim_safe(const struct xe_device *xe);
 struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
 int xe_pm_module_init(void);
 
+int xe_pm_vrsr_enable(struct xe_device *xe, bool enable);
 #endif
-- 
2.34.1

Re: [v4,06/11] drm/xe/vrsr: Initialize VRSR feature

[Poosa, Karthik]

On 29-05-2025 16:46, Badal Nilawar wrote:
> Add the API xe_pm_vrsr_enable to initialize the VRSR feature,
> requesting AUX power limit and PERST# assertion delay.
>
> V2: Add retry mechanism while requesting AUX power limit
>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_device_types.h |   1 +
>   drivers/gpu/drm/xe/xe_pcode_api.h    |   7 ++
>   drivers/gpu/drm/xe/xe_pm.c           | 105 ++++++++++++++++++++++++++-
>   drivers/gpu/drm/xe/xe_pm.h           |   1 +
>   4 files changed, 113 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 3a15b3a252fd..5f9a1a358468 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -7,6 +7,7 @@
>   #define _XE_DEVICE_TYPES_H_
>   
>   #include <linux/pci.h>
> +#include <linux/pci-acpi.h>
>   
>   #include <drm/drm_device.h>
>   #include <drm/drm_file.h>
> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
> index 127d4d26c4cf..f54ef03799d4 100644
> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
> @@ -43,6 +43,13 @@
>   #define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
>   #define	    POWER_SETUP_I1_DATA_MASK		REG_GENMASK(15, 0)
>   
> +#define	  PCODE_D3_VRAM_SELF_REFRESH	0x71
> +#define	    PCODE_D3_VRSR_SC_DISABLE	0x0
> +#define	    PCODE_D3_VRSR_SC_ENABLE	0x1
> +#define     PCODE_D3_VRSR_SC_AUX_PL_AND_PERST_DELAY	0x2
> +#define	    POWER_D3_VRSR_PERST_MASK	REG_GENMASK(31, 16)
> +#define	    POWER_D3_VRSR_AUX_PL_MASK	REG_GENMASK(15, 0)
> +
>   #define   PCODE_FREQUENCY_CONFIG		0x6e
>   /* Frequency Config Sub Commands (param1) */
>   #define     PCODE_MBOX_FC_SC_READ_FUSED_P0	0x0
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index c9395e62d21d..278f2eeeaab6 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -5,6 +5,7 @@
>   
>   #include "xe_pm.h"
>   
> +#include <linux/delay.h>
>   #include <linux/fault-inject.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/suspend.h>
> @@ -23,6 +24,7 @@
>   #include "xe_guc.h"
>   #include "xe_irq.h"
>   #include "xe_mmio.h"
> +#include "xe_pcode_api.h"
>   #include "xe_pcode.h"
>   #include "xe_pxp.h"
>   #include "xe_trace.h"
> @@ -260,6 +262,107 @@ static bool xe_pm_vrsr_capable(struct xe_device *xe)
>   	return val & VRAM_SR_SUPPORTED;
>   }
>   
> +static int pci_acpi_aux_power_setup(struct xe_device *xe)
> +{
> +	struct xe_tile *root_tile = xe_device_get_root_tile(xe);
> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> +	struct pci_dev *root_pdev;
> +	int ret;
> +	u32 uval;
> +	u32 aux_pwr_limit;
> +	u32 retry_interval;
> +	u32 perst_delay;
> +
> +	root_pdev = pcie_find_root_port(pdev);
> +	if (!root_pdev)
> +		return -EINVAL;
> +
> +	ret = xe_pcode_read(root_tile, PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
> +			    PCODE_D3_VRSR_SC_AUX_PL_AND_PERST_DELAY, 0),
> +			    &uval, NULL);
> +
> +	if (ret)
> +		return ret;
> +
> +	aux_pwr_limit = REG_FIELD_GET(POWER_D3_VRSR_AUX_PL_MASK, uval);
> +	perst_delay = REG_FIELD_GET(POWER_D3_VRSR_PERST_MASK, uval);
> +
> +	drm_dbg(&xe->drm, "Aux Power limit = %d mW\n", aux_pwr_limit);
> +	drm_dbg(&xe->drm, "PERST# Assertion delay = %d microseconds\n", perst_delay);
> +
> +retry:
> +	ret = pci_acpi_request_d3cold_aux_power(root_pdev, aux_pwr_limit, &retry_interval);
> +
> +	if (ret == -EAGAIN) {
> +		drm_warn(&xe->drm, "D3cold Aux Power request needs retry interval: %d seconds\n",
> +			 retry_interval);
> +		msleep(retry_interval * 1000);
> +		goto retry;
Instead of indefinite retry, can you retries with count, ~3 times, after 
which we can return failure.
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_acpi_add_perst_assertion_delay(root_pdev, perst_delay);
> +
> +	return ret;
> +}
> +
> +static void xe_pm_vrsr_init(struct xe_device *xe)
> +{
> +	int ret;
> +
> +	/* Check if platform support D3Cold VRSR */

This can be rephrased to, "Check if Xe should support VRSR for the platform."

> +	if (!xe->info.has_vrsr)

> +		return;
> +
Can you add a comment here also, viz "Check if GPU firmware supports VRSR"
> +	if (!xe_pm_vrsr_capable(xe))
> +		return;
> +
> +	/*
> +	 * If the VRSR initialization fails, the device will proceed with the regular
> +	 * D3Cold flow
> +	 */
> +	ret = pci_acpi_aux_power_setup(xe);
> +	if (ret) {
> +		drm_info(&xe->drm, "VRSR capable: No\n");
> +		return;
> +	}
> +
> +	xe->d3cold.vrsr_capable = true;
> +	drm_info(&xe->drm, "VRSR capable: Yes\n");
> +}
> +
> +/**
> + * xe_pm_vrsr_enable - Enable VRAM self refresh

as this function does both enable & disable VRSR, this can be renamed to 
xe_pm_set_vrsr(xe, flag)

> + * @xe: The xe device.
> + * @enable: true: Enable, false: Disable
> + *
> + * This function enables the VRSR feature in D3Cold path.
> + *
> + * Return: It returns 0 on success and errno on failure.
> + */
> +int xe_pm_vrsr_enable(struct xe_device *xe, bool enable)
> +{
> +	struct xe_tile *root_tile = xe_device_get_root_tile(xe);
> +	int ret;
> +	u32 uval = 0;
> +
> +	if (!xe->d3cold.vrsr_capable)
> +		return -ENXIO;
> +
> +	drm_dbg(&xe->drm, "%s VRSR\n", enable ? "Enabling" : "Disabling");
> +
> +	if (enable)
> +		ret = xe_pcode_write(root_tile, PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
> +				     PCODE_D3_VRSR_SC_ENABLE, 0), uval);
> +	else
> +		ret = xe_pcode_write(root_tile, PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
> +				     PCODE_D3_VRSR_SC_DISABLE, 0), uval);
> +
> +	return ret;
> +}
> +
>   static void xe_pm_runtime_init(struct xe_device *xe)
>   {
>   	struct device *dev = xe->drm.dev;
> @@ -374,7 +477,7 @@ int xe_pm_init(struct xe_device *xe)
>   		err = xe_pm_set_vram_threshold(xe, vram_threshold);
>   		if (err)
>   			goto err_unregister;
> -		xe->d3cold.vrsr_capable = xe_pm_vrsr_capable(xe);
> +		xe_pm_vrsr_init(xe);
>   	}
>   
>   	xe_pm_runtime_init(xe);
> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> index 59678b310e55..ba550281b130 100644
> --- a/drivers/gpu/drm/xe/xe_pm.h
> +++ b/drivers/gpu/drm/xe/xe_pm.h
> @@ -35,4 +35,5 @@ bool xe_rpm_reclaim_safe(const struct xe_device *xe);
>   struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
>   int xe_pm_module_init(void);
>   
> +int xe_pm_vrsr_enable(struct xe_device *xe, bool enable);
>   #endif

Re: [v4,06/11] drm/xe/vrsr: Initialize VRSR feature

[Nilawar, Badal]

On 24-06-2025 15:58, Poosa, Karthik wrote:
>
> On 29-05-2025 16:46, Badal Nilawar wrote:
>> Add the API xe_pm_vrsr_enable to initialize the VRSR feature,
>> requesting AUX power limit and PERST# assertion delay.
>>
>> V2: Add retry mechanism while requesting AUX power limit
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_device_types.h |   1 +
>>   drivers/gpu/drm/xe/xe_pcode_api.h    |   7 ++
>>   drivers/gpu/drm/xe/xe_pm.c           | 105 ++++++++++++++++++++++++++-
>>   drivers/gpu/drm/xe/xe_pm.h           |   1 +
>>   4 files changed, 113 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h 
>> b/drivers/gpu/drm/xe/xe_device_types.h
>> index 3a15b3a252fd..5f9a1a358468 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -7,6 +7,7 @@
>>   #define _XE_DEVICE_TYPES_H_
>>     #include <linux/pci.h>
>> +#include <linux/pci-acpi.h>
>>     #include <drm/drm_device.h>
>>   #include <drm/drm_file.h>
>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h 
>> b/drivers/gpu/drm/xe/xe_pcode_api.h
>> index 127d4d26c4cf..f54ef03799d4 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>> @@ -43,6 +43,13 @@
>>   #define        POWER_SETUP_I1_SHIFT        6    /* 10.6 fixed point 
>> format */
>>   #define        POWER_SETUP_I1_DATA_MASK        REG_GENMASK(15, 0)
>>   +#define      PCODE_D3_VRAM_SELF_REFRESH    0x71
>> +#define        PCODE_D3_VRSR_SC_DISABLE    0x0
>> +#define        PCODE_D3_VRSR_SC_ENABLE    0x1
>> +#define     PCODE_D3_VRSR_SC_AUX_PL_AND_PERST_DELAY    0x2
>> +#define        POWER_D3_VRSR_PERST_MASK    REG_GENMASK(31, 16)
>> +#define        POWER_D3_VRSR_AUX_PL_MASK    REG_GENMASK(15, 0)
>> +
>>   #define   PCODE_FREQUENCY_CONFIG        0x6e
>>   /* Frequency Config Sub Commands (param1) */
>>   #define     PCODE_MBOX_FC_SC_READ_FUSED_P0    0x0
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index c9395e62d21d..278f2eeeaab6 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -5,6 +5,7 @@
>>     #include "xe_pm.h"
>>   +#include <linux/delay.h>
>>   #include <linux/fault-inject.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/suspend.h>
>> @@ -23,6 +24,7 @@
>>   #include "xe_guc.h"
>>   #include "xe_irq.h"
>>   #include "xe_mmio.h"
>> +#include "xe_pcode_api.h"
>>   #include "xe_pcode.h"
>>   #include "xe_pxp.h"
>>   #include "xe_trace.h"
>> @@ -260,6 +262,107 @@ static bool xe_pm_vrsr_capable(struct xe_device 
>> *xe)
>>       return val & VRAM_SR_SUPPORTED;
>>   }
>>   +static int pci_acpi_aux_power_setup(struct xe_device *xe)
>> +{
>> +    struct xe_tile *root_tile = xe_device_get_root_tile(xe);
>> +    struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>> +    struct pci_dev *root_pdev;
>> +    int ret;
>> +    u32 uval;
>> +    u32 aux_pwr_limit;
>> +    u32 retry_interval;
>> +    u32 perst_delay;
>> +
>> +    root_pdev = pcie_find_root_port(pdev);
>> +    if (!root_pdev)
>> +        return -EINVAL;
>> +
>> +    ret = xe_pcode_read(root_tile, 
>> PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
>> +                PCODE_D3_VRSR_SC_AUX_PL_AND_PERST_DELAY, 0),
>> +                &uval, NULL);
>> +
>> +    if (ret)
>> +        return ret;
>> +
>> +    aux_pwr_limit = REG_FIELD_GET(POWER_D3_VRSR_AUX_PL_MASK, uval);
>> +    perst_delay = REG_FIELD_GET(POWER_D3_VRSR_PERST_MASK, uval);
>> +
>> +    drm_dbg(&xe->drm, "Aux Power limit = %d mW\n", aux_pwr_limit);
>> +    drm_dbg(&xe->drm, "PERST# Assertion delay = %d microseconds\n", 
>> perst_delay);
>> +
>> +retry:
>> +    ret = pci_acpi_request_d3cold_aux_power(root_pdev, 
>> aux_pwr_limit, &retry_interval);
>> +
>> +    if (ret == -EAGAIN) {
>> +        drm_warn(&xe->drm, "D3cold Aux Power request needs retry 
>> interval: %d seconds\n",
>> +             retry_interval);
>> +        msleep(retry_interval * 1000);
>> +        goto retry;
> Instead of indefinite retry, can you retries with count, ~3 times, 
> after which we can return failure.

Retry count is not needed because as per PCI firmware specifications, 
section 4.6.10 Rev 3.3. Firmware is not permitted to return a value
in this range more than once for each _DSM instance (located within the 
ACPI namespace of a single downstream port DeviceObject),
unless there is a subsequent invocation of this function before the 
previously returned retry interval has expired.

>> +    }
>> +
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = pci_acpi_add_perst_assertion_delay(root_pdev, perst_delay);
>> +
>> +    return ret;
>> +}
>> +
>> +static void xe_pm_vrsr_init(struct xe_device *xe)
>> +{
>> +    int ret;
>> +
>> +    /* Check if platform support D3Cold VRSR */
>
> This can be rephrased to, "Check if Xe should support VRSR for the 
> platform."

Here with has_vrsr we are checking if DGFX platform support VRSR or not. 
If supported, VRSR will be enabled in Xe kmd.
I think let's keep the comment as it is.

>
>> +    if (!xe->info.has_vrsr)
>
>> +        return;
>> +
> Can you add a comment here also, viz "Check if GPU firmware supports 
> VRSR" 
Sure, I will add.
>> +    if (!xe_pm_vrsr_capable(xe))
>> +        return;
>> +
>> +    /*
>> +     * If the VRSR initialization fails, the device will proceed 
>> with the regular
>> +     * D3Cold flow
>> +     */
>> +    ret = pci_acpi_aux_power_setup(xe);
>> +    if (ret) {
>> +        drm_info(&xe->drm, "VRSR capable: No\n");
>> +        return;
>> +    }
>> +
>> +    xe->d3cold.vrsr_capable = true;
>> +    drm_info(&xe->drm, "VRSR capable: Yes\n");
>> +}
>> +
>> +/**
>> + * xe_pm_vrsr_enable - Enable VRAM self refresh
>
> as this function does both enable & disable VRSR, this can be renamed 
> to xe_pm_set_vrsr(xe, flag)

IMO, _vrsr_enable() is a more suitable name than _set_vrsr. Since the 
subsequent patches don't involve disabling VRSR, I can remove the flag 
and retain only the code for enabling it.
Note that VRSR is internally disabled by pcode upon exiting D3Cold. 
Therefore, in subsequent patches, VRSR is re-enabled each time before 
entering D3Cold.

Thanks,
Badal

>> + * @xe: The xe device.
>> + * @enable: true: Enable, false: Disable
>> + *
>> + * This function enables the VRSR feature in D3Cold path.
>> + *
>> + * Return: It returns 0 on success and errno on failure.
>> + */
>> +int xe_pm_vrsr_enable(struct xe_device *xe, bool enable)
>> +{
>> +    struct xe_tile *root_tile = xe_device_get_root_tile(xe);
>> +    int ret;
>> +    u32 uval = 0;
>> +
>> +    if (!xe->d3cold.vrsr_capable)
>> +        return -ENXIO;
>> +
>> +    drm_dbg(&xe->drm, "%s VRSR\n", enable ? "Enabling" : "Disabling");
>> +
>> +    if (enable)
>> +        ret = xe_pcode_write(root_tile, 
>> PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
>> +                     PCODE_D3_VRSR_SC_ENABLE, 0), uval);
>> +    else
>> +        ret = xe_pcode_write(root_tile, 
>> PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
>> +                     PCODE_D3_VRSR_SC_DISABLE, 0), uval);
>> +
>> +    return ret;
>> +}
>> +
>>   static void xe_pm_runtime_init(struct xe_device *xe)
>>   {
>>       struct device *dev = xe->drm.dev;
>> @@ -374,7 +477,7 @@ int xe_pm_init(struct xe_device *xe)
>>           err = xe_pm_set_vram_threshold(xe, vram_threshold);
>>           if (err)
>>               goto err_unregister;
>> -        xe->d3cold.vrsr_capable = xe_pm_vrsr_capable(xe);
>> +        xe_pm_vrsr_init(xe);
>>       }
>>         xe_pm_runtime_init(xe);
>> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
>> index 59678b310e55..ba550281b130 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.h
>> +++ b/drivers/gpu/drm/xe/xe_pm.h
>> @@ -35,4 +35,5 @@ bool xe_rpm_reclaim_safe(const struct xe_device *xe);
>>   struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
>>   int xe_pm_module_init(void);
>>   +int xe_pm_vrsr_enable(struct xe_device *xe, bool enable);
>>   #endif

Re: [v4,06/11] drm/xe/vrsr: Initialize VRSR feature

[Poosa, Karthik]

On 03-09-2025 19:09, Nilawar, Badal wrote:
>
> On 24-06-2025 15:58, Poosa, Karthik wrote:
>>
>> On 29-05-2025 16:46, Badal Nilawar wrote:
>>> Add the API xe_pm_vrsr_enable to initialize the VRSR feature,
>>> requesting AUX power limit and PERST# assertion delay.
>>>
>>> V2: Add retry mechanism while requesting AUX power limit
>>>
>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_device_types.h |   1 +
>>>   drivers/gpu/drm/xe/xe_pcode_api.h    |   7 ++
>>>   drivers/gpu/drm/xe/xe_pm.c           | 105 
>>> ++++++++++++++++++++++++++-
>>>   drivers/gpu/drm/xe/xe_pm.h           |   1 +
>>>   4 files changed, 113 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h 
>>> b/drivers/gpu/drm/xe/xe_device_types.h
>>> index 3a15b3a252fd..5f9a1a358468 100644
>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>> @@ -7,6 +7,7 @@
>>>   #define _XE_DEVICE_TYPES_H_
>>>     #include <linux/pci.h>
>>> +#include <linux/pci-acpi.h>
>>>     #include <drm/drm_device.h>
>>>   #include <drm/drm_file.h>
>>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h 
>>> b/drivers/gpu/drm/xe/xe_pcode_api.h
>>> index 127d4d26c4cf..f54ef03799d4 100644
>>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>>> @@ -43,6 +43,13 @@
>>>   #define        POWER_SETUP_I1_SHIFT        6    /* 10.6 fixed 
>>> point format */
>>>   #define        POWER_SETUP_I1_DATA_MASK REG_GENMASK(15, 0)
>>>   +#define      PCODE_D3_VRAM_SELF_REFRESH    0x71
>>> +#define        PCODE_D3_VRSR_SC_DISABLE    0x0
>>> +#define        PCODE_D3_VRSR_SC_ENABLE    0x1
>>> +#define     PCODE_D3_VRSR_SC_AUX_PL_AND_PERST_DELAY    0x2
>>> +#define        POWER_D3_VRSR_PERST_MASK    REG_GENMASK(31, 16)
>>> +#define        POWER_D3_VRSR_AUX_PL_MASK    REG_GENMASK(15, 0)
>>> +
>>>   #define   PCODE_FREQUENCY_CONFIG        0x6e
>>>   /* Frequency Config Sub Commands (param1) */
>>>   #define     PCODE_MBOX_FC_SC_READ_FUSED_P0    0x0
>>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>>> index c9395e62d21d..278f2eeeaab6 100644
>>> --- a/drivers/gpu/drm/xe/xe_pm.c
>>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>>> @@ -5,6 +5,7 @@
>>>     #include "xe_pm.h"
>>>   +#include <linux/delay.h>
>>>   #include <linux/fault-inject.h>
>>>   #include <linux/pm_runtime.h>
>>>   #include <linux/suspend.h>
>>> @@ -23,6 +24,7 @@
>>>   #include "xe_guc.h"
>>>   #include "xe_irq.h"
>>>   #include "xe_mmio.h"
>>> +#include "xe_pcode_api.h"
>>>   #include "xe_pcode.h"
>>>   #include "xe_pxp.h"
>>>   #include "xe_trace.h"
>>> @@ -260,6 +262,107 @@ static bool xe_pm_vrsr_capable(struct 
>>> xe_device *xe)
>>>       return val & VRAM_SR_SUPPORTED;
>>>   }
>>>   +static int pci_acpi_aux_power_setup(struct xe_device *xe)
>>> +{
>>> +    struct xe_tile *root_tile = xe_device_get_root_tile(xe);
>>> +    struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>>> +    struct pci_dev *root_pdev;
>>> +    int ret;
>>> +    u32 uval;
>>> +    u32 aux_pwr_limit;
>>> +    u32 retry_interval;
>>> +    u32 perst_delay;
>>> +
>>> +    root_pdev = pcie_find_root_port(pdev);
>>> +    if (!root_pdev)
>>> +        return -EINVAL;
>>> +
>>> +    ret = xe_pcode_read(root_tile, 
>>> PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
>>> +                PCODE_D3_VRSR_SC_AUX_PL_AND_PERST_DELAY, 0),
>>> +                &uval, NULL);
>>> +
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    aux_pwr_limit = REG_FIELD_GET(POWER_D3_VRSR_AUX_PL_MASK, uval);
>>> +    perst_delay = REG_FIELD_GET(POWER_D3_VRSR_PERST_MASK, uval);
>>> +
>>> +    drm_dbg(&xe->drm, "Aux Power limit = %d mW\n", aux_pwr_limit);
>>> +    drm_dbg(&xe->drm, "PERST# Assertion delay = %d microseconds\n", 
>>> perst_delay);
>>> +
>>> +retry:
>>> +    ret = pci_acpi_request_d3cold_aux_power(root_pdev, 
>>> aux_pwr_limit, &retry_interval);
>>> +
>>> +    if (ret == -EAGAIN) {
>>> +        drm_warn(&xe->drm, "D3cold Aux Power request needs retry 
>>> interval: %d seconds\n",
>>> +             retry_interval);
>>> +        msleep(retry_interval * 1000);
>>> +        goto retry;
>> Instead of indefinite retry, can you retries with count, ~3 times, 
>> after which we can return failure.
>
> Retry count is not needed because as per PCI firmware specifications, 
> section 4.6.10 Rev 3.3. Firmware is not permitted to return a value
> in this range more than once for each _DSM instance (located within 
> the ACPI namespace of a single downstream port DeviceObject),
> unless there is a subsequent invocation of this function before the 
> previously returned retry interval has expired.

okay, so -EGAIN shall be returned only once from the firmware, right ?

>
>>> +    }
>>> +
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = pci_acpi_add_perst_assertion_delay(root_pdev, perst_delay);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static void xe_pm_vrsr_init(struct xe_device *xe)
>>> +{
>>> +    int ret;
>>> +
>>> +    /* Check if platform support D3Cold VRSR */
>>
>> This can be rephrased to, "Check if Xe should support VRSR for the 
>> platform."
>
> Here with has_vrsr we are checking if DGFX platform support VRSR or 
> not. If supported, VRSR will be enabled in Xe kmd.
> I think let's keep the comment as it is.

"platform support" here is confusing, as there is another check below 
in xe_pm_vrsr_capable for actual platform support.

I think we may not need need has_vrsr at all !

xe_pm_vrsr_capable should suffice.

>
>>
>>> +    if (!xe->info.has_vrsr)
>>
>>> +        return;
>>> +
>> Can you add a comment here also, viz "Check if GPU firmware supports 
>> VRSR" 
> Sure, I will add.
>>> +    if (!xe_pm_vrsr_capable(xe))
>>> +        return;
>>> +
>>> +    /*
>>> +     * If the VRSR initialization fails, the device will proceed 
>>> with the regular
>>> +     * D3Cold flow
>>> +     */
>>> +    ret = pci_acpi_aux_power_setup(xe);
>>> +    if (ret) {
>>> +        drm_info(&xe->drm, "VRSR capable: No\n");
>>> +        return;
>>> +    }
>>> +
>>> +    xe->d3cold.vrsr_capable = true;
>>> +    drm_info(&xe->drm, "VRSR capable: Yes\n");
>>> +}
>>> +
>>> +/**
>>> + * xe_pm_vrsr_enable - Enable VRAM self refresh
>>
>> as this function does both enable & disable VRSR, this can be renamed 
>> to xe_pm_set_vrsr(xe, flag)
>
> IMO, _vrsr_enable() is a more suitable name than _set_vrsr. Since the 
> subsequent patches don't involve disabling VRSR, I can remove the flag 
> and retain only the code for enabling it.
> Note that VRSR is internally disabled by pcode upon exiting D3Cold. 
> Therefore, in subsequent patches, VRSR is re-enabled each time before 
> entering D3Cold.
>
> Thanks,
> Badal

No need to change the implementation, let the enable flag be there for 
future usage, just update description to

This function enables/disables the VRSR feature on the GPU.

>
>>> + * @xe: The xe device.
>>> + * @enable: true: Enable, false: Disable
>>> + *
>>> + * This function enables the VRSR feature in D3Cold path.
>>> + *
>>> + * Return: It returns 0 on success and errno on failure.
>>> + */
>>> +int xe_pm_vrsr_enable(struct xe_device *xe, bool enable)
>>> +{
>>> +    struct xe_tile *root_tile = xe_device_get_root_tile(xe);
>>> +    int ret;
>>> +    u32 uval = 0;
>>> +
>>> +    if (!xe->d3cold.vrsr_capable)
>>> +        return -ENXIO;
>>> +
>>> +    drm_dbg(&xe->drm, "%s VRSR\n", enable ? "Enabling" : "Disabling");
>>> +
>>> +    if (enable)
>>> +        ret = xe_pcode_write(root_tile, 
>>> PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
>>> +                     PCODE_D3_VRSR_SC_ENABLE, 0), uval);
>>> +    else
>>> +        ret = xe_pcode_write(root_tile, 
>>> PCODE_MBOX(PCODE_D3_VRAM_SELF_REFRESH,
>>> +                     PCODE_D3_VRSR_SC_DISABLE, 0), uval);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   static void xe_pm_runtime_init(struct xe_device *xe)
>>>   {
>>>       struct device *dev = xe->drm.dev;
>>> @@ -374,7 +477,7 @@ int xe_pm_init(struct xe_device *xe)
>>>           err = xe_pm_set_vram_threshold(xe, vram_threshold);
>>>           if (err)
>>>               goto err_unregister;
>>> -        xe->d3cold.vrsr_capable = xe_pm_vrsr_capable(xe);
>>> +        xe_pm_vrsr_init(xe);
>>>       }
>>>         xe_pm_runtime_init(xe);
>>> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
>>> index 59678b310e55..ba550281b130 100644
>>> --- a/drivers/gpu/drm/xe/xe_pm.h
>>> +++ b/drivers/gpu/drm/xe/xe_pm.h
>>> @@ -35,4 +35,5 @@ bool xe_rpm_reclaim_safe(const struct xe_device *xe);
>>>   struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
>>>   int xe_pm_module_init(void);
>>>   +int xe_pm_vrsr_enable(struct xe_device *xe, bool enable);
>>>   #endif

[PATCH v4 07/11] drm/xe/vrsr: Enable VRSR on default VGA boot device

[Badal Nilawar]

The VRSR feature is to enhance the display screen refresh experience
when the device exits from the D3Cold state. Therefore, apply the VRSR
feature to the default VGA boot device and when a display is connected.

Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/xe/display/xe_display.c | 22 ++++++++++++++++++++++
 drivers/gpu/drm/xe/display/xe_display.h |  2 ++
 drivers/gpu/drm/xe/xe_pm.c              |  5 +++++
 3 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index 20c3bcd953b7..b3da88b12b35 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -88,6 +88,28 @@ static void display_destroy(struct drm_device *dev, void *dummy)
 	destroy_workqueue(xe->display.hotplug.dp_wq);
 }
 
+bool xe_display_connected(struct xe_device *xe)
+{
+	struct drm_connector *list_connector;
+	struct drm_connector_list_iter iter;
+	bool ret = false;
+
+	mutex_lock(&xe->drm.mode_config.mutex);
+	drm_connector_list_iter_begin(&xe->drm, &iter);
+
+	drm_for_each_connector_iter(list_connector, &iter) {
+		if (list_connector->status == connector_status_connected) {
+			ret = true;
+			break;
+		}
+	}
+
+	drm_connector_list_iter_end(&iter);
+	mutex_unlock(&xe->drm.mode_config.mutex);
+
+	return ret;
+}
+
 /**
  * xe_display_create - create display struct
  * @xe: XE device instance
diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
index 46e14f8dee28..c79441bccb43 100644
--- a/drivers/gpu/drm/xe/display/xe_display.h
+++ b/drivers/gpu/drm/xe/display/xe_display.h
@@ -39,6 +39,7 @@ void xe_display_pm_resume(struct xe_device *xe);
 void xe_display_pm_runtime_suspend(struct xe_device *xe);
 void xe_display_pm_runtime_suspend_late(struct xe_device *xe);
 void xe_display_pm_runtime_resume(struct xe_device *xe);
+bool xe_display_connected(struct xe_device *xe);
 
 #else
 
@@ -71,5 +72,6 @@ static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
 static inline void xe_display_pm_runtime_suspend_late(struct xe_device *xe) {}
 static inline void xe_display_pm_runtime_resume(struct xe_device *xe) {}
 
+static inline bool xe_display_connected(struct xe_device *xe) { return false; }
 #endif /* CONFIG_DRM_XE_DISPLAY */
 #endif /* _XE_DISPLAY_H_ */
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index 278f2eeeaab6..c84b9b3f7371 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -9,6 +9,7 @@
 #include <linux/fault-inject.h>
 #include <linux/pm_runtime.h>
 #include <linux/suspend.h>
+#include <linux/vgaarb.h>
 
 #include <drm/drm_managed.h>
 #include <drm/ttm/ttm_placement.h>
@@ -310,6 +311,7 @@ static int pci_acpi_aux_power_setup(struct xe_device *xe)
 
 static void xe_pm_vrsr_init(struct xe_device *xe)
 {
+	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
 	int ret;
 
 	/* Check if platform support D3Cold VRSR */
@@ -319,6 +321,9 @@ static void xe_pm_vrsr_init(struct xe_device *xe)
 	if (!xe_pm_vrsr_capable(xe))
 		return;
 
+	if (pdev != vga_default_device() || !xe_display_connected(xe))
+		return;
+
 	/*
 	 * If the VRSR initialization fails, the device will proceed with the regular
 	 * D3Cold flow
-- 
2.34.1

Re: [PATCH v4 07/11] drm/xe/vrsr: Enable VRSR on default VGA boot device

[Jani Nikula]

On Thu, 29 May 2025, Badal Nilawar <badal.nilawar@intel.com> wrote:
> The VRSR feature is to enhance the display screen refresh experience
> when the device exits from the D3Cold state. Therefore, apply the VRSR
> feature to the default VGA boot device and when a display is connected.
>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/xe/display/xe_display.c | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/xe/display/xe_display.h |  2 ++
>  drivers/gpu/drm/xe/xe_pm.c              |  5 +++++
>  3 files changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index 20c3bcd953b7..b3da88b12b35 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -88,6 +88,28 @@ static void display_destroy(struct drm_device *dev, void *dummy)
>  	destroy_workqueue(xe->display.hotplug.dp_wq);
>  }
>  
> +bool xe_display_connected(struct xe_device *xe)
> +{
> +	struct drm_connector *list_connector;
> +	struct drm_connector_list_iter iter;
> +	bool ret = false;
> +
> +	mutex_lock(&xe->drm.mode_config.mutex);
> +	drm_connector_list_iter_begin(&xe->drm, &iter);
> +
> +	drm_for_each_connector_iter(list_connector, &iter) {
> +		if (list_connector->status == connector_status_connected) {
> +			ret = true;
> +			break;
> +		}
> +	}
> +
> +	drm_connector_list_iter_end(&iter);
> +	mutex_unlock(&xe->drm.mode_config.mutex);
> +
> +	return ret;
> +}
> +

Xe display/ should contain the *minimal* glue to attach i915 display to
it. It should *not* contain generic display stuff like this. At all.

The goal is for i915 display to become a dedicated kernel module. The
above should not be part of xe.ko.

BR,
Jani.

>  /**
>   * xe_display_create - create display struct
>   * @xe: XE device instance
> diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
> index 46e14f8dee28..c79441bccb43 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.h
> +++ b/drivers/gpu/drm/xe/display/xe_display.h
> @@ -39,6 +39,7 @@ void xe_display_pm_resume(struct xe_device *xe);
>  void xe_display_pm_runtime_suspend(struct xe_device *xe);
>  void xe_display_pm_runtime_suspend_late(struct xe_device *xe);
>  void xe_display_pm_runtime_resume(struct xe_device *xe);
> +bool xe_display_connected(struct xe_device *xe);
>  
>  #else
>  
> @@ -71,5 +72,6 @@ static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
>  static inline void xe_display_pm_runtime_suspend_late(struct xe_device *xe) {}
>  static inline void xe_display_pm_runtime_resume(struct xe_device *xe) {}
>  
> +static inline bool xe_display_connected(struct xe_device *xe) { return false; }
>  #endif /* CONFIG_DRM_XE_DISPLAY */
>  #endif /* _XE_DISPLAY_H_ */
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index 278f2eeeaab6..c84b9b3f7371 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -9,6 +9,7 @@
>  #include <linux/fault-inject.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/suspend.h>
> +#include <linux/vgaarb.h>
>  
>  #include <drm/drm_managed.h>
>  #include <drm/ttm/ttm_placement.h>
> @@ -310,6 +311,7 @@ static int pci_acpi_aux_power_setup(struct xe_device *xe)
>  
>  static void xe_pm_vrsr_init(struct xe_device *xe)
>  {
> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>  	int ret;
>  
>  	/* Check if platform support D3Cold VRSR */
> @@ -319,6 +321,9 @@ static void xe_pm_vrsr_init(struct xe_device *xe)
>  	if (!xe_pm_vrsr_capable(xe))
>  		return;
>  
> +	if (pdev != vga_default_device() || !xe_display_connected(xe))
> +		return;
> +
>  	/*
>  	 * If the VRSR initialization fails, the device will proceed with the regular
>  	 * D3Cold flow

-- 
Jani Nikula, Intel

Re: [PATCH v4 07/11] drm/xe/vrsr: Enable VRSR on default VGA boot device

[Nilawar, Badal]

On 06-06-2025 18:37, Jani Nikula wrote:
> On Thu, 29 May 2025, Badal Nilawar <badal.nilawar@intel.com> wrote:
>> The VRSR feature is to enhance the display screen refresh experience
>> when the device exits from the D3Cold state. Therefore, apply the VRSR
>> feature to the default VGA boot device and when a display is connected.
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>>   drivers/gpu/drm/xe/display/xe_display.c | 22 ++++++++++++++++++++++
>>   drivers/gpu/drm/xe/display/xe_display.h |  2 ++
>>   drivers/gpu/drm/xe/xe_pm.c              |  5 +++++
>>   3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
>> index 20c3bcd953b7..b3da88b12b35 100644
>> --- a/drivers/gpu/drm/xe/display/xe_display.c
>> +++ b/drivers/gpu/drm/xe/display/xe_display.c
>> @@ -88,6 +88,28 @@ static void display_destroy(struct drm_device *dev, void *dummy)
>>   	destroy_workqueue(xe->display.hotplug.dp_wq);
>>   }
>>   
>> +bool xe_display_connected(struct xe_device *xe)
>> +{
>> +	struct drm_connector *list_connector;
>> +	struct drm_connector_list_iter iter;
>> +	bool ret = false;
>> +
>> +	mutex_lock(&xe->drm.mode_config.mutex);
>> +	drm_connector_list_iter_begin(&xe->drm, &iter);
>> +
>> +	drm_for_each_connector_iter(list_connector, &iter) {
>> +		if (list_connector->status == connector_status_connected) {
>> +			ret = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	drm_connector_list_iter_end(&iter);
>> +	mutex_unlock(&xe->drm.mode_config.mutex);
>> +
>> +	return ret;
>> +}
>> +
> Xe display/ should contain the *minimal* glue to attach i915 display to
> it. It should *not* contain generic display stuff like this. At all.
>
> The goal is for i915 display to become a dedicated kernel module. The
> above should not be part of xe.ko.

I will move this to i915/display/intel_display.c.

Thanks,
Badal

>
> BR,
> Jani.
>
>>   /**
>>    * xe_display_create - create display struct
>>    * @xe: XE device instance
>> diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
>> index 46e14f8dee28..c79441bccb43 100644
>> --- a/drivers/gpu/drm/xe/display/xe_display.h
>> +++ b/drivers/gpu/drm/xe/display/xe_display.h
>> @@ -39,6 +39,7 @@ void xe_display_pm_resume(struct xe_device *xe);
>>   void xe_display_pm_runtime_suspend(struct xe_device *xe);
>>   void xe_display_pm_runtime_suspend_late(struct xe_device *xe);
>>   void xe_display_pm_runtime_resume(struct xe_device *xe);
>> +bool xe_display_connected(struct xe_device *xe);
>>   
>>   #else
>>   
>> @@ -71,5 +72,6 @@ static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
>>   static inline void xe_display_pm_runtime_suspend_late(struct xe_device *xe) {}
>>   static inline void xe_display_pm_runtime_resume(struct xe_device *xe) {}
>>   
>> +static inline bool xe_display_connected(struct xe_device *xe) { return false; }
>>   #endif /* CONFIG_DRM_XE_DISPLAY */
>>   #endif /* _XE_DISPLAY_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index 278f2eeeaab6..c84b9b3f7371 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/fault-inject.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/suspend.h>
>> +#include <linux/vgaarb.h>
>>   
>>   #include <drm/drm_managed.h>
>>   #include <drm/ttm/ttm_placement.h>
>> @@ -310,6 +311,7 @@ static int pci_acpi_aux_power_setup(struct xe_device *xe)
>>   
>>   static void xe_pm_vrsr_init(struct xe_device *xe)
>>   {
>> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>>   	int ret;
>>   
>>   	/* Check if platform support D3Cold VRSR */
>> @@ -319,6 +321,9 @@ static void xe_pm_vrsr_init(struct xe_device *xe)
>>   	if (!xe_pm_vrsr_capable(xe))
>>   		return;
>>   
>> +	if (pdev != vga_default_device() || !xe_display_connected(xe))
>> +		return;
>> +
>>   	/*
>>   	 * If the VRSR initialization fails, the device will proceed with the regular
>>   	 * D3Cold flow

[PATCH v4 08/11] drm/xe/vrsr: Refactor d3cold.allowed to a enum

[Badal Nilawar]

From: Anshuman Gupta <anshuman.gupta@intel.com>

Add xe_d3_state enum to add support for VRAM Self Refresh
d3cold state.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/xe/display/xe_display.c |  6 +++---
 drivers/gpu/drm/xe/xe_device_types.h    |  5 +++--
 drivers/gpu/drm/xe/xe_pci.c             |  6 +++---
 drivers/gpu/drm/xe/xe_pm.c              | 16 ++++++++--------
 drivers/gpu/drm/xe/xe_pm.h              |  8 +++++++-
 5 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index b3da88b12b35..7ccd9b447ace 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -433,7 +433,7 @@ void xe_display_pm_runtime_suspend(struct xe_device *xe)
 	if (!xe->info.probe_display)
 		return;
 
-	if (xe->d3cold.allowed) {
+	if (xe->d3cold.target_state) {
 		xe_display_enable_d3cold(xe);
 		return;
 	}
@@ -459,7 +459,7 @@ void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
 	if (!xe->info.probe_display)
 		return;
 
-	if (xe->d3cold.allowed)
+	if (xe->d3cold.target_state)
 		xe_display_pm_suspend_late(xe);
 
 	/*
@@ -537,7 +537,7 @@ void xe_display_pm_runtime_resume(struct xe_device *xe)
 	if (!xe->info.probe_display)
 		return;
 
-	if (xe->d3cold.allowed) {
+	if (xe->d3cold.target_state) {
 		xe_display_disable_d3cold(xe);
 		return;
 	}
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 5f9a1a358468..48ca0d8c4c45 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -20,6 +20,7 @@
 #include "xe_memirq_types.h"
 #include "xe_oa_types.h"
 #include "xe_platform_types.h"
+#include "xe_pm.h"
 #include "xe_pmu_types.h"
 #include "xe_pt_types.h"
 #include "xe_sriov_types.h"
@@ -510,8 +511,8 @@ struct xe_device {
 		/** @d3cold.capable: Indicates if root port is d3cold capable */
 		bool capable;
 
-		/** @d3cold.allowed: Indicates if d3cold is a valid device state */
-		bool allowed;
+		/** @d3cold.target_state: Indicates d3cold target state */
+		enum xe_d3_state target_state;
 
 		/** @d3cold.vrsr_capable: Indicates if d3cold VRAM Self Refresh is supported */
 		bool vrsr_capable;
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 46a99d6ef1a5..5ae1df345416 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -977,7 +977,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
 
 	pci_save_state(pdev);
 
-	if (xe->d3cold.allowed) {
+	if (xe->d3cold.target_state) {
 		d3cold_toggle(pdev, D3COLD_ENABLE);
 		pci_disable_device(pdev);
 		pci_ignore_hotplug(pdev);
@@ -1002,7 +1002,7 @@ static int xe_pci_runtime_resume(struct device *dev)
 
 	pci_restore_state(pdev);
 
-	if (xe->d3cold.allowed) {
+	if (xe->d3cold.target_state) {
 		err = pci_enable_device(pdev);
 		if (err)
 			return err;
@@ -1018,7 +1018,7 @@ static int xe_pci_runtime_idle(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct xe_device *xe = pdev_to_xe_device(pdev);
 
-	xe_pm_d3cold_allowed_toggle(xe);
+	xe_pm_d3cold_target_state_toggle(xe);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index c84b9b3f7371..b86e95493cb5 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -606,7 +606,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
 
 	xe_display_pm_runtime_suspend(xe);
 
-	if (xe->d3cold.allowed) {
+	if (xe->d3cold.target_state) {
 		err = xe_bo_evict_all(xe);
 		if (err)
 			goto out_resume;
@@ -653,7 +653,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
 
 	xe_rpm_lockmap_acquire(xe);
 
-	if (xe->d3cold.allowed) {
+	if (xe->d3cold.target_state) {
 		err = xe_pcode_ready(xe, true);
 		if (err)
 			goto out;
@@ -676,7 +676,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
 
 	xe_display_pm_runtime_resume(xe);
 
-	if (xe->d3cold.allowed) {
+	if (xe->d3cold.target_state) {
 		err = xe_bo_restore_late(xe);
 		if (err)
 			goto out;
@@ -918,13 +918,13 @@ int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold)
 }
 
 /**
- * xe_pm_d3cold_allowed_toggle - Check conditions to toggle d3cold.allowed
+ * xe_pm_d3cold_target_state_toggle - Check conditions to toggle target_state
  * @xe: xe device instance
  *
  * To be called during runtime_pm idle callback.
  * Check for all the D3Cold conditions ahead of runtime suspend.
  */
-void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
+void xe_pm_d3cold_target_state_toggle(struct xe_device *xe)
 {
 	struct ttm_resource_manager *man;
 	u32 total_vram_used_mb = 0;
@@ -932,7 +932,7 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
 	int i;
 
 	if (!xe->d3cold.capable) {
-		xe->d3cold.allowed = false;
+		xe->d3cold.target_state = XE_D3HOT;
 		return;
 	}
 
@@ -947,9 +947,9 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
 	mutex_lock(&xe->d3cold.lock);
 
 	if (total_vram_used_mb < xe->d3cold.vram_threshold)
-		xe->d3cold.allowed = true;
+		xe->d3cold.target_state = XE_D3COLD_OFF;
 	else
-		xe->d3cold.allowed = false;
+		xe->d3cold.target_state = XE_D3HOT;
 
 	mutex_unlock(&xe->d3cold.lock);
 }
diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
index ba550281b130..c6c2bd6187a5 100644
--- a/drivers/gpu/drm/xe/xe_pm.h
+++ b/drivers/gpu/drm/xe/xe_pm.h
@@ -12,6 +12,12 @@
 
 struct xe_device;
 
+enum xe_d3_state {
+	XE_D3HOT = 0,
+	XE_D3COLD_VRSR,
+	XE_D3COLD_OFF,
+};
+
 int xe_pm_suspend(struct xe_device *xe);
 int xe_pm_resume(struct xe_device *xe);
 
@@ -30,7 +36,7 @@ void xe_pm_runtime_get_noresume(struct xe_device *xe);
 bool xe_pm_runtime_resume_and_get(struct xe_device *xe);
 void xe_pm_assert_unbounded_bridge(struct xe_device *xe);
 int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold);
-void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);
+void xe_pm_d3cold_target_state_toggle(struct xe_device *xe);
 bool xe_rpm_reclaim_safe(const struct xe_device *xe);
 struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
 int xe_pm_module_init(void);
-- 
2.34.1

Re: [v4,08/11] drm/xe/vrsr: Refactor d3cold.allowed to a enum

[Poosa, Karthik]

[-- Attachment #1: Type: text/plain, Size: 6698 bytes --]

On 29-05-2025 16:46, Badal Nilawar wrote:
> From: Anshuman Gupta<anshuman.gupta@intel.com>
>
> Add xe_d3_state enum to add support for VRAM Self Refresh
> d3cold state.
>
> Signed-off-by: Anshuman Gupta<anshuman.gupta@intel.com>
> ---
>   drivers/gpu/drm/xe/display/xe_display.c |  6 +++---
>   drivers/gpu/drm/xe/xe_device_types.h    |  5 +++--
>   drivers/gpu/drm/xe/xe_pci.c             |  6 +++---
>   drivers/gpu/drm/xe/xe_pm.c              | 16 ++++++++--------
>   drivers/gpu/drm/xe/xe_pm.h              |  8 +++++++-
>   5 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index b3da88b12b35..7ccd9b447ace 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -433,7 +433,7 @@ void xe_display_pm_runtime_suspend(struct xe_device *xe)
>   	if (!xe->info.probe_display)
>   		return;
>   
> -	if (xe->d3cold.allowed) {
> +	if (xe->d3cold.target_state) {
>   		xe_display_enable_d3cold(xe);
>   		return;
>   	}
> @@ -459,7 +459,7 @@ void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
>   	if (!xe->info.probe_display)
>   		return;
>   
> -	if (xe->d3cold.allowed)
> +	if (xe->d3cold.target_state)
>   		xe_display_pm_suspend_late(xe);
>   
>   	/*
> @@ -537,7 +537,7 @@ void xe_display_pm_runtime_resume(struct xe_device *xe)
>   	if (!xe->info.probe_display)
>   		return;
>   
> -	if (xe->d3cold.allowed) {
> +	if (xe->d3cold.target_state) {
>   		xe_display_disable_d3cold(xe);
>   		return;
>   	}
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 5f9a1a358468..48ca0d8c4c45 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -20,6 +20,7 @@
>   #include "xe_memirq_types.h"
>   #include "xe_oa_types.h"
>   #include "xe_platform_types.h"
> +#include "xe_pm.h"
>   #include "xe_pmu_types.h"
>   #include "xe_pt_types.h"
>   #include "xe_sriov_types.h"
> @@ -510,8 +511,8 @@ struct xe_device {
>   		/** @d3cold.capable: Indicates if root port is d3cold capable */
>   		bool capable;
>   
> -		/** @d3cold.allowed: Indicates if d3cold is a valid device state */
> -		bool allowed;
> +		/** @d3cold.target_state: Indicates d3cold target state */
> +		enum xe_d3_state target_state;
>   
>   		/** @d3cold.vrsr_capable: Indicates if d3cold VRAM Self Refresh is supported */
>   		bool vrsr_capable;
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 46a99d6ef1a5..5ae1df345416 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -977,7 +977,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
>   
>   	pci_save_state(pdev);
>   
> -	if (xe->d3cold.allowed) {
> +	if (xe->d3cold.target_state) {
>   		d3cold_toggle(pdev, D3COLD_ENABLE);
>   		pci_disable_device(pdev);
>   		pci_ignore_hotplug(pdev);
> @@ -1002,7 +1002,7 @@ static int xe_pci_runtime_resume(struct device *dev)
>   
>   	pci_restore_state(pdev);
>   
> -	if (xe->d3cold.allowed) {
> +	if (xe->d3cold.target_state) {
>   		err = pci_enable_device(pdev);
>   		if (err)
>   			return err;
> @@ -1018,7 +1018,7 @@ static int xe_pci_runtime_idle(struct device *dev)
>   	struct pci_dev *pdev = to_pci_dev(dev);
>   	struct xe_device *xe = pdev_to_xe_device(pdev);
>   
> -	xe_pm_d3cold_allowed_toggle(xe);
> +	xe_pm_d3cold_target_state_toggle(xe);
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index c84b9b3f7371..b86e95493cb5 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -606,7 +606,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>   
>   	xe_display_pm_runtime_suspend(xe);
>   
> -	if (xe->d3cold.allowed) {
> +	if (xe->d3cold.target_state) {
>   		err = xe_bo_evict_all(xe);
>   		if (err)
>   			goto out_resume;
> @@ -653,7 +653,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>   
>   	xe_rpm_lockmap_acquire(xe);
>   
> -	if (xe->d3cold.allowed) {
> +	if (xe->d3cold.target_state) {
>   		err = xe_pcode_ready(xe, true);
>   		if (err)
>   			goto out;
> @@ -676,7 +676,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>   
>   	xe_display_pm_runtime_resume(xe);
>   
> -	if (xe->d3cold.allowed) {
> +	if (xe->d3cold.target_state) {
>   		err = xe_bo_restore_late(xe);
>   		if (err)
>   			goto out;
> @@ -918,13 +918,13 @@ int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold)
>   }
>   
>   /**
> - * xe_pm_d3cold_allowed_toggle - Check conditions to toggle d3cold.allowed
> + * xe_pm_d3cold_target_state_toggle - Check conditions to toggle target_state
>    * @xe: xe device instance
>    *
>    * To be called during runtime_pm idle callback.
>    * Check for all the D3Cold conditions ahead of runtime suspend.
>    */
> -void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
> +void xe_pm_d3cold_target_state_toggle(struct xe_device *xe)
>   {
>   	struct ttm_resource_manager *man;
>   	u32 total_vram_used_mb = 0;
> @@ -932,7 +932,7 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
>   	int i;
>   
>   	if (!xe->d3cold.capable) {
> -		xe->d3cold.allowed = false;
> +		xe->d3cold.target_state = XE_D3HOT;
>   		return;
>   	}
>   
> @@ -947,9 +947,9 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
>   	mutex_lock(&xe->d3cold.lock);
>   
>   	if (total_vram_used_mb < xe->d3cold.vram_threshold)
> -		xe->d3cold.allowed = true;
> +		xe->d3cold.target_state = XE_D3COLD_OFF;
>   	else
> -		xe->d3cold.allowed = false;
> +		xe->d3cold.target_state = XE_D3HOT;
>   
>   	mutex_unlock(&xe->d3cold.lock);
>   }
> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> index ba550281b130..c6c2bd6187a5 100644
> --- a/drivers/gpu/drm/xe/xe_pm.h
> +++ b/drivers/gpu/drm/xe/xe_pm.h
> @@ -12,6 +12,12 @@
>   
>   struct xe_device;
>   
> +enum xe_d3_state {
> +	XE_D3HOT = 0,
> +	XE_D3COLD_VRSR,
> +	XE_D3COLD_OFF,
> +};
> +
>   int xe_pm_suspend(struct xe_device *xe);
>   int xe_pm_resume(struct xe_device *xe);
>   
> @@ -30,7 +36,7 @@ void xe_pm_runtime_get_noresume(struct xe_device *xe);
>   bool xe_pm_runtime_resume_and_get(struct xe_device *xe);
>   void xe_pm_assert_unbounded_bridge(struct xe_device *xe);
>   int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold);
> -void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);
> +void xe_pm_d3cold_target_state_toggle(struct xe_device *xe);
>   bool xe_rpm_reclaim_safe(const struct xe_device *xe);
>   struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
>   int xe_pm_module_init(void);
LGTM.

Reviewed-by: Karthik Poosa <karthik.poosa@intel.com>


[-- Attachment #2: Type: text/html, Size: 42604 bytes --]

[PATCH v4 09/11] drm/xe/pm: D3Cold target state

[Badal Nilawar]

From: Anshuman Gupta <anshuman.gupta@intel.com>

Trade-off D3Cold target state based upon current vram usage.
If vram usage is greater than vram_d3cold_threshold and GPU
is VRSR capable target D3Cold state is D3Cold-VRSR
otherwise target state is D3Cold-Off.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/xe/xe_pm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index b86e95493cb5..1e061bfc3e52 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -113,6 +113,14 @@ static void xe_rpm_lockmap_release(const struct xe_device *xe)
 			 &xe_pm_runtime_d3cold_map);
 }
 
+static void xe_pm_suspend_prepare(struct xe_device *xe)
+{
+	if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE)
+		xe_pm_d3cold_target_state_toggle(xe);
+	else
+		xe->d3cold.target_state = XE_D3COLD_OFF;
+}
+
 /**
  * xe_pm_suspend - Helper for System suspend, i.e. S0->S3 / S0->S2idle
  * @xe: xe device instance
@@ -128,6 +136,8 @@ int xe_pm_suspend(struct xe_device *xe)
 	drm_dbg(&xe->drm, "Suspending device\n");
 	trace_xe_pm_suspend(xe, __builtin_return_address(0));
 
+	xe_pm_suspend_prepare(xe);
+
 	err = xe_pxp_pm_suspend(xe->pxp);
 	if (err)
 		goto err;
@@ -948,10 +958,14 @@ void xe_pm_d3cold_target_state_toggle(struct xe_device *xe)
 
 	if (total_vram_used_mb < xe->d3cold.vram_threshold)
 		xe->d3cold.target_state = XE_D3COLD_OFF;
+	else if (xe->d3cold.vrsr_capable)
+		xe->d3cold.target_state = XE_D3COLD_VRSR;
 	else
 		xe->d3cold.target_state = XE_D3HOT;
 
 	mutex_unlock(&xe->d3cold.lock);
+
+	drm_dbg(&xe->drm, "Xe D3cold target state %d\n", xe->d3cold.target_state);
 }
 
 /**
-- 
2.34.1

Re: [v4,09/11] drm/xe/pm: D3Cold target state

[Poosa, Karthik]

On 29-05-2025 16:46, Badal Nilawar wrote:
> From: Anshuman Gupta <anshuman.gupta@intel.com>
>
> Trade-off D3Cold target state based upon current vram usage.
> If vram usage is greater than vram_d3cold_threshold and GPU
> is VRSR capable target D3Cold state is D3Cold-VRSR

rephrase above line to,

is VRSR capable, then target D3Cold state is D3Cold-VRSR

> otherwise target state is D3Cold-Off.
>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_pm.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index b86e95493cb5..1e061bfc3e52 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -113,6 +113,14 @@ static void xe_rpm_lockmap_release(const struct xe_device *xe)
>   			 &xe_pm_runtime_d3cold_map);
>   }
>   
> +static void xe_pm_suspend_prepare(struct xe_device *xe)
> +{
> +	if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE)
> +		xe_pm_d3cold_target_state_toggle(xe);
> +	else
> +		xe->d3cold.target_state = XE_D3COLD_OFF;
> +}
> +
>   /**
>    * xe_pm_suspend - Helper for System suspend, i.e. S0->S3 / S0->S2idle
>    * @xe: xe device instance
> @@ -128,6 +136,8 @@ int xe_pm_suspend(struct xe_device *xe)
>   	drm_dbg(&xe->drm, "Suspending device\n");
>   	trace_xe_pm_suspend(xe, __builtin_return_address(0));
>   
> +	xe_pm_suspend_prepare(xe);
> +
>   	err = xe_pxp_pm_suspend(xe->pxp);
>   	if (err)
>   		goto err;
> @@ -948,10 +958,14 @@ void xe_pm_d3cold_target_state_toggle(struct xe_device *xe)
>   
>   	if (total_vram_used_mb < xe->d3cold.vram_threshold)
>   		xe->d3cold.target_state = XE_D3COLD_OFF;
> +	else if (xe->d3cold.vrsr_capable)
> +		xe->d3cold.target_state = XE_D3COLD_VRSR;
>   	else
>   		xe->d3cold.target_state = XE_D3HOT;
>   
>   	mutex_unlock(&xe->d3cold.lock);
> +
> +	drm_dbg(&xe->drm, "Xe D3cold target state %d\n", xe->d3cold.target_state);
>   }
>   
>   /**

LGTM.

With minor commit message change above, this is

Reviewed-by: Karthik Poosa <karthik.poosa@intel.com>

[PATCH v4 10/11] drm/xe/vrsr: Enable VRSR

[Badal Nilawar]

From: Anshuman Gupta <anshuman.gupta@intel.com>

Enabling VRSR in runtime suspend and also in System wide suspend.
Also fix couple of typo in xe_pm.c.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 drivers/gpu/drm/xe/xe_pci.c |  4 ++--
 drivers/gpu/drm/xe/xe_pm.c  | 46 +++++++++++++++++++++++++++----------
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 5ae1df345416..fdf878594fb0 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -926,7 +926,7 @@ static int xe_pci_suspend(struct device *dev)
 
 	/*
 	 * Enabling D3Cold is needed for S2Idle/S0ix.
-	 * It is save to allow here since xe_pm_suspend has evicted
+	 * It is safe to allow here since xe_pm_suspend has evicted
 	 * the local memory and the direct complete optimization is disabled.
 	 */
 	d3cold_toggle(pdev, D3COLD_ENABLE);
@@ -943,7 +943,7 @@ static int xe_pci_resume(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	int err;
 
-	/* Give back the D3Cold decision to the runtime P M*/
+	/* Give back the D3Cold decision to the runtime PM */
 	d3cold_toggle(pdev, D3COLD_DISABLE);
 
 	err = pci_set_power_state(pdev, PCI_D0);
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index 1e061bfc3e52..19596d467298 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -147,10 +147,12 @@ int xe_pm_suspend(struct xe_device *xe)
 
 	xe_display_pm_suspend(xe);
 
-	/* FIXME: Super racey... */
-	err = xe_bo_evict_all(xe);
-	if (err)
-		goto err_pxp;
+	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
+		/* FIXME: Super racey... */
+		err = xe_bo_evict_all(xe);
+		if (err)
+			goto err_pxp;
+	}
 
 	for_each_gt(gt, xe, id) {
 		err = xe_gt_suspend(gt);
@@ -162,6 +164,12 @@ int xe_pm_suspend(struct xe_device *xe)
 
 	xe_display_pm_suspend_late(xe);
 
+	if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
+		err = xe_pm_vrsr_enable(xe, true);
+			if (err)
+				goto err_display;
+	}
+
 	drm_dbg(&xe->drm, "Device suspended\n");
 	return 0;
 
@@ -203,9 +211,11 @@ int xe_pm_resume(struct xe_device *xe)
 	 * This only restores pinned memory which is the memory required for the
 	 * GT(s) to resume.
 	 */
-	err = xe_bo_restore_early(xe);
-	if (err)
-		goto err;
+	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
+		err = xe_bo_restore_early(xe);
+		if (err)
+			goto err;
+	}
 
 	xe_irq_resume(xe);
 
@@ -214,9 +224,11 @@ int xe_pm_resume(struct xe_device *xe)
 
 	xe_display_pm_resume(xe);
 
-	err = xe_bo_restore_late(xe);
-	if (err)
-		goto err;
+	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
+		err = xe_bo_restore_late(xe);
+		if (err)
+			goto err;
+	}
 
 	xe_pxp_pm_resume(xe->pxp);
 
@@ -616,7 +628,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
 
 	xe_display_pm_runtime_suspend(xe);
 
-	if (xe->d3cold.target_state) {
+	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
 		err = xe_bo_evict_all(xe);
 		if (err)
 			goto out_resume;
@@ -632,6 +644,14 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
 
 	xe_display_pm_runtime_suspend_late(xe);
 
+	if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
+		err = xe_pm_vrsr_enable(xe, true);
+			if (err) {
+				drm_err(&xe->drm, "Failed to enable VRSR: %d\n", err);
+				goto out_resume;
+			}
+	}
+
 	xe_rpm_lockmap_release(xe);
 	xe_pm_write_callback_task(xe, NULL);
 	return 0;
@@ -669,7 +689,9 @@ int xe_pm_runtime_resume(struct xe_device *xe)
 			goto out;
 
 		xe_display_pm_resume_early(xe);
+	}
 
+	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
 		/*
 		 * This only restores pinned memory which is the memory
 		 * required for the GT(s) to resume.
@@ -686,7 +708,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
 
 	xe_display_pm_runtime_resume(xe);
 
-	if (xe->d3cold.target_state) {
+	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
 		err = xe_bo_restore_late(xe);
 		if (err)
 			goto out;
-- 
2.34.1

Re: [v4,10/11] drm/xe/vrsr: Enable VRSR

[Poosa, Karthik]

On 29-05-2025 16:46, Badal Nilawar wrote:
> From: Anshuman Gupta <anshuman.gupta@intel.com>
>
> Enabling VRSR in runtime suspend and also in System wide suspend.

This can be rephrased to -

Integrate VRSR into both system-wide and runtime suspend-resume flows.

> Also fix couple of typo in xe_pm.c.
>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_pci.c |  4 ++--
>   drivers/gpu/drm/xe/xe_pm.c  | 46 +++++++++++++++++++++++++++----------
>   2 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 5ae1df345416..fdf878594fb0 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -926,7 +926,7 @@ static int xe_pci_suspend(struct device *dev)
>   
>   	/*
>   	 * Enabling D3Cold is needed for S2Idle/S0ix.
> -	 * It is save to allow here since xe_pm_suspend has evicted
> +	 * It is safe to allow here since xe_pm_suspend has evicted
>   	 * the local memory and the direct complete optimization is disabled.
>   	 */
>   	d3cold_toggle(pdev, D3COLD_ENABLE);
> @@ -943,7 +943,7 @@ static int xe_pci_resume(struct device *dev)
>   	struct pci_dev *pdev = to_pci_dev(dev);
>   	int err;
>   
> -	/* Give back the D3Cold decision to the runtime P M*/
> +	/* Give back the D3Cold decision to the runtime PM */
>   	d3cold_toggle(pdev, D3COLD_DISABLE);
>   
>   	err = pci_set_power_state(pdev, PCI_D0);
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index 1e061bfc3e52..19596d467298 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -147,10 +147,12 @@ int xe_pm_suspend(struct xe_device *xe)
>   
>   	xe_display_pm_suspend(xe);
>   
> -	/* FIXME: Super racey... */
> -	err = xe_bo_evict_all(xe);
> -	if (err)
> -		goto err_pxp;
> +	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
> +		/* FIXME: Super racey... */
> +		err = xe_bo_evict_all(xe);
> +		if (err)
> +			goto err_pxp;
> +	}
>   
>   	for_each_gt(gt, xe, id) {
>   		err = xe_gt_suspend(gt);
> @@ -162,6 +164,12 @@ int xe_pm_suspend(struct xe_device *xe)
>   
>   	xe_display_pm_suspend_late(xe);
>   
> +	if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
> +		err = xe_pm_vrsr_enable(xe, true);
> +			if (err)
> +				goto err_display;
> +	}
> +
>   	drm_dbg(&xe->drm, "Device suspended\n");
>   	return 0;
>   
> @@ -203,9 +211,11 @@ int xe_pm_resume(struct xe_device *xe)
>   	 * This only restores pinned memory which is the memory required for the
>   	 * GT(s) to resume.
>   	 */
> -	err = xe_bo_restore_early(xe);
> -	if (err)
> -		goto err;
> +	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
> +		err = xe_bo_restore_early(xe);
> +		if (err)
> +			goto err;
> +	}
>   
>   	xe_irq_resume(xe);
>   
> @@ -214,9 +224,11 @@ int xe_pm_resume(struct xe_device *xe)
>   
>   	xe_display_pm_resume(xe);
>   
> -	err = xe_bo_restore_late(xe);
> -	if (err)
> -		goto err;
> +	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
> +		err = xe_bo_restore_late(xe);
> +		if (err)
> +			goto err;
> +	}
>   
>   	xe_pxp_pm_resume(xe->pxp);
>   
> @@ -616,7 +628,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>   
>   	xe_display_pm_runtime_suspend(xe);
>   
> -	if (xe->d3cold.target_state) {
> +	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>   		err = xe_bo_evict_all(xe);
>   		if (err)
>   			goto out_resume;
> @@ -632,6 +644,14 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>   
>   	xe_display_pm_runtime_suspend_late(xe);
>   
> +	if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
> +		err = xe_pm_vrsr_enable(xe, true);
> +			if (err) {
> +				drm_err(&xe->drm, "Failed to enable VRSR: %d\n", err);
> +				goto out_resume;
> +			}
> +	}
> +
>   	xe_rpm_lockmap_release(xe);
>   	xe_pm_write_callback_task(xe, NULL);
>   	return 0;
> @@ -669,7 +689,9 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>   			goto out;
>   
>   		xe_display_pm_resume_early(xe);
> +	}
>   
> +	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>   		/*
>   		 * This only restores pinned memory which is the memory
>   		 * required for the GT(s) to resume.
> @@ -686,7 +708,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>   
>   	xe_display_pm_runtime_resume(xe);
>   
> -	if (xe->d3cold.target_state) {
> +	if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>   		err = xe_bo_restore_late(xe);
>   		if (err)
>   			goto out;

Please rephrase commit message. Other than that, code code flow looks fine.

Acked-by: Karthik Poosa <karthik.poosa@intel.com>

Re: [v4,10/11] drm/xe/vrsr: Enable VRSR

[Nilawar, Badal]

On 24-06-2025 17:15, Poosa, Karthik wrote:
>
> On 29-05-2025 16:46, Badal Nilawar wrote:
>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>
>> Enabling VRSR in runtime suspend and also in System wide suspend.
>
> This can be rephrased to -
>
> Integrate VRSR into both system-wide and runtime suspend-resume flows.

I will drop gerund.  So it will be "Enable VRSR  in runtime suspend and 
in System wide suspend".

Thanks,
Badal

>
>> Also fix couple of typo in xe_pm.c.
>>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pci.c |  4 ++--
>>   drivers/gpu/drm/xe/xe_pm.c  | 46 +++++++++++++++++++++++++++----------
>>   2 files changed, 36 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 5ae1df345416..fdf878594fb0 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -926,7 +926,7 @@ static int xe_pci_suspend(struct device *dev)
>>         /*
>>        * Enabling D3Cold is needed for S2Idle/S0ix.
>> -     * It is save to allow here since xe_pm_suspend has evicted
>> +     * It is safe to allow here since xe_pm_suspend has evicted
>>        * the local memory and the direct complete optimization is 
>> disabled.
>>        */
>>       d3cold_toggle(pdev, D3COLD_ENABLE);
>> @@ -943,7 +943,7 @@ static int xe_pci_resume(struct device *dev)
>>       struct pci_dev *pdev = to_pci_dev(dev);
>>       int err;
>>   -    /* Give back the D3Cold decision to the runtime P M*/
>> +    /* Give back the D3Cold decision to the runtime PM */
>>       d3cold_toggle(pdev, D3COLD_DISABLE);
>>         err = pci_set_power_state(pdev, PCI_D0);
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index 1e061bfc3e52..19596d467298 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -147,10 +147,12 @@ int xe_pm_suspend(struct xe_device *xe)
>>         xe_display_pm_suspend(xe);
>>   -    /* FIXME: Super racey... */
>> -    err = xe_bo_evict_all(xe);
>> -    if (err)
>> -        goto err_pxp;
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>> +        /* FIXME: Super racey... */
>> +        err = xe_bo_evict_all(xe);
>> +        if (err)
>> +            goto err_pxp;
>> +    }
>>         for_each_gt(gt, xe, id) {
>>           err = xe_gt_suspend(gt);
>> @@ -162,6 +164,12 @@ int xe_pm_suspend(struct xe_device *xe)
>>         xe_display_pm_suspend_late(xe);
>>   +    if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
>> +        err = xe_pm_vrsr_enable(xe, true);
>> +            if (err)
>> +                goto err_display;
>> +    }
>> +
>>       drm_dbg(&xe->drm, "Device suspended\n");
>>       return 0;
>>   @@ -203,9 +211,11 @@ int xe_pm_resume(struct xe_device *xe)
>>        * This only restores pinned memory which is the memory 
>> required for the
>>        * GT(s) to resume.
>>        */
>> -    err = xe_bo_restore_early(xe);
>> -    if (err)
>> -        goto err;
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>> +        err = xe_bo_restore_early(xe);
>> +        if (err)
>> +            goto err;
>> +    }
>>         xe_irq_resume(xe);
>>   @@ -214,9 +224,11 @@ int xe_pm_resume(struct xe_device *xe)
>>         xe_display_pm_resume(xe);
>>   -    err = xe_bo_restore_late(xe);
>> -    if (err)
>> -        goto err;
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>> +        err = xe_bo_restore_late(xe);
>> +        if (err)
>> +            goto err;
>> +    }
>>         xe_pxp_pm_resume(xe->pxp);
>>   @@ -616,7 +628,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>         xe_display_pm_runtime_suspend(xe);
>>   -    if (xe->d3cold.target_state) {
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>           err = xe_bo_evict_all(xe);
>>           if (err)
>>               goto out_resume;
>> @@ -632,6 +644,14 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>         xe_display_pm_runtime_suspend_late(xe);
>>   +    if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
>> +        err = xe_pm_vrsr_enable(xe, true);
>> +            if (err) {
>> +                drm_err(&xe->drm, "Failed to enable VRSR: %d\n", err);
>> +                goto out_resume;
>> +            }
>> +    }
>> +
>>       xe_rpm_lockmap_release(xe);
>>       xe_pm_write_callback_task(xe, NULL);
>>       return 0;
>> @@ -669,7 +689,9 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>               goto out;
>>             xe_display_pm_resume_early(xe);
>> +    }
>>   +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>           /*
>>            * This only restores pinned memory which is the memory
>>            * required for the GT(s) to resume.
>> @@ -686,7 +708,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>         xe_display_pm_runtime_resume(xe);
>>   -    if (xe->d3cold.target_state) {
>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>           err = xe_bo_restore_late(xe);
>>           if (err)
>>               goto out;
>
> Please rephrase commit message. Other than that, code code flow looks 
> fine.
>
> Acked-by: Karthik Poosa <karthik.poosa@intel.com>
>

Re: [v4,10/11] drm/xe/vrsr: Enable VRSR

[Poosa, Karthik]

On 03-09-2025 19:46, Nilawar, Badal wrote:
>
> On 24-06-2025 17:15, Poosa, Karthik wrote:
>>
>> On 29-05-2025 16:46, Badal Nilawar wrote:
>>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>>
>>> Enabling VRSR in runtime suspend and also in System wide suspend.
>>
>> This can be rephrased to -
>>
>> Integrate VRSR into both system-wide and runtime suspend-resume flows.
>
> I will drop gerund.  So it will be "Enable VRSR  in runtime suspend 
> and in System wide suspend".
Okay
>
> Thanks,
> Badal
>
>>
>>> Also fix couple of typo in xe_pm.c.
>>>
>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_pci.c |  4 ++--
>>>   drivers/gpu/drm/xe/xe_pm.c  | 46 
>>> +++++++++++++++++++++++++++----------
>>>   2 files changed, 36 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>> index 5ae1df345416..fdf878594fb0 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> @@ -926,7 +926,7 @@ static int xe_pci_suspend(struct device *dev)
>>>         /*
>>>        * Enabling D3Cold is needed for S2Idle/S0ix.
>>> -     * It is save to allow here since xe_pm_suspend has evicted
>>> +     * It is safe to allow here since xe_pm_suspend has evicted
>>>        * the local memory and the direct complete optimization is 
>>> disabled.
>>>        */
>>>       d3cold_toggle(pdev, D3COLD_ENABLE);
>>> @@ -943,7 +943,7 @@ static int xe_pci_resume(struct device *dev)
>>>       struct pci_dev *pdev = to_pci_dev(dev);
>>>       int err;
>>>   -    /* Give back the D3Cold decision to the runtime P M*/
>>> +    /* Give back the D3Cold decision to the runtime PM */
>>>       d3cold_toggle(pdev, D3COLD_DISABLE);
>>>         err = pci_set_power_state(pdev, PCI_D0);
>>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>>> index 1e061bfc3e52..19596d467298 100644
>>> --- a/drivers/gpu/drm/xe/xe_pm.c
>>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>>> @@ -147,10 +147,12 @@ int xe_pm_suspend(struct xe_device *xe)
>>>         xe_display_pm_suspend(xe);
>>>   -    /* FIXME: Super racey... */
>>> -    err = xe_bo_evict_all(xe);
>>> -    if (err)
>>> -        goto err_pxp;
>>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>> +        /* FIXME: Super racey... */
>>> +        err = xe_bo_evict_all(xe);
>>> +        if (err)
>>> +            goto err_pxp;
>>> +    }
>>>         for_each_gt(gt, xe, id) {
>>>           err = xe_gt_suspend(gt);
>>> @@ -162,6 +164,12 @@ int xe_pm_suspend(struct xe_device *xe)
>>>         xe_display_pm_suspend_late(xe);
>>>   +    if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
>>> +        err = xe_pm_vrsr_enable(xe, true);
>>> +            if (err)
>>> +                goto err_display;
>>> +    }
>>> +
>>>       drm_dbg(&xe->drm, "Device suspended\n");
>>>       return 0;
>>>   @@ -203,9 +211,11 @@ int xe_pm_resume(struct xe_device *xe)
>>>        * This only restores pinned memory which is the memory 
>>> required for the
>>>        * GT(s) to resume.
>>>        */
>>> -    err = xe_bo_restore_early(xe);
>>> -    if (err)
>>> -        goto err;
>>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>> +        err = xe_bo_restore_early(xe);
>>> +        if (err)
>>> +            goto err;
>>> +    }
>>>         xe_irq_resume(xe);
>>>   @@ -214,9 +224,11 @@ int xe_pm_resume(struct xe_device *xe)
>>>         xe_display_pm_resume(xe);
>>>   -    err = xe_bo_restore_late(xe);
>>> -    if (err)
>>> -        goto err;
>>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>> +        err = xe_bo_restore_late(xe);
>>> +        if (err)
>>> +            goto err;
>>> +    }
>>>         xe_pxp_pm_resume(xe->pxp);
>>>   @@ -616,7 +628,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>>         xe_display_pm_runtime_suspend(xe);
>>>   -    if (xe->d3cold.target_state) {
>>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>>           err = xe_bo_evict_all(xe);
>>>           if (err)
>>>               goto out_resume;
>>> @@ -632,6 +644,14 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>>         xe_display_pm_runtime_suspend_late(xe);
>>>   +    if (xe->d3cold.target_state == XE_D3COLD_VRSR) {
>>> +        err = xe_pm_vrsr_enable(xe, true);
>>> +            if (err) {
>>> +                drm_err(&xe->drm, "Failed to enable VRSR: %d\n", err);
>>> +                goto out_resume;
>>> +            }
>>> +    }
>>> +
>>>       xe_rpm_lockmap_release(xe);
>>>       xe_pm_write_callback_task(xe, NULL);
>>>       return 0;
>>> @@ -669,7 +689,9 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>>               goto out;
>>>             xe_display_pm_resume_early(xe);
>>> +    }
>>>   +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>>           /*
>>>            * This only restores pinned memory which is the memory
>>>            * required for the GT(s) to resume.
>>> @@ -686,7 +708,7 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>>         xe_display_pm_runtime_resume(xe);
>>>   -    if (xe->d3cold.target_state) {
>>> +    if (xe->d3cold.target_state == XE_D3COLD_OFF) {
>>>           err = xe_bo_restore_late(xe);
>>>           if (err)
>>>               goto out;
>>
>> Please rephrase commit message. Other than that, code code flow looks 
>> fine.
>>
>> Acked-by: Karthik Poosa <karthik.poosa@intel.com>
>>

[PATCH v4 11/11] drm/xe/vrsr: Introduce a debugfs node named vrsr_capable

[Badal Nilawar]

Add a debugfs node named vrsr_capable to check if the device
supports VRSR.

Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 drivers/gpu/drm/xe/xe_debugfs.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
index d83cd6ed3fa8..d969a8f6d430 100644
--- a/drivers/gpu/drm/xe/xe_debugfs.c
+++ b/drivers/gpu/drm/xe/xe_debugfs.c
@@ -226,6 +226,23 @@ static const struct file_operations atomic_svm_timeslice_ms_fops = {
 	.write = atomic_svm_timeslice_ms_set,
 };
 
+static ssize_t vrsr_capable_show(struct file *f, char __user *ubuf,
+				 size_t size, loff_t *pos)
+{
+	struct xe_device *xe = file_inode(f)->i_private;
+	char buf[32];
+	int len = 0;
+
+	len = scnprintf(buf, sizeof(buf), "%s\n", xe->d3cold.vrsr_capable ? "true" : "false");
+
+	return simple_read_from_buffer(ubuf, size, pos, buf, len);
+}
+
+static const struct file_operations vrsr_capable_fops = {
+	.owner = THIS_MODULE,
+	.read = vrsr_capable_show,
+};
+
 void xe_debugfs_register(struct xe_device *xe)
 {
 	struct ttm_device *bdev = &xe->ttm;
@@ -249,6 +266,9 @@ void xe_debugfs_register(struct xe_device *xe)
 	debugfs_create_file("atomic_svm_timeslice_ms", 0600, root, xe,
 			    &atomic_svm_timeslice_ms_fops);
 
+	debugfs_create_file("vrsr_capable", 0400, root, xe,
+			    &vrsr_capable_fops);
+
 	for (mem_type = XE_PL_VRAM0; mem_type <= XE_PL_VRAM1; ++mem_type) {
 		man = ttm_manager_type(bdev, mem_type);
 
-- 
2.34.1

Re: [v4,11/11] drm/xe/vrsr: Introduce a debugfs node named vrsr_capable

[Poosa, Karthik]

[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]


On 29-05-2025 16:46, Badal Nilawar wrote:
> Add a debugfs node named vrsr_capable to check if the device
> supports VRSR.
>
> Signed-off-by: Badal Nilawar<badal.nilawar@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_debugfs.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> index d83cd6ed3fa8..d969a8f6d430 100644
> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> @@ -226,6 +226,23 @@ static const struct file_operations atomic_svm_timeslice_ms_fops = {
>   	.write = atomic_svm_timeslice_ms_set,
>   };
>   
> +static ssize_t vrsr_capable_show(struct file *f, char __user *ubuf,
> +				 size_t size, loff_t *pos)
> +{
> +	struct xe_device *xe = file_inode(f)->i_private;
> +	char buf[32];
> +	int len = 0;
> +
> +	len = scnprintf(buf, sizeof(buf), "%s\n", xe->d3cold.vrsr_capable ? "true" : "false");
> +
> +	return simple_read_from_buffer(ubuf, size, pos, buf, len);
> +}
> +
> +static const struct file_operations vrsr_capable_fops = {
> +	.owner = THIS_MODULE,
> +	.read = vrsr_capable_show,
> +};
> +
>   void xe_debugfs_register(struct xe_device *xe)
>   {
>   	struct ttm_device *bdev = &xe->ttm;
> @@ -249,6 +266,9 @@ void xe_debugfs_register(struct xe_device *xe)
>   	debugfs_create_file("atomic_svm_timeslice_ms", 0600, root, xe,
>   			    &atomic_svm_timeslice_ms_fops);
>   
> +	debugfs_create_file("vrsr_capable", 0400, root, xe,
> +			    &vrsr_capable_fops);
Instead of debugfs_create_file, you can use debugfs_create_bool here, 
which will simplify the code.
> +
>   	for (mem_type = XE_PL_VRAM0; mem_type <= XE_PL_VRAM1; ++mem_type) {
>   		man = ttm_manager_type(bdev, mem_type);
>   

[-- Attachment #2: Type: text/html, Size: 2413 bytes --]

✗ CI.Patch_applied: failure for VRAM Self Refresh (rev4)

[Patchwork]

== Series Details ==

Series: VRAM Self Refresh (rev4)
URL   : https://patchwork.freedesktop.org/series/145342/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: bf0749a9930e drm-tip: 2025y-05m-29d-05h-28m-18s UTC integration manifest
=== git am output follows ===
error: patch failed: drivers/gpu/drm/xe/display/xe_display.c:88
error: drivers/gpu/drm/xe/display/xe_display.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: PCI/ACPI: Add D3cold Aux Power Limit_DSM method
Applying: PCI/ACPI: Per root port allow one Aux power limit request
Applying: PCI/ACPI: Add PERST# Assertion Delay _DSM method
Applying: drm/xe/vrsr: Introduce flag has_vrsr
Applying: drm/xe/vrsr: Detect VRSR Capability
Applying: drm/xe/vrsr: Initialize VRSR feature
Applying: drm/xe/vrsr: Enable VRSR on default VGA boot device
Patch failed at 0007 drm/xe/vrsr: Enable VRSR on default VGA boot device
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".