Hi Hans,
and thanks also for the source code review. I will address those valid points once I know whether the patch might be accepted, see below.
On 2/11/21 5:24 PM, Hans de Goede wrote:
> Hi Alexander,
>
> On 2/10/21 6:51 PM, Alexander Kobel wrote:
>> <snip>>> Works like a charm.
>> I realized that the device also emits 0x60c0 (TP_HKEY_EV_TABLET_CHANGED) when the keyboard cover is attached or detached, yet *not* when it's folded. I don't quite get why I nevertheless receive only one notification to userspace according to acpi_listen, despite the fact that the 0x60c0 handler also calls tpacpi_input_send_tabletsw and hotkey_tablet_mode_notify_change. Is there a deduplication behind the scenes?
>
> Yes the input subsystem layer will not send events when nothing has changed.
I see, thanks for the confirmation.
>> I also realized that intel_vbtn reports the change, too. Would it be in order to modify intel_vbtn in a next step and blacklist this device to avoid duplicates?
>
> Hmm, that is a bit of a problem I would prefer to avoid having to deny-list things in intel_vbtn.
Agreed.
> So do the 2 behave exactly the same? Also wrt when the kbd is folded behind the kbd. IOW
> are the 2 SW_TABLET_MODE reports fully in sync in all possible states:
>
> 1. Just the tablet
> 2. Tablet + keyboard attached, keyboard in use
> 3. Tablet + keyboard attached, keyboard folded away behind tablet
>
> ?
They are in sync, at least as soon as the state changes and an event is emitted. The only functional difference seems to be that thinkpad_acpi offers the sysfs entry hotkey_tablet_mode to read the current state, that's it.
This is nice after bootup or for scripts started at random time without the chance of observing state changes. E.g., I'd like to have autorotation triggered via the orientation sensor, but only if the device is in tablet mode; so my autorotation handler would read that sysfs entry as the first thing. If there's no way to read the state, I have to resort to watching the state toggle and cache it for myself in userspace.
But perhaps intel-vbtn offers a similar interface for that purpose that I don't know?
I can almost work around that by checking for the existence of the "PRIMAX ThinkPad X1 Tablet Thin Keyboard Gen 2" input. But that's not a nice workaround, and it doesn't detect the folding away (input is still registered, although the firmware doesn't send key presses anymore).
So, indeed the benefit of my patch is rather minor. If that means it should be discarded, that's fine for me (I learned a lot while writing and refining it, always nice). If someone can give me a hint on how to read intel-vbtn state one-shot, even better, then it'd be mostly obsolete.
What might be more interesting is the potential handling different attachable devices, such as the portable dock (I guess this is the "Pico Cartridge", since it comes with another battery). For this one, I would actually expect an SW_DOCK event, and via the GTOP queries, this could be detected and distinguished from other attachment options. I assume intel-vbtn can't cover that case out-of-the-box.
Unfortunately, I don't have such a dock (yet), so that's just guessing.
Out of curiosity: is your ThinkPad 10 fully handled by intel-vbtn?
>> On the other hand, userspace should expect duplicate messages to some degree and use a hysteresis approach anyway. Every now and then, the contact of the magnetic plug is not established perfectly on the first attempt. So perhaps not really an issue.
>
> The only userspace consumer of this which I know is mutter (part of gnome-shell) and it
> will just take the value from the last event. So if the 2 are always in sync then
> the event send by the second input-dev will essentially be a no-op since the value will
> be the same as the other event.
Well, naturally another consumer is the acpi framework, e.g., acpi_listen or acpid. There, we have the possibility to install user-defined handlers. The same holds for window manager handlers such as sway's bindswitch tablet:{on,off,toggle} or, I presume, xbindkeys.
IMHO all reasonable handlers are idempotent, but users can be arbitrarily crazy. As mentioned, even if events are emitted exactly once on the software side, non-idempotent behavior will still occasionally be buggy, because the hardware connection is dodgy at times.
> We do need to resolve the question of how to handle this before I can merge the patch,
> atm I think that just having it reported twice is fine (as long as both reports are in
> sync).
They are in sync, that much I can confirm. But as mentioned, if you refrain from integrating the patch, I'm fine with that.
In that case, we should probably just add a dummy handler for HKEYs 0x4012 and 0x4013 with comments towards intel-vbtn, to avoid the unknown HKEY warning cluttering the system log.
Cheers,
Alexander
> Regards,
>
> Hans
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
