quo / ithc-linux

Linux driver for Intel Touch Host Controller
37 stars 7 forks source link

Should it be `PM_QOS_CPU_LATENCY_DEFAULT_VALUE` instead of `PM_QOS_DEFAULT_VALUE`? #27

Closed christophfink closed 1 year ago

christophfink commented 1 year ago

I’m trying to get my CPU to throttle at least a bit while using touch, and as part of that try to understand what ithc does. I noticed that it calls cpu_latency_qos_update_request(), and resets the value after a timeout. However, if I understand correctly, this is currently resetting to the wrong default value.

See torvalds/linux:include/linux/pm_qos.h, and src/ithc-main.c#L248

As stated elsewhere, I’m travelling and did not bring my Surface with me, I’ll test this once I return and submit a merge request after that.

Please advice whether I understood your code correctly ;)

christophfink commented 1 year ago

Also src/ithc-main.c#543

StollD commented 1 year ago

I have no experience with the QoS subsystem but I believe that PM_QOS_DEFAULT_VALUE is correct. But also, you are not wrong.

PM_QOS_DEFAULT_VALUE is a magic value that automatically applies the configured default value: https://github.com/torvalds/linux/blob/master/kernel/power/qos.c#L107-L110.

This value is initialized with PM_QOS_CPU_LATENCY_DEFAULT_VALUE here: https://github.com/torvalds/linux/blob/master/kernel/power/qos.c#L218. I dont really understand under which circumstances this value can change, but it seems to me that the behaviour before and after you change should be the same.

quo commented 1 year ago

Yeah, PM_QOS_DEFAULT_VALUE is fine.

$ git grep 'cpu_latency_qos_update_request.*PM_QOS_DEFAULT_VALUE'
arch/x86/platform/intel/iosf_mbi.c: cpu_latency_qos_update_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
drivers/gpu/drm/i915/display/intel_dp_aux.c:    cpu_latency_qos_update_request(&intel_dp->pm_qos, PM_QOS_DEFAULT_VALUE);
drivers/staging/media/atomisp/pci/atomisp_v4l2.c:   cpu_latency_qos_update_request(&isp->pm_qos, PM_QOS_DEFAULT_VALUE);
sound/soc/intel/atom/sst/sst_loader.c:  cpu_latency_qos_update_request(sst_drv_ctx->qos, PM_QOS_DEFAULT_VALUE);
$ git grep 'cpu_latency_qos_update_request.*PM_QOS_CPU_LATENCY_DEFAULT_VALUE'
christophfink commented 1 year ago

Alright, I missed that somehow. Thanks!

quo commented 1 year ago

There's probably a better way to handle the pm_qos stuff though. I don't really know how devices handle this usually, but maybe one of the unknown IRQ bits is for a DMA start IRQ, so I could set latency to 0 only during the actual DMA transfer. Might try that when I have some time.