This patch series introduces changes necessary to support devices
using simple haptic HID pages.
Implementation attempts to follow the discussion below:
https://www.spinics.net/lists/linux-input/msg61091.html
Introduce new haptic defines as specified in HID Usage Tables.
Add new force feedback effect type in order to facilitate using
simple haptic force feedback.
Add INPUT_PROP_HAPTIC_TOUCHPAD to mark touchpad exposing simple haptic
support.
Add new struct hid_haptic_device so as to gather simple haptic related
configuration and current state of the device.
Function mt_get_feature() gets renamed to hid_get_feature() and is moved
to hid-core.c as it is not specific to hid multitouch driver and may be
reused, for instance by simple haptic specific source.
Add new functions to be triggered during HID input mapping and
configuration in order to detect simple haptic devices.
Modify HID input so that haptic output reports are parsed.
Initialize a haptic device.
Modify FF core so that effect IDs can be shared between multiple open file
handles.
Add shared release and press effects for a simple haptic device.
Calculate pressure resolution if units are grams or newtons.
Add support for kernel-driven mode of simple haptic device.
Toggle ABS_PRESSURE generation by input-mt on request.
Implement functions allowing switching between kernel-managed mode
and autonomous mode.
Add simple haptic support for hid-multitouch driver.
Implement EVIOCFF(TAKE|RELEASE)CONTROL ioctls so that userspace can take
and release control of shared release and press effects.
Fix i2c_hid_set_or_send_report so that report IDs larger than 0xF are
handled correctly.
Angela Czubak (18):
HID: add haptics page defines
Input: add FF_HID effect type
Input: add INPUT_PROP_HAPTIC_TOUCHPAD
HID: haptic: introduce hid_haptic_device
HID: introduce hid_get_feature
HID: haptic: add functions for mapping and configuration
HID: input: allow mapping of haptic output
HID: haptic: initialize haptic device
Input: add shared effects
HID: haptic: implement release and press effects
HID: input: calculate resolution for pressure
HID: haptic: add functions handling events
Input: MT - toggle ABS_PRESSURE pointer emulation
HID: haptic: add hid_haptic_switch_mode
HID: multitouch: add haptic multitouch support
Input: introduce EVIOCFF(TAKE|RELEASE)CONTROL
HID: haptic: add hid_haptic_change_control
HID: i2c-hid: fix i2c_hid_set_or_send_report
drivers/hid/Kconfig | 15 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-core.c | 39 ++
drivers/hid/hid-haptic.c | 745 +++++++++++++++++++++++++
drivers/hid/hid-haptic.h | 150 +++++
drivers/hid/hid-input.c | 18 +-
drivers/hid/hid-multitouch.c | 109 ++--
drivers/hid/i2c-hid/i2c-hid-core.c | 12 +-
drivers/input/evdev.c | 6 +
drivers/input/ff-core.c | 129 ++++-
drivers/input/input-mt.c | 18 +-
include/linux/hid.h | 24 +
include/linux/input.h | 5 +
include/linux/input/mt.h | 4 +
include/uapi/linux/input-event-codes.h | 1 +
include/uapi/linux/input.h | 26 +-
16 files changed, 1247 insertions(+), 55 deletions(-)
create mode 100644 drivers/hid/hid-haptic.c
create mode 100644 drivers/hid/hid-haptic.h
--
2.34.1.307.g9b7440fafd-goog
INPUT_PROP_HAPTIC_TOUCHPAD property is to be set for a device with simple
haptic capabilities.
Signed-off-by: Angela Czubak <redacted>
---
include/uapi/linux/input-event-codes.h | 1 +
1 file changed, 1 insertion(+)
@@ -27,6 +27,7 @@#define INPUT_PROP_TOPBUTTONPAD 0x04 /* softbuttons at top of pad */#define INPUT_PROP_POINTING_STICK 0x05 /* is a pointing stick */#define INPUT_PROP_ACCELEROMETER 0x06 /* has accelerometer */+#define INPUT_PROP_HAPTIC_TOUCHPAD 0x07 /* is a haptic touchpad */#define INPUT_PROP_MAX 0x1f#define INPUT_PROP_CNT (INPUT_PROP_MAX + 1)
@@ -460,6 +478,7 @@ struct ff_effect {structff_periodic_effectperiodic;structff_condition_effectcondition[2];/* One for each axis */structff_rumble_effectrumble;+structff_hid_effecthid;}u;};
Move mt_get_feature from hid-multitouch to hid-core as it is a generic
function that can be used by other drivers as well.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-core.c | 39 ++++++++++++++++++++++++++++++++++++
drivers/hid/hid-multitouch.c | 38 +++--------------------------------
include/linux/hid.h | 1 +
3 files changed, 43 insertions(+), 35 deletions(-)
@@ -498,7 +466,7 @@ static void mt_feature_mapping(struct hid_device *hdev,case0xff0000c5:/* Retrieve the Win8 blob once to enable some devices */if(usage->usage_index==0)-mt_get_feature(hdev,field->report);+hid_get_feature(hdev,field->report);break;}}
Add functions that recognize auto trigger and manual trigger reports
as well as save their addresses.
Verify that the pressure unit is either grams or newtons.
Mark the input device as a haptic touchpad if the unit is correct and
the reports are found.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-haptic.c | 63 ++++++++++++++++++++++++++++++++++++++++
drivers/hid/hid-haptic.h | 46 +++++++++++++++++++++++++++++
2 files changed, 109 insertions(+)
@@ -7,4 +7,67 @@/**/+#include"hid-haptic.h"++voidhid_haptic_feature_mapping(structhid_device*hdev,+structhid_haptic_device*haptic,+structhid_field*field,structhid_usage*usage)+{+if(usage->hid==HID_HP_AUTOTRIGGER){+if(usage->usage_index>=field->report_count){+dev_err(&hdev->dev,+"HID_HP_AUTOTRIGGER out of range\n");+return;+}++hid_get_feature(hdev,field->report);+haptic->default_auto_trigger=+field->value[usage->usage_index];+haptic->auto_trigger_report=field->report;+}+}+EXPORT_SYMBOL_GPL(hid_haptic_feature_mapping);++boolhid_haptic_check_pressure_unit(structhid_haptic_device*haptic,+structhid_input*hi,structhid_field*field)+{+/* Accepted units are either grams or newtons. */+if(field->unit==0x0101||field->unit==0xe111)+returntrue;+returnfalse;+}+EXPORT_SYMBOL_GPL(hid_haptic_check_pressure_unit);++inthid_haptic_input_mapping(structhid_device*hdev,+structhid_haptic_device*haptic,+structhid_input*hi,+structhid_field*field,structhid_usage*usage,+unsignedlong**bit,int*max)+{+if(usage->hid==HID_HP_MANUALTRIGGER){+haptic->manual_trigger_report=field->report;+/* we don't really want to map these fields */+return-1;+}++return0;+}+EXPORT_SYMBOL_GPL(hid_haptic_input_mapping);++inthid_haptic_input_configured(structhid_device*hdev,+structhid_haptic_device*haptic,+structhid_input*hi)+{++if(hi->application==HID_DG_TOUCHPAD){+if(haptic->auto_trigger_report&&+haptic->manual_trigger_report){+__set_bit(INPUT_PROP_HAPTIC_TOUCHPAD,hi->input->propbit);+return1;+}+return0;+}+return-1;+}+EXPORT_SYMBOL_GPL(hid_haptic_input_configured);
This change makes it possible to parse output reports by input mapping
functions by HID drivers.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-input.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -599,9 +599,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fielif(field->report_count<1)gotoignore;-/* only LED usages are supported in output fields */+/* only LED and HAPTIC usages are supported in output fields */if(field->report_type==HID_OUTPUT_REPORT&&-(usage->hid&HID_USAGE_PAGE)!=HID_UP_LED){+(usage->hid&HID_USAGE_PAGE)!=HID_UP_LED&&+(usage->hid&HID_USAGE_PAGE)!=HID_UP_HAPTIC){gotoignore;}
@@ -71,3 +89,430 @@ int hid_haptic_input_configured(struct hid_device *hdev,return-1;}EXPORT_SYMBOL_GPL(hid_haptic_input_configured);++staticvoidparse_auto_trigger_field(structhid_haptic_device*haptic,+structhid_field*field)+{+intcount=field->report_count;+intn;+u16usage_hid;++for(n=0;n<count;n++){+switch(field->usage[n].hid&HID_USAGE_PAGE){+caseHID_UP_ORDINAL:+usage_hid=field->usage[n].hid&HID_USAGE;+switch(field->logical){+caseHID_HP_WAVEFORMLIST:+haptic->hid_usage_map[usage_hid]=field->value[n];+if(field->value[n]==+(HID_HP_WAVEFORMPRESS&HID_USAGE)){+haptic->press_ordinal_orig=usage_hid;+haptic->press_ordinal_cur=usage_hid;+}elseif(field->value[n]==+(HID_HP_WAVEFORMRELEASE&HID_USAGE)){+haptic->release_ordinal_orig=usage_hid;+haptic->release_ordinal_cur=usage_hid;+}+break;+caseHID_HP_DURATIONLIST:+haptic->duration_map[usage_hid]=+field->value[n];+break;+default:+break;+}+break;+caseHID_UP_HAPTIC:+switch(field->usage[n].hid){+caseHID_HP_WAVEFORMVENDORID:+haptic->vendor_id=field->value[n];+break;+caseHID_HP_WAVEFORMVENDORPAGE:+haptic->vendor_page=field->value[n];+break;+default:+break;+}+break;+default:+/* Should not really happen */+break;+}+}+}++staticvoidfill_effect_buf(structhid_haptic_device*haptic,+structff_hid_effect*effect,+structhid_haptic_effect*haptic_effect,+intwaveform_ordinal)+{+structhid_report*rep=haptic->manual_trigger_report;+structhid_usage*usage;+structhid_field*field;+s32value;+inti,j;+u8*buf=haptic_effect->report_buf;++mutex_lock(&haptic->manual_trigger_mutex);+for(i=0;i<rep->maxfield;i++){+field=rep->field[i];+/* Ignore if report count is out of bounds. */+if(field->report_count<1)+continue;++for(j=0;j<field->maxusage;j++){+usage=&field->usage[j];++switch(usage->hid){+caseHID_HP_INTENSITY:+if(effect->intensity>100){+value=field->logical_maximum;+}else{+value=field->logical_minimum++effect->intensity*+(field->logical_maximum-+field->logical_minimum)/100;+}+break;+caseHID_HP_REPEATCOUNT:+value=effect->repeat_count;+break;+caseHID_HP_RETRIGGERPERIOD:+value=effect->retrigger_period;+break;+caseHID_HP_MANUALTRIGGER:+value=waveform_ordinal;+break;+default:+break;+}++field->value[j]=value;+}+}++hid_output_report(rep,buf);+mutex_unlock(&haptic->manual_trigger_mutex);+}++staticinthid_haptic_upload_effect(structinput_dev*dev,structff_effect*effect,+structff_effect*old)+{+structff_device*ff=dev->ff;+structhid_haptic_device*haptic=ff->private;+inti,ordinal=0;++/* If vendor range, check vendor id and page */+if(effect->u.hid.hid_usage>=(HID_HP_VENDORWAVEFORMMIN&HID_USAGE)&&+effect->u.hid.hid_usage<=(HID_HP_VENDORWAVEFORMMAX&HID_USAGE)&&+(effect->u.hid.vendor_id!=haptic->vendor_id||+effect->u.hid.vendor_waveform_page!=haptic->vendor_page))+return-EINVAL;++/* Check hid_usage */+for(i=1;i<haptic->max_waveform_id;i++){+if(haptic->hid_usage_map[i]==effect->u.hid.hid_usage){+ordinal=i;+break;+}+}+if(ordinal<1)+return-EINVAL;++/* Fill the buffer for the efect id */+fill_effect_buf(haptic,&effect->u.hid,&haptic->effect[effect->id],+ordinal);++return0;+}++staticintplay_effect(structhid_device*hdev,structhid_haptic_device*haptic,+structhid_haptic_effect*effect)+{+intret;++ret=hid_hw_output_report(hdev,effect->report_buf,+haptic->manual_trigger_report_len);+if(ret<0){+ret=hid_hw_raw_request(hdev,+haptic->manual_trigger_report->id,+effect->report_buf,+haptic->manual_trigger_report_len,+HID_OUTPUT_REPORT,HID_REQ_SET_REPORT);++}++returnret;+}++staticvoidhaptic_work_handler(structwork_struct*work)+{++structhid_haptic_effect*effect=container_of(work,+structhid_haptic_effect,+work);+structinput_dev*dev=effect->input_dev;+structhid_device*hdev=input_get_drvdata(dev);+structhid_haptic_device*haptic=dev->ff->private;++mutex_lock(&haptic->manual_trigger_mutex);+if(effect!=&haptic->stop_effect)+play_effect(hdev,haptic,&haptic->stop_effect);++play_effect(hdev,haptic,effect);+mutex_unlock(&haptic->manual_trigger_mutex);++}++staticinthid_haptic_playback(structinput_dev*dev,inteffect_id,intvalue)+{+structhid_haptic_device*haptic=dev->ff->private;++if(value)+queue_work(haptic->wq,&haptic->effect[effect_id].work);+else+queue_work(haptic->wq,&haptic->stop_effect.work);++return0;+}++staticvoideffect_set_default(structff_effect*effect)+{+effect->type=FF_HID;+effect->id=-1;+effect->u.hid.hid_usage=HID_HP_WAVEFORMNONE&HID_USAGE;+effect->u.hid.intensity=100;+effect->u.hid.retrigger_period=0;+effect->u.hid.repeat_count=0;+}++staticinthid_haptic_erase(structinput_dev*dev,inteffect_id)+{+structhid_haptic_device*haptic=dev->ff->private;+structff_effecteffect;+intordinal;++effect_set_default(&effect);+switch(effect_id){+caseHID_HAPTIC_RELEASE_EFFECT_ID:+ordinal=haptic->release_ordinal_orig;+if(!ordinal)+ordinal=HID_HAPTIC_ORDINAL_WAVEFORMNONE;+else+effect.u.hid.hid_usage=HID_HP_WAVEFORMRELEASE&+HID_USAGE;+fill_effect_buf(haptic,&effect.u.hid,&haptic->effect[effect_id],+ordinal);+break;+caseHID_HAPTIC_PRESS_EFFECT_ID:+ordinal=haptic->press_ordinal_orig;+if(!ordinal)+ordinal=HID_HAPTIC_ORDINAL_WAVEFORMNONE;+else+effect.u.hid.hid_usage=HID_HP_WAVEFORMPRESS&+HID_USAGE;+fill_effect_buf(haptic,&effect.u.hid,&haptic->effect[effect_id],+ordinal);+break;+default:+break;+}++return0;+}++staticvoidhid_haptic_destroy(structff_device*ff)+{+structhid_haptic_device*haptic=ff->private;+structhid_device*hdev=haptic->hdev;+intr;++if(hdev)+put_device(&hdev->dev);++kfree(haptic->stop_effect.report_buf);+haptic->stop_effect.report_buf=NULL;++if(haptic->effect){+for(r=0;r<ff->max_effects;r++)+kfree(haptic->effect[r].report_buf);+kfree(haptic->effect);+}+haptic->effect=NULL;++destroy_workqueue(haptic->wq);+haptic->wq=NULL;++kfree(haptic->duration_map);+haptic->duration_map=NULL;++kfree(haptic->hid_usage_map);+haptic->hid_usage_map=NULL;++module_put(THIS_MODULE);+}++inthid_haptic_init(structhid_device*hdev,+structhid_haptic_device**haptic_ptr)+{+structhid_haptic_device*haptic=*haptic_ptr;+structinput_dev*dev=NULL;+structhid_input*hidinput;+structff_device*ff;+intret=0,r;+structff_hid_effectstop_effect={+.hid_usage=HID_HP_WAVEFORMSTOP&HID_USAGE,+};+constchar*prefix="hid-haptic";+char*name;+int(*flush)(structinput_dev*dev,structfile*file);+int(*event)(structinput_dev*dev,unsignedinttype,unsignedintcode,intvalue);++haptic->hdev=hdev;+haptic->max_waveform_id=max(2u,haptic->max_waveform_id);+haptic->max_duration_id=max(2u,haptic->max_duration_id);++haptic->hid_usage_map=kcalloc(haptic->max_waveform_id+1,+sizeof(__u16),GFP_KERNEL);+if(!haptic->hid_usage_map){+ret=-ENOMEM;+gotoexit;+}+haptic->duration_map=kcalloc(haptic->max_duration_id+1,+sizeof(__u32),GFP_KERNEL);+if(!haptic->duration_map){+ret=-ENOMEM;+gotousage_map;+}++if(haptic->max_waveform_id!=haptic->max_duration_id)+dev_warn(&hdev->dev,+"Haptic duration and waveform lists have different max id (%u and %u).\n",+haptic->max_duration_id,haptic->max_waveform_id);++haptic->hid_usage_map[HID_HAPTIC_ORDINAL_WAVEFORMNONE]=+HID_HP_WAVEFORMNONE&HID_USAGE;+haptic->hid_usage_map[HID_HAPTIC_ORDINAL_WAVEFORMSTOP]=+HID_HP_WAVEFORMSTOP&HID_USAGE;++for(r=0;r<haptic->auto_trigger_report->maxfield;r++)+parse_auto_trigger_field(haptic,haptic->auto_trigger_report->field[r]);++list_for_each_entry(hidinput,&hdev->inputs,list){+if(hidinput->application==HID_DG_TOUCHPAD){+dev=hidinput->input;+break;+}+}++if(!dev){+dev_err(&hdev->dev,"Failed to find the input device\n");+ret=-ENODEV;+gotoduration_map;+}++haptic->input_dev=dev;+haptic->manual_trigger_report_len=+hid_report_len(haptic->manual_trigger_report);+mutex_init(&haptic->manual_trigger_mutex);+name=kmalloc(strlen(prefix)+strlen(hdev->name)+2,GFP_KERNEL);+if(name){+sprintf(name,"%s %s",prefix,hdev->name);+haptic->wq=create_singlethread_workqueue(name);+kfree(name);+}+if(!haptic->wq){+ret=-ENOMEM;+gotoduration_map;+}+haptic->effect=kcalloc(FF_MAX_EFFECTS,+sizeof(structhid_haptic_effect),GFP_KERNEL);+if(!haptic->effect){+ret=-ENOMEM;+gotooutput_queue;+}+for(r=0;r<FF_MAX_EFFECTS;r++){+haptic->effect[r].report_buf=+hid_alloc_report_buf(haptic->manual_trigger_report,+GFP_KERNEL);+if(!haptic->effect[r].report_buf){+dev_err(&hdev->dev,+"Failed to allocate a buffer for an effect.\n");+ret=-ENOMEM;+gotobuffer_free;+}+haptic->effect[r].input_dev=dev;+INIT_WORK(&haptic->effect[r].work,haptic_work_handler);+}+haptic->stop_effect.report_buf=+hid_alloc_report_buf(haptic->manual_trigger_report,+GFP_KERNEL);+if(!haptic->stop_effect.report_buf){+dev_err(&hdev->dev,+"Failed to allocate a buffer for stop effect.\n");+ret=-ENOMEM;+gotobuffer_free;+}+haptic->stop_effect.input_dev=dev;+INIT_WORK(&haptic->stop_effect.work,haptic_work_handler);+fill_effect_buf(haptic,&stop_effect,&haptic->stop_effect,+HID_HAPTIC_ORDINAL_WAVEFORMSTOP);++input_set_capability(dev,EV_FF,FF_HID);++flush=dev->flush;+event=dev->event;+ret=input_ff_create(dev,FF_MAX_EFFECTS);+if(ret){+dev_err(&hdev->dev,"Failed to create ff device.\n");+gotostop_buffer_free;+}++ff=dev->ff;+ff->private=haptic;+ff->upload=hid_haptic_upload_effect;+ff->playback=hid_haptic_playback;+ff->erase=hid_haptic_erase;+ff->destroy=hid_haptic_destroy;+if(!try_module_get(THIS_MODULE)){+dev_err(&hdev->dev,"Failed to increase module count.\n");+gotoinput_free;+}+if(!get_device(&hdev->dev)){+dev_err(&hdev->dev,"Failed to get hdev device.\n");+module_put(THIS_MODULE);+gotoinput_free;+}+return0;++input_free:+input_ff_destroy(dev);+/* Do not let double free happen, input_ff_destroy will call+*hid_haptic_destroy.+*/+*haptic_ptr=NULL;+/* Restore dev flush and event */+dev->flush=flush;+dev->event=event;+returnret;+stop_buffer_free:+kfree(haptic->stop_effect.report_buf);+haptic->stop_effect.report_buf=NULL;+buffer_free:+while(--r>=0)+kfree(haptic->effect[r].report_buf);+kfree(haptic->effect);+haptic->effect=NULL;+output_queue:+destroy_workqueue(haptic->wq);+haptic->wq=NULL;+duration_map:+kfree(haptic->duration_map);+haptic->duration_map=NULL;+usage_map:+kfree(haptic->hid_usage_map);+haptic->hid_usage_map=NULL;+exit:+returnret;+}+EXPORT_SYMBOL_GPL(hid_haptic_init);
If an effect is uploaded with file handle equal UINTPTR_MAX assume this
effect should be shared and so may be modified using different file
handles.
Signed-off-by: Angela Czubak <redacted>
---
drivers/input/ff-core.c | 40 ++++++++++++++++++++++++++++++++++------
1 file changed, 34 insertions(+), 6 deletions(-)
@@ -367,6 +367,7 @@ int hid_haptic_init(struct hid_device *hdev,char*name;int(*flush)(structinput_dev*dev,structfile*file);int(*event)(structinput_dev*dev,unsignedinttype,unsignedintcode,intvalue);+structff_effectrelease_effect,press_effect;haptic->hdev=hdev;haptic->max_waveform_id=max(2u,haptic->max_waveform_id);
@@ -483,8 +484,41 @@ int hid_haptic_init(struct hid_device *hdev,module_put(THIS_MODULE);gotoinput_free;}++effect_set_default(&release_effect);+if(haptic->release_ordinal_orig)+release_effect.u.hid.hid_usage=HID_HP_WAVEFORMRELEASE&+HID_USAGE;+ret=input_ff_upload(dev,&release_effect,(structfile*)UINTPTR_MAX);+if(ret||release_effect.id!=HID_HAPTIC_RELEASE_EFFECT_ID){+if(!ret){+ret=-EBUSY;+input_ff_erase(dev,release_effect.id,+(structfile*)UINTPTR_MAX);+}+dev_err(&hdev->dev,+"Failed to allocate id 0 for release effect.\n");+gotoinput_free;+}+effect_set_default(&press_effect);+if(haptic->press_ordinal_orig)+press_effect.u.hid.hid_usage=HID_HP_WAVEFORMPRESS&HID_USAGE;+ret=input_ff_upload(dev,&press_effect,(structfile*)UINTPTR_MAX);+if(ret||press_effect.id!=HID_HAPTIC_PRESS_EFFECT_ID){+if(!ret){+ret=-EBUSY;+input_ff_erase(dev,press_effect.id,+(structfile*)UINTPTR_MAX);+}+dev_err(&hdev->dev,+"Failed to allocate id 1 for press effect.\n");+gotorelease_free;+}+return0;+release_free:+input_ff_erase(dev,release_effect.id,(structfile*)UINTPTR_MAX);input_free:input_ff_destroy(dev);/* Do not let double free happen, input_ff_destroy will call
Assume that if the pressure is given in newtons it should be normalized
to grams. If the pressure has no unit do not calculate resolution.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-input.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
Implement hid_haptic_handle_press_release() which generates haptic feedback
as well as saves the pressed state of the haptic device.
Function hid_haptic_handle_input() inserts BTN_LEFT and ABS_PRESSURE events
if the device is in kernel mode.
Add functions to increase and reset the state of the pressure detected by
the device.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-haptic.c | 72 +++++++++++++++++++++++++++++++++++++++-
drivers/hid/hid-haptic.h | 20 +++++++++++
2 files changed, 91 insertions(+), 1 deletion(-)
@@ -51,8 +51,13 @@ bool hid_haptic_check_pressure_unit(struct hid_haptic_device *haptic,structhid_input*hi,structhid_field*field){/* Accepted units are either grams or newtons. */-if(field->unit==0x0101||field->unit==0xe111)+if(field->unit==0x0101||field->unit==0xe111){+haptic->force_logical_minimum=field->logical_minimum;+haptic->force_physical_minimum=field->physical_minimum;+haptic->force_resolution=input_abs_get_res(hi->input,+ABS_MT_PRESSURE);returntrue;+}returnfalse;}EXPORT_SYMBOL_GPL(hid_haptic_check_pressure_unit);
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
Signed-off-by: Angela Czubak <redacted>
---
drivers/input/input-mt.c | 18 ++++++++++++++++--
include/linux/input/mt.h | 4 ++++
2 files changed, 20 insertions(+), 2 deletions(-)
Function hid_haptic_switch_mode() can be used to turn off and on the
autonomoums mode for the device. If the device supports press and release
waveforms, let the kernel handle generation of haptic feedback instead of
the device itself.
Implement hid_haptic_resume() and hid_haptic_suspend() so that the
autonomous mode gets switched off at resume and switched on at suspend.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-haptic.c | 89 +++++++++++++++++++++++++++++++++++++---
drivers/hid/hid-haptic.h | 10 +++++
2 files changed, 93 insertions(+), 6 deletions(-)
@@ -200,9 +201,61 @@ static void fill_effect_buf(struct hid_haptic_device *haptic,mutex_unlock(&haptic->manual_trigger_mutex);}+staticvoidswitch_mode(structhid_device*hdev,structhid_haptic_device*haptic,+intmode)+{+structhid_report*rep=haptic->auto_trigger_report;+structhid_field*field;+s32value;+inti,j;++if(mode==HID_HAPTIC_MODE_KERNEL){+value=HID_HAPTIC_ORDINAL_WAVEFORMSTOP;+input_mt_pressure_toggle(haptic->input_dev,0);+}else{+value=haptic->default_auto_trigger;+input_mt_pressure_toggle(haptic->input_dev,1);+}++mutex_lock(&haptic->auto_trigger_mutex);+for(i=0;i<rep->maxfield;i++){+field=rep->field[i];+/* Ignore if report count is out of bounds. */+if(field->report_count<1)+continue;++for(j=0;j<field->maxusage;j++){+if(field->usage[j].hid==HID_HP_AUTOTRIGGER)+field->value[j]=value;+}+}++/* send the report */+hid_hw_request(hdev,rep,HID_REQ_SET_REPORT);+mutex_unlock(&haptic->auto_trigger_mutex);+haptic->mode=mode;+}++#ifdef CONFIG_PM+voidhid_haptic_resume(structhid_device*hdev,structhid_haptic_device*haptic)+{+if(haptic->press_ordinal_cur&&haptic->release_ordinal_cur)+switch_mode(hdev,haptic,HID_HAPTIC_MODE_KERNEL);+}+EXPORT_SYMBOL_GPL(hid_haptic_resume);++voidhid_haptic_suspend(structhid_device*hdev,structhid_haptic_device*haptic)+{+if(haptic->press_ordinal_cur&&haptic->release_ordinal_cur)+switch_mode(hdev,haptic,HID_HAPTIC_MODE_DEVICE);+}+EXPORT_SYMBOL_GPL(hid_haptic_suspend);+#endif+staticinthid_haptic_upload_effect(structinput_dev*dev,structff_effect*effect,structff_effect*old){+structhid_device*hdev=input_get_drvdata(dev);structff_device*ff=dev->ff;structhid_haptic_device*haptic=ff->private;inti,ordinal=0;
Add new option (MULTITOUCH_HAPTIC) to mark whether hid-multitouch
should try and configure simple haptic device.
Once this option is configured, and the device is recognized to have simple
haptic capabilities, check input frames for pressure and handle it using
hid_haptic_* API.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/Kconfig | 11 ++++++
drivers/hid/hid-multitouch.c | 71 +++++++++++++++++++++++++++++++++++-
2 files changed, 80 insertions(+), 2 deletions(-)
@@ -48,6 +48,8 @@ MODULE_LICENSE("GPL");#include"hid-ids.h"+#include"hid-haptic.h"+/* quirks to control the device */#define MT_QUIRK_NOT_SEEN_MEANS_UP BIT(0)#define MT_QUIRK_SLOT_IS_CONTACTID BIT(1)
@@ -159,11 +161,13 @@ struct mt_report_data {structmt_device{structmt_classmtclass;/* our mt device class */structtimer_listrelease_timer;/* to release sticky fingers */+structhid_haptic_device*haptic;/* haptic related configuration */structhid_device*hdev;/* hid_device we're attached to */unsignedlongmt_io_flags;/* mt flags (MT_IO_FLAGS_*) */__u8inputmode_value;/* InputMode HID feature value */__u8maxcontacts;boolis_buttonpad;/* is this device a button pad? */+boolis_haptic_touchpad;/* is this device a haptic touchpad? */boolserial_maybe;/* need to check for serial protocol */structlist_headapplications;
@@ -1343,6 +1366,11 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,if(field->physical==HID_DG_STYLUS)hi->application=HID_DG_STYLUS;+ret=hid_haptic_input_mapping(hdev,td->haptic,hi,field,usage,bit,+max);+if(ret!=0)+returnret;+/* let hid-core decide for the others */return0;}
@@ -1735,6 +1774,17 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)mt_set_modes(hdev,HID_LATENCY_NORMAL,true,true);+if(td->is_haptic_touchpad){+if(hid_haptic_init(hdev,&td->haptic)){+dev_warn(&hdev->dev,"Cannot allocate haptic for %s\n",+hdev->name);+td->is_haptic_touchpad=false;+kfree(td->haptic);+}+}else{+kfree(td->haptic);+}+return0;}
@@ -1742,6 +1792,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)staticintmt_suspend(structhid_device*hdev,pm_message_tstate){structmt_device*td=hid_get_drvdata(hdev);+structhid_haptic_device*haptic=td->haptic;/* High latency is desirable for power savings during S3/S0ix */if((td->mtclass.quirks&MT_QUIRK_DISABLE_WAKEUP)||
@@ -1750,18 +1801,31 @@ static int mt_suspend(struct hid_device *hdev, pm_message_t state)elsemt_set_modes(hdev,HID_LATENCY_HIGH,true,true);+if(td->is_haptic_touchpad)+hid_haptic_resume(hdev,haptic);+return0;}staticintmt_reset_resume(structhid_device*hdev){+structmt_device*td=hid_get_drvdata(hdev);+structhid_haptic_device*haptic=td->haptic;+mt_release_contacts(hdev);mt_set_modes(hdev,HID_LATENCY_NORMAL,true,true);++if(td->is_haptic_touchpad)+hid_haptic_resume(hdev,haptic);+return0;}staticintmt_resume(structhid_device*hdev){+structmt_device*td=hid_get_drvdata(hdev);+structhid_haptic_device*haptic=td->haptic;+/* Some Elan legacy devices require SET_IDLE to be set on resume.*Itshouldbesafetosendittootherdevicestoo.*Testedon3M,Stantum,Cypress,Zytronic,eGalax,andElanpanels.*/
@@ -1770,6 +1834,9 @@ static int mt_resume(struct hid_device *hdev)mt_set_modes(hdev,HID_LATENCY_NORMAL,true,true);+if(td->is_haptic_touchpad)+hid_haptic_suspend(hdev,haptic);+return0;}#endif
Add new ioctls which can be used for simple haptic force feedback effects.
Once the control is taken over the effect the kernel does not generate it
on its own (EVIOCFFTAKECONTROL).
To revert this action use EVIOCFFRELEASECONTROL.
Signed-off-by: Angela Czubak <redacted>
---
drivers/input/evdev.c | 6 +++
drivers/input/ff-core.c | 89 +++++++++++++++++++++++++++++++++++++-
include/linux/input.h | 5 +++
include/uapi/linux/input.h | 4 ++
4 files changed, 103 insertions(+), 1 deletion(-)
@@ -570,6 +573,8 @@ int input_ff_event(struct input_dev *dev, unsigned int type, unsigned int code,intinput_ff_upload(structinput_dev*dev,structff_effect*effect,structfile*file);intinput_ff_erase(structinput_dev*dev,inteffect_id,structfile*file);+intinput_ff_take_control(structinput_dev*dev,inteffect_id,structfile*file);+intinput_ff_release_control(structinput_dev*dev,inteffect_id,structfile*file);intinput_ff_flush(structinput_dev*dev,structfile*file);intinput_ff_create_memless(structinput_dev*dev,void*data,
@@ -178,6 +178,10 @@ struct input_mask {#define EVIOCSFF _IOW('E', 0x80, struct ff_effect) /* send a force effect to a force feedback device */#define EVIOCRMFF _IOW('E', 0x81, int) /* Erase a force effect */+/* Take control over a force effect */+#define EVIOCFFTAKECONTROL _IOW('E', 0x82, int)+/* Release control over a force effect */+#define EVIOCFFRELEASECONTROL _IOW('E', 0x83, int)#define EVIOCGEFFECTS _IOR('E', 0x84, int) /* Report number of effects playable at the same time */#define EVIOCGRAB _IOW('E', 0x90, int) /* Grab/Release device */
Implement change_control callbacks for simple haptic device.
If anybody has requested control over an effect, do not generate it
in kernel.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-haptic.c | 50 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 48 insertions(+), 2 deletions(-)
@@ -348,6 +349,46 @@ static int hid_haptic_playback(struct input_dev *dev, int effect_id, int value)return0;}+staticinthid_haptic_change_control(structinput_dev*dev,inteffect_id,+structfile*file,inttake)+{+structhid_haptic_device*haptic=dev->ff->private;+structhid_haptic_effect_node*effect_node;+structhid_haptic_effect*effect;+boolfound=false;+intret=0;++effect=&haptic->effect[effect_id];+mutex_lock(&effect->control_mutex);+list_for_each_entry(effect_node,&effect->control,node){+if(effect_node->file==file){+found=true;+break;+}+}+if(take){+if(!found){+effect_node=kvzalloc(sizeof(structhid_haptic_effect),+GFP_KERNEL);+if(!effect_node){+ret=-ENOMEM;+gotoexit;+}+effect_node->file=file;+}+list_add(&effect_node->node,&effect->control);+}else{+if(found){+list_del(&effect_node->node);+kvfree(effect_node);+}+}+exit:+mutex_unlock(&effect->control_mutex);++returnret;+}+staticvoideffect_set_default(structff_effect*effect){effect->type=FF_HID;
@@ -533,6 +574,8 @@ int hid_haptic_init(struct hid_device *hdev,}haptic->effect[r].input_dev=dev;INIT_WORK(&haptic->effect[r].work,haptic_work_handler);+INIT_LIST_HEAD(&haptic->effect[r].control);+mutex_init(&haptic->effect[r].control_mutex);}haptic->stop_effect.report_buf=hid_alloc_report_buf(haptic->manual_trigger_report,
@@ -569,6 +612,7 @@ int hid_haptic_init(struct hid_device *hdev,ff->private=haptic;ff->upload=hid_haptic_upload_effect;ff->playback=hid_haptic_playback;+ff->change_control=hid_haptic_change_control;ff->erase=hid_haptic_erase;ff->destroy=hid_haptic_destroy;if(!try_module_get(THIS_MODULE)){
If command & data registers are to be used and report ID >= 0xF
use the sentinel value for report ID in the command.
Do not alter the report ID itself as it needs to be inserted into the args
buffer. If output register is to be used there is no need to insert
report IDs >= 0xF.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -384,7 +384,7 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,memcpy(&args[index],buf,data_len);-ret=__i2c_hid_command(client,hidcmd,reportID,+ret=__i2c_hid_command(client,hidcmd,cmdReportID,reportType,args,args_len,NULL,0);if(ret){dev_err(&client->dev,"failed to set a report to device.\n");
Hi Angela,
Thanks for sharing. I would like to have a look as well at this
series. As I briefly mentioned in another thread at Sony we have a
need for haptics as well for our DualSense controller for the
PlayStation 5. There is some overlap and differences. For our use
case, FF is really showing its limits (it really doesn't work). The
key question I have is whether FF is really a good fit for your use
case or not. I don't understand this type of device well enough yet.
There might be enough demand for either a new framework (or it is the
end of the road for evdev).
Thanks,
Roderick
On Wed, Dec 22, 2021 at 7:59 AM Angela Czubak [off-list ref] wrote:
This patch series introduces changes necessary to support devices
using simple haptic HID pages.
Implementation attempts to follow the discussion below:
https://www.spinics.net/lists/linux-input/msg61091.html
Introduce new haptic defines as specified in HID Usage Tables.
Add new force feedback effect type in order to facilitate using
simple haptic force feedback.
Add INPUT_PROP_HAPTIC_TOUCHPAD to mark touchpad exposing simple haptic
support.
Add new struct hid_haptic_device so as to gather simple haptic related
configuration and current state of the device.
Function mt_get_feature() gets renamed to hid_get_feature() and is moved
to hid-core.c as it is not specific to hid multitouch driver and may be
reused, for instance by simple haptic specific source.
Add new functions to be triggered during HID input mapping and
configuration in order to detect simple haptic devices.
Modify HID input so that haptic output reports are parsed.
Initialize a haptic device.
Modify FF core so that effect IDs can be shared between multiple open file
handles.
Add shared release and press effects for a simple haptic device.
Calculate pressure resolution if units are grams or newtons.
Add support for kernel-driven mode of simple haptic device.
Toggle ABS_PRESSURE generation by input-mt on request.
Implement functions allowing switching between kernel-managed mode
and autonomous mode.
Add simple haptic support for hid-multitouch driver.
Implement EVIOCFF(TAKE|RELEASE)CONTROL ioctls so that userspace can take
and release control of shared release and press effects.
Fix i2c_hid_set_or_send_report so that report IDs larger than 0xF are
handled correctly.
Angela Czubak (18):
HID: add haptics page defines
Input: add FF_HID effect type
Input: add INPUT_PROP_HAPTIC_TOUCHPAD
HID: haptic: introduce hid_haptic_device
HID: introduce hid_get_feature
HID: haptic: add functions for mapping and configuration
HID: input: allow mapping of haptic output
HID: haptic: initialize haptic device
Input: add shared effects
HID: haptic: implement release and press effects
HID: input: calculate resolution for pressure
HID: haptic: add functions handling events
Input: MT - toggle ABS_PRESSURE pointer emulation
HID: haptic: add hid_haptic_switch_mode
HID: multitouch: add haptic multitouch support
Input: introduce EVIOCFF(TAKE|RELEASE)CONTROL
HID: haptic: add hid_haptic_change_control
HID: i2c-hid: fix i2c_hid_set_or_send_report
drivers/hid/Kconfig | 15 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-core.c | 39 ++
drivers/hid/hid-haptic.c | 745 +++++++++++++++++++++++++
drivers/hid/hid-haptic.h | 150 +++++
drivers/hid/hid-input.c | 18 +-
drivers/hid/hid-multitouch.c | 109 ++--
drivers/hid/i2c-hid/i2c-hid-core.c | 12 +-
drivers/input/evdev.c | 6 +
drivers/input/ff-core.c | 129 ++++-
drivers/input/input-mt.c | 18 +-
include/linux/hid.h | 24 +
include/linux/input.h | 5 +
include/linux/input/mt.h | 4 +
include/uapi/linux/input-event-codes.h | 1 +
include/uapi/linux/input.h | 26 +-
16 files changed, 1247 insertions(+), 55 deletions(-)
create mode 100644 drivers/hid/hid-haptic.c
create mode 100644 drivers/hid/hid-haptic.h
--
2.34.1.307.g9b7440fafd-goog
Looks like you forgot to change vendor_id to vendor_waveform_page on
the line above.
quoted hunk
+ * @intensity: strength of the effect as percentage
+ * @repeat_count: number of times to retrigger effect
+ * @retrigger_period: time before effect is retriggered (in ms)
+ */
+struct ff_hid_effect {
+ __u16 hid_usage;
+ __u16 vendor_id;
+ __u8 vendor_waveform_page;
+ __u16 intensity;
+ __u16 repeat_count;
+ __u16 retrigger_period;
+};
+
/**
* struct ff_effect - defines force feedback effect
* @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
@@ -460,6 +478,7 @@ struct ff_effect { struct ff_periodic_effect periodic; struct ff_condition_effect condition[2]; /* One for each axis */ struct ff_rumble_effect rumble;+ struct ff_hid_effect hid; } u; };
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:28PM +0000, Angela Czubak wrote:
INPUT_PROP_HAPTIC_TOUCHPAD property is to be set for a device with simple
haptic capabilities.
This needs to have corresponding documentation in
Documentation/input/event-codes.rst explaning the exact meaning of this
property as it was discussed in the forcepad email thread started by
Sean.
Thanks.
@@ -27,6 +27,7 @@#define INPUT_PROP_TOPBUTTONPAD 0x04 /* softbuttons at top of pad */#define INPUT_PROP_POINTING_STICK 0x05 /* is a pointing stick */#define INPUT_PROP_ACCELEROMETER 0x06 /* has accelerometer */+#define INPUT_PROP_HAPTIC_TOUCHPAD 0x07 /* is a haptic touchpad */#define INPUT_PROP_MAX 0x1f#define INPUT_PROP_CNT (INPUT_PROP_MAX + 1)
@@ -599,9 +599,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fielif(field->report_count<1)gotoignore;-/* only LED usages are supported in output fields */+/* only LED and HAPTIC usages are supported in output fields */if(field->report_type==HID_OUTPUT_REPORT&&-(usage->hid&HID_USAGE_PAGE)!=HID_UP_LED){+(usage->hid&HID_USAGE_PAGE)!=HID_UP_LED&&+(usage->hid&HID_USAGE_PAGE)!=HID_UP_HAPTIC){gotoignore;}
On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
quoted hunk
Move mt_get_feature from hid-multitouch to hid-core as it is a generic
function that can be used by other drivers as well.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-core.c | 39 ++++++++++++++++++++++++++++++++++++
drivers/hid/hid-multitouch.c | 38 +++--------------------------------
include/linux/hid.h | 1 +
3 files changed, 43 insertions(+), 35 deletions(-)
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
Thanks.
--
Dmitry
On Tue, Dec 21, 2021 at 07:17:36PM +0000, Angela Czubak wrote:
quoted hunk
Assume that if the pressure is given in newtons it should be normalized
to grams. If the pressure has no unit do not calculate resolution.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-input.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:43PM +0000, Angela Czubak wrote:
If command & data registers are to be used and report ID >= 0xF
use the sentinel value for report ID in the command.
Do not alter the report ID itself as it needs to be inserted into the args
buffer. If output register is to be used there is no need to insert
report IDs >= 0xF.
OK, I see what you mean, but I believe that i2c_hid_set_or_send_report()
is the wrong place to implement this handling to begin with. How about a
modified version of your patch that I am pasting below (not tested)?
Thanks,
Dmitry
-- >8 -- >8 --
HID: i2c-hid: fix handling numbered reports with IDs of 15 and above
From: Angela Czubak <redacted>
Special handling of numbered reports with IDs of 15 and above is only
needed when executing what HID-I2C spec is calling "Class Specific
Requests", and not when simply sending output reports.
Additionally, our mangling of report ID in i2c_hid_set_or_send_report()
resulted in incorrect report ID being written into SET_REPORT command
payload.
To solve it let's move all the report ID manipulation into
__i2c_hid_command() where we form the command data structure.
Signed-off-by: Angela Czubak <redacted>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
What is this comment block for? Actually I do not see why this needs to
be a separate patch.
I have seen this kind of comment block in both hid-multitouch.c and
hid-core.c, though I can remove it.
Just to be sure: is it OK to introduce all fields/structures at once
with path no 8 ("HID: haptic: initialize haptic device")? Or would you
prefer if I extend the structures as necessary?
I use them as indices in the effect array of effects below, though
they are actually mentioned in Sean O'Brien's kernel design proposal.
Please let me know if you would rather move them above. Perhaps it
should be even somehow exported via uapi (so that userspace does not
hardcode it separately).
On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
[off-list ref] wrote:
On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
quoted
Move mt_get_feature from hid-multitouch to hid-core as it is a generic
function that can be used by other drivers as well.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-core.c | 39 ++++++++++++++++++++++++++++++++++++
drivers/hid/hid-multitouch.c | 38 +++--------------------------------
include/linux/hid.h | 1 +
3 files changed, 43 insertions(+), 35 deletions(-)
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode. Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
[off-list ref] wrote:
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
quoted
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
Fair point.
quoted
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
quoted
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.
Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.
Now, the question would be where BTN_LEFT events should be generated.
Normally it happens in hid-multitouch and I override it in hid-haptic.c
This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
Does anyone mind such behaviour?
quoted
Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
(I suppose I wanted to say I would implement it in such case)
From: Sean O'Brien <hidden> Date: 2022-01-12 02:19:34
On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak [off-list ref] wrote:
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
quoted
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
Fair point.
quoted
quoted
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
quoted
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.
Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.
I agree that we should switch to device-controlled mode if press/release are
not defined by the device, and userspace has not supplied alternative
waveforms for either. If we kept it in kernel-controlled mode, there would be
no effect for click/release. This can be achieved by userspace by emitting
EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
This also allows for the case where userspace may want to send haptics for UX
effects, while still relying on the device for traditional press and release
haptics (in the case where the device doesn't define press/release
waveforms).
Now, the question would be where BTN_LEFT events should be generated.
Normally it happens in hid-multitouch and I override it in hid-haptic.c
This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
Does anyone mind such behaviour?
quoted
quoted
Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
(I suppose I wanted to say I would implement it in such case)
On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak [off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
quoted
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
Fair point.
quoted
quoted
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
quoted
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.
I'd say that if hid_haptic_init() fails we should not say that the
device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
the device instantiation, which we probably should not).
quoted
Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.
I agree that we should switch to device-controlled mode if press/release are
not defined by the device, and userspace has not supplied alternative
waveforms for either. If we kept it in kernel-controlled mode, there would be
no effect for click/release. This can be achieved by userspace by emitting
EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
What is wrong for not having effect for press/release if userspace did
not bother to set it up? I think this is reasonably to expect that if
user enabled support for haptic touchpad in kernel they should also have
userspace that knows how to handle it. If we go with this requirement I
think we will reduce a lot of complexity.
Benjamin, Jiri, Peter, I'd like you to chime in please.
This also allows for the case where userspace may want to send haptics for UX
effects, while still relying on the device for traditional press and release
haptics (in the case where the device doesn't define press/release
waveforms).
Again, what is the difference between press/release and other UX
effects? They seem to be the same to me...
quoted
Now, the question would be where BTN_LEFT events should be generated.
Normally it happens in hid-multitouch and I override it in hid-haptic.c
This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
Does anyone mind such behaviour?
quoted
quoted
Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
(I suppose I wanted to say I would implement it in such case)
On Wed, Jan 12, 2022 at 3:52 AM Dmitry Torokhov
[off-list ref] wrote:
On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
quoted
On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak [off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
quoted
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
Fair point.
quoted
quoted
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
quoted
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.
I'd say that if hid_haptic_init() fails we should not say that the
device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
the device instantiation, which we probably should not).
It would still be a haptic touchpad, I suppose; the fact that
initialization failed isn't going
to do anything with the device, really, rather some failure in memory
allocation etc.
If you are worried about how the userspace can be sure that
initialization succeeded,
if hid_haptic_init() succeeds then the device should report EV_FF
events, otherwise not.
From my point of view it would be also somewhat unnecessary for
hid_haptic_init()
failure to break device instantiation as it would mean that the touchpad gets
nonfunctional.
Please let me know if there are any known obstacles that should discourage us
from using the device after failed hid_haptic_init() or you have some
more thoughts on
INPUT_PROP_HAPTIC_TOUCHPAD.
quoted
quoted
Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.
I agree that we should switch to device-controlled mode if press/release are
not defined by the device, and userspace has not supplied alternative
waveforms for either. If we kept it in kernel-controlled mode, there would be
no effect for click/release. This can be achieved by userspace by emitting
EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
What is wrong for not having effect for press/release if userspace did
not bother to set it up? I think this is reasonably to expect that if
user enabled support for haptic touchpad in kernel they should also have
userspace that knows how to handle it. If we go with this requirement I
think we will reduce a lot of complexity.
Benjamin, Jiri, Peter, I'd like you to chime in please.
Dmitry, just to be sure, are you suggesting that kernel should not
trigger haptic feedback
at all as well?
I suppose most of the cases a haptic device will supply waveforms for
press and release in
its waveformlist (exposed in autotrigger report) and so the kernel
will use them.
I am just mentioning a case where switching back to device/autonomous
mode would be
somehow justifiable.
quoted
This also allows for the case where userspace may want to send haptics for UX
effects, while still relying on the device for traditional press and release
haptics (in the case where the device doesn't define press/release
waveforms).
Again, what is the difference between press/release and other UX
effects? They seem to be the same to me...
I suppose the userspace may want the user to feel a different
sensation at times.
For instance, let's say that user tries to click on a button/switch
that is blocked for them
(perhaps this option is not allowed by their admin?). The userspace
may want to trigger
a different haptic effect to let them know in yet another way that
their actions haven't
succeeded, something is prohibited etc.
Sean, you may have some better real-life applications, perhaps it
should be you to
explain this point more clearly.
quoted
quoted
Now, the question would be where BTN_LEFT events should be generated.
Normally it happens in hid-multitouch and I override it in hid-haptic.c
This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
Does anyone mind such behaviour?
quoted
quoted
Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
(I suppose I wanted to say I would implement it in such case)
From: Benjamin Tissoires <hidden> Date: 2022-01-12 09:17:53
On Wed, Jan 12, 2022 at 3:52 AM Dmitry Torokhov
[off-list ref] wrote:
On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
quoted
On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak [off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
quoted
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
Fair point.
quoted
quoted
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
quoted
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.
I'd say that if hid_haptic_init() fails we should not say that the
device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
the device instantiation, which we probably should not).
Agree. Userspace should know that the device is a pressure pad based
on the unit provided in ABS_MT_PRESSURE IIRC.
So setting the resolution is enough for userspace to emulate the
button clicks based on the pressure. libinput already has code for
that.
So basically, INPUT_PROP_HAPTIC_TOUCHPAD is only an indication that
the haptic is configurable. And if haptic_init() fails, it should not
expose that property.
And BTW, why "TOUCHPAD" in INPUT_PROP_HAPTIC_TOUCHPAD? The Surface
Dial could benefit from that implementation and it is not a
touchpad...
quoted
quoted
Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.
I agree that we should switch to device-controlled mode if press/release are
not defined by the device, and userspace has not supplied alternative
waveforms for either. If we kept it in kernel-controlled mode, there would be
no effect for click/release. This can be achieved by userspace by emitting
EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
What is wrong for not having effect for press/release if userspace did
not bother to set it up? I think this is reasonably to expect that if
user enabled support for haptic touchpad in kernel they should also have
userspace that knows how to handle it. If we go with this requirement I
think we will reduce a lot of complexity.
Benjamin, Jiri, Peter, I'd like you to chime in please.
[FWIW, lei saved me on this one for not being Cc-ed since the
beginning of this thread]
I think we should keep it simple:
- the device configuration should be static (i.e.
ABS_PRESSURE/ABS_MT_PRESSURE, pointer emulation, button emulation,
...) always present
- userspace should pick up what it needs based on its own state:
if there is a need to compute a total pressure, userspace is capable
of computing itself, and generates its own button press/release
- the haptic is a global state of the device, so any decision you make
is going to have corner cases with more than one userspace (or if the
userspace daemon/lib is restarted)
So to me, we should keep the kernel device emulation, export what
needs to be for userspace to make its own decision and have the haptic
side as a "nice to have" feature but distinct from the event
processing.
I didn't want to chime into this thread because I am currently working
on 2 big series that might also be helpful here:
- the first one, which is almost ready, consists in rethinking how the
HID events are processed, meaning we can ensure that some events are
always processed before others. The net benefit is that I can now
express the Win8 multitouch protocol in hid-generic without too much
pain, meaning that hid-haptic.c could be a leaf driver instead of
being an API.
The net benefit of not having hid-haptic.c as an API is that we can
always rmmod it to disable the entire haptic system if there is
something wrong.
- the second one is the eBPF bindings for HID (see
https://lore.kernel.org/all/20211215134220.1735144-1-tero.kristo@linux.intel.com/
and the other versions for some more discussions)
Basically BPF allows to avoid specific kernel APIs and userspace is in
charge of loading the bridge between its API and the device. It
definitely has the potential to solve many limitations we are seeing
now in all the various input/ff protocols IMO.
quoted
This also allows for the case where userspace may want to send haptics for UX
effects, while still relying on the device for traditional press and release
haptics (in the case where the device doesn't define press/release
waveforms).
Again, what is the difference between press/release and other UX
effects? They seem to be the same to me...
quoted
quoted
Now, the question would be where BTN_LEFT events should be generated.
Normally it happens in hid-multitouch and I override it in hid-haptic.c
This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
Does anyone mind such behaviour?
Again, why is there a need to have some complex behavior there? Just
let userspace do its own fancy computation and keep it simple in the
kernel.
Well, with eBPF, you could let userspace put the BTN_LEFT emulation in
the kernel by loading a specific program, but that would be in charge
of the userspace to make this choice, not the kernel.
Cheers,
Benjamin
quoted
quoted
quoted
quoted
Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
(I suppose I wanted to say I would implement it in such case)
From: Benjamin Tissoires <hidden> Date: 2022-01-12 09:43:54
On 1/10/22 20:43, Angela Czubak wrote:
On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
quoted
Move mt_get_feature from hid-multitouch to hid-core as it is a generic
function that can be used by other drivers as well.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-core.c | 39 ++++++++++++++++++++++++++++++++++++
drivers/hid/hid-multitouch.c | 38 +++--------------------------------
include/linux/hid.h | 1 +
3 files changed, 43 insertions(+), 35 deletions(-)
The hack allows to not have to use hid_device_io_{start|stop}(), which
is probably not clean.
As for the return value, hid_hw_request() can be used as asynchronous,
which is why it returns void. However, returning an actual int would
definitively be better because some cases are failing silently (like if
the device is not io started).
Cheers,
Benjamin
On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
[off-list ref] wrote:
On 1/10/22 20:43, Angela Czubak wrote:
quoted
On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
quoted
Move mt_get_feature from hid-multitouch to hid-core as it is a generic
function that can be used by other drivers as well.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-core.c | 39 ++++++++++++++++++++++++++++++++++++
drivers/hid/hid-multitouch.c | 38 +++--------------------------------
include/linux/hid.h | 1 +
3 files changed, 43 insertions(+), 35 deletions(-)
The hack allows to not have to use hid_device_io_{start|stop}(), which
is probably not clean.
As for the return value, hid_hw_request() can be used as asynchronous,
which is why it returns void. However, returning an actual int would
definitively be better because some cases are failing silently (like if
the device is not io started).
I am slightly confused; it is hid_hw_raw_request() that is used and it does
not seem asynchronous to me; is there no guarantee that the response
has already been received? It seemed to me that the main purpose of
this function is to retrieve information an have it correctly parsed.
I literally issue it once to learn if auto trigger has been set by default and
to know the durations of waveforms, learn ordinals etc.
I could introduce a new function for the purpose of haptic API, it just
seemed redundant as the one in hid-multitouch.c does what I need.
From: Benjamin Tissoires <hidden> Date: 2022-01-13 09:54:21
On Wed, Jan 12, 2022 at 12:26 PM Angela Czubak [off-list ref] wrote:
On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
[off-list ref] wrote:
quoted
On 1/10/22 20:43, Angela Czubak wrote:
quoted
On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
quoted
Move mt_get_feature from hid-multitouch to hid-core as it is a generic
function that can be used by other drivers as well.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-core.c | 39 ++++++++++++++++++++++++++++++++++++
drivers/hid/hid-multitouch.c | 38 +++--------------------------------
include/linux/hid.h | 1 +
3 files changed, 43 insertions(+), 35 deletions(-)
The hack allows to not have to use hid_device_io_{start|stop}(), which
is probably not clean.
As for the return value, hid_hw_request() can be used as asynchronous,
which is why it returns void. However, returning an actual int would
definitively be better because some cases are failing silently (like if
the device is not io started).
I am slightly confused; it is hid_hw_raw_request() that is used and it does
not seem asynchronous to me; is there no guarantee that the response
has already been received?
In the case of usbhid, hid_hw_request() calls directly
__usbhid_submit_report() which is asynchronous.
So no, we have no guarantees that the answer is there.
It seemed to me that the main purpose of
this function is to retrieve information an have it correctly parsed.
I literally issue it once to learn if auto trigger has been set by default and
to know the durations of waveforms, learn ordinals etc.
I could introduce a new function for the purpose of haptic API, it just
seemed redundant as the one in hid-multitouch.c does what I need.
Again, the one in hid-multitouch is a hack against
hid_device_io_{start|stop}(). So if you need to change something, it's
the hid-multitouch code, not reuse that hack :)
Cheers,
Benjamin
Hi Benjamin,
On Thu, Jan 13, 2022 at 10:54 AM Benjamin Tissoires
[off-list ref] wrote:
On Wed, Jan 12, 2022 at 12:26 PM Angela Czubak [off-list ref] wrote:
quoted
On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
[off-list ref] wrote:
quoted
On 1/10/22 20:43, Angela Czubak wrote:
quoted
On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
quoted
Move mt_get_feature from hid-multitouch to hid-core as it is a generic
function that can be used by other drivers as well.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-core.c | 39 ++++++++++++++++++++++++++++++++++++
drivers/hid/hid-multitouch.c | 38 +++--------------------------------
include/linux/hid.h | 1 +
3 files changed, 43 insertions(+), 35 deletions(-)
The hack allows to not have to use hid_device_io_{start|stop}(), which
is probably not clean.
As for the return value, hid_hw_request() can be used as asynchronous,
which is why it returns void. However, returning an actual int would
definitively be better because some cases are failing silently (like if
the device is not io started).
I am slightly confused; it is hid_hw_raw_request() that is used and it does
not seem asynchronous to me; is there no guarantee that the response
has already been received?
In the case of usbhid, hid_hw_request() calls directly
__usbhid_submit_report() which is asynchronous.
So no, we have no guarantees that the answer is there.
quoted
It seemed to me that the main purpose of
this function is to retrieve information an have it correctly parsed.
I literally issue it once to learn if auto trigger has been set by default and
to know the durations of waveforms, learn ordinals etc.
I could introduce a new function for the purpose of haptic API, it just
seemed redundant as the one in hid-multitouch.c does what I need.
Again, the one in hid-multitouch is a hack against
hid_device_io_{start|stop}(). So if you need to change something, it's
the hid-multitouch code, not reuse that hack :)
ACK, I will use hid_hw_request() and hid_hw_wait() instead as suggested.
BTW is hid_device_io_{start|stop} required to do anything meaningful?
It just seems to me that it currently sets some variable which is not really
useful anywhere else.
On Wed, Jan 12, 2022 at 10:17 AM Benjamin Tissoires
[off-list ref] wrote:
On Wed, Jan 12, 2022 at 3:52 AM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
quoted
On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak [off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
quoted
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
Fair point.
quoted
quoted
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
quoted
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.
I'd say that if hid_haptic_init() fails we should not say that the
device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
the device instantiation, which we probably should not).
Agree. Userspace should know that the device is a pressure pad based
on the unit provided in ABS_MT_PRESSURE IIRC.
So setting the resolution is enough for userspace to emulate the
button clicks based on the pressure. libinput already has code for
that.
A quick glance [1] and it seems that libinput chooses to ignore ABS_MT_PRESSURE
it the resolution is non-zero (though I might have been looking in a
wrong place).
Mentioning just because someone might lead me to a proper place/library actually
using ABS_MT_PRESSURE as force.
So basically, INPUT_PROP_HAPTIC_TOUCHPAD is only an indication that
the haptic is configurable. And if haptic_init() fails, it should not
expose that property.
And BTW, why "TOUCHPAD" in INPUT_PROP_HAPTIC_TOUCHPAD? The Surface
Dial could benefit from that implementation and it is not a
touchpad...
Ok, so looking back at the old discussion it seems to me that the
property originally
suggested is INPUT_PROP_FORCEPAD and it was initially intended to mean that
ABS_MT_PRESSURE events should be interpreted as force and not area/pressure.
In case units are grams or newtons I calculate the resolution, but it
seems that Peter
has previously stated it is not enough:
And we can't just assume "if resolution is set, units are $foo" because
nothing written in the last decade or so will assume that. Some extra flag
is needed, like INPUT_PROP_FORCEPAD.
I think Benjamin originally suggested this flag so that userspace knows
ABS_MT_PRESSURE should mean force.
However, it was a very long time ago. It seems that about a year ago it was
defined that non-zero pressure resolution means units/grams is used.
It seems to me that we could assume that reporting FF_HID events implicates
how ABS_MT_PRESSURE should be interpreted, so I could get rid of this
flag, if that is what you prefer.
quoted
quoted
quoted
Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.
I agree that we should switch to device-controlled mode if press/release are
not defined by the device, and userspace has not supplied alternative
waveforms for either. If we kept it in kernel-controlled mode, there would be
no effect for click/release. This can be achieved by userspace by emitting
EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
What is wrong for not having effect for press/release if userspace did
not bother to set it up? I think this is reasonably to expect that if
user enabled support for haptic touchpad in kernel they should also have
userspace that knows how to handle it. If we go with this requirement I
think we will reduce a lot of complexity.
Benjamin, Jiri, Peter, I'd like you to chime in please.
[FWIW, lei saved me on this one for not being Cc-ed since the
beginning of this thread]
I think we should keep it simple:
- the device configuration should be static (i.e.
ABS_PRESSURE/ABS_MT_PRESSURE, pointer emulation, button emulation,
...) always present
- userspace should pick up what it needs based on its own state:
if there is a need to compute a total pressure, userspace is capable
of computing itself, and generates its own button press/release
- the haptic is a global state of the device, so any decision you make
is going to have corner cases with more than one userspace (or if the
userspace daemon/lib is restarted)
So to me, we should keep the kernel device emulation, export what
needs to be for userspace to make its own decision and have the haptic
side as a "nice to have" feature but distinct from the event
processing.
I didn't want to chime into this thread because I am currently working
on 2 big series that might also be helpful here:
- the first one, which is almost ready, consists in rethinking how the
HID events are processed, meaning we can ensure that some events are
always processed before others. The net benefit is that I can now
express the Win8 multitouch protocol in hid-generic without too much
pain, meaning that hid-haptic.c could be a leaf driver instead of
being an API.
The net benefit of not having hid-haptic.c as an API is that we can
always rmmod it to disable the entire haptic system if there is
something wrong.
- the second one is the eBPF bindings for HID (see
https://lore.kernel.org/all/20211215134220.1735144-1-tero.kristo@linux.intel.com/
and the other versions for some more discussions)
Basically BPF allows to avoid specific kernel APIs and userspace is in
charge of loading the bridge between its API and the device. It
definitely has the potential to solve many limitations we are seeing
now in all the various input/ff protocols IMO.
quoted
quoted
This also allows for the case where userspace may want to send haptics for UX
effects, while still relying on the device for traditional press and release
haptics (in the case where the device doesn't define press/release
waveforms).
Again, what is the difference between press/release and other UX
effects? They seem to be the same to me...
quoted
quoted
Now, the question would be where BTN_LEFT events should be generated.
Normally it happens in hid-multitouch and I override it in hid-haptic.c
This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
Does anyone mind such behaviour?
Again, why is there a need to have some complex behavior there? Just
let userspace do its own fancy computation and keep it simple in the
kernel.
I thought it was requested based on the following discussion [2]:
quoted
ABS_PRESSURE may be optionally reported as the total force applied to the
forcepad.
The device/driver shouldn’t detect button clicks, this is left to the userspace
gesture library. Accordingly, the driver should not sent BTN_* events to
userspace in normal operating mode. However it should still report the ability
to produce such events, for use in autonomous mode.
For backward compatibility, and to be able to debug it properly, you
should keep the BTN_* events emulated in all cases.
The userspace can ignore the events it doesn't want this way, but you
will be able to debug the btn emulations on your current session
without having to kill your compositor.
There shouldn't be much of a head over forwarding those events, as it
will never come alone, and will always be with an other one at least
(pressure being 0 or less).
Also, not sending BTN_TOUCH and BTN_LEFT might give some headaches to
legacy applications.
I can remove such behaviour if I misunderstood or it is no longer valid.
Well, with eBPF, you could let userspace put the BTN_LEFT emulation in
the kernel by loading a specific program, but that would be in charge
of the userspace to make this choice, not the kernel.
Cheers,
Benjamin
quoted
quoted
quoted
quoted
quoted
Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
(I suppose I wanted to say I would implement it in such case)
From: Benjamin Tissoires <hidden> Date: 2022-01-17 10:08:56
On Fri, Jan 14, 2022 at 7:24 PM Angela Czubak [off-list ref] wrote:
Hi Benjamin,
On Thu, Jan 13, 2022 at 10:54 AM Benjamin Tissoires
[off-list ref] wrote:
quoted
On Wed, Jan 12, 2022 at 12:26 PM Angela Czubak [off-list ref] wrote:
quoted
On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
[off-list ref] wrote:
quoted
On 1/10/22 20:43, Angela Czubak wrote:
quoted
On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
quoted
Move mt_get_feature from hid-multitouch to hid-core as it is a generic
function that can be used by other drivers as well.
Signed-off-by: Angela Czubak <redacted>
---
drivers/hid/hid-core.c | 39 ++++++++++++++++++++++++++++++++++++
drivers/hid/hid-multitouch.c | 38 +++--------------------------------
include/linux/hid.h | 1 +
3 files changed, 43 insertions(+), 35 deletions(-)
The hack allows to not have to use hid_device_io_{start|stop}(), which
is probably not clean.
As for the return value, hid_hw_request() can be used as asynchronous,
which is why it returns void. However, returning an actual int would
definitively be better because some cases are failing silently (like if
the device is not io started).
I am slightly confused; it is hid_hw_raw_request() that is used and it does
not seem asynchronous to me; is there no guarantee that the response
has already been received?
In the case of usbhid, hid_hw_request() calls directly
__usbhid_submit_report() which is asynchronous.
So no, we have no guarantees that the answer is there.
quoted
It seemed to me that the main purpose of
this function is to retrieve information an have it correctly parsed.
I literally issue it once to learn if auto trigger has been set by default and
to know the durations of waveforms, learn ordinals etc.
I could introduce a new function for the purpose of haptic API, it just
seemed redundant as the one in hid-multitouch.c does what I need.
Again, the one in hid-multitouch is a hack against
hid_device_io_{start|stop}(). So if you need to change something, it's
the hid-multitouch code, not reuse that hack :)
ACK, I will use hid_hw_request() and hid_hw_wait() instead as suggested.
BTW is hid_device_io_{start|stop} required to do anything meaningful?
It just seems to me that it currently sets some variable which is not really
useful anywhere else.
The thing it does is to release the semaphore
hid->driver_input_lock(), which is then in turn used in hid-core.c, in
hid_input_report(). Without this, hid_input_report() aborts earlier
and the field->values[] are not populated because HID considers it
should ignore the incoming reports.
So without this, you can not access the new values apart from doing
what hid-multitouch does: trick the system and force inject an event
in the processing pipeline.
Cheers,
Benjamin
From: Peter Hutterer <hidden> Date: 2022-01-21 06:10:47
On Tue, Jan 11, 2022 at 06:52:27PM -0800, Dmitry Torokhov wrote:
On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
quoted
On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak [off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
quoted
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
Fair point.
quoted
quoted
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
quoted
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.
I'd say that if hid_haptic_init() fails we should not say that the
device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
the device instantiation, which we probably should not).
quoted
quoted
Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.
I agree that we should switch to device-controlled mode if press/release are
not defined by the device, and userspace has not supplied alternative
waveforms for either. If we kept it in kernel-controlled mode, there would be
no effect for click/release. This can be achieved by userspace by emitting
EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
What is wrong for not having effect for press/release if userspace did
not bother to set it up? I think this is reasonably to expect that if
user enabled support for haptic touchpad in kernel they should also have
userspace that knows how to handle it. If we go with this requirement I
think we will reduce a lot of complexity.
Benjamin, Jiri, Peter, I'd like you to chime in please.
quoted
This also allows for the case where userspace may want to send haptics for UX
effects, while still relying on the device for traditional press and release
haptics (in the case where the device doesn't define press/release
waveforms).
Again, what is the difference between press/release and other UX
effects? They seem to be the same to me...
Agree with Dmitry here - have a sensible default in the kernel and if
userspace changes it, it's now userspace's problem to do it right. Anything
more complex is just making things more complicated for niche cases that may
never happen.
Cheers,
Peter
Hi Peter, Dmitry, Benjamin, Sean,
On Fri, Jan 21, 2022 at 7:10 AM Peter Hutterer [off-list ref] wrote:
On Tue, Jan 11, 2022 at 06:52:27PM -0800, Dmitry Torokhov wrote:
quoted
On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
quoted
On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak [off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
quoted
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
Fair point.
quoted
quoted
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
quoted
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.
I'd say that if hid_haptic_init() fails we should not say that the
device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
the device instantiation, which we probably should not).
quoted
quoted
Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.
I agree that we should switch to device-controlled mode if press/release are
not defined by the device, and userspace has not supplied alternative
waveforms for either. If we kept it in kernel-controlled mode, there would be
no effect for click/release. This can be achieved by userspace by emitting
EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
What is wrong for not having effect for press/release if userspace did
not bother to set it up? I think this is reasonably to expect that if
user enabled support for haptic touchpad in kernel they should also have
userspace that knows how to handle it. If we go with this requirement I
think we will reduce a lot of complexity.
Benjamin, Jiri, Peter, I'd like you to chime in please.
quoted
This also allows for the case where userspace may want to send haptics for UX
effects, while still relying on the device for traditional press and release
haptics (in the case where the device doesn't define press/release
waveforms).
Again, what is the difference between press/release and other UX
effects? They seem to be the same to me...
Agree with Dmitry here - have a sensible default in the kernel and if
userspace changes it, it's now userspace's problem to do it right. Anything
more complex is just making things more complicated for niche cases that may
never happen.
Could you please relate to the following statements/questions? I would like to
make sure I am nearer to your understanding of how the things should be.
I wouldn't say they constitute my plan, I am just wondering if shared effects
are acceptable at all since their handling seems questionable.
1. Kernel mode - is it OK to have any default at all? Or would you rather say
it's userspace's responsibility to issue force feedback entirely? I am just
wondering how much simplification you would actually prefer to have.
In the current patchset the kernel can issue haptic feedback itself
(based on the pressure/force sums calculated).
2. The patches introduce shared effects. This allows userspace to modify
kernel mode behaviour, i.e. the waveforms it issues when press/release
has been detected, which means both uploading and erasing those
effects is possible.
On the other hand, closing event fd triggers removing effects uploaded for
that fd. I would assume removing shared effects is allowed as well
since we can update them with upload. Should it be disallowed/prohibited?
I mean that perhaps erasing shared effects should never really take place
as we may end up removing something that has not been altered by
userspace.
I am worried since simply opening and closing the event file could possibly
cause a change in behaviour if we actually let effects be completely
removed.
3. Switching to kernel mode should happen at the instantiation and then only
during suspend/resume cycle. If the shared press/release effect gets
removed (even caused by input device flush), then we don't want any haptic
feedback in kernel mode anyway.
4. Should I just not care and not sum the pressures across all slots? It just
seemed to me there was a reason to choose one slot and pass it as
ABS_PRESSURE in input-mt.c, and I just suspected it would be more
logical to pass the sum of forces if the unit suggests it is force.
From: Peter Hutterer <hidden> Date: 2022-01-28 05:25:20
On Tue, Jan 25, 2022 at 05:56:17PM +0100, Angela Czubak wrote:
Hi Peter, Dmitry, Benjamin, Sean,
On Fri, Jan 21, 2022 at 7:10 AM Peter Hutterer [off-list ref] wrote:
quoted
On Tue, Jan 11, 2022 at 06:52:27PM -0800, Dmitry Torokhov wrote:
quoted
On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
quoted
On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak [off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
quoted
Hi Dmitry,
On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:
quoted
Hi Angela,
On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
quoted
Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.
This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().
Or did I misunderstand the point?
I would say you understood it correctly, though to my mind it is not a
static behaviour,
It should be, otherwise how will userspace know the meaning of the
event?
Fair point.
quoted
quoted
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
No.
quoted
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode.
Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.
In my mind we have following cases:
- OS does not know about these haptics devices (touchpads). They work in
device (?) mode and provide haptic feedback on their own.
- OS does know about haptics devices (that includes having both kernel
*and* userspace support for them. If one is missing then the other
should not be enabled, it is up to the distro to make sure all pieces
are there). In this case OS controls haptics effects all the time,
except:
- OS supports haptics, but switched it to device mode to allow haptics
effect playback when waking up.
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.
I'd say that if hid_haptic_init() fails we should not say that the
device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
the device instantiation, which we probably should not).
quoted
quoted
Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.
I agree that we should switch to device-controlled mode if press/release are
not defined by the device, and userspace has not supplied alternative
waveforms for either. If we kept it in kernel-controlled mode, there would be
no effect for click/release. This can be achieved by userspace by emitting
EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
What is wrong for not having effect for press/release if userspace did
not bother to set it up? I think this is reasonably to expect that if
user enabled support for haptic touchpad in kernel they should also have
userspace that knows how to handle it. If we go with this requirement I
think we will reduce a lot of complexity.
Benjamin, Jiri, Peter, I'd like you to chime in please.
quoted
This also allows for the case where userspace may want to send haptics for UX
effects, while still relying on the device for traditional press and release
haptics (in the case where the device doesn't define press/release
waveforms).
Again, what is the difference between press/release and other UX
effects? They seem to be the same to me...
Agree with Dmitry here - have a sensible default in the kernel and if
userspace changes it, it's now userspace's problem to do it right. Anything
more complex is just making things more complicated for niche cases that may
never happen.
Could you please relate to the following statements/questions? I would like to
make sure I am nearer to your understanding of how the things should be.
I wouldn't say they constitute my plan, I am just wondering if shared effects
are acceptable at all since their handling seems questionable.
1. Kernel mode - is it OK to have any default at all? Or would you rather say
it's userspace's responsibility to issue force feedback entirely? I am just
wondering how much simplification you would actually prefer to have.
In the current patchset the kernel can issue haptic feedback itself
(based on the pressure/force sums calculated).
IMO the kernel should have a default. without any force feedback the devices
are going to be difficult to use.
2. The patches introduce shared effects. This allows userspace to modify
kernel mode behaviour, i.e. the waveforms it issues when press/release
has been detected, which means both uploading and erasing those
effects is possible.
On the other hand, closing event fd triggers removing effects uploaded for
that fd. I would assume removing shared effects is allowed as well
since we can update them with upload. Should it be disallowed/prohibited?
I mean that perhaps erasing shared effects should never really take place
as we may end up removing something that has not been altered by
userspace.
I am worried since simply opening and closing the event file could possibly
cause a change in behaviour if we actually let effects be completely
removed.
if you remove effects on fd close, you're effectively enforcing that some
process always needs to keep the fd to this device open just to have it
respond correctly. It may also prevent the device from going to sleep, right?
So I think there's an argument to leaving the configured waveforms there after
closing the effect - similar things already happen with other evdev ioctls.
3. Switching to kernel mode should happen at the instantiation and then only
during suspend/resume cycle. If the shared press/release effect gets
removed (even caused by input device flush), then we don't want any haptic
feedback in kernel mode anyway.
4. Should I just not care and not sum the pressures across all slots? It just
seemed to me there was a reason to choose one slot and pass it as
ABS_PRESSURE in input-mt.c, and I just suspected it would be more
logical to pass the sum of forces if the unit suggests it is force.
I'm not quite keeping up with the details in the patches but - whatever is the
most accurate physical measurement? :)
Cheers,
Peter