[igt-dev] [PATCH i-g-t v3 0/5] Cold Reset IGT Test

[igt-dev] [PATCH i-g-t v3 0/5] Cold Reset IGT Test

[Anshuman Gupta]

Adding Cold Reset IGT support

v3:
- Address the review comment from Aravind.

Anshuman Gupta (5):
  lib/igt_pci: helpers to get PCI capabilities offset
  lib/igt_pci: Add PCIe slot cap
  lib/igt_pm: Refactor get firmware_node fd
  test/device_reset: Refactor initiate_device_reset
  tests/device_reset: Add cold reset IGT test

 lib/igt_pci.c        |  44 ++++++++++++
 lib/igt_pci.h        |  35 +++++++++
 lib/igt_pm.c         |  76 +++++++++++++++++---
 lib/igt_pm.h         |   1 +
 lib/meson.build      |   1 +
 tests/device_reset.c | 167 +++++++++++++++++++++++++++++++++++++++----
 6 files changed, 300 insertions(+), 24 deletions(-)
 create mode 100644 lib/igt_pci.c
 create mode 100644 lib/igt_pci.h

-- 
2.25.1

[igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset

[Anshuman Gupta]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 2900 bytes --]

Added helper functions to search for PCI capabilities
with help of libpciaccess library.

Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 lib/igt_pci.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_pci.h   | 32 ++++++++++++++++++++++++++++++++
 lib/meson.build |  1 +
 3 files changed, 77 insertions(+)
 create mode 100644 lib/igt_pci.c
 create mode 100644 lib/igt_pci.h

diff --git a/lib/igt_pci.c b/lib/igt_pci.c
new file mode 100644
index 0000000000..bc0369341d
--- /dev/null
+++ b/lib/igt_pci.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include <pciaccess.h>
+#include "igt_pci.h"
+
+/**
+ * find_pci_cap_offset:
+ * @dev: pci device
+ * @cap_id: searched capability id, 0 means any capability
+ * @start_offset: offset in config space from which we start searching
+ *
+ * return:
+ * -1 on config read error, 0 if capability is not found,
+ * otherwise offset at which capability with cap_id is found
+ */
+int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
+			   int start_offset)
+{
+	uint8_t offset;
+	uint16_t cap_header;
+	int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE)
+			/ sizeof(cap_header);
+
+	if (pci_device_cfg_read_u8(dev, &offset, start_offset))
+		return -1;
+
+	while (loop--) {
+		if (offset < PCI_TYPE0_1_HEADER_SIZE)
+			break;
+
+		if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC)))
+			return -1;
+
+		if (!cap_id || cap_id == (cap_header & 0xFF))
+			return offset;
+
+		offset = cap_header >> 8;
+	}
+
+	return 0;
+}
diff --git a/lib/igt_pci.h b/lib/igt_pci.h
new file mode 100644
index 0000000000..68afd2dacb
--- /dev/null
+++ b/lib/igt_pci.h
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef __IGT_PCI_H__
+#define __IGT_PCI_H__
+
+#include <stdint.h>
+#include <endian.h>
+
+/* forward declaration */
+struct pci_device;
+
+#define PCI_TYPE0_1_HEADER_SIZE 0x40
+#define PCI_CAPS_START 0x34
+#define PCI_CFG_SPACE_SIZE 0x100
+#define PCIE_CFG_SPACE_SIZE 0x1000
+
+enum pci_cap_id {
+	PCI_EXPRESS_CAP_ID = 0x10
+};
+
+int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
+			   int start_offset);
+
+static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id)
+{
+	return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START);
+}
+
+#endif
diff --git a/lib/meson.build b/lib/meson.build
index cef2d0ff3d..e0fb7ddfed 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -33,6 +33,7 @@ lib_sources = [
 	'igt_pipe_crc.c',
 	'igt_power.c',
 	'igt_primes.c',
+	'igt_pci.c',
 	'igt_rand.c',
 	'igt_stats.c',
 	'igt_syncobj.c',
-- 
2.25.1

Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset

[Nilawar, Badal]

Hi Anshuman,

On 23-11-2022 14:24, Anshuman Gupta wrote:
> Added helper functions to search for PCI capabilities
> with help of libpciaccess library.
> 
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   lib/igt_pci.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   lib/igt_pci.h   | 32 ++++++++++++++++++++++++++++++++
>   lib/meson.build |  1 +
>   3 files changed, 77 insertions(+)
>   create mode 100644 lib/igt_pci.c
>   create mode 100644 lib/igt_pci.h
> 
> diff --git a/lib/igt_pci.c b/lib/igt_pci.c
> new file mode 100644
> index 0000000000..bc0369341d
> --- /dev/null
> +++ b/lib/igt_pci.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#include <pciaccess.h>
> +#include "igt_pci.h"
> +
> +/**
> + * find_pci_cap_offset:
> + * @dev: pci device
> + * @cap_id: searched capability id, 0 means any capability
> + * @start_offset: offset in config space from which we start searching
> + *
> + * return:
> + * -1 on config read error, 0 if capability is not found,
> + * otherwise offset at which capability with cap_id is found
> + */
> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
> +			   int start_offset)
> +{
> +	uint8_t offset;
> +	uint16_t cap_header;
> +	int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE)
> +			/ sizeof(cap_header);
> +
> +	if (pci_device_cfg_read_u8(dev, &offset, start_offset))
> +		return -1;
> +
> +	while (loop--) {
> +		if (offset < PCI_TYPE0_1_HEADER_SIZE)
> +			break;
> +
> +		if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC)))
> +			return -1;
> +
> +		if (!cap_id || cap_id == (cap_header & 0xFF))
> +			return offset;
> +
> +		offset = cap_header >> 8;
For the last capability, next capability address will always be 0. So 
above instead of using while(loop--) we can use while(offset).

Regards,
Badal
> +	}
> +
> +	return 0;
> +}
> diff --git a/lib/igt_pci.h b/lib/igt_pci.h
> new file mode 100644
> index 0000000000..68afd2dacb
> --- /dev/null
> +++ b/lib/igt_pci.h
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#ifndef __IGT_PCI_H__
> +#define __IGT_PCI_H__
> +
> +#include <stdint.h>
> +#include <endian.h>
> +
> +/* forward declaration */
> +struct pci_device;
> +
> +#define PCI_TYPE0_1_HEADER_SIZE 0x40
> +#define PCI_CAPS_START 0x34
> +#define PCI_CFG_SPACE_SIZE 0x100
> +#define PCIE_CFG_SPACE_SIZE 0x1000
> +
> +enum pci_cap_id {
> +	PCI_EXPRESS_CAP_ID = 0x10
> +};
> +
> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
> +			   int start_offset);
> +
> +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id)
> +{
> +	return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START);
> +}
> +
> +#endif
> diff --git a/lib/meson.build b/lib/meson.build
> index cef2d0ff3d..e0fb7ddfed 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -33,6 +33,7 @@ lib_sources = [
>   	'igt_pipe_crc.c',
>   	'igt_power.c',
>   	'igt_primes.c',
> +	'igt_pci.c',
>   	'igt_rand.c',
>   	'igt_stats.c',
>   	'igt_syncobj.c',

Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset

[Kamil Konieczny]

Hi,

On 2022-11-29 at 17:30:54 +0530, Nilawar, Badal wrote:
> Hi Anshuman,
> 
> On 23-11-2022 14:24, Anshuman Gupta wrote:
> > Added helper functions to search for PCI capabilities
> > with help of libpciaccess library.
> > 
> > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> > Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >   lib/igt_pci.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >   lib/igt_pci.h   | 32 ++++++++++++++++++++++++++++++++
> >   lib/meson.build |  1 +
> >   3 files changed, 77 insertions(+)
> >   create mode 100644 lib/igt_pci.c
> >   create mode 100644 lib/igt_pci.h
> > 
> > diff --git a/lib/igt_pci.c b/lib/igt_pci.c
> > new file mode 100644
> > index 0000000000..bc0369341d
> > --- /dev/null
> > +++ b/lib/igt_pci.c
> > @@ -0,0 +1,44 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2022 Intel Corporation
> > + */
> > +
> > +#include <pciaccess.h>
> > +#include "igt_pci.h"
> > +
> > +/**
> > + * find_pci_cap_offset:
> > + * @dev: pci device
> > + * @cap_id: searched capability id, 0 means any capability
> > + * @start_offset: offset in config space from which we start searching
> > + *
> > + * return:
> > + * -1 on config read error, 0 if capability is not found,
> > + * otherwise offset at which capability with cap_id is found
> > + */
> > +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
> > +			   int start_offset)
> > +{
> > +	uint8_t offset;
> > +	uint16_t cap_header;
> > +	int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE)
> > +			/ sizeof(cap_header);
> > +
> > +	if (pci_device_cfg_read_u8(dev, &offset, start_offset))
> > +		return -1;
> > +
> > +	while (loop--) {
> > +		if (offset < PCI_TYPE0_1_HEADER_SIZE)
> > +			break;
> > +
> > +		if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC)))
> > +			return -1;
> > +
> > +		if (!cap_id || cap_id == (cap_header & 0xFF))
> > +			return offset;
> > +
> > +		offset = cap_header >> 8;
> For the last capability, next capability address will always be 0. So above
> instead of using while(loop--) we can use while(offset).
> 
> Regards,
> Badal

This way we are guarded for endless looping, btw check for zero
is just at loop enter (first if).

> > +	}
> > +

I would like to see additional check here:

	if (loop <= 0 && offset)
		return -1;

or maybe assert here ?

> > +	return 0;
> > +}
> > diff --git a/lib/igt_pci.h b/lib/igt_pci.h
> > new file mode 100644
> > index 0000000000..68afd2dacb
> > --- /dev/null
> > +++ b/lib/igt_pci.h
> > @@ -0,0 +1,32 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2022 Intel Corporation
> > + */
> > +
> > +#ifndef __IGT_PCI_H__
> > +#define __IGT_PCI_H__
> > +
> > +#include <stdint.h>
> > +#include <endian.h>
> > +
> > +/* forward declaration */
> > +struct pci_device;
> > +
> > +#define PCI_TYPE0_1_HEADER_SIZE 0x40
> > +#define PCI_CAPS_START 0x34
> > +#define PCI_CFG_SPACE_SIZE 0x100
> > +#define PCIE_CFG_SPACE_SIZE 0x1000
------------ ^
Remove this, it is unused.

> > +
> > +enum pci_cap_id {
> > +	PCI_EXPRESS_CAP_ID = 0x10
> > +};
> > +
> > +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
> > +			   int start_offset);
> > +
> > +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id)
---- ^ ---- ^
Remove this, it is work for optimizitaion in compiler, no need
for it here.
Btw do you expect user to use both functions ?

+cc Marcin

Regards,
Kamil

> > +{
> > +	return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START);
> > +}
> > +
> > +#endif
> > diff --git a/lib/meson.build b/lib/meson.build
> > index cef2d0ff3d..e0fb7ddfed 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -33,6 +33,7 @@ lib_sources = [
> >   	'igt_pipe_crc.c',
> >   	'igt_power.c',
> >   	'igt_primes.c',
> > +	'igt_pci.c',
> >   	'igt_rand.c',
> >   	'igt_stats.c',
> >   	'igt_syncobj.c',

Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset

[Nilawar, Badal]

On 30-11-2022 18:13, Kamil Konieczny wrote:
> Hi,
> 
> On 2022-11-29 at 17:30:54 +0530, Nilawar, Badal wrote:
>> Hi Anshuman,
>>
>> On 23-11-2022 14:24, Anshuman Gupta wrote:
>>> Added helper functions to search for PCI capabilities
>>> with help of libpciaccess library.
>>>
>>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>> Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>> ---
>>>    lib/igt_pci.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>>    lib/igt_pci.h   | 32 ++++++++++++++++++++++++++++++++
>>>    lib/meson.build |  1 +
>>>    3 files changed, 77 insertions(+)
>>>    create mode 100644 lib/igt_pci.c
>>>    create mode 100644 lib/igt_pci.h
>>>
>>> diff --git a/lib/igt_pci.c b/lib/igt_pci.c
>>> new file mode 100644
>>> index 0000000000..bc0369341d
>>> --- /dev/null
>>> +++ b/lib/igt_pci.c
>>> @@ -0,0 +1,44 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2022 Intel Corporation
>>> + */
>>> +
>>> +#include <pciaccess.h>
>>> +#include "igt_pci.h"
>>> +
>>> +/**
>>> + * find_pci_cap_offset:
>>> + * @dev: pci device
>>> + * @cap_id: searched capability id, 0 means any capability
>>> + * @start_offset: offset in config space from which we start searching
>>> + *
>>> + * return:
>>> + * -1 on config read error, 0 if capability is not found,
>>> + * otherwise offset at which capability with cap_id is found
>>> + */
>>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
>>> +			   int start_offset)
>>> +{
>>> +	uint8_t offset;
>>> +	uint16_t cap_header;
>>> +	int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE)
>>> +			/ sizeof(cap_header);
>>> +
>>> +	if (pci_device_cfg_read_u8(dev, &offset, start_offset))
>>> +		return -1;
>>> +
>>> +	while (loop--) {
>>> +		if (offset < PCI_TYPE0_1_HEADER_SIZE)
>>> +			break;
>>> +
>>> +		if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC)))
>>> +			return -1;
>>> +
>>> +		if (!cap_id || cap_id == (cap_header & 0xFF))
>>> +			return offset;
>>> +
>>> +		offset = cap_header >> 8;
>> For the last capability, next capability address will always be 0. So above
>> instead of using while(loop--) we can use while(offset).
>>
>> Regards,
>> Badal
> 
> This way we are guarded for endless looping, btw check for zero
> is just at loop enter (first if).
Agreed.

It is seen that when config space inaccessible it throws value 0xff.
So inside loop we should check if offset or capid != 0xff and break the 
loop otherwise.
> 
>>> +	}
>>> +
> 
> I would like to see additional check here:
> 
> 	if (loop <= 0 && offset)
> 		return -1;
> 
> or maybe assert here ?
> 
>>> +	return 0;
>>> +}
>>> diff --git a/lib/igt_pci.h b/lib/igt_pci.h
>>> new file mode 100644
>>> index 0000000000..68afd2dacb
>>> --- /dev/null
>>> +++ b/lib/igt_pci.h
>>> @@ -0,0 +1,32 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2022 Intel Corporation
>>> + */
>>> +
>>> +#ifndef __IGT_PCI_H__
>>> +#define __IGT_PCI_H__
>>> +
>>> +#include <stdint.h>
>>> +#include <endian.h>
>>> +
>>> +/* forward declaration */
>>> +struct pci_device;
>>> +
>>> +#define PCI_TYPE0_1_HEADER_SIZE 0x40
>>> +#define PCI_CAPS_START 0x34
>>> +#define PCI_CFG_SPACE_SIZE 0x100
>>> +#define PCIE_CFG_SPACE_SIZE 0x1000
> ------------ ^
> Remove this, it is unused.
> 
>>> +
>>> +enum pci_cap_id {
>>> +	PCI_EXPRESS_CAP_ID = 0x10
>>> +};
>>> +
>>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
>>> +			   int start_offset);
>>> +
>>> +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id)
> ---- ^ ---- ^
> Remove this, it is work for optimizitaion in compiler, no need
> for it here.
> Btw do you expect user to use both functions ?
> 
> +cc Marcin
> 
> Regards,
> Kamil
> 
>>> +{
>>> +	return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START);
>>> +}
>>> +
>>> +#endif
>>> diff --git a/lib/meson.build b/lib/meson.build
>>> index cef2d0ff3d..e0fb7ddfed 100644
>>> --- a/lib/meson.build
>>> +++ b/lib/meson.build
>>> @@ -33,6 +33,7 @@ lib_sources = [
>>>    	'igt_pipe_crc.c',
>>>    	'igt_power.c',
>>>    	'igt_primes.c',
>>> +	'igt_pci.c',
>>>    	'igt_rand.c',
>>>    	'igt_stats.c',
>>>    	'igt_syncobj.c',

Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset

[Bernatowicz, Marcin]

On 11/30/2022 1:56 PM, Nilawar, Badal wrote:
> 
> 
> On 30-11-2022 18:13, Kamil Konieczny wrote:
>> Hi,
>>
>> On 2022-11-29 at 17:30:54 +0530, Nilawar, Badal wrote:
>>> Hi Anshuman,
>>>
>>> On 23-11-2022 14:24, Anshuman Gupta wrote:
>>>> Added helper functions to search for PCI capabilities
>>>> with help of libpciaccess library.
>>>>
>>>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>>> Co-developed-by: Marcin Bernatowicz 
>>>> <marcin.bernatowicz@linux.intel.com>
>>>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>>> ---
>>>>    lib/igt_pci.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>>>    lib/igt_pci.h   | 32 ++++++++++++++++++++++++++++++++
>>>>    lib/meson.build |  1 +
>>>>    3 files changed, 77 insertions(+)
>>>>    create mode 100644 lib/igt_pci.c
>>>>    create mode 100644 lib/igt_pci.h
>>>>
>>>> diff --git a/lib/igt_pci.c b/lib/igt_pci.c
>>>> new file mode 100644
>>>> index 0000000000..bc0369341d
>>>> --- /dev/null
>>>> +++ b/lib/igt_pci.c
>>>> @@ -0,0 +1,44 @@
>>>> +// SPDX-License-Identifier: MIT
>>>> +/*
>>>> + * Copyright © 2022 Intel Corporation
>>>> + */
>>>> +
>>>> +#include <pciaccess.h>
>>>> +#include "igt_pci.h"
>>>> +
>>>> +/**
>>>> + * find_pci_cap_offset:
>>>> + * @dev: pci device
>>>> + * @cap_id: searched capability id, 0 means any capability
>>>> + * @start_offset: offset in config space from which we start searching
>>>> + *
>>>> + * return:
>>>> + * -1 on config read error, 0 if capability is not found,
>>>> + * otherwise offset at which capability with cap_id is found
>>>> + */
>>>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id 
>>>> cap_id,
>>>> +               int start_offset)
>>>> +{
>>>> +    uint8_t offset;
>>>> +    uint16_t cap_header;
>>>> +    int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE)
>>>> +            / sizeof(cap_header);
>>>> +
>>>> +    if (pci_device_cfg_read_u8(dev, &offset, start_offset))
>>>> +        return -1;
>>>> +
>>>> +    while (loop--) {
>>>> +        if (offset < PCI_TYPE0_1_HEADER_SIZE)
>>>> +            break;
>>>> +
>>>> +        if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 
>>>> 0xFC)))
>>>> +            return -1;
>>>> +
>>>> +        if (!cap_id || cap_id == (cap_header & 0xFF))
>>>> +            return offset;
>>>> +
>>>> +        offset = cap_header >> 8;
>>> For the last capability, next capability address will always be 0. So 
>>> above
>>> instead of using while(loop--) we can use while(offset).
>>>
>>> Regards,
>>> Badal
>>
>> This way we are guarded for endless looping, btw check for zero
>> is just at loop enter (first if).
> Agreed.
> 
> It is seen that when config space inaccessible it throws value 0xff.
> So inside loop we should check if offset or capid != 0xff and break the 
> loop otherwise.

In such case I would expect pci_device_cfg_read_XXX to return != 0 => 
break the loop.
>>
>>>> +    }
>>>> +
>>
>> I would like to see additional check here:
>>
>>     if (loop <= 0 && offset)
>>         return -1;
>>
>> or maybe assert here ?
>>
>>>> +    return 0;
>>>> +}
>>>> diff --git a/lib/igt_pci.h b/lib/igt_pci.h
>>>> new file mode 100644
>>>> index 0000000000..68afd2dacb
>>>> --- /dev/null
>>>> +++ b/lib/igt_pci.h
>>>> @@ -0,0 +1,32 @@
>>>> +// SPDX-License-Identifier: MIT
>>>> +/*
>>>> + * Copyright © 2022 Intel Corporation
>>>> + */
>>>> +
>>>> +#ifndef __IGT_PCI_H__
>>>> +#define __IGT_PCI_H__
>>>> +
>>>> +#include <stdint.h>
>>>> +#include <endian.h>
>>>> +
>>>> +/* forward declaration */
>>>> +struct pci_device;
>>>> +
>>>> +#define PCI_TYPE0_1_HEADER_SIZE 0x40
>>>> +#define PCI_CAPS_START 0x34
>>>> +#define PCI_CFG_SPACE_SIZE 0x100
>>>> +#define PCIE_CFG_SPACE_SIZE 0x1000
>> ------------ ^
>> Remove this, it is unused.
>>
>>>> +
>>>> +enum pci_cap_id {
>>>> +    PCI_EXPRESS_CAP_ID = 0x10
>>>> +};
>>>> +
>>>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id 
>>>> cap_id,
>>>> +               int start_offset);
>>>> +
>>>> +static inline int find_pci_cap_offset(struct pci_device *dev, enum 
>>>> pci_cap_id cap_id)
>> ---- ^ ---- ^
>> Remove this, it is work for optimizitaion in compiler, no need
>> for it here.
>> Btw do you expect user to use both functions ?
>>
>> +cc Marcin
>>
>> Regards,
>> Kamil
>>
>>>> +{
>>>> +    return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START);
>>>> +}
>>>> +
>>>> +#endif
>>>> diff --git a/lib/meson.build b/lib/meson.build
>>>> index cef2d0ff3d..e0fb7ddfed 100644
>>>> --- a/lib/meson.build
>>>> +++ b/lib/meson.build
>>>> @@ -33,6 +33,7 @@ lib_sources = [
>>>>        'igt_pipe_crc.c',
>>>>        'igt_power.c',
>>>>        'igt_primes.c',
>>>> +    'igt_pci.c',
>>>>        'igt_rand.c',
>>>>        'igt_stats.c',
>>>>        'igt_syncobj.c',

Re: [igt-dev] [PATCH i-g-t v3 1/5] lib/igt_pci: helpers to get PCI capabilities offset

[Bernatowicz, Marcin]

On 11/30/2022 1:43 PM, Kamil Konieczny wrote:
> Hi,
> 
> On 2022-11-29 at 17:30:54 +0530, Nilawar, Badal wrote:
>> Hi Anshuman,
>>
>> On 23-11-2022 14:24, Anshuman Gupta wrote:
>>> Added helper functions to search for PCI capabilities
>>> with help of libpciaccess library.
>>>
>>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>> Co-developed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>> ---
>>>    lib/igt_pci.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>>    lib/igt_pci.h   | 32 ++++++++++++++++++++++++++++++++
>>>    lib/meson.build |  1 +
>>>    3 files changed, 77 insertions(+)
>>>    create mode 100644 lib/igt_pci.c
>>>    create mode 100644 lib/igt_pci.h
>>>
>>> diff --git a/lib/igt_pci.c b/lib/igt_pci.c
>>> new file mode 100644
>>> index 0000000000..bc0369341d
>>> --- /dev/null
>>> +++ b/lib/igt_pci.c
>>> @@ -0,0 +1,44 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2022 Intel Corporation
>>> + */
>>> +
>>> +#include <pciaccess.h>
>>> +#include "igt_pci.h"
>>> +
>>> +/**
>>> + * find_pci_cap_offset:
>>> + * @dev: pci device
>>> + * @cap_id: searched capability id, 0 means any capability
>>> + * @start_offset: offset in config space from which we start searching
>>> + *
>>> + * return:
>>> + * -1 on config read error, 0 if capability is not found,
>>> + * otherwise offset at which capability with cap_id is found
>>> + */
>>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
>>> +			   int start_offset)
>>> +{
>>> +	uint8_t offset;
>>> +	uint16_t cap_header;
>>> +	int loop = (PCI_CFG_SPACE_SIZE - PCI_TYPE0_1_HEADER_SIZE)
>>> +			/ sizeof(cap_header);
>>> +
>>> +	if (pci_device_cfg_read_u8(dev, &offset, start_offset))
>>> +		return -1;
>>> +
>>> +	while (loop--) {
>>> +		if (offset < PCI_TYPE0_1_HEADER_SIZE)
>>> +			break;
>>> +
>>> +		if (pci_device_cfg_read_u16(dev, &cap_header, (offset & 0xFC)))
>>> +			return -1;
>>> +
>>> +		if (!cap_id || cap_id == (cap_header & 0xFF))
>>> +			return offset;
>>> +
>>> +		offset = cap_header >> 8;
>> For the last capability, next capability address will always be 0. So above
>> instead of using while(loop--) we can use while(offset).
>>
>> Regards,
>> Badal
> 
> This way we are guarded for endless looping, btw check for zero
> is just at loop enter (first if).
> 
>>> +	}
>>> +
> 
> I would like to see additional check here:
> 
> 	if (loop <= 0 && offset)
> 		return -1;
> 
> or maybe assert here ?

why ? the -1 is on config read error

* return:
* -1 on config read error, 0 if capability is not found,
* otherwise offset at which capability with cap_id is found

> 
>>> +	return 0;
>>> +}
>>> diff --git a/lib/igt_pci.h b/lib/igt_pci.h
>>> new file mode 100644
>>> index 0000000000..68afd2dacb
>>> --- /dev/null
>>> +++ b/lib/igt_pci.h
>>> @@ -0,0 +1,32 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2022 Intel Corporation
>>> + */
>>> +
>>> +#ifndef __IGT_PCI_H__
>>> +#define __IGT_PCI_H__
>>> +
>>> +#include <stdint.h>
>>> +#include <endian.h>
>>> +
>>> +/* forward declaration */
>>> +struct pci_device;
>>> +
>>> +#define PCI_TYPE0_1_HEADER_SIZE 0x40
>>> +#define PCI_CAPS_START 0x34
>>> +#define PCI_CFG_SPACE_SIZE 0x100
>>> +#define PCIE_CFG_SPACE_SIZE 0x1000
> ------------ ^
> Remove this, it is unused.
> 
>>> +
>>> +enum pci_cap_id {
>>> +	PCI_EXPRESS_CAP_ID = 0x10
>>> +};
>>> +
>>> +int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
>>> +			   int start_offset);
>>> +
>>> +static inline int find_pci_cap_offset(struct pci_device *dev, enum pci_cap_id cap_id)
> ---- ^ ---- ^
> Remove this, it is work for optimizitaion in compiler, no need
> for it here.
> Btw do you expect user to use both functions ?
> 
> +cc Marcin
> 
> Regards,
> Kamil
> 
>>> +{
>>> +	return find_pci_cap_offset_at(dev, cap_id, PCI_CAPS_START);
>>> +}
>>> +
>>> +#endif
>>> diff --git a/lib/meson.build b/lib/meson.build
>>> index cef2d0ff3d..e0fb7ddfed 100644
>>> --- a/lib/meson.build
>>> +++ b/lib/meson.build
>>> @@ -33,6 +33,7 @@ lib_sources = [
>>>    	'igt_pipe_crc.c',
>>>    	'igt_power.c',
>>>    	'igt_primes.c',
>>> +	'igt_pci.c',
>>>    	'igt_rand.c',
>>>    	'igt_stats.c',
>>>    	'igt_syncobj.c',

[igt-dev] [PATCH i-g-t v3 2/5] lib/igt_pci: Add PCIe slot cap

[Anshuman Gupta]

Adding PCIe slot cap register offset and Power Controller Present
bit mask macros.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 lib/igt_pci.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/igt_pci.h b/lib/igt_pci.h
index 68afd2dacb..5c2863d657 100644
--- a/lib/igt_pci.h
+++ b/lib/igt_pci.h
@@ -21,6 +21,9 @@ enum pci_cap_id {
 	PCI_EXPRESS_CAP_ID = 0x10
 };
 
+#define PCI_SLOT_CAP_OFFSET 0x14  /* PCIe specs chapter 7.8.9 */
+#define  PCI_SLOT_PWR_CTRL_PRESENT (1 << 1)
+
 int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
 			   int start_offset);
 
-- 
2.25.1

Re: [igt-dev] [PATCH i-g-t v3 2/5] lib/igt_pci: Add PCIe slot cap

[Nilawar, Badal]

Hi Anshuman,

With a comment this is
Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
On 23-11-2022 14:24, Anshuman Gupta wrote:
> Adding PCIe slot cap register offset and Power Controller Present
> bit mask macros.
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   lib/igt_pci.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/lib/igt_pci.h b/lib/igt_pci.h
> index 68afd2dacb..5c2863d657 100644
> --- a/lib/igt_pci.h
> +++ b/lib/igt_pci.h
> @@ -21,6 +21,9 @@ enum pci_cap_id {
>   	PCI_EXPRESS_CAP_ID = 0x10
>   };
>   
> +#define PCI_SLOT_CAP_OFFSET 0x14  /* PCIe specs chapter 7.8.9 */
I think lets avoid adding comment about chapter number. Chapter number 
can change as specs keep updating.

Regards,
Badal
> +#define  PCI_SLOT_PWR_CTRL_PRESENT (1 << 1)
> +
>   int find_pci_cap_offset_at(struct pci_device *dev, enum pci_cap_id cap_id,
>   			   int start_offset);
>   

[igt-dev] [PATCH i-g-t v3 3/5] lib/igt_pm: Refactor get firmware_node fd

[Anshuman Gupta]

Created igt_pm_open_pci_firmware_node() to refactor
the retrieving the firmware_node fd code.

igt_pm_open_pci_firmware_node() will be used by other
firmware_node consumers.

While doing that fixed the leaked fd as well.

v2:
- return false instead of igt_require on no firmware_node_fd. [Kamil]
- use igt_assert() when failed to open 'real_power_state' on error
  other then ENONET.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 lib/igt_pm.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 1e6e9ed3ff..4f0cfbdd10 100644
--- a/lib/igt_pm.c
+++ b/lib/igt_pm.c
@@ -863,6 +863,20 @@ bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
 	return strstr(buf, "LPSP: capable");
 }
 
+static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
+{
+	char name[PATH_MAX];
+	int dir;
+
+	snprintf(name, PATH_MAX,
+		 "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/firmware_node",
+		 pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
+
+	dir = open(name, O_RDONLY);
+
+	return dir;
+}
+
 /**
  * igt_pm_acpi_d3cold_supported:
  * @pci_dev: root port pci_dev.
@@ -873,23 +887,23 @@ bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
  */
 bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev)
 {
-	char name[PATH_MAX];
-	int dir, fd;
-
-	snprintf(name, PATH_MAX,
-		 "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/firmware_node",
-		 pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
+	int firmware_node_fd, fd;
 
-	dir = open(name, O_RDONLY);
-	igt_require(dir > 0);
+	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
+	if (firmware_node_fd < 0)
+		return false;
 
 	/* BIOS need to enable ACPI D3Cold Support.*/
-	fd = openat(dir, "real_power_state", O_RDONLY);
-	if (fd < 0 && errno == ENOENT)
+	fd = openat(firmware_node_fd, "real_power_state", O_RDONLY);
+	if (fd < 0 && errno == ENOENT) {
+		close(firmware_node_fd);
 		return false;
+	}
 
-	igt_require(fd > 0);
+	igt_assert(errno > 0);
 
+	close(firmware_node_fd);
+	close(fd);
 	return true;
 }
 
-- 
2.25.1

Re: [igt-dev] [PATCH i-g-t v3 3/5] lib/igt_pm: Refactor get firmware_node fd

[Kamil Konieczny]

Hi Anshuman,

one small nit, see below.

On 2022-11-23 at 14:24:39 +0530, Anshuman Gupta wrote:
> Created igt_pm_open_pci_firmware_node() to refactor
> the retrieving the firmware_node fd code.
> 
> igt_pm_open_pci_firmware_node() will be used by other
> firmware_node consumers.
> 
> While doing that fixed the leaked fd as well.
> 
> v2:
> - return false instead of igt_require on no firmware_node_fd. [Kamil]
> - use igt_assert() when failed to open 'real_power_state' on error
>   other then ENONET.
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  lib/igt_pm.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 1e6e9ed3ff..4f0cfbdd10 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -863,6 +863,20 @@ bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
>  	return strstr(buf, "LPSP: capable");
>  }
>  
> +static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
> +{
> +	char name[PATH_MAX];
> +	int dir;
> +
> +	snprintf(name, PATH_MAX,
> +		 "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/firmware_node",
> +		 pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
> +
> +	dir = open(name, O_RDONLY);
> +
> +	return dir;
> +}
> +
>  /**
>   * igt_pm_acpi_d3cold_supported:
>   * @pci_dev: root port pci_dev.
> @@ -873,23 +887,23 @@ bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
>   */
>  bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev)
>  {
> -	char name[PATH_MAX];
> -	int dir, fd;
> -
> -	snprintf(name, PATH_MAX,
> -		 "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/firmware_node",
> -		 pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
> +	int firmware_node_fd, fd;
>  
> -	dir = open(name, O_RDONLY);
> -	igt_require(dir > 0);
> +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> +	if (firmware_node_fd < 0)
> +		return false;
>  
>  	/* BIOS need to enable ACPI D3Cold Support.*/
> -	fd = openat(dir, "real_power_state", O_RDONLY);
> -	if (fd < 0 && errno == ENOENT)
> +	fd = openat(firmware_node_fd, "real_power_state", O_RDONLY);
> +	if (fd < 0 && errno == ENOENT) {
> +		close(firmware_node_fd);
>  		return false;
> +	}
>  
> -	igt_require(fd > 0);
> +	igt_assert(errno > 0);

You want here no error, so
	igt_assert(errno == 0);
or better:
	igt_assert_f(fd > 0, "failed to open real_power_state, errno=%d\n", errno);

Regards,
Kamil

>  
> +	close(firmware_node_fd);
> +	close(fd);
>  	return true;
>  }
>  
> -- 
> 2.25.1
> 

Re: [igt-dev] [PATCH i-g-t v3 3/5] lib/igt_pm: Refactor get firmware_node fd

[Gupta, Anshuman]

> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Sent: Wednesday, November 30, 2022 2:14 PM
> To: igt-dev@lists.freedesktop.org
> Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>; kamil.konieczny@linux.intel.com; Tangudu, Tilak
> <tilak.tangudu@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> Iddamsetty, Aravind <aravind.iddamsetty@intel.com>
> Subject: Re: [PATCH i-g-t v3 3/5] lib/igt_pm: Refactor get firmware_node fd
> 
> Hi Anshuman,
> 
> one small nit, see below.
> 
> On 2022-11-23 at 14:24:39 +0530, Anshuman Gupta wrote:
> > Created igt_pm_open_pci_firmware_node() to refactor the retrieving the
> > firmware_node fd code.
> >
> > igt_pm_open_pci_firmware_node() will be used by other firmware_node
> > consumers.
> >
> > While doing that fixed the leaked fd as well.
> >
> > v2:
> > - return false instead of igt_require on no firmware_node_fd. [Kamil]
> > - use igt_assert() when failed to open 'real_power_state' on error
> >   other then ENONET.
> >
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  lib/igt_pm.c | 36 +++++++++++++++++++++++++-----------
> >  1 file changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/igt_pm.c b/lib/igt_pm.c index 1e6e9ed3ff..4f0cfbdd10
> > 100644
> > --- a/lib/igt_pm.c
> > +++ b/lib/igt_pm.c
> > @@ -863,6 +863,20 @@ bool i915_output_is_lpsp_capable(int drm_fd,
> igt_output_t *output)
> >  	return strstr(buf, "LPSP: capable");  }
> >
> > +static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
> > +{
> > +	char name[PATH_MAX];
> > +	int dir;
> > +
> > +	snprintf(name, PATH_MAX,
> > +
> "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/firmware_node",
> > +		 pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev-
> >func);
> > +
> > +	dir = open(name, O_RDONLY);
> > +
> > +	return dir;
> > +}
> > +
> >  /**
> >   * igt_pm_acpi_d3cold_supported:
> >   * @pci_dev: root port pci_dev.
> > @@ -873,23 +887,23 @@ bool i915_output_is_lpsp_capable(int drm_fd,
> igt_output_t *output)
> >   */
> >  bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev)  {
> > -	char name[PATH_MAX];
> > -	int dir, fd;
> > -
> > -	snprintf(name, PATH_MAX,
> > -
> "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/firmware_node",
> > -		 pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev-
> >func);
> > +	int firmware_node_fd, fd;
> >
> > -	dir = open(name, O_RDONLY);
> > -	igt_require(dir > 0);
> > +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> > +	if (firmware_node_fd < 0)
> > +		return false;
> >
> >  	/* BIOS need to enable ACPI D3Cold Support.*/
> > -	fd = openat(dir, "real_power_state", O_RDONLY);
> > -	if (fd < 0 && errno == ENOENT)
> > +	fd = openat(firmware_node_fd, "real_power_state", O_RDONLY);
> > +	if (fd < 0 && errno == ENOENT) {
> > +		close(firmware_node_fd);
> >  		return false;
> > +	}
> >
> > -	igt_require(fd > 0);
> > +	igt_assert(errno > 0);
> 
> You want here no error, so
> 	igt_assert(errno == 0);
> or better:
> 	igt_assert_f(fd > 0, "failed to open real_power_state, errno=%d\n",
> errno);
Thanks for pointing it out.
I will fix it.
BR,
Anshuman.
> 
> Regards,
> Kamil
> 
> >
> > +	close(firmware_node_fd);
> > +	close(fd);
> >  	return true;
> >  }
> >
> > --
> > 2.25.1
> >

[igt-dev] [PATCH i-g-t v3 4/5] test/device_reset: Refactor initiate_device_reset

[Anshuman Gupta]

Added a reset type enum to support multiple types
of reset like WARM, COLD and FLR reset.

No functional change.

v2:
- Removed WARM_RESET enum as not used yet. [Badal]

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/device_reset.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/tests/device_reset.c b/tests/device_reset.c
index e60d4c7fde..0c477a02c0 100644
--- a/tests/device_reset.c
+++ b/tests/device_reset.c
@@ -19,6 +19,11 @@ IGT_TEST_DESCRIPTION("Examine behavior of a driver on device sysfs reset");
 #define DEV_PATH_LEN 80
 #define DEV_BUS_ADDR_LEN 13 /* addr has form 0000:00:00.0 */
 
+enum reset {
+	COLD_RESET,
+	FLR_RESET
+};
+
 /**
  * Helper structure containing file descriptors
  * and bus address related to tested device
@@ -222,10 +227,13 @@ static void driver_bind(struct device_fds *dev)
 }
 
 /* Initiate device reset */
-static void initiate_device_reset(struct device_fds *dev)
+static void initiate_device_reset(struct device_fds *dev, enum reset type)
 {
 	igt_debug("reset device\n");
-	igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
+
+	if (type == FLR_RESET)
+		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
+
 }
 
 static bool is_i915_wedged(int i915)
@@ -274,14 +282,14 @@ static void set_device_filter(const char* dev_path)
 	igt_assert_eq(igt_device_filter_add(filter), 1);
 }
 
-static void unbind_reset_rebind(struct device_fds *dev)
+static void unbind_reset_rebind(struct device_fds *dev, enum reset type)
 {
 	igt_debug("close the device\n");
 	close_if_opened(&dev->fds.dev);
 
 	driver_unbind(dev);
 
-	initiate_device_reset(dev);
+	initiate_device_reset(dev, type);
 
 	driver_bind(dev);
 }
@@ -306,13 +314,13 @@ igt_main
 	igt_describe("Unbinds driver from device, initiates reset"
 		     " then rebinds driver to device");
 	igt_subtest("unbind-reset-rebind") {
-		unbind_reset_rebind(&dev);
+		unbind_reset_rebind(&dev, FLR_RESET);
 		healthcheck(&dev);
 	}
 
 	igt_describe("Resets device with bound driver");
 	igt_subtest("reset-bound") {
-		initiate_device_reset(&dev);
+		initiate_device_reset(&dev, FLR_RESET);
 		healthcheck(&dev);
 	}
 
-- 
2.25.1

[igt-dev] [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset IGT test

[Anshuman Gupta]

Add cold-reset IGT subtest, IGT subtest will use
/sys/bus/pci/slots/$SUN/power sysfs in order to
initiate a cold reset sequence. $SUN value will be
retrieved from PCIe device ACPI firmware node.

v2:
- %s/igt_require(fd > 0)/igt_assert(errno >0)
v3:
- Added Slot Power Controller capability check. [Aravind]
- Check whether slot is powered down before powering up. [Aravind]

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
---
 lib/igt_pm.c         |  40 +++++++++++
 lib/igt_pm.h         |   1 +
 tests/device_reset.c | 153 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 184 insertions(+), 10 deletions(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 4f0cfbdd10..9b7ed4be64 100644
--- a/lib/igt_pm.c
+++ b/lib/igt_pm.c
@@ -877,6 +877,46 @@ static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
 	return dir;
 }
 
+/**
+ * igt_pm_get_pcie_acpihp_slot:
+ * @pci_dev: pci bridge device.
+ * Get pci bridge acpi hotplug slot number, if bridge's ACPI firmware_node
+ * handle supports _SUN method.
+ *
+ * Returns:
+ * PCIe bridge Slot number.
+ * Returns -ENOENT number in case firmware_node/sun is not supported by the
+ * bridge.
+ */
+int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev)
+{
+	int firmware_node_fd, fd, n_read;
+	char sun[8];
+
+	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
+
+	if (firmware_node_fd < 0 && errno == ENOENT)
+		return -ENOENT;
+
+	igt_require(firmware_node_fd > 0);
+
+	fd = openat(firmware_node_fd, "sun", O_RDONLY);
+	if (fd < 0 && errno == ENOENT) {
+		close(firmware_node_fd);
+		return -ENOENT;
+	}
+
+	igt_assert(errno > 0);
+
+	n_read = read(fd, sun, sizeof(sun));
+	igt_assert(n_read > 0);
+
+	close(firmware_node_fd);
+	close(fd);
+
+	return strtol(sun, NULL, 10);
+}
+
 /**
  * igt_pm_acpi_d3cold_supported:
  * @pci_dev: root port pci_dev.
diff --git a/lib/igt_pm.h b/lib/igt_pm.h
index e81fceb897..f65b960c38 100644
--- a/lib/igt_pm.h
+++ b/lib/igt_pm.h
@@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
 bool igt_pm_dmc_loaded(int debugfs);
 bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
 bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
+int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
 bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
 enum igt_acpi_d_state
 igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev);
diff --git a/tests/device_reset.c b/tests/device_reset.c
index 0c477a02c0..4c624ce7d3 100644
--- a/tests/device_reset.c
+++ b/tests/device_reset.c
@@ -9,7 +9,9 @@
 
 #include "i915/gem.h"
 #include "igt.h"
+#include "igt_device.h"
 #include "igt_device_scan.h"
+#include "igt_pci.h"
 #include "igt_sysfs.h"
 #include "igt_kmod.h"
 
@@ -33,6 +35,7 @@ struct device_fds {
 		int dev;
 		int dev_dir;
 		int drv_dir;
+		int slot_dir; /* pci hotplug slots fd */
 	} fds;
 	char dev_bus_addr[DEV_BUS_ADDR_LEN];
 	bool snd_unload;
@@ -62,6 +65,45 @@ static int open_driver_sysfs_dir(int fd)
 	return __open_sysfs_dir(fd, "device/driver");
 }
 
+static int open_slot_sysfs_dir(int fd)
+{
+	struct pci_device *pci_dev = NULL;
+	int slot_fd = -1, slot;
+	char slot_fd_path[PATH_MAX];
+
+	pci_dev = igt_device_get_pci_device(fd);
+	igt_require(pci_dev);
+
+	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
+		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
+		if (slot == -ENOENT) {
+			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x does not support acpihp slot\n",
+				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
+			continue;
+		}
+
+		/*
+		 * Upon getting the valid acpihp slot number break the loop.
+		 * It is the desired acpihp slot for gfx card.
+		 */
+		if (slot > 0) {
+			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x associated acpihp slot %d\n",
+				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func, slot);
+			break;
+		}
+	}
+
+	if (!pci_dev)
+		return slot_fd;
+
+	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
+	slot_fd = open(slot_fd_path, O_RDONLY);
+	if (slot_fd < 0)
+		return -errno;
+
+	return slot_fd;
+}
+
 /**
  * device_sysfs_path:
  * @fd: opened device file descriptor
@@ -124,6 +166,8 @@ static void init_device_fds(struct device_fds *dev)
 
 	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
 	igt_assert_fd(dev->fds.drv_dir);
+
+	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
 }
 
 static int close_if_opened(int *fd)
@@ -142,6 +186,7 @@ static void cleanup_device_fds(struct device_fds *dev)
 	igt_ignore_warn(close_if_opened(&dev->fds.dev));
 	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
 	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
+	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
 }
 
 /**
@@ -179,6 +224,60 @@ static bool is_sysfs_reset_supported(int fd)
 	return true;
 }
 
+/**
+ * is_sysfs_cold_reset_supported:
+ * @fd: opened device file descriptor
+ *
+ * Check if device supports cold reset based on sysfs file presence.
+ *
+ * Returns:
+ * True if device supports reset, false otherwise.
+ */
+static bool is_sysfs_cold_reset_supported(int slot_fd)
+{
+	struct stat st;
+	int rc;
+	int cold_reset_fd = -1;
+
+	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
+
+	if (cold_reset_fd < 0)
+		return false;
+
+	rc = fstat(cold_reset_fd, &st);
+	close(cold_reset_fd);
+
+	if (rc || !S_ISREG(st.st_mode))
+		return false;
+
+	return true;
+}
+
+static bool is_pci_power_ctrl_present(struct pci_device *dev)
+{
+	int offset;
+	uint32_t slot_cap;
+
+	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
+	igt_require_f(offset > 0, "PCI Express Capability not found\n");
+	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset + PCI_SLOT_CAP_OFFSET));
+	igt_debug("slot cap register 0x%x\n", slot_cap);
+
+	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT;
+}
+
+static bool is_slot_power_ctrl_present(int fd)
+{
+	struct pci_device *root;
+
+	/*
+	 * Card root port Slot Capabilities Register
+	 * determines Power Controller Presence.
+	 */
+	root = igt_device_get_pci_root_port(fd);
+	return is_pci_power_ctrl_present(root);
+}
+
 /* Unbind the driver from the device */
 static void driver_unbind(struct device_fds *dev)
 {
@@ -231,8 +330,13 @@ static void initiate_device_reset(struct device_fds *dev, enum reset type)
 {
 	igt_debug("reset device\n");
 
-	if (type == FLR_RESET)
+	if (type == FLR_RESET) {
 		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
+	} else if (type == COLD_RESET) {
+		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
+		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir, "power"));
+		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
+	}
 
 }
 
@@ -311,17 +415,46 @@ igt_main
 		igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
 	}
 
-	igt_describe("Unbinds driver from device, initiates reset"
-		     " then rebinds driver to device");
-	igt_subtest("unbind-reset-rebind") {
-		unbind_reset_rebind(&dev, FLR_RESET);
-		healthcheck(&dev);
+	igt_subtest_group {
+		igt_describe("Unbinds driver from device, initiates reset"
+			     " then rebinds driver to device");
+		igt_subtest("unbind-reset-rebind") {
+			unbind_reset_rebind(&dev, FLR_RESET);
+			healthcheck(&dev);
+		}
+
+		igt_describe("Resets device with bound driver");
+		igt_subtest("reset-bound") {
+			initiate_device_reset(&dev, FLR_RESET);
+			/*
+			 * Cold reset will initiate card boot sequence again,
+			 * therefore let healthcheck() re-epen the dev fd.
+			 */
+			dev.fds.dev = -1;
+			healthcheck(&dev);
+		}
 	}
 
-	igt_describe("Resets device with bound driver");
-	igt_subtest("reset-bound") {
-		initiate_device_reset(&dev, FLR_RESET);
-		healthcheck(&dev);
+	igt_subtest_group {
+		igt_fixture {
+			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
+				      "pcie slot for cold reset\n");
+			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
+			igt_skip_on(!is_slot_power_ctrl_present(dev.fds.dev));
+		}
+
+		igt_describe("Unbinds driver from device, initiates cold reset"
+			     " then rebinds driver to device");
+		igt_subtest("unbind-cold-reset-rebind") {
+			unbind_reset_rebind(&dev, COLD_RESET);
+			healthcheck(&dev);
+		}
+
+		igt_describe("Cold Resets device with bound driver");
+		igt_subtest("cold-reset-bound") {
+			initiate_device_reset(&dev, COLD_RESET);
+			healthcheck(&dev);
+		}
 	}
 
 	igt_fixture {
-- 
2.25.1

Re: [igt-dev] [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset IGT test

[Iddamsetty, Aravind]

On 23-11-2022 14:24, Anshuman Gupta wrote:
> Add cold-reset IGT subtest, IGT subtest will use
> /sys/bus/pci/slots/$SUN/power sysfs in order to
> initiate a cold reset sequence. $SUN value will be
> retrieved from PCIe device ACPI firmware node.
> 
> v2:
> - %s/igt_require(fd > 0)/igt_assert(errno >0)
> v3:
> - Added Slot Power Controller capability check. [Aravind]
> - Check whether slot is powered down before powering up. [Aravind]
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>  lib/igt_pm.c         |  40 +++++++++++
>  lib/igt_pm.h         |   1 +
>  tests/device_reset.c | 153 ++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 184 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 4f0cfbdd10..9b7ed4be64 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -877,6 +877,46 @@ static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
>  	return dir;
>  }
>  
> +/**
> + * igt_pm_get_pcie_acpihp_slot:
> + * @pci_dev: pci bridge device.
> + * Get pci bridge acpi hotplug slot number, if bridge's ACPI firmware_node
> + * handle supports _SUN method.
> + *
> + * Returns:
> + * PCIe bridge Slot number.
> + * Returns -ENOENT number in case firmware_node/sun is not supported by the
> + * bridge.
> + */
> +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev)
> +{
> +	int firmware_node_fd, fd, n_read;
> +	char sun[8];
> +
> +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> +
> +	if (firmware_node_fd < 0 && errno == ENOENT)
> +		return -ENOENT;
> +
> +	igt_require(firmware_node_fd > 0);
> +
> +	fd = openat(firmware_node_fd, "sun", O_RDONLY);
> +	if (fd < 0 && errno == ENOENT) {
> +		close(firmware_node_fd);
> +		return -ENOENT;
> +	}
> +
> +	igt_assert(errno > 0);
> +
> +	n_read = read(fd, sun, sizeof(sun));
> +	igt_assert(n_read > 0);
> +
> +	close(firmware_node_fd);
> +	close(fd);
> +
> +	return strtol(sun, NULL, 10);
> +}
> +
>  /**
>   * igt_pm_acpi_d3cold_supported:
>   * @pci_dev: root port pci_dev.
> diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> index e81fceb897..f65b960c38 100644
> --- a/lib/igt_pm.h
> +++ b/lib/igt_pm.h
> @@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
>  bool igt_pm_dmc_loaded(int debugfs);
>  bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
>  bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
> +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
>  bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
>  enum igt_acpi_d_state
>  igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev);
> diff --git a/tests/device_reset.c b/tests/device_reset.c
> index 0c477a02c0..4c624ce7d3 100644
> --- a/tests/device_reset.c
> +++ b/tests/device_reset.c
> @@ -9,7 +9,9 @@
>  
>  #include "i915/gem.h"
>  #include "igt.h"
> +#include "igt_device.h"
>  #include "igt_device_scan.h"
> +#include "igt_pci.h"
>  #include "igt_sysfs.h"
>  #include "igt_kmod.h"
>  
> @@ -33,6 +35,7 @@ struct device_fds {
>  		int dev;
>  		int dev_dir;
>  		int drv_dir;
> +		int slot_dir; /* pci hotplug slots fd */
>  	} fds;
>  	char dev_bus_addr[DEV_BUS_ADDR_LEN];
>  	bool snd_unload;
> @@ -62,6 +65,45 @@ static int open_driver_sysfs_dir(int fd)
>  	return __open_sysfs_dir(fd, "device/driver");
>  }
>  
> +static int open_slot_sysfs_dir(int fd)
> +{
> +	struct pci_device *pci_dev = NULL;
> +	int slot_fd = -1, slot;
> +	char slot_fd_path[PATH_MAX];
> +
> +	pci_dev = igt_device_get_pci_device(fd);
> +	igt_require(pci_dev);
> +
> +	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
> +		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
> +		if (slot == -ENOENT) {
> +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x does not support acpihp slot\n",
> +				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
> +			continue;
> +		}
> +
> +		/*
> +		 * Upon getting the valid acpihp slot number break the loop.
> +		 * It is the desired acpihp slot for gfx card.
> +		 */
> +		if (slot > 0) {
> +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x associated acpihp slot %d\n",
> +				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func, slot);
> +			break;
> +		}
> +	}
> +
> +	if (!pci_dev)
> +		return slot_fd;
> +
> +	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
> +	slot_fd = open(slot_fd_path, O_RDONLY);
> +	if (slot_fd < 0)
> +		return -errno;
> +
> +	return slot_fd;
> +}
> +
>  /**
>   * device_sysfs_path:
>   * @fd: opened device file descriptor
> @@ -124,6 +166,8 @@ static void init_device_fds(struct device_fds *dev)
>  
>  	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
>  	igt_assert_fd(dev->fds.drv_dir);
> +
> +	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
>  }
>  
>  static int close_if_opened(int *fd)
> @@ -142,6 +186,7 @@ static void cleanup_device_fds(struct device_fds *dev)
>  	igt_ignore_warn(close_if_opened(&dev->fds.dev));
>  	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
>  	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
> +	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
>  }
>  
>  /**
> @@ -179,6 +224,60 @@ static bool is_sysfs_reset_supported(int fd)
>  	return true;
>  }
>  
> +/**
> + * is_sysfs_cold_reset_supported:
> + * @fd: opened device file descriptor
> + *
> + * Check if device supports cold reset based on sysfs file presence.
> + *
> + * Returns:
> + * True if device supports reset, false otherwise.
> + */
> +static bool is_sysfs_cold_reset_supported(int slot_fd)
> +{
> +	struct stat st;
> +	int rc;
> +	int cold_reset_fd = -1;
> +
> +	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
> +
> +	if (cold_reset_fd < 0)
> +		return false;
> +
> +	rc = fstat(cold_reset_fd, &st);
> +	close(cold_reset_fd);
> +
> +	if (rc || !S_ISREG(st.st_mode))
> +		return false;
> +
> +	return true;
> +}
> +

why do not you combine the below two checks into
is_sysfs_cold_reset_supported as the check will be done only if power
sysfs is present.

Thanks,
Aravind.
> +static bool is_pci_power_ctrl_present(struct pci_device *dev)
> +{
> +	int offset;
> +	uint32_t slot_cap;
> +
> +	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
> +	igt_require_f(offset > 0, "PCI Express Capability not found\n");
> +	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset + PCI_SLOT_CAP_OFFSET));
> +	igt_debug("slot cap register 0x%x\n", slot_cap);
> +
> +	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT;
> +}
> +
> +static bool is_slot_power_ctrl_present(int fd)
> +{
> +	struct pci_device *root;
> +
> +	/*
> +	 * Card root port Slot Capabilities Register
> +	 * determines Power Controller Presence.
> +	 */
> +	root = igt_device_get_pci_root_port(fd);
> +	return is_pci_power_ctrl_present(root);
> +}
> +
>  /* Unbind the driver from the device */
>  static void driver_unbind(struct device_fds *dev)
>  {
> @@ -231,8 +330,13 @@ static void initiate_device_reset(struct device_fds *dev, enum reset type)
>  {
>  	igt_debug("reset device\n");
>  
> -	if (type == FLR_RESET)
> +	if (type == FLR_RESET) {
>  		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> +	} else if (type == COLD_RESET) {
> +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
> +		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir, "power"));
> +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
> +	}
>  
>  }
>  
> @@ -311,17 +415,46 @@ igt_main
>  		igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
>  	}
>  
> -	igt_describe("Unbinds driver from device, initiates reset"
> -		     " then rebinds driver to device");
> -	igt_subtest("unbind-reset-rebind") {
> -		unbind_reset_rebind(&dev, FLR_RESET);
> -		healthcheck(&dev);
> +	igt_subtest_group {
> +		igt_describe("Unbinds driver from device, initiates reset"
> +			     " then rebinds driver to device");
> +		igt_subtest("unbind-reset-rebind") {
> +			unbind_reset_rebind(&dev, FLR_RESET);
> +			healthcheck(&dev);
> +		}
> +
> +		igt_describe("Resets device with bound driver");
> +		igt_subtest("reset-bound") {
> +			initiate_device_reset(&dev, FLR_RESET);
> +			/*
> +			 * Cold reset will initiate card boot sequence again,
> +			 * therefore let healthcheck() re-epen the dev fd.
> +			 */
> +			dev.fds.dev = -1;
> +			healthcheck(&dev);
> +		}
>  	}
>  
> -	igt_describe("Resets device with bound driver");
> -	igt_subtest("reset-bound") {
> -		initiate_device_reset(&dev, FLR_RESET);
> -		healthcheck(&dev);
> +	igt_subtest_group {
> +		igt_fixture {
> +			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
> +				      "pcie slot for cold reset\n");
> +			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> +			igt_skip_on(!is_slot_power_ctrl_present(dev.fds.dev));
> +		}
> +
> +		igt_describe("Unbinds driver from device, initiates cold reset"
> +			     " then rebinds driver to device");
> +		igt_subtest("unbind-cold-reset-rebind") {
> +			unbind_reset_rebind(&dev, COLD_RESET);
> +			healthcheck(&dev);
> +		}
> +
> +		igt_describe("Cold Resets device with bound driver");
> +		igt_subtest("cold-reset-bound") {
> +			initiate_device_reset(&dev, COLD_RESET);
> +			healthcheck(&dev);
> +		}
>  	}
>  
>  	igt_fixture {

Re: [igt-dev] [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset IGT test

[Gupta, Anshuman]

> -----Original Message-----
> From: Iddamsetty, Aravind <aravind.iddamsetty@intel.com>
> Sent: Friday, November 25, 2022 9:57 AM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>; igt-
> dev@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; kamil.konieczny@linux.intel.com;
> Tangudu, Tilak <tilak.tangudu@intel.com>; Nilawar, Badal
> <badal.nilawar@intel.com>
> Subject: Re: [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset IGT test
> 
> 
> 
> On 23-11-2022 14:24, Anshuman Gupta wrote:
> > Add cold-reset IGT subtest, IGT subtest will use
> > /sys/bus/pci/slots/$SUN/power sysfs in order to initiate a cold reset
> > sequence. $SUN value will be retrieved from PCIe device ACPI firmware
> > node.
> >
> > v2:
> > - %s/igt_require(fd > 0)/igt_assert(errno >0)
> > v3:
> > - Added Slot Power Controller capability check. [Aravind]
> > - Check whether slot is powered down before powering up. [Aravind]
> >
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
> > ---
> >  lib/igt_pm.c         |  40 +++++++++++
> >  lib/igt_pm.h         |   1 +
> >  tests/device_reset.c | 153
> > ++++++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 184 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/igt_pm.c b/lib/igt_pm.c index 4f0cfbdd10..9b7ed4be64
> > 100644
> > --- a/lib/igt_pm.c
> > +++ b/lib/igt_pm.c
> > @@ -877,6 +877,46 @@ static int igt_pm_open_pci_firmware_node(struct
> pci_device *pci_dev)
> >  	return dir;
> >  }
> >
> > +/**
> > + * igt_pm_get_pcie_acpihp_slot:
> > + * @pci_dev: pci bridge device.
> > + * Get pci bridge acpi hotplug slot number, if bridge's ACPI
> > +firmware_node
> > + * handle supports _SUN method.
> > + *
> > + * Returns:
> > + * PCIe bridge Slot number.
> > + * Returns -ENOENT number in case firmware_node/sun is not supported
> > +by the
> > + * bridge.
> > + */
> > +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev) {
> > +	int firmware_node_fd, fd, n_read;
> > +	char sun[8];
> > +
> > +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> > +
> > +	if (firmware_node_fd < 0 && errno == ENOENT)
> > +		return -ENOENT;
> > +
> > +	igt_require(firmware_node_fd > 0);
> > +
> > +	fd = openat(firmware_node_fd, "sun", O_RDONLY);
> > +	if (fd < 0 && errno == ENOENT) {
> > +		close(firmware_node_fd);
> > +		return -ENOENT;
> > +	}
> > +
> > +	igt_assert(errno > 0);
> > +
> > +	n_read = read(fd, sun, sizeof(sun));
> > +	igt_assert(n_read > 0);
> > +
> > +	close(firmware_node_fd);
> > +	close(fd);
> > +
> > +	return strtol(sun, NULL, 10);
> > +}
> > +
> >  /**
> >   * igt_pm_acpi_d3cold_supported:
> >   * @pci_dev: root port pci_dev.
> > diff --git a/lib/igt_pm.h b/lib/igt_pm.h index e81fceb897..f65b960c38
> > 100644
> > --- a/lib/igt_pm.h
> > +++ b/lib/igt_pm.h
> > @@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum
> > igt_runtime_pm_status status);  bool igt_pm_dmc_loaded(int debugfs);
> > bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);  bool
> > i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
> > +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
> >  bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);  enum
> > igt_acpi_d_state  igt_pm_get_acpi_real_d_state(struct pci_device
> > *pci_dev); diff --git a/tests/device_reset.c b/tests/device_reset.c
> > index 0c477a02c0..4c624ce7d3 100644
> > --- a/tests/device_reset.c
> > +++ b/tests/device_reset.c
> > @@ -9,7 +9,9 @@
> >
> >  #include "i915/gem.h"
> >  #include "igt.h"
> > +#include "igt_device.h"
> >  #include "igt_device_scan.h"
> > +#include "igt_pci.h"
> >  #include "igt_sysfs.h"
> >  #include "igt_kmod.h"
> >
> > @@ -33,6 +35,7 @@ struct device_fds {
> >  		int dev;
> >  		int dev_dir;
> >  		int drv_dir;
> > +		int slot_dir; /* pci hotplug slots fd */
> >  	} fds;
> >  	char dev_bus_addr[DEV_BUS_ADDR_LEN];
> >  	bool snd_unload;
> > @@ -62,6 +65,45 @@ static int open_driver_sysfs_dir(int fd)
> >  	return __open_sysfs_dir(fd, "device/driver");  }
> >
> > +static int open_slot_sysfs_dir(int fd) {
> > +	struct pci_device *pci_dev = NULL;
> > +	int slot_fd = -1, slot;
> > +	char slot_fd_path[PATH_MAX];
> > +
> > +	pci_dev = igt_device_get_pci_device(fd);
> > +	igt_require(pci_dev);
> > +
> > +	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
> > +		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
> > +		if (slot == -ENOENT) {
> > +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x
> does not support acpihp slot\n",
> > +				  pci_dev->domain, pci_dev->bus, pci_dev-
> >dev, pci_dev->func);
> > +			continue;
> > +		}
> > +
> > +		/*
> > +		 * Upon getting the valid acpihp slot number break the loop.
> > +		 * It is the desired acpihp slot for gfx card.
> > +		 */
> > +		if (slot > 0) {
> > +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x
> associated acpihp slot %d\n",
> > +				  pci_dev->domain, pci_dev->bus, pci_dev-
> >dev, pci_dev->func, slot);
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!pci_dev)
> > +		return slot_fd;
> > +
> > +	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
> > +	slot_fd = open(slot_fd_path, O_RDONLY);
> > +	if (slot_fd < 0)
> > +		return -errno;
> > +
> > +	return slot_fd;
> > +}
> > +
> >  /**
> >   * device_sysfs_path:
> >   * @fd: opened device file descriptor @@ -124,6 +166,8 @@ static void
> > init_device_fds(struct device_fds *dev)
> >
> >  	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
> >  	igt_assert_fd(dev->fds.drv_dir);
> > +
> > +	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
> >  }
> >
> >  static int close_if_opened(int *fd)
> > @@ -142,6 +186,7 @@ static void cleanup_device_fds(struct device_fds
> *dev)
> >  	igt_ignore_warn(close_if_opened(&dev->fds.dev));
> >  	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
> >  	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
> > +	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
> >  }
> >
> >  /**
> > @@ -179,6 +224,60 @@ static bool is_sysfs_reset_supported(int fd)
> >  	return true;
> >  }
> >
> > +/**
> > + * is_sysfs_cold_reset_supported:
> > + * @fd: opened device file descriptor
> > + *
> > + * Check if device supports cold reset based on sysfs file presence.
> > + *
> > + * Returns:
> > + * True if device supports reset, false otherwise.
> > + */
> > +static bool is_sysfs_cold_reset_supported(int slot_fd) {
> > +	struct stat st;
> > +	int rc;
> > +	int cold_reset_fd = -1;
> > +
> > +	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
> > +
> > +	if (cold_reset_fd < 0)
> > +		return false;
> > +
> > +	rc = fstat(cold_reset_fd, &st);
> > +	close(cold_reset_fd);
> > +
> > +	if (rc || !S_ISREG(st.st_mode))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> 
> why do not you combine the below two checks into
> is_sysfs_cold_reset_supported as the check will be done only if power sysfs
> is present.
Thanks for review after this comment, I am pondering that  it would be better to check the 
is_slot_power_ctrl_present()  even before checking for slot number to save the CI time.
As without power controller capability it is in vain to check for slot number. 
 igt_skip_on_f(!is_slot_power_ctrl_present(dev.fds.dev), "Gfx Card root port does not support power ctrl");
 igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
				      "pcie slot for cold reset\n");
 igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
 igt_skip_on(!is_slot_power_ctrl_present(dev.fds.dev));

BR,
Anshuman Gupta.
> 
> Thanks,
> Aravind.
> > +static bool is_pci_power_ctrl_present(struct pci_device *dev) {
> > +	int offset;
> > +	uint32_t slot_cap;
> > +
> > +	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
> > +	igt_require_f(offset > 0, "PCI Express Capability not found\n");
> > +	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset +
> PCI_SLOT_CAP_OFFSET));
> > +	igt_debug("slot cap register 0x%x\n", slot_cap);
> > +
> > +	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT; }
> > +
> > +static bool is_slot_power_ctrl_present(int fd) {
> > +	struct pci_device *root;
> > +
> > +	/*
> > +	 * Card root port Slot Capabilities Register
> > +	 * determines Power Controller Presence.
> > +	 */
> > +	root = igt_device_get_pci_root_port(fd);
> > +	return is_pci_power_ctrl_present(root); }
> > +
> >  /* Unbind the driver from the device */  static void
> > driver_unbind(struct device_fds *dev)  { @@ -231,8 +330,13 @@ static
> > void initiate_device_reset(struct device_fds *dev, enum reset type)  {
> >  	igt_debug("reset device\n");
> >
> > -	if (type == FLR_RESET)
> > +	if (type == FLR_RESET) {
> >  		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> > +	} else if (type == COLD_RESET) {
> > +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
> > +		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir,
> "power"));
> > +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
> > +	}
> >
> >  }
> >
> > @@ -311,17 +415,46 @@ igt_main
> >  		igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
> >  	}
> >
> > -	igt_describe("Unbinds driver from device, initiates reset"
> > -		     " then rebinds driver to device");
> > -	igt_subtest("unbind-reset-rebind") {
> > -		unbind_reset_rebind(&dev, FLR_RESET);
> > -		healthcheck(&dev);
> > +	igt_subtest_group {
> > +		igt_describe("Unbinds driver from device, initiates reset"
> > +			     " then rebinds driver to device");
> > +		igt_subtest("unbind-reset-rebind") {
> > +			unbind_reset_rebind(&dev, FLR_RESET);
> > +			healthcheck(&dev);
> > +		}
> > +
> > +		igt_describe("Resets device with bound driver");
> > +		igt_subtest("reset-bound") {
> > +			initiate_device_reset(&dev, FLR_RESET);
> > +			/*
> > +			 * Cold reset will initiate card boot sequence again,
> > +			 * therefore let healthcheck() re-epen the dev fd.
> > +			 */
> > +			dev.fds.dev = -1;
> > +			healthcheck(&dev);
> > +		}
> >  	}
> >
> > -	igt_describe("Resets device with bound driver");
> > -	igt_subtest("reset-bound") {
> > -		initiate_device_reset(&dev, FLR_RESET);
> > -		healthcheck(&dev);
> > +	igt_subtest_group {
> > +		igt_fixture {
> > +			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does
> not support any "
> > +				      "pcie slot for cold reset\n");
> > +
> 	igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> > +
> 	igt_skip_on(!is_slot_power_ctrl_present(dev.fds.dev));
> > +		}
> > +
> > +		igt_describe("Unbinds driver from device, initiates cold
> reset"
> > +			     " then rebinds driver to device");
> > +		igt_subtest("unbind-cold-reset-rebind") {
> > +			unbind_reset_rebind(&dev, COLD_RESET);
> > +			healthcheck(&dev);
> > +		}
> > +
> > +		igt_describe("Cold Resets device with bound driver");
> > +		igt_subtest("cold-reset-bound") {
> > +			initiate_device_reset(&dev, COLD_RESET);
> > +			healthcheck(&dev);
> > +		}
> >  	}
> >
> >  	igt_fixture {

Re: [igt-dev] [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset IGT test

[Iddamsetty, Aravind]

> -----Original Message-----
> From: Gupta, Anshuman <anshuman.gupta@intel.com>
> Sent: Friday, November 25, 2022 10:24 AM
> To: Iddamsetty, Aravind <aravind.iddamsetty@intel.com>; igt-
> dev@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; kamil.konieczny@linux.intel.com;
> Tangudu, Tilak <tilak.tangudu@intel.com>; Nilawar, Badal
> <badal.nilawar@intel.com>
> Subject: RE: [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset IGT test
> 
> 
> 
> > -----Original Message-----
> > From: Iddamsetty, Aravind <aravind.iddamsetty@intel.com>
> > Sent: Friday, November 25, 2022 9:57 AM
> > To: Gupta, Anshuman <anshuman.gupta@intel.com>; igt-
> > dev@lists.freedesktop.org
> > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> > kamil.konieczny@linux.intel.com; Tangudu, Tilak
> > <tilak.tangudu@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>
> > Subject: Re: [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset
> > IGT test
> >
> >
> >
> > On 23-11-2022 14:24, Anshuman Gupta wrote:
> > > Add cold-reset IGT subtest, IGT subtest will use
> > > /sys/bus/pci/slots/$SUN/power sysfs in order to initiate a cold
> > > reset sequence. $SUN value will be retrieved from PCIe device ACPI
> > > firmware node.
> > >
> > > v2:
> > > - %s/igt_require(fd > 0)/igt_assert(errno >0)
> > > v3:
> > > - Added Slot Power Controller capability check. [Aravind]
> > > - Check whether slot is powered down before powering up. [Aravind]
> > >
> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
> > > ---
> > >  lib/igt_pm.c         |  40 +++++++++++
> > >  lib/igt_pm.h         |   1 +
> > >  tests/device_reset.c | 153
> > > ++++++++++++++++++++++++++++++++++++++++---
> > >  3 files changed, 184 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/lib/igt_pm.c b/lib/igt_pm.c index
> > > 4f0cfbdd10..9b7ed4be64
> > > 100644
> > > --- a/lib/igt_pm.c
> > > +++ b/lib/igt_pm.c
> > > @@ -877,6 +877,46 @@ static int
> igt_pm_open_pci_firmware_node(struct
> > pci_device *pci_dev)
> > >  	return dir;
> > >  }
> > >
> > > +/**
> > > + * igt_pm_get_pcie_acpihp_slot:
> > > + * @pci_dev: pci bridge device.
> > > + * Get pci bridge acpi hotplug slot number, if bridge's ACPI
> > > +firmware_node
> > > + * handle supports _SUN method.
> > > + *
> > > + * Returns:
> > > + * PCIe bridge Slot number.
> > > + * Returns -ENOENT number in case firmware_node/sun is not
> > > +supported by the
> > > + * bridge.
> > > + */
> > > +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev) {
> > > +	int firmware_node_fd, fd, n_read;
> > > +	char sun[8];
> > > +
> > > +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> > > +
> > > +	if (firmware_node_fd < 0 && errno == ENOENT)
> > > +		return -ENOENT;
> > > +
> > > +	igt_require(firmware_node_fd > 0);
> > > +
> > > +	fd = openat(firmware_node_fd, "sun", O_RDONLY);
> > > +	if (fd < 0 && errno == ENOENT) {
> > > +		close(firmware_node_fd);
> > > +		return -ENOENT;
> > > +	}
> > > +
> > > +	igt_assert(errno > 0);
> > > +
> > > +	n_read = read(fd, sun, sizeof(sun));
> > > +	igt_assert(n_read > 0);
> > > +
> > > +	close(firmware_node_fd);
> > > +	close(fd);
> > > +
> > > +	return strtol(sun, NULL, 10);
> > > +}
> > > +
> > >  /**
> > >   * igt_pm_acpi_d3cold_supported:
> > >   * @pci_dev: root port pci_dev.
> > > diff --git a/lib/igt_pm.h b/lib/igt_pm.h index
> > > e81fceb897..f65b960c38
> > > 100644
> > > --- a/lib/igt_pm.h
> > > +++ b/lib/igt_pm.h
> > > @@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum
> > > igt_runtime_pm_status status);  bool igt_pm_dmc_loaded(int debugfs);
> > > bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);  bool
> > > i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
> > > +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
> > >  bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
> > > enum igt_acpi_d_state  igt_pm_get_acpi_real_d_state(struct
> > > pci_device *pci_dev); diff --git a/tests/device_reset.c
> > > b/tests/device_reset.c index 0c477a02c0..4c624ce7d3 100644
> > > --- a/tests/device_reset.c
> > > +++ b/tests/device_reset.c
> > > @@ -9,7 +9,9 @@
> > >
> > >  #include "i915/gem.h"
> > >  #include "igt.h"
> > > +#include "igt_device.h"
> > >  #include "igt_device_scan.h"
> > > +#include "igt_pci.h"
> > >  #include "igt_sysfs.h"
> > >  #include "igt_kmod.h"
> > >
> > > @@ -33,6 +35,7 @@ struct device_fds {
> > >  		int dev;
> > >  		int dev_dir;
> > >  		int drv_dir;
> > > +		int slot_dir; /* pci hotplug slots fd */
> > >  	} fds;
> > >  	char dev_bus_addr[DEV_BUS_ADDR_LEN];
> > >  	bool snd_unload;
> > > @@ -62,6 +65,45 @@ static int open_driver_sysfs_dir(int fd)
> > >  	return __open_sysfs_dir(fd, "device/driver");  }
> > >
> > > +static int open_slot_sysfs_dir(int fd) {
> > > +	struct pci_device *pci_dev = NULL;
> > > +	int slot_fd = -1, slot;
> > > +	char slot_fd_path[PATH_MAX];
> > > +
> > > +	pci_dev = igt_device_get_pci_device(fd);
> > > +	igt_require(pci_dev);
> > > +
> > > +	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
> > > +		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
> > > +		if (slot == -ENOENT) {
> > > +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x
> > does not support acpihp slot\n",
> > > +				  pci_dev->domain, pci_dev->bus, pci_dev-
> > >dev, pci_dev->func);
> > > +			continue;
> > > +		}
> > > +
> > > +		/*
> > > +		 * Upon getting the valid acpihp slot number break the loop.
> > > +		 * It is the desired acpihp slot for gfx card.
> > > +		 */
> > > +		if (slot > 0) {
> > > +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x
> > associated acpihp slot %d\n",
> > > +				  pci_dev->domain, pci_dev->bus, pci_dev-
> > >dev, pci_dev->func, slot);
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (!pci_dev)
> > > +		return slot_fd;
> > > +
> > > +	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
> > > +	slot_fd = open(slot_fd_path, O_RDONLY);
> > > +	if (slot_fd < 0)
> > > +		return -errno;
> > > +
> > > +	return slot_fd;
> > > +}
> > > +
> > >  /**
> > >   * device_sysfs_path:
> > >   * @fd: opened device file descriptor @@ -124,6 +166,8 @@ static
> > > void init_device_fds(struct device_fds *dev)
> > >
> > >  	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
> > >  	igt_assert_fd(dev->fds.drv_dir);
> > > +
> > > +	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
> > >  }
> > >
> > >  static int close_if_opened(int *fd) @@ -142,6 +186,7 @@ static void
> > > cleanup_device_fds(struct device_fds
> > *dev)
> > >  	igt_ignore_warn(close_if_opened(&dev->fds.dev));
> > >  	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
> > >  	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
> > > +	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
> > >  }
> > >
> > >  /**
> > > @@ -179,6 +224,60 @@ static bool is_sysfs_reset_supported(int fd)
> > >  	return true;
> > >  }
> > >
> > > +/**
> > > + * is_sysfs_cold_reset_supported:
> > > + * @fd: opened device file descriptor
> > > + *
> > > + * Check if device supports cold reset based on sysfs file presence.
> > > + *
> > > + * Returns:
> > > + * True if device supports reset, false otherwise.
> > > + */
> > > +static bool is_sysfs_cold_reset_supported(int slot_fd) {
> > > +	struct stat st;
> > > +	int rc;
> > > +	int cold_reset_fd = -1;
> > > +
> > > +	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
> > > +
> > > +	if (cold_reset_fd < 0)
> > > +		return false;
> > > +
> > > +	rc = fstat(cold_reset_fd, &st);
> > > +	close(cold_reset_fd);
> > > +
> > > +	if (rc || !S_ISREG(st.st_mode))
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> >
> > why do not you combine the below two checks into
> > is_sysfs_cold_reset_supported as the check will be done only if power
> > sysfs is present.
> Thanks for review after this comment, I am pondering that  it would be better
> to check the
> is_slot_power_ctrl_present()  even before checking for slot number to save
> the CI time.
> As without power controller capability it is in vain to check for slot number.
>  igt_skip_on_f(!is_slot_power_ctrl_present(dev.fds.dev), "Gfx Card root port
> does not support power ctrl");  igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card
> does not support any "
> 				      "pcie slot for cold reset\n");
> igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
>  igt_skip_on(!is_slot_power_ctrl_present(dev.fds.dev));

That's true.

Regards,
Aravind.
> 
> BR,
> Anshuman Gupta.
> >
> > Thanks,
> > Aravind.
> > > +static bool is_pci_power_ctrl_present(struct pci_device *dev) {
> > > +	int offset;
> > > +	uint32_t slot_cap;
> > > +
> > > +	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
> > > +	igt_require_f(offset > 0, "PCI Express Capability not found\n");
> > > +	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset +
> > PCI_SLOT_CAP_OFFSET));
> > > +	igt_debug("slot cap register 0x%x\n", slot_cap);
> > > +
> > > +	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT; }
> > > +
> > > +static bool is_slot_power_ctrl_present(int fd) {
> > > +	struct pci_device *root;
> > > +
> > > +	/*
> > > +	 * Card root port Slot Capabilities Register
> > > +	 * determines Power Controller Presence.
> > > +	 */
> > > +	root = igt_device_get_pci_root_port(fd);
> > > +	return is_pci_power_ctrl_present(root); }
> > > +
> > >  /* Unbind the driver from the device */  static void
> > > driver_unbind(struct device_fds *dev)  { @@ -231,8 +330,13 @@ static
> > > void initiate_device_reset(struct device_fds *dev, enum reset type)  {
> > >  	igt_debug("reset device\n");
> > >
> > > -	if (type == FLR_RESET)
> > > +	if (type == FLR_RESET) {
> > >  		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> > > +	} else if (type == COLD_RESET) {
> > > +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
> > > +		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir,
> > "power"));
> > > +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
> > > +	}
> > >
> > >  }
> > >
> > > @@ -311,17 +415,46 @@ igt_main
> > >  		igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
> > >  	}
> > >
> > > -	igt_describe("Unbinds driver from device, initiates reset"
> > > -		     " then rebinds driver to device");
> > > -	igt_subtest("unbind-reset-rebind") {
> > > -		unbind_reset_rebind(&dev, FLR_RESET);
> > > -		healthcheck(&dev);
> > > +	igt_subtest_group {
> > > +		igt_describe("Unbinds driver from device, initiates reset"
> > > +			     " then rebinds driver to device");
> > > +		igt_subtest("unbind-reset-rebind") {
> > > +			unbind_reset_rebind(&dev, FLR_RESET);
> > > +			healthcheck(&dev);
> > > +		}
> > > +
> > > +		igt_describe("Resets device with bound driver");
> > > +		igt_subtest("reset-bound") {
> > > +			initiate_device_reset(&dev, FLR_RESET);
> > > +			/*
> > > +			 * Cold reset will initiate card boot sequence again,
> > > +			 * therefore let healthcheck() re-epen the dev fd.
> > > +			 */
> > > +			dev.fds.dev = -1;
> > > +			healthcheck(&dev);
> > > +		}
> > >  	}
> > >
> > > -	igt_describe("Resets device with bound driver");
> > > -	igt_subtest("reset-bound") {
> > > -		initiate_device_reset(&dev, FLR_RESET);
> > > -		healthcheck(&dev);
> > > +	igt_subtest_group {
> > > +		igt_fixture {
> > > +			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does
> > not support any "
> > > +				      "pcie slot for cold reset\n");
> > > +
> > 	igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> > > +
> > 	igt_skip_on(!is_slot_power_ctrl_present(dev.fds.dev));
> > > +		}
> > > +
> > > +		igt_describe("Unbinds driver from device, initiates cold
> > reset"
> > > +			     " then rebinds driver to device");
> > > +		igt_subtest("unbind-cold-reset-rebind") {
> > > +			unbind_reset_rebind(&dev, COLD_RESET);
> > > +			healthcheck(&dev);
> > > +		}
> > > +
> > > +		igt_describe("Cold Resets device with bound driver");
> > > +		igt_subtest("cold-reset-bound") {
> > > +			initiate_device_reset(&dev, COLD_RESET);
> > > +			healthcheck(&dev);
> > > +		}
> > >  	}
> > >
> > >  	igt_fixture {

[igt-dev] [PATCH i-g-t v4 4/5] test/device_reset: Refactor initiate_device_reset

[Anshuman Gupta]

Added a reset type enum to support multiple types
of reset like WARM, COLD and FLR reset.

No functional change.

v2:
- Removed WARM_RESET enum as not used yet. [Badal]

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/device_reset.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/tests/device_reset.c b/tests/device_reset.c
index e60d4c7fde..0c477a02c0 100644
--- a/tests/device_reset.c
+++ b/tests/device_reset.c
@@ -19,6 +19,11 @@ IGT_TEST_DESCRIPTION("Examine behavior of a driver on device sysfs reset");
 #define DEV_PATH_LEN 80
 #define DEV_BUS_ADDR_LEN 13 /* addr has form 0000:00:00.0 */
 
+enum reset {
+	COLD_RESET,
+	FLR_RESET
+};
+
 /**
  * Helper structure containing file descriptors
  * and bus address related to tested device
@@ -222,10 +227,13 @@ static void driver_bind(struct device_fds *dev)
 }
 
 /* Initiate device reset */
-static void initiate_device_reset(struct device_fds *dev)
+static void initiate_device_reset(struct device_fds *dev, enum reset type)
 {
 	igt_debug("reset device\n");
-	igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
+
+	if (type == FLR_RESET)
+		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
+
 }
 
 static bool is_i915_wedged(int i915)
@@ -274,14 +282,14 @@ static void set_device_filter(const char* dev_path)
 	igt_assert_eq(igt_device_filter_add(filter), 1);
 }
 
-static void unbind_reset_rebind(struct device_fds *dev)
+static void unbind_reset_rebind(struct device_fds *dev, enum reset type)
 {
 	igt_debug("close the device\n");
 	close_if_opened(&dev->fds.dev);
 
 	driver_unbind(dev);
 
-	initiate_device_reset(dev);
+	initiate_device_reset(dev, type);
 
 	driver_bind(dev);
 }
@@ -306,13 +314,13 @@ igt_main
 	igt_describe("Unbinds driver from device, initiates reset"
 		     " then rebinds driver to device");
 	igt_subtest("unbind-reset-rebind") {
-		unbind_reset_rebind(&dev);
+		unbind_reset_rebind(&dev, FLR_RESET);
 		healthcheck(&dev);
 	}
 
 	igt_describe("Resets device with bound driver");
 	igt_subtest("reset-bound") {
-		initiate_device_reset(&dev);
+		initiate_device_reset(&dev, FLR_RESET);
 		healthcheck(&dev);
 	}
 
-- 
2.25.1

Re: [igt-dev] [PATCH i-g-t v4 4/5] test/device_reset: Refactor initiate_device_reset

[Nilawar, Badal]

On 25-11-2022 13:13, Anshuman Gupta wrote:
> Added a reset type enum to support multiple types
> of reset like WARM, COLD and FLR reset.
> 
> No functional change.
> 
> v2:
> - Removed WARM_RESET enum as not used yet. [Badal]
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>   tests/device_reset.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/device_reset.c b/tests/device_reset.c
> index e60d4c7fde..0c477a02c0 100644
> --- a/tests/device_reset.c
> +++ b/tests/device_reset.c
> @@ -19,6 +19,11 @@ IGT_TEST_DESCRIPTION("Examine behavior of a driver on device sysfs reset");
>   #define DEV_PATH_LEN 80
>   #define DEV_BUS_ADDR_LEN 13 /* addr has form 0000:00:00.0 */
>   
> +enum reset {
> +	COLD_RESET,
> +	FLR_RESET
> +};
> +
Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
>   /**
>    * Helper structure containing file descriptors
>    * and bus address related to tested device
> @@ -222,10 +227,13 @@ static void driver_bind(struct device_fds *dev)
>   }
>   
>   /* Initiate device reset */
> -static void initiate_device_reset(struct device_fds *dev)
> +static void initiate_device_reset(struct device_fds *dev, enum reset type)
>   {
>   	igt_debug("reset device\n");
> -	igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> +
> +	if (type == FLR_RESET)
> +		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> +
>   }
>   
>   static bool is_i915_wedged(int i915)
> @@ -274,14 +282,14 @@ static void set_device_filter(const char* dev_path)
>   	igt_assert_eq(igt_device_filter_add(filter), 1);
>   }
>   
> -static void unbind_reset_rebind(struct device_fds *dev)
> +static void unbind_reset_rebind(struct device_fds *dev, enum reset type)
>   {
>   	igt_debug("close the device\n");
>   	close_if_opened(&dev->fds.dev);
>   
>   	driver_unbind(dev);
>   
> -	initiate_device_reset(dev);
> +	initiate_device_reset(dev, type);
>   
>   	driver_bind(dev);
>   }
> @@ -306,13 +314,13 @@ igt_main
>   	igt_describe("Unbinds driver from device, initiates reset"
>   		     " then rebinds driver to device");
>   	igt_subtest("unbind-reset-rebind") {
> -		unbind_reset_rebind(&dev);
> +		unbind_reset_rebind(&dev, FLR_RESET);
>   		healthcheck(&dev);
>   	}
>   
>   	igt_describe("Resets device with bound driver");
>   	igt_subtest("reset-bound") {
> -		initiate_device_reset(&dev);
> +		initiate_device_reset(&dev, FLR_RESET);
>   		healthcheck(&dev);
>   	}
>   

Re: [igt-dev] [PATCH i-g-t v4 4/5] test/device_reset: Refactor initiate_device_reset

[Kamil Konieczny]

Hi Anshuman,

On 2022-11-25 at 13:13:39 +0530, Anshuman Gupta wrote:

please correct message commit (subject):
test/device_reset: Refactor initiate_device_reset
---^ 's' is missing, to

tests/device_reset: Refactor initiate_device_reset

> Added a reset type enum to support multiple types
> of reset like WARM, COLD and FLR reset.
> 
> No functional change.
> 
> v2:
> - Removed WARM_RESET enum as not used yet. [Badal]
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tests/device_reset.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/device_reset.c b/tests/device_reset.c
> index e60d4c7fde..0c477a02c0 100644
> --- a/tests/device_reset.c
> +++ b/tests/device_reset.c
> @@ -19,6 +19,11 @@ IGT_TEST_DESCRIPTION("Examine behavior of a driver on device sysfs reset");
>  #define DEV_PATH_LEN 80
>  #define DEV_BUS_ADDR_LEN 13 /* addr has form 0000:00:00.0 */
>  
> +enum reset {
> +	COLD_RESET,
> +	FLR_RESET
> +};
> +
>  /**
>   * Helper structure containing file descriptors
>   * and bus address related to tested device
> @@ -222,10 +227,13 @@ static void driver_bind(struct device_fds *dev)
>  }
>  
>  /* Initiate device reset */
> -static void initiate_device_reset(struct device_fds *dev)
> +static void initiate_device_reset(struct device_fds *dev, enum reset type)
>  {
>  	igt_debug("reset device\n");
> -	igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> +
> +	if (type == FLR_RESET)
> +		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> +
Remove empty line.

Regards,
Kamil

>  }
>  
>  static bool is_i915_wedged(int i915)
> @@ -274,14 +282,14 @@ static void set_device_filter(const char* dev_path)
>  	igt_assert_eq(igt_device_filter_add(filter), 1);
>  }
>  
> -static void unbind_reset_rebind(struct device_fds *dev)
> +static void unbind_reset_rebind(struct device_fds *dev, enum reset type)
>  {
>  	igt_debug("close the device\n");
>  	close_if_opened(&dev->fds.dev);
>  
>  	driver_unbind(dev);
>  
> -	initiate_device_reset(dev);
> +	initiate_device_reset(dev, type);
>  
>  	driver_bind(dev);
>  }
> @@ -306,13 +314,13 @@ igt_main
>  	igt_describe("Unbinds driver from device, initiates reset"
>  		     " then rebinds driver to device");
>  	igt_subtest("unbind-reset-rebind") {
> -		unbind_reset_rebind(&dev);
> +		unbind_reset_rebind(&dev, FLR_RESET);
>  		healthcheck(&dev);
>  	}
>  
>  	igt_describe("Resets device with bound driver");
>  	igt_subtest("reset-bound") {
> -		initiate_device_reset(&dev);
> +		initiate_device_reset(&dev, FLR_RESET);
>  		healthcheck(&dev);
>  	}
>  
> -- 
> 2.25.1
> 

[igt-dev] [PATCH i-g-t v4 5/5] tests/device_reset: Add cold reset IGT test

[Anshuman Gupta]

Add cold-reset IGT subtest, IGT subtest will use
/sys/bus/pci/slots/$SUN/power sysfs in order to
initiate a cold reset sequence. $SUN value will be
retrieved from PCIe device ACPI firmware node.

v2:
- %s/igt_require(fd > 0)/igt_assert(errno >0)
v3:
- Added Slot Power Controller capability check. [Aravind]
- Check whether slot is powered down before powering up. [Aravind]
v4:
- Check power ctrl presence before slot presence to save CI
  execution time.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
---
 lib/igt_pm.c         |  40 +++++++++++
 lib/igt_pm.h         |   1 +
 tests/device_reset.c | 156 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 187 insertions(+), 10 deletions(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 4f0cfbdd10..9b7ed4be64 100644
--- a/lib/igt_pm.c
+++ b/lib/igt_pm.c
@@ -877,6 +877,46 @@ static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
 	return dir;
 }
 
+/**
+ * igt_pm_get_pcie_acpihp_slot:
+ * @pci_dev: pci bridge device.
+ * Get pci bridge acpi hotplug slot number, if bridge's ACPI firmware_node
+ * handle supports _SUN method.
+ *
+ * Returns:
+ * PCIe bridge Slot number.
+ * Returns -ENOENT number in case firmware_node/sun is not supported by the
+ * bridge.
+ */
+int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev)
+{
+	int firmware_node_fd, fd, n_read;
+	char sun[8];
+
+	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
+
+	if (firmware_node_fd < 0 && errno == ENOENT)
+		return -ENOENT;
+
+	igt_require(firmware_node_fd > 0);
+
+	fd = openat(firmware_node_fd, "sun", O_RDONLY);
+	if (fd < 0 && errno == ENOENT) {
+		close(firmware_node_fd);
+		return -ENOENT;
+	}
+
+	igt_assert(errno > 0);
+
+	n_read = read(fd, sun, sizeof(sun));
+	igt_assert(n_read > 0);
+
+	close(firmware_node_fd);
+	close(fd);
+
+	return strtol(sun, NULL, 10);
+}
+
 /**
  * igt_pm_acpi_d3cold_supported:
  * @pci_dev: root port pci_dev.
diff --git a/lib/igt_pm.h b/lib/igt_pm.h
index e81fceb897..f65b960c38 100644
--- a/lib/igt_pm.h
+++ b/lib/igt_pm.h
@@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
 bool igt_pm_dmc_loaded(int debugfs);
 bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
 bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
+int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
 bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
 enum igt_acpi_d_state
 igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev);
diff --git a/tests/device_reset.c b/tests/device_reset.c
index 0c477a02c0..b65395fbb9 100644
--- a/tests/device_reset.c
+++ b/tests/device_reset.c
@@ -9,7 +9,9 @@
 
 #include "i915/gem.h"
 #include "igt.h"
+#include "igt_device.h"
 #include "igt_device_scan.h"
+#include "igt_pci.h"
 #include "igt_sysfs.h"
 #include "igt_kmod.h"
 
@@ -33,6 +35,7 @@ struct device_fds {
 		int dev;
 		int dev_dir;
 		int drv_dir;
+		int slot_dir; /* pci hotplug slots fd */
 	} fds;
 	char dev_bus_addr[DEV_BUS_ADDR_LEN];
 	bool snd_unload;
@@ -62,6 +65,74 @@ static int open_driver_sysfs_dir(int fd)
 	return __open_sysfs_dir(fd, "device/driver");
 }
 
+static bool is_pci_power_ctrl_present(struct pci_device *dev)
+{
+	int offset;
+	uint32_t slot_cap;
+
+	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
+	igt_require_f(offset > 0, "PCI Express Capability not found\n");
+	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset + PCI_SLOT_CAP_OFFSET));
+	igt_debug("slot cap register 0x%x\n", slot_cap);
+
+	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT;
+}
+
+static bool is_slot_power_ctrl_present(int fd)
+{
+	struct pci_device *root;
+
+	/*
+	 * Card root port Slot Capabilities Register
+	 * determines Power Controller Presence.
+	 */
+	root = igt_device_get_pci_root_port(fd);
+	return is_pci_power_ctrl_present(root);
+}
+
+static int open_slot_sysfs_dir(int fd)
+{
+	struct pci_device *pci_dev = NULL;
+	int slot_fd = -1, slot;
+	char slot_fd_path[PATH_MAX];
+
+	/* Don't search for slot if root port doesn't support power ctrl */
+	if (!is_slot_power_ctrl_present(fd))
+		return slot_fd;
+
+	pci_dev = igt_device_get_pci_device(fd);
+	igt_require(pci_dev);
+
+	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
+		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
+		if (slot == -ENOENT) {
+			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x does not support acpihp slot\n",
+				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
+			continue;
+		}
+
+		/*
+		 * Upon getting the valid acpihp slot number break the loop.
+		 * It is the desired acpihp slot for gfx card.
+		 */
+		if (slot > 0) {
+			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x associated acpihp slot %d\n",
+				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func, slot);
+			break;
+		}
+	}
+
+	if (!pci_dev)
+		return slot_fd;
+
+	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
+	slot_fd = open(slot_fd_path, O_RDONLY);
+	if (slot_fd < 0)
+		return -errno;
+
+	return slot_fd;
+}
+
 /**
  * device_sysfs_path:
  * @fd: opened device file descriptor
@@ -124,6 +195,8 @@ static void init_device_fds(struct device_fds *dev)
 
 	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
 	igt_assert_fd(dev->fds.drv_dir);
+
+	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
 }
 
 static int close_if_opened(int *fd)
@@ -142,6 +215,7 @@ static void cleanup_device_fds(struct device_fds *dev)
 	igt_ignore_warn(close_if_opened(&dev->fds.dev));
 	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
 	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
+	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
 }
 
 /**
@@ -179,6 +253,35 @@ static bool is_sysfs_reset_supported(int fd)
 	return true;
 }
 
+/**
+ * is_sysfs_cold_reset_supported:
+ * @fd: opened device file descriptor
+ *
+ * Check if device supports cold reset based on sysfs file presence.
+ *
+ * Returns:
+ * True if device supports reset, false otherwise.
+ */
+static bool is_sysfs_cold_reset_supported(int slot_fd)
+{
+	struct stat st;
+	int rc;
+	int cold_reset_fd = -1;
+
+	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
+
+	if (cold_reset_fd < 0)
+		return false;
+
+	rc = fstat(cold_reset_fd, &st);
+	close(cold_reset_fd);
+
+	if (rc || !S_ISREG(st.st_mode))
+		return false;
+
+	return true;
+}
+
 /* Unbind the driver from the device */
 static void driver_unbind(struct device_fds *dev)
 {
@@ -231,8 +334,13 @@ static void initiate_device_reset(struct device_fds *dev, enum reset type)
 {
 	igt_debug("reset device\n");
 
-	if (type == FLR_RESET)
+	if (type == FLR_RESET) {
 		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
+	} else if (type == COLD_RESET) {
+		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
+		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir, "power"));
+		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
+	}
 
 }
 
@@ -311,17 +419,45 @@ igt_main
 		igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
 	}
 
-	igt_describe("Unbinds driver from device, initiates reset"
-		     " then rebinds driver to device");
-	igt_subtest("unbind-reset-rebind") {
-		unbind_reset_rebind(&dev, FLR_RESET);
-		healthcheck(&dev);
+	igt_subtest_group {
+		igt_describe("Unbinds driver from device, initiates reset"
+			     " then rebinds driver to device");
+		igt_subtest("unbind-reset-rebind") {
+			unbind_reset_rebind(&dev, FLR_RESET);
+			healthcheck(&dev);
+		}
+
+		igt_describe("Resets device with bound driver");
+		igt_subtest("reset-bound") {
+			initiate_device_reset(&dev, FLR_RESET);
+			/*
+			 * Cold reset will initiate card boot sequence again,
+			 * therefore let healthcheck() re-epen the dev fd.
+			 */
+			dev.fds.dev = -1;
+			healthcheck(&dev);
+		}
 	}
 
-	igt_describe("Resets device with bound driver");
-	igt_subtest("reset-bound") {
-		initiate_device_reset(&dev, FLR_RESET);
-		healthcheck(&dev);
+	igt_subtest_group {
+		igt_fixture {
+			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
+				      "pcie slot for cold reset\n");
+			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
+		}
+
+		igt_describe("Unbinds driver from device, initiates cold reset"
+			     " then rebinds driver to device");
+		igt_subtest("unbind-cold-reset-rebind") {
+			unbind_reset_rebind(&dev, COLD_RESET);
+			healthcheck(&dev);
+		}
+
+		igt_describe("Cold Resets device with bound driver");
+		igt_subtest("cold-reset-bound") {
+			initiate_device_reset(&dev, COLD_RESET);
+			healthcheck(&dev);
+		}
 	}
 
 	igt_fixture {
-- 
2.25.1

Re: [igt-dev] [PATCH i-g-t v4 5/5] tests/device_reset: Add cold reset IGT test

[Kamil Konieczny]

Hi,

please send new version (v5) in complete, e.g. all patches
even those which do not changed.

+cc Marcin

On 2022-11-28 at 15:50:31 +0530, Anshuman Gupta wrote:
> Add cold-reset IGT subtest, IGT subtest will use
> /sys/bus/pci/slots/$SUN/power sysfs in order to
> initiate a cold reset sequence. $SUN value will be
> retrieved from PCIe device ACPI firmware node.
> 
> v2:
> - %s/igt_require(fd > 0)/igt_assert(errno >0)
> v3:
> - Added Slot Power Controller capability check. [Aravind]
> - Check whether slot is powered down before powering up. [Aravind]
> v4:
> - Check power ctrl presence before slot presence to save CI
>   execution time.
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>  lib/igt_pm.c         |  40 +++++++++++
>  lib/igt_pm.h         |   1 +
>  tests/device_reset.c | 156 ++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 187 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 4f0cfbdd10..9b7ed4be64 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -877,6 +877,46 @@ static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
>  	return dir;
>  }
>  
> +/**
> + * igt_pm_get_pcie_acpihp_slot:
> + * @pci_dev: pci bridge device.
> + * Get pci bridge acpi hotplug slot number, if bridge's ACPI firmware_node
> + * handle supports _SUN method.
> + *
> + * Returns:
> + * PCIe bridge Slot number.
> + * Returns -ENOENT number in case firmware_node/sun is not supported by the
> + * bridge.
> + */
> +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev)
> +{
> +	int firmware_node_fd, fd, n_read;
> +	char sun[8];
> +
> +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> +
> +	if (firmware_node_fd < 0 && errno == ENOENT)
> +		return -ENOENT;
> +
> +	igt_require(firmware_node_fd > 0);
> +
> +	fd = openat(firmware_node_fd, "sun", O_RDONLY);
> +	if (fd < 0 && errno == ENOENT) {
> +		close(firmware_node_fd);
> +		return -ENOENT;
> +	}
> +
> +	igt_assert(errno > 0);
------- ^
Check for fd > 0 here.

> +
> +	n_read = read(fd, sun, sizeof(sun));
> +	igt_assert(n_read > 0);
------- ^
Move this assert after close, add also
	igt_assert(n_read < 8);

> +
> +	close(firmware_node_fd);
> +	close(fd);

Add zero to string end, sun[n_read] = 0;

> +
> +	return strtol(sun, NULL, 10);
> +}
> +
>  /**
>   * igt_pm_acpi_d3cold_supported:
>   * @pci_dev: root port pci_dev.
> diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> index e81fceb897..f65b960c38 100644
> --- a/lib/igt_pm.h
> +++ b/lib/igt_pm.h
> @@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
>  bool igt_pm_dmc_loaded(int debugfs);
>  bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
>  bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
> +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
>  bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
>  enum igt_acpi_d_state
>  igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev);
> diff --git a/tests/device_reset.c b/tests/device_reset.c
> index 0c477a02c0..b65395fbb9 100644
> --- a/tests/device_reset.c
> +++ b/tests/device_reset.c
> @@ -9,7 +9,9 @@
>  
>  #include "i915/gem.h"
>  #include "igt.h"
> +#include "igt_device.h"
>  #include "igt_device_scan.h"
> +#include "igt_pci.h"
>  #include "igt_sysfs.h"
>  #include "igt_kmod.h"
>  
> @@ -33,6 +35,7 @@ struct device_fds {
>  		int dev;
>  		int dev_dir;
>  		int drv_dir;
> +		int slot_dir; /* pci hotplug slots fd */
>  	} fds;
>  	char dev_bus_addr[DEV_BUS_ADDR_LEN];
>  	bool snd_unload;
> @@ -62,6 +65,74 @@ static int open_driver_sysfs_dir(int fd)
>  	return __open_sysfs_dir(fd, "device/driver");
>  }
>  
> +static bool is_pci_power_ctrl_present(struct pci_device *dev)
> +{
> +	int offset;
> +	uint32_t slot_cap;
> +
> +	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
> +	igt_require_f(offset > 0, "PCI Express Capability not found\n");
> +	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset + PCI_SLOT_CAP_OFFSET));
> +	igt_debug("slot cap register 0x%x\n", slot_cap);
> +
> +	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT;
> +}
> +
> +static bool is_slot_power_ctrl_present(int fd)
> +{
> +	struct pci_device *root;
> +
> +	/*
> +	 * Card root port Slot Capabilities Register
> +	 * determines Power Controller Presence.
> +	 */
> +	root = igt_device_get_pci_root_port(fd);
> +	return is_pci_power_ctrl_present(root);
> +}
> +
> +static int open_slot_sysfs_dir(int fd)
> +{
> +	struct pci_device *pci_dev = NULL;
> +	int slot_fd = -1, slot;
> +	char slot_fd_path[PATH_MAX];
> +
> +	/* Don't search for slot if root port doesn't support power ctrl */
> +	if (!is_slot_power_ctrl_present(fd))
> +		return slot_fd;
---------------------- ^
-1 would be better or maybe -ENOTSUP ?

> +
> +	pci_dev = igt_device_get_pci_device(fd);
> +	igt_require(pci_dev);
> +
> +	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
> +		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
> +		if (slot == -ENOENT) {
> +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x does not support acpihp slot\n",
> +				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
> +			continue;
> +		}
> +
> +		/*
> +		 * Upon getting the valid acpihp slot number break the loop.
> +		 * It is the desired acpihp slot for gfx card.
> +		 */
> +		if (slot > 0) {
> +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x associated acpihp slot %d\n",
> +				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func, slot);
> +			break;
> +		}
> +	}
> +
> +	if (!pci_dev)
> +		return slot_fd;
---------------------- ^
-1 would be better

> +
> +	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
> +	slot_fd = open(slot_fd_path, O_RDONLY);
> +	if (slot_fd < 0)
> +		return -errno;
> +
> +	return slot_fd;
> +}
> +
>  /**
>   * device_sysfs_path:
>   * @fd: opened device file descriptor
> @@ -124,6 +195,8 @@ static void init_device_fds(struct device_fds *dev)
>  
>  	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
>  	igt_assert_fd(dev->fds.drv_dir);
> +
> +	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
>  }
>  
>  static int close_if_opened(int *fd)
> @@ -142,6 +215,7 @@ static void cleanup_device_fds(struct device_fds *dev)
>  	igt_ignore_warn(close_if_opened(&dev->fds.dev));
>  	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
>  	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
> +	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
>  }
>  
>  /**
> @@ -179,6 +253,35 @@ static bool is_sysfs_reset_supported(int fd)
>  	return true;
>  }
>  
> +/**
> + * is_sysfs_cold_reset_supported:
> + * @fd: opened device file descriptor
> + *
> + * Check if device supports cold reset based on sysfs file presence.
> + *
> + * Returns:
> + * True if device supports reset, false otherwise.
> + */
> +static bool is_sysfs_cold_reset_supported(int slot_fd)
> +{
> +	struct stat st;
> +	int rc;
> +	int cold_reset_fd = -1;
> +
> +	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
> +
> +	if (cold_reset_fd < 0)
> +		return false;
> +
> +	rc = fstat(cold_reset_fd, &st);
> +	close(cold_reset_fd);
> +
> +	if (rc || !S_ISREG(st.st_mode))
> +		return false;
> +
> +	return true;
> +}
> +
>  /* Unbind the driver from the device */
>  static void driver_unbind(struct device_fds *dev)
>  {
> @@ -231,8 +334,13 @@ static void initiate_device_reset(struct device_fds *dev, enum reset type)
>  {
>  	igt_debug("reset device\n");
>  
> -	if (type == FLR_RESET)
> +	if (type == FLR_RESET) {
>  		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> +	} else if (type == COLD_RESET) {
> +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
> +		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir, "power"));
> +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
> +	}
>  
>  }
>  
> @@ -311,17 +419,45 @@ igt_main
>  		igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
>  	}
>  
> -	igt_describe("Unbinds driver from device, initiates reset"
> -		     " then rebinds driver to device");
> -	igt_subtest("unbind-reset-rebind") {
> -		unbind_reset_rebind(&dev, FLR_RESET);
> -		healthcheck(&dev);
> +	igt_subtest_group {

Why do you move these into group ?

> +		igt_describe("Unbinds driver from device, initiates reset"
> +			     " then rebinds driver to device");
> +		igt_subtest("unbind-reset-rebind") {
> +			unbind_reset_rebind(&dev, FLR_RESET);
> +			healthcheck(&dev);
> +		}
> +
> +		igt_describe("Resets device with bound driver");
> +		igt_subtest("reset-bound") {
> +			initiate_device_reset(&dev, FLR_RESET);
--------------------------------------------------- ^
> +			/*
> +			 * Cold reset will initiate card boot sequence again,
-------------------------- ^
This comment is missplaced.

Regards,
Kamil

> +			 * therefore let healthcheck() re-epen the dev fd.
> +			 */
> +			dev.fds.dev = -1;
> +			healthcheck(&dev);
> +		}
>  	}
>  
> -	igt_describe("Resets device with bound driver");
> -	igt_subtest("reset-bound") {
> -		initiate_device_reset(&dev, FLR_RESET);
> -		healthcheck(&dev);
> +	igt_subtest_group {
> +		igt_fixture {
> +			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
> +				      "pcie slot for cold reset\n");
> +			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> +		}
> +
> +		igt_describe("Unbinds driver from device, initiates cold reset"
> +			     " then rebinds driver to device");
> +		igt_subtest("unbind-cold-reset-rebind") {
> +			unbind_reset_rebind(&dev, COLD_RESET);
> +			healthcheck(&dev);
> +		}
> +
> +		igt_describe("Cold Resets device with bound driver");
> +		igt_subtest("cold-reset-bound") {
> +			initiate_device_reset(&dev, COLD_RESET);
> +			healthcheck(&dev);
> +		}
>  	}
>  
>  	igt_fixture {
> -- 
> 2.25.1
> 

Re: [igt-dev] [PATCH i-g-t v4 5/5] tests/device_reset: Add cold reset IGT test

[Nilawar, Badal]

Hi Anshuman,

On 28-11-2022 15:50, Anshuman Gupta wrote:
> Add cold-reset IGT subtest, IGT subtest will use
> /sys/bus/pci/slots/$SUN/power sysfs in order to
> initiate a cold reset sequence. $SUN value will be
> retrieved from PCIe device ACPI firmware node.
> 
> v2:
> - %s/igt_require(fd > 0)/igt_assert(errno >0)
> v3:
> - Added Slot Power Controller capability check. [Aravind]
> - Check whether slot is powered down before powering up. [Aravind]
> v4:
> - Check power ctrl presence before slot presence to save CI
>    execution time.
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>   lib/igt_pm.c         |  40 +++++++++++
>   lib/igt_pm.h         |   1 +
>   tests/device_reset.c | 156 ++++++++++++++++++++++++++++++++++++++++---
>   3 files changed, 187 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 4f0cfbdd10..9b7ed4be64 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -877,6 +877,46 @@ static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
>   	return dir;
>   }
>   
> +/**
> + * igt_pm_get_pcie_acpihp_slot:
> + * @pci_dev: pci bridge device.
> + * Get pci bridge acpi hotplug slot number, if bridge's ACPI firmware_node
> + * handle supports _SUN method.
> + *
> + * Returns:
> + * PCIe bridge Slot number.
> + * Returns -ENOENT number in case firmware_node/sun is not supported by the
> + * bridge.
> + */
> +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev)
> +{
> +	int firmware_node_fd, fd, n_read;
> +	char sun[8];
> +
> +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> +
> +	if (firmware_node_fd < 0 && errno == ENOENT)
> +		return -ENOENT;
> +
> +	igt_require(firmware_node_fd > 0);
> +
> +	fd = openat(firmware_node_fd, "sun", O_RDONLY);
> +	if (fd < 0 && errno == ENOENT) {
> +		close(firmware_node_fd);
> +		return -ENOENT;
> +	}
> +
> +	igt_assert(errno > 0);
> +
> +	n_read = read(fd, sun, sizeof(sun));
> +	igt_assert(n_read > 0);
> +
> +	close(firmware_node_fd);
> +	close(fd);
> +
> +	return strtol(sun, NULL, 10);
> +}
> +
>   /**
>    * igt_pm_acpi_d3cold_supported:
>    * @pci_dev: root port pci_dev.
> diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> index e81fceb897..f65b960c38 100644
> --- a/lib/igt_pm.h
> +++ b/lib/igt_pm.h
> @@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
>   bool igt_pm_dmc_loaded(int debugfs);
>   bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
>   bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
> +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
>   bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
>   enum igt_acpi_d_state
>   igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev);
> diff --git a/tests/device_reset.c b/tests/device_reset.c
> index 0c477a02c0..b65395fbb9 100644
> --- a/tests/device_reset.c
> +++ b/tests/device_reset.c
> @@ -9,7 +9,9 @@
>   
>   #include "i915/gem.h"
>   #include "igt.h"
> +#include "igt_device.h"
>   #include "igt_device_scan.h"
> +#include "igt_pci.h"
>   #include "igt_sysfs.h"
>   #include "igt_kmod.h"
>   
> @@ -33,6 +35,7 @@ struct device_fds {
>   		int dev;
>   		int dev_dir;
>   		int drv_dir;
> +		int slot_dir; /* pci hotplug slots fd */
>   	} fds;
>   	char dev_bus_addr[DEV_BUS_ADDR_LEN];
>   	bool snd_unload;
> @@ -62,6 +65,74 @@ static int open_driver_sysfs_dir(int fd)
>   	return __open_sysfs_dir(fd, "device/driver");
>   }
>   
> +static bool is_pci_power_ctrl_present(struct pci_device *dev)
> +{
> +	int offset;
> +	uint32_t slot_cap;
> +
> +	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
> +	igt_require_f(offset > 0, "PCI Express Capability not found\n");
> +	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset + PCI_SLOT_CAP_OFFSET));
> +	igt_debug("slot cap register 0x%x\n", slot_cap);
> +
> +	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT;
> +}
> +
> +static bool is_slot_power_ctrl_present(int fd)
> +{
> +	struct pci_device *root;
> +
> +	/*
> +	 * Card root port Slot Capabilities Register
> +	 * determines Power Controller Presence.
> +	 */
> +	root = igt_device_get_pci_root_port(fd);
> +	return is_pci_power_ctrl_present(root);
Why power controller capability of root port is checked here?
Shouldn't we check the power controller capability of downstream 
switch/bridge on which slot is found?

Regards,
Badal
> +}
> +
> +static int open_slot_sysfs_dir(int fd)
> +{
> +	struct pci_device *pci_dev = NULL;
> +	int slot_fd = -1, slot;
> +	char slot_fd_path[PATH_MAX];
> +
> +	/* Don't search for slot if root port doesn't support power ctrl */
> +	if (!is_slot_power_ctrl_present(fd))
> +		return slot_fd;
> +
> +	pci_dev = igt_device_get_pci_device(fd);
> +	igt_require(pci_dev);
> +
> +	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
> +		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
> +		if (slot == -ENOENT) {
> +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x does not support acpihp slot\n",
> +				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
> +			continue;
> +		}
> +
> +		/*
> +		 * Upon getting the valid acpihp slot number break the loop.
> +		 * It is the desired acpihp slot for gfx card.
> +		 */
> +		if (slot > 0) {
> +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x associated acpihp slot %d\n",
> +				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func, slot);
> +			break;
> +		}
> +	}
> +
> +	if (!pci_dev)
> +		return slot_fd;
> +
> +	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
> +	slot_fd = open(slot_fd_path, O_RDONLY);
> +	if (slot_fd < 0)
> +		return -errno;
> +
> +	return slot_fd;
> +}
> +
>   /**
>    * device_sysfs_path:
>    * @fd: opened device file descriptor
> @@ -124,6 +195,8 @@ static void init_device_fds(struct device_fds *dev)
>   
>   	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
>   	igt_assert_fd(dev->fds.drv_dir);
> +
> +	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
>   }
>   
>   static int close_if_opened(int *fd)
> @@ -142,6 +215,7 @@ static void cleanup_device_fds(struct device_fds *dev)
>   	igt_ignore_warn(close_if_opened(&dev->fds.dev));
>   	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
>   	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
> +	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
>   }
>   
>   /**
> @@ -179,6 +253,35 @@ static bool is_sysfs_reset_supported(int fd)
>   	return true;
>   }
>   
> +/**
> + * is_sysfs_cold_reset_supported:
> + * @fd: opened device file descriptor
> + *
> + * Check if device supports cold reset based on sysfs file presence.
> + *
> + * Returns:
> + * True if device supports reset, false otherwise.
> + */
> +static bool is_sysfs_cold_reset_supported(int slot_fd)
> +{
> +	struct stat st;
> +	int rc;
> +	int cold_reset_fd = -1;
> +
> +	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
> +
> +	if (cold_reset_fd < 0)
> +		return false;
> +
> +	rc = fstat(cold_reset_fd, &st);
> +	close(cold_reset_fd);
> +
> +	if (rc || !S_ISREG(st.st_mode))
> +		return false;
> +
> +	return true;
> +}
> +
>   /* Unbind the driver from the device */
>   static void driver_unbind(struct device_fds *dev)
>   {
> @@ -231,8 +334,13 @@ static void initiate_device_reset(struct device_fds *dev, enum reset type)
>   {
>   	igt_debug("reset device\n");
>   
> -	if (type == FLR_RESET)
> +	if (type == FLR_RESET) {
>   		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> +	} else if (type == COLD_RESET) {
> +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
> +		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir, "power"));
> +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
> +	}
>   
>   }
>   
> @@ -311,17 +419,45 @@ igt_main
>   		igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
>   	}
>   
> -	igt_describe("Unbinds driver from device, initiates reset"
> -		     " then rebinds driver to device");
> -	igt_subtest("unbind-reset-rebind") {
> -		unbind_reset_rebind(&dev, FLR_RESET);
> -		healthcheck(&dev);
> +	igt_subtest_group {
> +		igt_describe("Unbinds driver from device, initiates reset"
> +			     " then rebinds driver to device");
> +		igt_subtest("unbind-reset-rebind") {
> +			unbind_reset_rebind(&dev, FLR_RESET);
> +			healthcheck(&dev);
> +		}
> +
> +		igt_describe("Resets device with bound driver");
> +		igt_subtest("reset-bound") {
> +			initiate_device_reset(&dev, FLR_RESET);
> +			/*
> +			 * Cold reset will initiate card boot sequence again,
> +			 * therefore let healthcheck() re-epen the dev fd.
> +			 */
> +			dev.fds.dev = -1;
> +			healthcheck(&dev);
> +		}
>   	}
>   
> -	igt_describe("Resets device with bound driver");
> -	igt_subtest("reset-bound") {
> -		initiate_device_reset(&dev, FLR_RESET);
> -		healthcheck(&dev);
> +	igt_subtest_group {
> +		igt_fixture {
> +			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
> +				      "pcie slot for cold reset\n");
> +			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> +		}
> +
> +		igt_describe("Unbinds driver from device, initiates cold reset"
> +			     " then rebinds driver to device");
> +		igt_subtest("unbind-cold-reset-rebind") {
> +			unbind_reset_rebind(&dev, COLD_RESET);
> +			healthcheck(&dev);
> +		}
> +
> +		igt_describe("Cold Resets device with bound driver");
> +		igt_subtest("cold-reset-bound") {
> +			initiate_device_reset(&dev, COLD_RESET);
> +			healthcheck(&dev);
> +		}
>   	}
>   
>   	igt_fixture {

Re: [igt-dev] [PATCH i-g-t v4 5/5] tests/device_reset: Add cold reset IGT test

[Gupta, Anshuman]

> -----Original Message-----
> From: Nilawar, Badal <badal.nilawar@intel.com>
> Sent: Wednesday, November 30, 2022 6:55 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>; igt-
> dev@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; kamil.konieczny@linux.intel.com;
> Tangudu, Tilak <tilak.tangudu@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; Dixit, Ashutosh
> <ashutosh.dixit@intel.com>
> Subject: Re: [PATCH i-g-t v4 5/5] tests/device_reset: Add cold reset IGT test
> 
> Hi Anshuman,
> 
> On 28-11-2022 15:50, Anshuman Gupta wrote:
> > Add cold-reset IGT subtest, IGT subtest will use
> > /sys/bus/pci/slots/$SUN/power sysfs in order to initiate a cold reset
> > sequence. $SUN value will be retrieved from PCIe device ACPI firmware
> > node.
> >
> > v2:
> > - %s/igt_require(fd > 0)/igt_assert(errno >0)
> > v3:
> > - Added Slot Power Controller capability check. [Aravind]
> > - Check whether slot is powered down before powering up. [Aravind]
> > v4:
> > - Check power ctrl presence before slot presence to save CI
> >    execution time.
> >
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
> > ---
> >   lib/igt_pm.c         |  40 +++++++++++
> >   lib/igt_pm.h         |   1 +
> >   tests/device_reset.c | 156
> ++++++++++++++++++++++++++++++++++++++++---
> >   3 files changed, 187 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/igt_pm.c b/lib/igt_pm.c index 4f0cfbdd10..9b7ed4be64
> > 100644
> > --- a/lib/igt_pm.c
> > +++ b/lib/igt_pm.c
> > @@ -877,6 +877,46 @@ static int igt_pm_open_pci_firmware_node(struct
> pci_device *pci_dev)
> >   	return dir;
> >   }
> >
> > +/**
> > + * igt_pm_get_pcie_acpihp_slot:
> > + * @pci_dev: pci bridge device.
> > + * Get pci bridge acpi hotplug slot number, if bridge's ACPI
> > +firmware_node
> > + * handle supports _SUN method.
> > + *
> > + * Returns:
> > + * PCIe bridge Slot number.
> > + * Returns -ENOENT number in case firmware_node/sun is not supported
> > +by the
> > + * bridge.
> > + */
> > +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev) {
> > +	int firmware_node_fd, fd, n_read;
> > +	char sun[8];
> > +
> > +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> > +
> > +	if (firmware_node_fd < 0 && errno == ENOENT)
> > +		return -ENOENT;
> > +
> > +	igt_require(firmware_node_fd > 0);
> > +
> > +	fd = openat(firmware_node_fd, "sun", O_RDONLY);
> > +	if (fd < 0 && errno == ENOENT) {
> > +		close(firmware_node_fd);
> > +		return -ENOENT;
> > +	}
> > +
> > +	igt_assert(errno > 0);
> > +
> > +	n_read = read(fd, sun, sizeof(sun));
> > +	igt_assert(n_read > 0);
> > +
> > +	close(firmware_node_fd);
> > +	close(fd);
> > +
> > +	return strtol(sun, NULL, 10);
> > +}
> > +
> >   /**
> >    * igt_pm_acpi_d3cold_supported:
> >    * @pci_dev: root port pci_dev.
> > diff --git a/lib/igt_pm.h b/lib/igt_pm.h index e81fceb897..f65b960c38
> > 100644
> > --- a/lib/igt_pm.h
> > +++ b/lib/igt_pm.h
> > @@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum
> igt_runtime_pm_status status);
> >   bool igt_pm_dmc_loaded(int debugfs);
> >   bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
> >   bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
> > +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
> >   bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
> >   enum igt_acpi_d_state
> >   igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev); diff --git
> > a/tests/device_reset.c b/tests/device_reset.c index
> > 0c477a02c0..b65395fbb9 100644
> > --- a/tests/device_reset.c
> > +++ b/tests/device_reset.c
> > @@ -9,7 +9,9 @@
> >
> >   #include "i915/gem.h"
> >   #include "igt.h"
> > +#include "igt_device.h"
> >   #include "igt_device_scan.h"
> > +#include "igt_pci.h"
> >   #include "igt_sysfs.h"
> >   #include "igt_kmod.h"
> >
> > @@ -33,6 +35,7 @@ struct device_fds {
> >   		int dev;
> >   		int dev_dir;
> >   		int drv_dir;
> > +		int slot_dir; /* pci hotplug slots fd */
> >   	} fds;
> >   	char dev_bus_addr[DEV_BUS_ADDR_LEN];
> >   	bool snd_unload;
> > @@ -62,6 +65,74 @@ static int open_driver_sysfs_dir(int fd)
> >   	return __open_sysfs_dir(fd, "device/driver");
> >   }
> >
> > +static bool is_pci_power_ctrl_present(struct pci_device *dev) {
> > +	int offset;
> > +	uint32_t slot_cap;
> > +
> > +	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
> > +	igt_require_f(offset > 0, "PCI Express Capability not found\n");
> > +	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset +
> PCI_SLOT_CAP_OFFSET));
> > +	igt_debug("slot cap register 0x%x\n", slot_cap);
> > +
> > +	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT; }
> > +
> > +static bool is_slot_power_ctrl_present(int fd) {
> > +	struct pci_device *root;
> > +
> > +	/*
> > +	 * Card root port Slot Capabilities Register
> > +	 * determines Power Controller Presence.
> > +	 */
> > +	root = igt_device_get_pci_root_port(fd);
> > +	return is_pci_power_ctrl_present(root);
> Why power controller capability of root port is checked here?
> Shouldn't we check the power controller capability of downstream
> switch/bridge on which slot is found?
This is to switch on/off the power of card and as per PCI specs the slot capability register 
Are only applicable to downstream port or root port. Not for upstream switch port.
Therefore we only requires to check for root port because we are operating on card here.
BR,
Anshuman Gupta.
> 
> Regards,
> Badal
> > +}
> > +
> > +static int open_slot_sysfs_dir(int fd) {
> > +	struct pci_device *pci_dev = NULL;
> > +	int slot_fd = -1, slot;
> > +	char slot_fd_path[PATH_MAX];
> > +
> > +	/* Don't search for slot if root port doesn't support power ctrl */
> > +	if (!is_slot_power_ctrl_present(fd))
> > +		return slot_fd;
> > +
> > +	pci_dev = igt_device_get_pci_device(fd);
> > +	igt_require(pci_dev);
> > +
> > +	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
> > +		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
> > +		if (slot == -ENOENT) {
> > +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x
> does not support acpihp slot\n",
> > +				  pci_dev->domain, pci_dev->bus, pci_dev-
> >dev, pci_dev->func);
> > +			continue;
> > +		}
> > +
> > +		/*
> > +		 * Upon getting the valid acpihp slot number break the loop.
> > +		 * It is the desired acpihp slot for gfx card.
> > +		 */
> > +		if (slot > 0) {
> > +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x
> associated acpihp slot %d\n",
> > +				  pci_dev->domain, pci_dev->bus, pci_dev-
> >dev, pci_dev->func, slot);
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!pci_dev)
> > +		return slot_fd;
> > +
> > +	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
> > +	slot_fd = open(slot_fd_path, O_RDONLY);
> > +	if (slot_fd < 0)
> > +		return -errno;
> > +
> > +	return slot_fd;
> > +}
> > +
> >   /**
> >    * device_sysfs_path:
> >    * @fd: opened device file descriptor @@ -124,6 +195,8 @@ static
> > void init_device_fds(struct device_fds *dev)
> >
> >   	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
> >   	igt_assert_fd(dev->fds.drv_dir);
> > +
> > +	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
> >   }
> >
> >   static int close_if_opened(int *fd)
> > @@ -142,6 +215,7 @@ static void cleanup_device_fds(struct device_fds
> *dev)
> >   	igt_ignore_warn(close_if_opened(&dev->fds.dev));
> >   	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
> >   	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
> > +	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
> >   }
> >
> >   /**
> > @@ -179,6 +253,35 @@ static bool is_sysfs_reset_supported(int fd)
> >   	return true;
> >   }
> >
> > +/**
> > + * is_sysfs_cold_reset_supported:
> > + * @fd: opened device file descriptor
> > + *
> > + * Check if device supports cold reset based on sysfs file presence.
> > + *
> > + * Returns:
> > + * True if device supports reset, false otherwise.
> > + */
> > +static bool is_sysfs_cold_reset_supported(int slot_fd) {
> > +	struct stat st;
> > +	int rc;
> > +	int cold_reset_fd = -1;
> > +
> > +	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
> > +
> > +	if (cold_reset_fd < 0)
> > +		return false;
> > +
> > +	rc = fstat(cold_reset_fd, &st);
> > +	close(cold_reset_fd);
> > +
> > +	if (rc || !S_ISREG(st.st_mode))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >   /* Unbind the driver from the device */
> >   static void driver_unbind(struct device_fds *dev)
> >   {
> > @@ -231,8 +334,13 @@ static void initiate_device_reset(struct device_fds
> *dev, enum reset type)
> >   {
> >   	igt_debug("reset device\n");
> >
> > -	if (type == FLR_RESET)
> > +	if (type == FLR_RESET) {
> >   		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> > +	} else if (type == COLD_RESET) {
> > +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
> > +		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir,
> "power"));
> > +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
> > +	}
> >
> >   }
> >
> > @@ -311,17 +419,45 @@ igt_main
> >   		igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
> >   	}
> >
> > -	igt_describe("Unbinds driver from device, initiates reset"
> > -		     " then rebinds driver to device");
> > -	igt_subtest("unbind-reset-rebind") {
> > -		unbind_reset_rebind(&dev, FLR_RESET);
> > -		healthcheck(&dev);
> > +	igt_subtest_group {
> > +		igt_describe("Unbinds driver from device, initiates reset"
> > +			     " then rebinds driver to device");
> > +		igt_subtest("unbind-reset-rebind") {
> > +			unbind_reset_rebind(&dev, FLR_RESET);
> > +			healthcheck(&dev);
> > +		}
> > +
> > +		igt_describe("Resets device with bound driver");
> > +		igt_subtest("reset-bound") {
> > +			initiate_device_reset(&dev, FLR_RESET);
> > +			/*
> > +			 * Cold reset will initiate card boot sequence again,
> > +			 * therefore let healthcheck() re-epen the dev fd.
> > +			 */
> > +			dev.fds.dev = -1;
> > +			healthcheck(&dev);
> > +		}
> >   	}
> >
> > -	igt_describe("Resets device with bound driver");
> > -	igt_subtest("reset-bound") {
> > -		initiate_device_reset(&dev, FLR_RESET);
> > -		healthcheck(&dev);
> > +	igt_subtest_group {
> > +		igt_fixture {
> > +			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does
> not support any "
> > +				      "pcie slot for cold reset\n");
> > +
> 	igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> > +		}
> > +
> > +		igt_describe("Unbinds driver from device, initiates cold
> reset"
> > +			     " then rebinds driver to device");
> > +		igt_subtest("unbind-cold-reset-rebind") {
> > +			unbind_reset_rebind(&dev, COLD_RESET);
> > +			healthcheck(&dev);
> > +		}
> > +
> > +		igt_describe("Cold Resets device with bound driver");
> > +		igt_subtest("cold-reset-bound") {
> > +			initiate_device_reset(&dev, COLD_RESET);
> > +			healthcheck(&dev);
> > +		}
> >   	}
> >
> >   	igt_fixture {

[igt-dev] ✓ Fi.CI.BAT: success for Cold Reset IGT Test

[Patchwork]

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

== Series Details ==

Series: Cold Reset IGT Test
URL   : https://patchwork.freedesktop.org/series/111245/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12420 -> IGTPW_8144
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/index.html

Participating hosts (35 -> 35)
------------------------------

  Additional (3): bat-kbl-2 bat-jsl-3 bat-dg1-5 
  Missing    (3): fi-kbl-soraka fi-rkl-11600 bat-atsm-1 

Known issues
------------

  Here are the changes found in IGTPW_8144 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap@basic:
    - bat-dg1-5:          NOTRUN -> [SKIP][1] ([i915#4083])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@gem_mmap@basic.html

  * igt@gem_tiled_fence_blits@basic:
    - bat-dg1-5:          NOTRUN -> [SKIP][2] ([i915#4077]) +2 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@gem_tiled_fence_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-dg1-5:          NOTRUN -> [SKIP][3] ([i915#4079]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - bat-dg1-5:          NOTRUN -> [SKIP][4] ([i915#7561])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg1-5:          NOTRUN -> [SKIP][5] ([i915#6621])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [PASS][6] -> [INCOMPLETE][7] ([i915#4785])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12420/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html

  * igt@kms_addfb_basic@basic-x-tiled-legacy:
    - bat-dg1-5:          NOTRUN -> [SKIP][8] ([i915#4212]) +7 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@kms_addfb_basic@basic-x-tiled-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg1-5:          NOTRUN -> [SKIP][9] ([i915#4215])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - bat-dg1-5:          NOTRUN -> [SKIP][10] ([fdo#111827]) +8 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
    - bat-dg1-5:          NOTRUN -> [SKIP][11] ([i915#4103] / [i915#4213])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg1-5:          NOTRUN -> [SKIP][12] ([fdo#109285])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@sprite_plane_onoff:
    - bat-dg1-5:          NOTRUN -> [SKIP][13] ([i915#1072] / [i915#4078]) +3 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@kms_psr@sprite_plane_onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg1-5:          NOTRUN -> [SKIP][14] ([i915#3555])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-read:
    - bat-dg1-5:          NOTRUN -> [SKIP][15] ([i915#3708]) +3 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@prime_vgem@basic-fence-read.html

  * igt@prime_vgem@basic-gtt:
    - bat-dg1-5:          NOTRUN -> [SKIP][16] ([i915#3708] / [i915#4077]) +1 similar issue
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@prime_vgem@basic-gtt.html

  * igt@prime_vgem@basic-userptr:
    - bat-dg1-5:          NOTRUN -> [SKIP][17] ([i915#3708] / [i915#4873])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-dg1-5/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-hsw-4770:        NOTRUN -> [FAIL][18] ([fdo#109271] / [i915#4312] / [i915#5594])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/fi-hsw-4770/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@fbdev@read:
    - {bat-rpls-2}:       [SKIP][19] ([i915#2582]) -> [PASS][20] +4 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12420/bat-rpls-2/igt@fbdev@read.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-rpls-2/igt@fbdev@read.html

  * igt@gem_exec_suspend@basic-s0@smem:
    - {bat-rplp-1}:       [DMESG-WARN][21] ([i915#2867]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12420/bat-rplp-1/igt@gem_exec_suspend@basic-s0@smem.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-rplp-1/igt@gem_exec_suspend@basic-s0@smem.html

  * igt@i915_selftest@live@migrate:
    - {bat-adlp-6}:       [INCOMPLETE][23] ([i915#7308] / [i915#7348]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12420/bat-adlp-6/igt@i915_selftest@live@migrate.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/bat-adlp-6/igt@i915_selftest@live@migrate.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size:
    - fi-bsw-kefka:       [FAIL][25] ([i915#6298]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12420/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#3003]: https://gitlab.freedesktop.org/drm/intel/issues/3003
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5594]: https://gitlab.freedesktop.org/drm/intel/issues/5594
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#7308]: https://gitlab.freedesktop.org/drm/intel/issues/7308
  [i915#7348]: https://gitlab.freedesktop.org/drm/intel/issues/7348
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7071 -> IGTPW_8144

  CI-20190529: 20190529
  CI_DRM_12420: 6e8acf83016689ea4cdfcbbdd994e5127a84e6ef @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8144: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/index.html
  IGT_7071: 0801475083ccb938b1d3b358502ff97fdb435585 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git


Testlist changes
----------------

+igt@device_reset@cold-reset-bound
+igt@device_reset@unbind-cold-reset-rebind
-igt@gem_shrink@shrink-vs-evict

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8144/index.html

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

[igt-dev] ✓ Fi.CI.BAT: success for Cold Reset IGT Test (rev2)

[Patchwork]

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

== Series Details ==

Series: Cold Reset IGT Test (rev2)
URL   : https://patchwork.freedesktop.org/series/111245/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12434 -> IGTPW_8151
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/index.html

Participating hosts (36 -> 39)
------------------------------

  Additional (4): fi-bdw-gvtdvm fi-cfl-guc fi-tgl-dsi bat-dg1-6 
  Missing    (1): fi-ctg-p8600 

Known issues
------------

  Here are the changes found in IGTPW_8151 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_lmem_swapping@verify-random:
    - fi-cfl-guc:         NOTRUN -> [SKIP][1] ([fdo#109271] / [i915#4613]) +3 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/fi-cfl-guc/igt@gem_lmem_swapping@verify-random.html

  * igt@gem_mmap@basic:
    - bat-dg1-6:          NOTRUN -> [SKIP][2] ([i915#4083])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@gem_mmap@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-dg1-6:          NOTRUN -> [SKIP][3] ([i915#4079]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_fence_blits@basic:
    - bat-dg1-6:          NOTRUN -> [SKIP][4] ([i915#4077]) +2 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@gem_tiled_fence_blits@basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - bat-dg1-6:          NOTRUN -> [SKIP][5] ([i915#7561])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg1-6:          NOTRUN -> [SKIP][6] ([i915#6621])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@i915_pm_rps@basic-api.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - fi-bdw-gvtdvm:      NOTRUN -> [INCOMPLETE][7] ([i915#146])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/fi-bdw-gvtdvm/igt@i915_suspend@basic-s2idle-without-i915.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       [PASS][8] -> [FAIL][9] ([fdo#103375]) +1 similar issue
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12434/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg1-6:          NOTRUN -> [SKIP][10] ([i915#4215])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_addfb_basic@tile-pitch-mismatch:
    - bat-dg1-6:          NOTRUN -> [SKIP][11] ([i915#4212]) +7 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@kms_addfb_basic@tile-pitch-mismatch.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - bat-adlp-4:         NOTRUN -> [SKIP][12] ([fdo#111827])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-adlp-4/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-cfl-guc:         NOTRUN -> [SKIP][13] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/fi-cfl-guc/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - bat-dg1-6:          NOTRUN -> [SKIP][14] ([fdo#111827]) +8 similar issues
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@kms_chamelium@hdmi-crc-fast.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-bdw-gvtdvm:      NOTRUN -> [SKIP][15] ([fdo#109271] / [fdo#111827]) +7 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/fi-bdw-gvtdvm/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
    - bat-dg1-6:          NOTRUN -> [SKIP][16] ([i915#4103] / [i915#4213])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html

  * igt@kms_flip@basic-plain-flip:
    - fi-bdw-gvtdvm:      NOTRUN -> [SKIP][17] ([fdo#109271]) +38 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/fi-bdw-gvtdvm/igt@kms_flip@basic-plain-flip.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg1-6:          NOTRUN -> [SKIP][18] ([fdo#109285])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-adlp-4:         NOTRUN -> [SKIP][19] ([i915#3546])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-adlp-4/igt@kms_pipe_crc_basic@suspend-read-crc.html

  * igt@kms_psr@sprite_plane_onoff:
    - bat-dg1-6:          NOTRUN -> [SKIP][20] ([i915#1072] / [i915#4078]) +3 similar issues
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@kms_psr@sprite_plane_onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg1-6:          NOTRUN -> [SKIP][21] ([i915#3555])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-gtt:
    - bat-dg1-6:          NOTRUN -> [SKIP][22] ([i915#3708] / [i915#4077]) +1 similar issue
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@prime_vgem@basic-gtt.html

  * igt@prime_vgem@basic-read:
    - bat-dg1-6:          NOTRUN -> [SKIP][23] ([i915#3708]) +3 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-userptr:
    - fi-cfl-guc:         NOTRUN -> [SKIP][24] ([fdo#109271]) +9 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/fi-cfl-guc/igt@prime_vgem@basic-userptr.html
    - bat-dg1-6:          NOTRUN -> [SKIP][25] ([i915#3708] / [i915#4873])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-dg1-6/igt@prime_vgem@basic-userptr.html

  
#### Possible fixes ####

  * igt@gem_exec_gttfill@basic:
    - fi-pnv-d510:        [FAIL][26] ([i915#7229]) -> [PASS][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12434/fi-pnv-d510/igt@gem_exec_gttfill@basic.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/fi-pnv-d510/igt@gem_exec_gttfill@basic.html

  * igt@gem_exec_suspend@basic-s0@smem:
    - {bat-rplp-1}:       [DMESG-WARN][28] ([i915#2867]) -> [PASS][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12434/bat-rplp-1/igt@gem_exec_suspend@basic-s0@smem.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-rplp-1/igt@gem_exec_suspend@basic-s0@smem.html
    - {fi-ehl-2}:         [DMESG-WARN][30] ([i915#5122]) -> [PASS][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12434/fi-ehl-2/igt@gem_exec_suspend@basic-s0@smem.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/fi-ehl-2/igt@gem_exec_suspend@basic-s0@smem.html

  * igt@i915_selftest@live@gt_pm:
    - {bat-adln-1}:       [DMESG-FAIL][32] ([i915#4258]) -> [PASS][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12434/bat-adln-1/igt@i915_selftest@live@gt_pm.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-adln-1/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@migrate:
    - bat-adlp-4:         [INCOMPLETE][34] ([i915#7308] / [i915#7348]) -> [PASS][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12434/bat-adlp-4/igt@i915_selftest@live@migrate.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-adlp-4/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@requests:
    - {bat-rpls-1}:       [INCOMPLETE][36] ([i915#4983] / [i915#6257]) -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12434/bat-rpls-1/igt@i915_selftest@live@requests.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-rpls-1/igt@i915_selftest@live@requests.html

  * igt@kms_frontbuffer_tracking@basic:
    - {bat-rpls-2}:       [SKIP][38] ([i915#1849]) -> [PASS][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12434/bat-rpls-2/igt@kms_frontbuffer_tracking@basic.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-rpls-2/igt@kms_frontbuffer_tracking@basic.html

  * igt@prime_vgem@basic-fence-flip:
    - {bat-rpls-2}:       [SKIP][40] ([fdo#109295] / [i915#1845] / [i915#3708]) -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12434/bat-rpls-2/igt@prime_vgem@basic-fence-flip.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/bat-rpls-2/igt@prime_vgem@basic-fence-flip.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2411]: https://gitlab.freedesktop.org/drm/intel/issues/2411
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5122]: https://gitlab.freedesktop.org/drm/intel/issues/5122
  [i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#6856]: https://gitlab.freedesktop.org/drm/intel/issues/6856
  [i915#7125]: https://gitlab.freedesktop.org/drm/intel/issues/7125
  [i915#7229]: https://gitlab.freedesktop.org/drm/intel/issues/7229
  [i915#7308]: https://gitlab.freedesktop.org/drm/intel/issues/7308
  [i915#7346]: https://gitlab.freedesktop.org/drm/intel/issues/7346
  [i915#7348]: https://gitlab.freedesktop.org/drm/intel/issues/7348
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7072 -> IGTPW_8151

  CI-20190529: 20190529
  CI_DRM_12434: 985931f189852e505af573997ddbf63bfbe2570b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8151: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/index.html
  IGT_7072: 69ba7163475925cdc69aebbdfa0e87453ae165c7 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8151/index.html

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

[igt-dev] ✓ Fi.CI.BAT: success for Cold Reset IGT Test (rev3)

[Patchwork]

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

== Series Details ==

Series: Cold Reset IGT Test (rev3)
URL   : https://patchwork.freedesktop.org/series/111245/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12437 -> IGTPW_8159
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/index.html

Participating hosts (30 -> 30)
------------------------------

  Additional (1): bat-dg1-6 
  Missing    (1): fi-tgl-dsi 

Known issues
------------

  Here are the changes found in IGTPW_8159 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap@basic:
    - bat-dg1-6:          NOTRUN -> [SKIP][1] ([i915#4083])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@gem_mmap@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-dg1-6:          NOTRUN -> [SKIP][2] ([i915#4079]) +1 similar issue
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_fence_blits@basic:
    - bat-dg1-6:          NOTRUN -> [SKIP][3] ([i915#4077]) +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@gem_tiled_fence_blits@basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - bat-dg1-6:          NOTRUN -> [SKIP][4] ([i915#7561])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg1-6:          NOTRUN -> [SKIP][5] ([i915#6621])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@i915_pm_rps@basic-api.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg1-6:          NOTRUN -> [SKIP][6] ([i915#4215])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_addfb_basic@tile-pitch-mismatch:
    - bat-dg1-6:          NOTRUN -> [SKIP][7] ([i915#4212]) +7 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@kms_addfb_basic@tile-pitch-mismatch.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - bat-dg1-6:          NOTRUN -> [SKIP][8] ([fdo#111827]) +8 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@kms_chamelium@hdmi-crc-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
    - bat-dg1-6:          NOTRUN -> [SKIP][9] ([i915#4103] / [i915#4213])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg1-6:          NOTRUN -> [SKIP][10] ([fdo#109285])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-hdmi-a-2:
    - fi-icl-u2:          [PASS][11] -> [DMESG-WARN][12] ([i915#2867])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12437/fi-icl-u2/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-hdmi-a-2.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/fi-icl-u2/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-hdmi-a-2.html

  * igt@kms_psr@sprite_plane_onoff:
    - bat-dg1-6:          NOTRUN -> [SKIP][13] ([i915#1072] / [i915#4078]) +3 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@kms_psr@sprite_plane_onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg1-6:          NOTRUN -> [SKIP][14] ([i915#3555])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-gtt:
    - bat-dg1-6:          NOTRUN -> [SKIP][15] ([i915#3708] / [i915#4077]) +1 similar issue
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@prime_vgem@basic-gtt.html

  * igt@prime_vgem@basic-read:
    - bat-dg1-6:          NOTRUN -> [SKIP][16] ([i915#3708]) +3 similar issues
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-userptr:
    - bat-dg1-6:          NOTRUN -> [SKIP][17] ([i915#3708] / [i915#4873])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg1-6/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-rkl-11600:       NOTRUN -> [FAIL][18] ([i915#4312])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/fi-rkl-11600/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [DMESG-FAIL][19] ([i915#5334]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12437/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@hugepages:
    - {bat-rpls-2}:       [DMESG-WARN][21] ([i915#5278]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12437/bat-rpls-2/igt@i915_selftest@live@hugepages.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-rpls-2/igt@i915_selftest@live@hugepages.html

  * igt@i915_selftest@live@migrate:
    - {bat-adlp-6}:       [INCOMPLETE][23] ([i915#7348]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12437/bat-adlp-6/igt@i915_selftest@live@migrate.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-adlp-6/igt@i915_selftest@live@migrate.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size:
    - fi-bsw-kefka:       [FAIL][25] ([i915#6298]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12437/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2:
    - {bat-dg2-11}:       [FAIL][27] ([i915#7336]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12437/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#5278]: https://gitlab.freedesktop.org/drm/intel/issues/5278
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7336]: https://gitlab.freedesktop.org/drm/intel/issues/7336
  [i915#7348]: https://gitlab.freedesktop.org/drm/intel/issues/7348
  [i915#7359]: https://gitlab.freedesktop.org/drm/intel/issues/7359
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7072 -> IGTPW_8159

  CI-20190529: 20190529
  CI_DRM_12437: a25c2a69c2f1c9f8e8bc07df1b7185c8749692f3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8159: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/index.html
  IGT_7072: 69ba7163475925cdc69aebbdfa0e87453ae165c7 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git


Testlist changes
----------------

+igt@device_reset@cold-reset-bound
+igt@device_reset@unbind-cold-reset-rebind

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8159/index.html

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

[igt-dev] [PATCH i-g-t v4 0/5] Cold Reset IGT Test

[Anshuman Gupta]

Adding Cold Reset IGT support

v4:
- Address the review comment from badal and kmail.

Anshuman Gupta (5):
  lib/igt_pci: helpers to get PCI capabilities offset
  lib/igt_pci: Add PCIe slot cap
  lib/igt_pm: Refactor get firmware_node fd
  test/device_reset: Refactor initiate_device_reset
  tests/device_reset: Add cold reset IGT test

 lib/igt_pci.c        |  56 ++++++++++++++++
 lib/igt_pci.h        |  28 ++++++++
 lib/igt_pm.c         |  77 ++++++++++++++++++----
 lib/igt_pm.h         |   1 +
 lib/meson.build      |   1 +
 tests/device_reset.c | 154 +++++++++++++++++++++++++++++++++++++++++--
 6 files changed, 300 insertions(+), 17 deletions(-)
 create mode 100644 lib/igt_pci.c
 create mode 100644 lib/igt_pci.h

-- 
2.25.1

[igt-dev] [PATCH i-g-t v4 5/5] tests/device_reset: Add cold reset IGT test

[Anshuman Gupta]

Add cold-reset IGT subtest, IGT subtest will use
/sys/bus/pci/slots/$SUN/power sysfs in order to
initiate a cold reset sequence. $SUN value will be
retrieved from PCIe device ACPI firmware node.

v2:
- %s/igt_require(fd > 0)/igt_assert(errno >0)
v3:
- Added Slot Power Controller capability check. [Aravind]
- Check whether slot is powered down before powering up. [Aravind]
v4:
- Check power ctrl presence before slot presence to save CI
  execution time.
v5:
- Review comment addressal about igt assertion,
  return type and null terminated string. [Kamil]
- Remove the FLR_RESET subgroup. [Kmail]
- Moved the misplaced code comment. [Kamil]

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
---
 lib/igt_pm.c         |  41 +++++++++++++
 lib/igt_pm.h         |   1 +
 tests/device_reset.c | 136 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 177 insertions(+), 1 deletion(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 1e144a7397..26e8c9f05f 100644
--- a/lib/igt_pm.c
+++ b/lib/igt_pm.c
@@ -877,6 +877,47 @@ static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
 	return dir;
 }
 
+/**
+ * igt_pm_get_pcie_acpihp_slot:
+ * @pci_dev: pci bridge device.
+ * Get pci bridge acpi hotplug slot number, if bridge's ACPI firmware_node
+ * handle supports _SUN method.
+ *
+ * Returns:
+ * PCIe bridge Slot number.
+ * Returns -ENOENT number in case firmware_node/sun is not supported by the
+ * bridge.
+ */
+int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev)
+{
+	int firmware_node_fd, fd, n_read;
+	char sun[8];
+
+	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
+
+	if (firmware_node_fd < 0 && errno == ENOENT)
+		return -ENOENT;
+
+	igt_require(firmware_node_fd > 0);
+
+	fd = openat(firmware_node_fd, "sun", O_RDONLY);
+	if (fd < 0 && errno == ENOENT) {
+		close(firmware_node_fd);
+		return -ENOENT;
+	}
+
+	igt_assert_f(fd > 0, "failed to open real_power_state, errno=%d\n", errno);
+
+	n_read = read(fd, sun, sizeof(sun));
+
+	close(firmware_node_fd);
+	close(fd);
+	igt_assert(n_read > 0 && n_read < sizeof(sun));
+	sun[n_read] = '\0';
+
+	return strtol(sun, NULL, 10);
+}
+
 /**
  * igt_pm_acpi_d3cold_supported:
  * @pci_dev: root port pci_dev.
diff --git a/lib/igt_pm.h b/lib/igt_pm.h
index e81fceb897..f65b960c38 100644
--- a/lib/igt_pm.h
+++ b/lib/igt_pm.h
@@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
 bool igt_pm_dmc_loaded(int debugfs);
 bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
 bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
+int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
 bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
 enum igt_acpi_d_state
 igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev);
diff --git a/tests/device_reset.c b/tests/device_reset.c
index 0c477a02c0..39ee8dca9f 100644
--- a/tests/device_reset.c
+++ b/tests/device_reset.c
@@ -9,7 +9,9 @@
 
 #include "i915/gem.h"
 #include "igt.h"
+#include "igt_device.h"
 #include "igt_device_scan.h"
+#include "igt_pci.h"
 #include "igt_sysfs.h"
 #include "igt_kmod.h"
 
@@ -33,6 +35,7 @@ struct device_fds {
 		int dev;
 		int dev_dir;
 		int drv_dir;
+		int slot_dir; /* pci hotplug slots fd */
 	} fds;
 	char dev_bus_addr[DEV_BUS_ADDR_LEN];
 	bool snd_unload;
@@ -62,6 +65,74 @@ static int open_driver_sysfs_dir(int fd)
 	return __open_sysfs_dir(fd, "device/driver");
 }
 
+static bool is_pci_power_ctrl_present(struct pci_device *dev)
+{
+	int offset;
+	uint32_t slot_cap;
+
+	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
+	igt_require_f(offset > 0, "PCI Express Capability not found\n");
+	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset + PCI_SLOT_CAP_OFFSET));
+	igt_debug("slot cap register 0x%x\n", slot_cap);
+
+	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT;
+}
+
+static bool is_slot_power_ctrl_present(int fd)
+{
+	struct pci_device *root;
+
+	/*
+	 * Card root port Slot Capabilities Register
+	 * determines Power Controller Presence.
+	 */
+	root = igt_device_get_pci_root_port(fd);
+	return is_pci_power_ctrl_present(root);
+}
+
+static int open_slot_sysfs_dir(int fd)
+{
+	struct pci_device *pci_dev = NULL;
+	int slot_fd = -1, slot;
+	char slot_fd_path[PATH_MAX];
+
+	/* Don't search for slot if root port doesn't support power ctrl */
+	if (!is_slot_power_ctrl_present(fd))
+		return -ENOTSUP;
+
+	pci_dev = igt_device_get_pci_device(fd);
+	igt_require(pci_dev);
+
+	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
+		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
+		if (slot == -ENOENT) {
+			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x does not support acpihp slot\n",
+				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
+			continue;
+		}
+
+		/*
+		 * Upon getting the valid acpihp slot number break the loop.
+		 * It is the desired acpihp slot for gfx card.
+		 */
+		if (slot > 0) {
+			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x associated acpihp slot %d\n",
+				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func, slot);
+			break;
+		}
+	}
+
+	if (!pci_dev)
+		return -1;
+
+	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
+	slot_fd = open(slot_fd_path, O_RDONLY);
+	if (slot_fd < 0)
+		return -errno;
+
+	return slot_fd;
+}
+
 /**
  * device_sysfs_path:
  * @fd: opened device file descriptor
@@ -124,6 +195,8 @@ static void init_device_fds(struct device_fds *dev)
 
 	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
 	igt_assert_fd(dev->fds.drv_dir);
+
+	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
 }
 
 static int close_if_opened(int *fd)
@@ -142,6 +215,7 @@ static void cleanup_device_fds(struct device_fds *dev)
 	igt_ignore_warn(close_if_opened(&dev->fds.dev));
 	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
 	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
+	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
 }
 
 /**
@@ -179,6 +253,35 @@ static bool is_sysfs_reset_supported(int fd)
 	return true;
 }
 
+/**
+ * is_sysfs_cold_reset_supported:
+ * @fd: opened device file descriptor
+ *
+ * Check if device supports cold reset based on sysfs file presence.
+ *
+ * Returns:
+ * True if device supports reset, false otherwise.
+ */
+static bool is_sysfs_cold_reset_supported(int slot_fd)
+{
+	struct stat st;
+	int rc;
+	int cold_reset_fd = -1;
+
+	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
+
+	if (cold_reset_fd < 0)
+		return false;
+
+	rc = fstat(cold_reset_fd, &st);
+	close(cold_reset_fd);
+
+	if (rc || !S_ISREG(st.st_mode))
+		return false;
+
+	return true;
+}
+
 /* Unbind the driver from the device */
 static void driver_unbind(struct device_fds *dev)
 {
@@ -231,8 +334,13 @@ static void initiate_device_reset(struct device_fds *dev, enum reset type)
 {
 	igt_debug("reset device\n");
 
-	if (type == FLR_RESET)
+	if (type == FLR_RESET) {
 		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
+	} else if (type == COLD_RESET) {
+		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
+		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir, "power"));
+		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
+	}
 
 }
 
@@ -324,6 +432,32 @@ igt_main
 		healthcheck(&dev);
 	}
 
+	igt_subtest_group {
+		igt_fixture {
+			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
+				      "pcie slot for cold reset\n");
+			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
+		}
+
+		igt_describe("Unbinds driver from device, initiates cold reset"
+			     " then rebinds driver to device");
+		igt_subtest("unbind-cold-reset-rebind") {
+			unbind_reset_rebind(&dev, COLD_RESET);
+			healthcheck(&dev);
+		}
+
+		igt_describe("Cold Resets device with bound driver");
+		igt_subtest("cold-reset-bound") {
+			initiate_device_reset(&dev, COLD_RESET);
+			/*
+			 * Cold reset will initiate card boot sequence again,
+			 * therefore let healthcheck() re-epen the dev fd.
+			 */
+			dev.fds.dev = -1;
+			healthcheck(&dev);
+		}
+	}
+
 	igt_fixture {
 		cleanup_device_fds(&dev);
 	}
-- 
2.25.1

Re: [igt-dev] [PATCH i-g-t v4 5/5] tests/device_reset: Add cold reset IGT test

[Nilawar, Badal]

Hi Anshuman,


On 07-12-2022 15:55, Anshuman Gupta wrote:
> Add cold-reset IGT subtest, IGT subtest will use
> /sys/bus/pci/slots/$SUN/power sysfs in order to
> initiate a cold reset sequence. $SUN value will be
> retrieved from PCIe device ACPI firmware node.
> 
> v2:
> - %s/igt_require(fd > 0)/igt_assert(errno >0)
> v3:
> - Added Slot Power Controller capability check. [Aravind]
> - Check whether slot is powered down before powering up. [Aravind]
> v4:
> - Check power ctrl presence before slot presence to save CI
>    execution time.
> v5:
> - Review comment addressal about igt assertion,
>    return type and null terminated string. [Kamil]
> - Remove the FLR_RESET subgroup. [Kmail]
> - Moved the misplaced code comment. [Kamil]
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
Looks good to me. You can keep my RB.

Regards,
Badal
> ---
>   lib/igt_pm.c         |  41 +++++++++++++
>   lib/igt_pm.h         |   1 +
>   tests/device_reset.c | 136 ++++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 177 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 1e144a7397..26e8c9f05f 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -877,6 +877,47 @@ static int igt_pm_open_pci_firmware_node(struct pci_device *pci_dev)
>   	return dir;
>   }
>   
> +/**
> + * igt_pm_get_pcie_acpihp_slot:
> + * @pci_dev: pci bridge device.
> + * Get pci bridge acpi hotplug slot number, if bridge's ACPI firmware_node
> + * handle supports _SUN method.
> + *
> + * Returns:
> + * PCIe bridge Slot number.
> + * Returns -ENOENT number in case firmware_node/sun is not supported by the
> + * bridge.
> + */
> +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev)
> +{
> +	int firmware_node_fd, fd, n_read;
> +	char sun[8];
> +
> +	firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> +
> +	if (firmware_node_fd < 0 && errno == ENOENT)
> +		return -ENOENT;
> +
> +	igt_require(firmware_node_fd > 0);
> +
> +	fd = openat(firmware_node_fd, "sun", O_RDONLY);
> +	if (fd < 0 && errno == ENOENT) {
> +		close(firmware_node_fd);
> +		return -ENOENT;
> +	}
> +
> +	igt_assert_f(fd > 0, "failed to open real_power_state, errno=%d\n", errno);
> +
> +	n_read = read(fd, sun, sizeof(sun));
> +
> +	close(firmware_node_fd);
> +	close(fd);
> +	igt_assert(n_read > 0 && n_read < sizeof(sun));
> +	sun[n_read] = '\0';
> +
> +	return strtol(sun, NULL, 10);
> +}
> +
>   /**
>    * igt_pm_acpi_d3cold_supported:
>    * @pci_dev: root port pci_dev.
> diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> index e81fceb897..f65b960c38 100644
> --- a/lib/igt_pm.h
> +++ b/lib/igt_pm.h
> @@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
>   bool igt_pm_dmc_loaded(int debugfs);
>   bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
>   bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
> +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
>   bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
>   enum igt_acpi_d_state
>   igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev);
> diff --git a/tests/device_reset.c b/tests/device_reset.c
> index 0c477a02c0..39ee8dca9f 100644
> --- a/tests/device_reset.c
> +++ b/tests/device_reset.c
> @@ -9,7 +9,9 @@
>   
>   #include "i915/gem.h"
>   #include "igt.h"
> +#include "igt_device.h"
>   #include "igt_device_scan.h"
> +#include "igt_pci.h"
>   #include "igt_sysfs.h"
>   #include "igt_kmod.h"
>   
> @@ -33,6 +35,7 @@ struct device_fds {
>   		int dev;
>   		int dev_dir;
>   		int drv_dir;
> +		int slot_dir; /* pci hotplug slots fd */
>   	} fds;
>   	char dev_bus_addr[DEV_BUS_ADDR_LEN];
>   	bool snd_unload;
> @@ -62,6 +65,74 @@ static int open_driver_sysfs_dir(int fd)
>   	return __open_sysfs_dir(fd, "device/driver");
>   }
>   
> +static bool is_pci_power_ctrl_present(struct pci_device *dev)
> +{
> +	int offset;
> +	uint32_t slot_cap;
> +
> +	offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
> +	igt_require_f(offset > 0, "PCI Express Capability not found\n");
> +	igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset + PCI_SLOT_CAP_OFFSET));
> +	igt_debug("slot cap register 0x%x\n", slot_cap);
> +
> +	return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT;
> +}
> +
> +static bool is_slot_power_ctrl_present(int fd)
> +{
> +	struct pci_device *root;
> +
> +	/*
> +	 * Card root port Slot Capabilities Register
> +	 * determines Power Controller Presence.
> +	 */
> +	root = igt_device_get_pci_root_port(fd);
> +	return is_pci_power_ctrl_present(root);
> +}
> +
> +static int open_slot_sysfs_dir(int fd)
> +{
> +	struct pci_device *pci_dev = NULL;
> +	int slot_fd = -1, slot;
> +	char slot_fd_path[PATH_MAX];
> +
> +	/* Don't search for slot if root port doesn't support power ctrl */
> +	if (!is_slot_power_ctrl_present(fd))
> +		return -ENOTSUP;
> +
> +	pci_dev = igt_device_get_pci_device(fd);
> +	igt_require(pci_dev);
> +
> +	while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
> +		slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
> +		if (slot == -ENOENT) {
> +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x does not support acpihp slot\n",
> +				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
> +			continue;
> +		}
> +
> +		/*
> +		 * Upon getting the valid acpihp slot number break the loop.
> +		 * It is the desired acpihp slot for gfx card.
> +		 */
> +		if (slot > 0) {
> +			igt_debug("Bridge PCI device %04x:%02x:%02x.%01x associated acpihp slot %d\n",
> +				  pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func, slot);
> +			break;
> +		}
> +	}
> +
> +	if (!pci_dev)
> +		return -1;
> +
> +	snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
> +	slot_fd = open(slot_fd_path, O_RDONLY);
> +	if (slot_fd < 0)
> +		return -errno;
> +
> +	return slot_fd;
> +}
> +
>   /**
>    * device_sysfs_path:
>    * @fd: opened device file descriptor
> @@ -124,6 +195,8 @@ static void init_device_fds(struct device_fds *dev)
>   
>   	dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
>   	igt_assert_fd(dev->fds.drv_dir);
> +
> +	dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
>   }
>   
>   static int close_if_opened(int *fd)
> @@ -142,6 +215,7 @@ static void cleanup_device_fds(struct device_fds *dev)
>   	igt_ignore_warn(close_if_opened(&dev->fds.dev));
>   	igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
>   	igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
> +	igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
>   }
>   
>   /**
> @@ -179,6 +253,35 @@ static bool is_sysfs_reset_supported(int fd)
>   	return true;
>   }
>   
> +/**
> + * is_sysfs_cold_reset_supported:
> + * @fd: opened device file descriptor
> + *
> + * Check if device supports cold reset based on sysfs file presence.
> + *
> + * Returns:
> + * True if device supports reset, false otherwise.
> + */
> +static bool is_sysfs_cold_reset_supported(int slot_fd)
> +{
> +	struct stat st;
> +	int rc;
> +	int cold_reset_fd = -1;
> +
> +	cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
> +
> +	if (cold_reset_fd < 0)
> +		return false;
> +
> +	rc = fstat(cold_reset_fd, &st);
> +	close(cold_reset_fd);
> +
> +	if (rc || !S_ISREG(st.st_mode))
> +		return false;
> +
> +	return true;
> +}
> +
>   /* Unbind the driver from the device */
>   static void driver_unbind(struct device_fds *dev)
>   {
> @@ -231,8 +334,13 @@ static void initiate_device_reset(struct device_fds *dev, enum reset type)
>   {
>   	igt_debug("reset device\n");
>   
> -	if (type == FLR_RESET)
> +	if (type == FLR_RESET) {
>   		igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> +	} else if (type == COLD_RESET) {
> +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
> +		igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir, "power"));
> +		igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
> +	}
>   
>   }
>   
> @@ -324,6 +432,32 @@ igt_main
>   		healthcheck(&dev);
>   	}
>   
> +	igt_subtest_group {
> +		igt_fixture {
> +			igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does not support any "
> +				      "pcie slot for cold reset\n");
> +			igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> +		}
> +
> +		igt_describe("Unbinds driver from device, initiates cold reset"
> +			     " then rebinds driver to device");
> +		igt_subtest("unbind-cold-reset-rebind") {
> +			unbind_reset_rebind(&dev, COLD_RESET);
> +			healthcheck(&dev);
> +		}
> +
> +		igt_describe("Cold Resets device with bound driver");
> +		igt_subtest("cold-reset-bound") {
> +			initiate_device_reset(&dev, COLD_RESET);
> +			/*
> +			 * Cold reset will initiate card boot sequence again,
> +			 * therefore let healthcheck() re-epen the dev fd.
> +			 */
> +			dev.fds.dev = -1;
> +			healthcheck(&dev);
> +		}
> +	}
> +
>   	igt_fixture {
>   		cleanup_device_fds(&dev);
>   	}