Re: [PATCH i-g-t v1] Add support for runtime pm for xe driver

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Mar 06, 2025 at 10:25:16AM -0600, Lucas De Marchi wrote:
> On Thu, Mar 06, 2025 at 10:06:01AM -0600, Gupta, Anshuman wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: De Marchi, Lucas <lucas.demarchi@intel.com>
> > > Sent: Thursday, March 6, 2025 9:20 PM
> > > To: Kamil Konieczny <kamil.konieczny@linux.intel.com>; Purkait, Soham
> > > <soham.purkait@intel.com>; igt-dev@lists.freedesktop.org; Tauro, Riana
> > > <riana.tauro@intel.com>; Belgaumkar, Vinay <vinay.belgaumkar@intel.com>;
> > > Gupta, Anshuman <anshuman.gupta@intel.com>; Vivi, Rodrigo
> > > <rodrigo.vivi@intel.com>
> > > Subject: Re: [PATCH i-g-t v1] Add support for runtime pm for xe driver
> > > 
> > > On Thu, Mar 06, 2025 at 02:51:00PM +0100, Kamil Konieczny wrote:
> > > >Hi Soham,
> > > >On 2025-03-06 at 16:57:54 +0530, Soham Purkait wrote:
> > > >
> > > >add prefix 'tools/intel_pm_rpm:' in subject, so it will be:
> > > >
> > > >[PATCH i-g-t v1] tools/intel_pm_rpm: Add support for Xe driver
> > > >
> > > >> Add support for runtime power management for Xe driver.
> > > >>
> > > >
> > > >Add here your s-o-b:
> > > >
> > > >Soham Purkait <soham.purkait@intel.com>
> > > >
> > > >Please use checkpatch.pl for some obvious corrections like this, you
> > > >can find this script in Linux kernel sources, also read CONTRIBUTING.md
> > > >in i-g-t sources for some options for checkpatch.
> > > >
> > > >> ---
> > > >>  tools/intel_pm_rpm.c | 2 +-
> > > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/tools/intel_pm_rpm.c b/tools/intel_pm_rpm.c index
> > > >> 08c25ca8a..945247d2e 100644
> > > >> --- a/tools/intel_pm_rpm.c
> > > >> +++ b/tools/intel_pm_rpm.c
> > > >> @@ -151,7 +151,7 @@ int main(int argc, char *argv[])
> > > >>  			goto exit;
> > > >>  		}
> > > >>  	} else {
> > > >> -		if (!igt_device_find_first_i915_discrete_card(&card)) {
> > > >> +		if (!igt_device_find_first_i915_discrete_card(&card) &&
> > > >> +!igt_device_find_first_xe_discrete_card(&card)) {
> > > >
> > > >Split this line:
> > > >		if (!igt_device_find_first_i915_discrete_card(&card) &&
> > > >		    !igt_device_find_first_xe_discrete_card(&card)) {
> > > 
> > > I'd rather stop spreading this. Particularly in **tools**.
> > > If env_device is NULL, why don 't we rather build a match string with the thing
> > > we want aka
> > > 
> > > 	"pci:vendor=8086,device=discrete"
> > > 
> > > I'd replace in tools the current uses of
> > > igt_device_find_first_i915_discrete_card() with:
> > > 
> > > 	"pci:vendor=8086,device=discrete,driver=i915"
> > > 
> > > ... and implement the driver= match in igt_device_scan if it's not there. Then if
> > > it works with either i915 or xe, we remove it.
> > > 
> > > I also think the tool is being less than helpful by trying to work with discrete-
> > > only device.
> > This tool implemented with intention put the discrete cards in runtime d3cold as it requires to tune each pcie device in the topology.
> > As d3cold only supported on  dgpu, we don't need this tool for igpu.
> 
> ok, but should this be a check by discrete for each driver or checking
> power state and d3cold_allowed in the pci layer?
> 
> What do you do if the user passed 00:02.0 as device?

This should be something similar we have on xe_pm for d3cold.
It should skip if we don't have the firmware_node/real_power_state.
But then if the goal is d3cold, we should also check this value to ensure
we are in d3cold.

So, something still seems off to me in this patch. Or we need a bigger
refactor in this test case here or improve the xe_pm with the missed case
*there* with what we are trying to cover with this patch here.

> 
> Lucas De Marchi
> 
> > Thanks,
> > Anshuman.
> > > 
> > > It looks like SET_I915_AUTOSUSPEND_DELAY also needs to be abstracted?
> > > 
> > > 
> > > Lucas De Marchi
> > > 
> > > 
> > > >
> > > >Btw should we also add support for other cards in multi-GPU case? If
> > > >yes this is some more work for another patch.
> > > >
> > > >Regards,
> > > >Kamil
> > > >
> > > >>  			igt_warn("No discrete gpu found\n");
> > > >>  			ret = EXIT_FAILURE;
> > > >>  			goto exit;
> > > >> --
> > > >> 2.34.1
> > > >>