On 2019/4/10 下午9:03, Benjamin Tissoires wrote:
On Wed, Apr 10, 2019 at 1:43 PM hwang4<hui.wang@xxxxxxxxxxxxx> wrote:
On 2019/4/9 下午11:42, Benjamin Tissoires wrote:
On Tue, Apr 9, 2019 at 3:32 PM hwang4<hui.wang@xxxxxxxxxxxxx> wrote:
On 2019/4/8 下午11:44, Benjamin Tissoires wrote:
Hi,
On Mon, Apr 8, 2019 at 6:07 AM Hui Wang<hui.wang@xxxxxxxxxxxxx> wrote:
We disabled the runtime PM for the synaptics touchpad (06cb:7e7e),
then it worked, but after S3, the touchpad didn't work again.
gen-1 is replaced by gen-2 already, we need to revert the commit
74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad")
OK, great. Thanks for the clarification.
So if gen-1 were only engineering samples, do we need to fix them at
all in the kernel?
No need to fix gen-1 in the kernel, I will send a revert patch for
74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad").
This patch should be applied to Goodix touchpad (27c6:01f0) if the
touchpad on Kai-Heng's machine doesn't work after s3. I need to verify
if that touchpad still work or not after s3, but I guess it will not
work since in the system resume it set the touchpad to be
I2C_HID_PWR_ON, then immediately it set the working mode via
hid->driver->reset_resume(hid), there is no delay between POW_ON and
setting work mode. At least on my machine, if there is no delay, the
working mode can't be set successfully and hid-multitouch can't parse
the data report.
I set the delay to 100ms based on my testing result. On my machine, I
originally set the delay to 10/20/30/40ms but the touchpad can't report
the data that hid-multitouch needed, After I set the delay to 50ms, the
driver works occasionally. Then to be safe, I set the delay to 100ms in
the patch. And this afternoon, I found this comment from Goodix " Our IC
needs about 60 ms to resume from sleep state, so we hope system can
extend the command delay time to avoid potential risk."
So if I understand correctly, the Goodix touchpad needs:
- to wait 60ms after a SET_POWER on
No need for gen-2. I tested it today.
- to switch back to multitouch mode after a SET_POWER sleep and back on
Needed for gen-2, but disable RUNTIME_PM can workaround it.
Thanks for the patch.
However, I have the impression we are totally wrong under Linux and
i2c-hid. This is yet an other quirk for a driver that should not have
any.
I definitively not want to maintain an endless list of quirks, but I'd
rather mimic what Windows is doing or this is going to never end.
I have been told a few times that the Windows driver was much less
aggressive than us regarding the POWER command. And from what I
remember from the spec, it actually makes no tsense to control runtime
PM on the touchpads by sending those POWER commands. I2C is a bus that
should not drain any power when not in use. And the touchpad has
enough information when to go into low power.
So I am going to install Windows on a i2c-hid laptop, and try to see
when these commands are sent, and which timing is used (should we
delay any command by 100ms after a POWER?).
I have two different laptops with the same touchpad hardware, one is
Are those 2 Synaptics or Goodix?
All of them are Gen-1 samples, they are Goodix samples but report
Synaptics id.
installed Linux, the touchpad can't work unless we apply this patch or
disable runtime in the i2c-hid-core.c, the other is installed windows10,
the touchapd can't work too after installation, after a upgrade online,
What do you mean by "after a upgrade online, the touchpad can work"?
Windows10 upgrade, it will search and install the drivers for hardware.
After installing the touchpad driver from the internet, the touchpad can
work.
the touchpad can work. I am willing to test commands and timing under
windows, but I don't know how to do that, is there a guide?
I am usinghttps://github.com/bentiss/SimplePeripheralBusProbe
It's quite complex to set up, but it's better than having to crack
open the laptop and put physical probes in it.
OK, I will try to setup it and catch the commands on my gen-1 touchpad.
Thanks. It could be interesting to see how Windows behave with it.
Feel free to ping me if you have trouble recompiling your DSDT.
So on my side, I have been checking on a Dell XPS 13 how Windows
handled the touchpad.
In term of features retrievals, we are similar to what Windows does.
There is a significant difference in how many times we switch the
device on and off during probe.
Windows basically switches it on once, and done.
Under Linux we tend to switch it off and on quite a lot, and this is
what I must have been told. This has a tendency to confuse some
hardware, so we better fix that.
Yes, I also observed many times of SLEEP and ON under Linux.
So, for that, I am thinking that maybe we should delay the last
`pm_runtime_put()` we do in `i2c_hid_probe()` by, for instance 5 or 10
secs. So for 5 seconds, we keep the device powered on, which should
give time to udev and the session to open the node, get its
information, close it and the session to get over it.
It shouldn't be hard to write, and might be save us some headaches.
If this design is implemented, so far we can remove NO_RUNTIME_PM_QUIRKS
since Xwindow will open the hid device within 5 secs, then the
runtime_suspend will not be called outside the system_suspend.
And if we add below code in the runtime_resume(), we can remove
NO_RUNTIME_PM_QUIRKS too. So far it looks like all touchpads which
applied this quirk will lose working mode after PWR_SLEEP, if we restore
working mode in the runtime_resume, they don't need to apply that quirk
anymore.
if (hid->driver && hid->driver->reset_resume) {
ret = hid->driver->reset_resume(hid);
return ret;
}
Also, once this is in, I think we can consider the use case of
opening/closing the device repeatedly as niche case and probably force
I2C_HID_QUIRK_DELAY_AFTER_SLEEP on all devices.
Good idea. I think it is safe to apply this quirk to all touchpads.
In terms of timing, I do not see any special sleep after a wake up. So
I guess this must be a goodix limitation.
Last, on S3 suspend of the touchpad, I see:
set feature latency low
SET_POWER sleep
then on resume:
SET_POWER on
set feature latency normal
set feature latency normal
set feature button on, surface on
I have an almost similar sequence on runtime pm (screen going black):
suspend:
SET_POWER sleep
then on resume:
SET_POWER on
set feature latency normal
set feature latency normal
set feature button on, surface on
The only difference is that when the system is in S3, the touchpad is
set in the low latency mode.
Are they got from Linux? if "set feature xxxx" are from
These traces were taken from the Windows XPS I instrumented.
hid->driver->reset_resume(), the existing runtime resume in linux
doesn't call "set feature xxx" at all, in current i2c-hid-core.c of
linux, the runtime resume is different from system resume, it doesn't
call hid->driver->reset_resume().
Yep. I am still pondering that. It feels weird to force the call of
runtime PM from i2c-hid.
Can't we set up the runtime PM calls from within hid-multitouch directly?
Having the transport level calling those manually seems weird.
Yes, to do so, need a big change.
Cheers,
Benjamin
