Re: [PATCH i-g-t v3 1/2] lib/igt_pm: Add CPU idle state control for vblank timing stability

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

On Mon, 2026-07-13 at 14:08 +0200, Kamil Konieczny wrote:
> Hi Jason-JH,
> On 2026-07-10 at 17:52:47 +0800, Jason-JH Lin wrote:
> > Add igt_pm_disable_cpu_deep_sleep() and
> > igt_pm_enable_cpu_deep_sleep()
> > APIs to control deep CPU idle states (C-states) via sysfs. This
> > prevents
> > IRQ latency spikes that affect vblank timestamp acquisition.
> > 
> > The functions control state1 and deeper states on all CPUs while
> > keeping
> > state0 (WFI) unchanged, providing a balance between timing accuracy
> > and
> > power efficiency.
> > 
> > Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
> > ---
> >  lib/igt_pm.c | 75
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_pm.h |  2 ++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> > index 64b15dd08302..7a8ad8ae3dea 100644
> > --- a/lib/igt_pm.c
> > +++ b/lib/igt_pm.c
> > @@ -1713,3 +1713,78 @@ unsigned int igt_read_pkgc_counter(int
> > debugfs_root_fd)
> >  
> >  	return igt_get_dc_counter(str);
> >  }
> > +
> > +static bool igt_pm_cpu_deep_sleep_control(bool disable)
> 
> Please do not use bool params. See below.
> 
> > +{
> > +	char cpu_path[256];
> > +	char state_path[512];
> > +	int cpu, state;
> > +	int count = 0;
> > +	FILE *fp;
> > +	const char *value = disable ? "1" : "0";
> 
> Pass it as param.
> 
> > +	const char *action = disable ? "Disabled" : "Restored";
> 
> Same here:
> 
> static bool igt_pm_cpu_deep_sleep_control(char *val, char *msg)
> 

OK, I'll modify it.

> > +
> > +	/* Control state1 and deeper for all CPUs */
> > +	for (cpu = 0; cpu < 256; cpu++) {
> 
> Should you read this '256' dynamically? Or make it a define or
> const int. Another idea below.
> 
> > +		snprintf(cpu_path, sizeof(cpu_path),
> > +			 "/sys/devices/system/cpu/cpu%d", cpu);
> > +
> > +		/* Check if CPU exists */
> > +		if (access(cpu_path, F_OK) != 0)
> > +			break;
> 
> Why not just readdir and match files starting with "cpu%d"?

OK, I'll change to readdir instead of using a const int for cpu%d.

> 
> > +
> > +		/* Control state1 and deeper (keep state0 WFI
> > unchanged) */
> > +		for (state = 1; state < 10; state++) {
> > +			snprintf(state_path, sizeof(state_path),
> > +				 "%s/cpuidle/state%d/disable",
> > cpu_path, state);
> > +
> > +			if (access(state_path, W_OK) != 0)
> > +				break;
> > +
> > +			/* Write '1' to disable, '0' to enable */
> > +			fp = fopen(state_path, "w");
> > +			if (fp) {
> > +				fprintf(fp, "%s", value);
> > +				fclose(fp);
> > +				count++;
> > +			}
> > +		}
> > +	}
> > +
> > +	if (count > 0)
> > +		igt_info("%s deep CPU idle states for %d CPU/state
> > combinations\n",
> > +			 action, count);
> > +	else
> > +		igt_warn("No CPU idle states found to control\n");
> 
> Why warning here? What if by default idle states are turned off?
> Btw when you run this in exit proc, any warn is unexpected and
> useless.

OK, I'll remove the warning here.

> 
> > +
> > +	return count > 0;
> > +}
> > +
> > +/**
> > + * igt_pm_disable_cpu_deep_sleep:
> > + *
> > + * Disables deep CPU idle states (C-states) to prevent IRQ latency
> > spikes
> > + * during vblank events. This is a system-wide operation that
> > affects all CPUs.
> > + *
> > + * Disables state1 and deeper states while keeping state0 (WFI)
> > unchanged
> > + * to ensure stable vblank timing.
> > + *
> > + * Returns: true on success, false on failure
> > + */
> > +bool igt_pm_disable_cpu_deep_sleep(void)
> > +{
> > +	return igt_pm_cpu_deep_sleep_control(true);
> 
> igt_pm_cpu_deep_sleep_control("1", "Disabled")
> 
> > +}
> > +
> > +/**
> > + * igt_pm_enable_cpu_deep_sleep:
> > + *
> > + * Re-enables deep CPU idle states (C-states) that were previously
> > disabled
> > + * by igt_pm_disable_cpu_deep_sleep(). This restores power-saving
> > idle states.
> > + *
> > + * Returns: true on success, false on failure
> > + */
> > +bool igt_pm_enable_cpu_deep_sleep(void)
> > +{
> > +	return igt_pm_cpu_deep_sleep_control(false);
> 
> igt_pm_cpu_deep_sleep_control("0", "Enabled")
> 

I think we can use one parameter to make it simpler and reduce
confusion.

E,g.
igt_pm_cpu_deep_sleep_control("Enabled");

What do you think?

Regards,
Jason-JH Lin

> Regards,
> Kamil