Date:   Fri, 31 Mar 2017 15:23:32 -0700
From:   Andrew Duggan <aduggan@...aptics.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
CC:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Peter Hutterer <peter.hutterer@...-t.net>,
        Benjamin Tissoires <benjamin.tissoires@...il.com>,
        Jason Ekstrand <jason@...kstrand.net>,
        Cameron Gutman <aicommander@...il.com>,
        Thorsten Leemhuis <linux@...mhuis.info>,
        Jiri Kosina <jikos@...nel.org>, Nick Dyer <nick@...anahar.org>,
        Christopher Heiny <cheiny@...aptics.com>,
        linux-input <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Synaptics RMI4 touchpad regression in 4.11-rc1

On 03/31/2017 01:57 AM, Benjamin Tissoires wrote:
> On Mar 29 2017 or thereabouts, Andrew Duggan wrote:
>>
>> On 03/29/2017 01:50 AM, Benjamin Tissoires wrote:
>>> On Mar 28 2017 or thereabouts, Andrew Duggan wrote:
>>>> On 03/19/2017 10:00 PM, Peter Hutterer wrote:
>>>>> On Fri, Mar 17, 2017 at 12:23:36PM -0700, Andrew Duggan wrote:
>>>>>> On 03/17/2017 09:57 AM, Benjamin Tissoires wrote:
>>>>>>> On Wed, Mar 15, 2017 at 2:19 AM, Andrew Duggan<aduggan@...aptics.com>  wrote:
>>>>>>>> On 03/13/2017 10:10 PM, Cameron Gutman wrote:
>>>>>>>>> On 03/13/2017 06:35 PM, Andrew Duggan wrote:
>>>>>>>>>> On 03/13/2017 06:15 AM, Benjamin Tissoires wrote:
>>>>>>>>>>> [Resending, forgot to add Jiri in CC]
>>>>>>>>>>>
>>>>>>>>>>> On Mar 13 2017 or thereabouts, Benjamin Tissoires wrote:
>>>>>>>>>>>> On Mar 13 2017 or thereabouts, Thorsten Leemhuis wrote:
>>>>>>>>>>>>> Lo! On 12.03.2017 02:55, Cameron Gutman wrote:
>>>>>>>>>>>>>> Beginning in 4.11-rc1, it looks like RMI4 is binding to my XPS 13
>>>>>>>>>>>>>> 9343's
>>>>>>>>>>>>>> Synaptics touchpad and dropping some errors into dmesg. Here are the
>>>>>>>>>>>>>> messages that seem RMI-related:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> rmi4_f34 rmi4-00.fn34: rmi_f34v7_probe: Unrecognized bootloader
>>>>>>>>>>>>>> version
>>>>>>>>>>>>>> rmi4_f34: probe of rmi4-00.fn34 failed with error -22
>>>>>>>>>>>>>> rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: Synaptics,
>>>>>>>>>>>>>> product: TM3038-001, fw id: 1832324
>>>>>>>>>>>>>> input: Synaptics TM3038-001 as
>>>>>>>>>>>>>> /devices/pci0000:00/INT3433:00/i2c-7/i2c-DLL0665:01/0018:06CB:76AD.0001/input/input19
>>>>>>>>>>>>>> hid-rmi 0018:06CB:76AD.0001: input,hidraw0: I2C HID v1.00 Mouse
>>>>>>>>>>>>>> [DLL0665:01 06CB:76AD] on i2c-DLL0665:01
>>>>>>>>>>>>> FWIW, I get this on my XPS 13 DE (9360) with 4.11-rc1:
>>>>>>>>>>>>>
>>>>>>>>>>>>> input: SynPS/2 Synaptics TouchPad as
>>>>>>>>>>>>> /devices/platform/i8042/serio1/input/input6
>>>>>>>>>>>>> rmi4_f34 rmi4-00.fn34: rmi_f34v7_probe: Unrecognized bootloader
>>>>>>>>>>>>> version
>>>>>>>>>>>>> rmi4_f34: probe of rmi4-00.fn34 failed with error -22
>>>>>>>>>>>>> rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: Synaptics,
>>>>>>>>>>>>> product: TM3038-003, fw id: 2375007
>>>>>>>>>>>>> input: Synaptics TM3038-003 as
>>>>>>>>>>>>>
>>>>>>>>>>>>> /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-8/i2c-DLL075B:01/0018:06CB:76AF.0001/input/input20
>>>>>>>>>>>>> hid-rmi 0018:06CB:76AF.0001: input,hidraw0: I2C HID v1.00 Mouse
>>>>>>>>>>>>> [DLL075B:01 06CB:76AF] on i2c-DLL075B:01
>>>>>>>>>>>>>
>>>>>>>>>>>>>> […]
>>>>>>>>>>>>>> Compared to hid-multitouch, the RMI stack seems to have completely
>>>>>>>>>>>>>> broken
>>>>>>>>>>>>>> palm rejection and introduced some random jumpiness during fine
>>>>>>>>>>>>>> pointing
>>>>>>>>>>>>>> motions. I don't know if these issues are caused by the above errors
>>>>>>>>>>>>>> or
>>>>>>>>>>>>>> are a separate issue.
>>>>>>>>>> The error about the bootloader version not being recognized just means
>>>>>>>>>> that updating the firmware is not supported on this touchpad. It is only the
>>>>>>>>>> F34 firmware update functionality which is failing to load. The palm
>>>>>>>>>> rejection and jumps are not related to this error.
>>>>>>>>>>
>>>>>>>>> Maybe that code path should be changed to not make as much noise when it
>>>>>>>>> runs
>>>>>>>>> on known unsupported hardware. Something like the attached patch?
>>>>>>>>>
>>>>>>>>>> Looking at how hid-multitouch handles palms it looks like palms should
>>>>>>>>>> not be reported as active when calling input_mt_report_slot_state(). I'm
>>>>>>>>>> setting the tool type to MT_TOOL_PALM when the firmware determines that a
>>>>>>>>>> contact is a palm. But, that does not seem to be sufficient enough to have
>>>>>>>>>> the palms filtered out. After some quick testing it looks like the change
>>>>>>>>>> below will restore palm rejection similar to that provided by
>>>>>>>>>> hid-multitouch.
>>>>>>>>>>
>>>>>>>>> It looks like your email client ate the tabs, but if I apply the change
>>>>>>>>> myself it seems to fix the palm rejection regression for me.
>>>>>>>>>
>>>>>>>>> Tested-by: Cameron Gutman<aicommander@...il.com>
>>>>>>>> Sorry, I was short on time and just copied the diff into the email. I'll
>>>>>>>> submit a proper patch soon with your tested-by included. Thanks for testing.
>>>>>>>>
>>>>>>>>
>>>>>>> I just pointed out this patch (well the actual submission) to Jason
>>>>>>> (Cc-ed). Given that there is no proper handling of MT_TOOL_PALM yet in
>>>>>>> userspace, I thought it was the easiest way.
>>>>>>> However, it seems that this doesn't enhance the jumps and just make it worse.
>>>>>> I was assuming that the jumps and palm rejection where two separate issues.
>>>>>> But, the palm rejection patch makes things worse?
>>>>>>
>>>>>>> Is there anything we can do to fix it (besides temporary disabling the
>>>>>>> automatic loading of hid-rmi)?
>>>>>> I do not have a fix for the jumps yet. My next step is to file a bug against
>>>>>> libinput or the kernel. I used evemu-record to capture a log with jumps, but
>>>>>> when I play it back with a different userspace input stack with an older
>>>>>> version of libinput I do not see the jumps. I see the jumps on Fedora 25
>>>>>> with libinput 1.6.3 vs Ubuntu 16.10 with libinput 1.4.3 with X). Or at least
>>>>>> the jumps are not as significant. But, its possible there is an issue with
>>>>>> how the events are being reported which is incorrect and confusing libinput.
>>>>>> The X and Y coordinates being reported by the firmware seem correct and I
>>>>>> haven't found a problem yet. I thought a bug would be a better place to
>>>>>> collect evemu-record logs and compare.
>>>>> fwiw, there's a fairly easy way to quickly check libinput for changes and
>>>>> that's by having the gtk3-headers installed at configure time and then
>>>>> running sudo ./tools/event-gui to visualize the movement (Esc quits)
>>>>>
>>>>> That tool uses libinput data directly to draw pointer motion etc, so it's a
>>>>> way to quickly bisect to where changes happen. Without anything else to go
>>>>> on, I'd say it's the new touchpad acceleration code from libinput 1.5 - the
>>>>> max accel factor has changed so depending on the speed of the jumps, you can
>>>>> now get stronger pointer movement.
>>>>>
>>>>> Cheers,
>>>>>       Peter
>>>> I have been looking into this on and off and I found a couple things, but
>>>> nothing conclusive yet. I think Peter is right that versions of libinput 1.5
>>>> and later do make the jump more pronounced. But, the new acceleration code
>>>> my simply be amplifying the jumps. I went ahead and filed a libinput bug
>>>> since the jumps are more significant in newer versions of libinput and I
>>>> attached some evemu-record logs.
>>>>
>>>> https://bugs.freedesktop.org/show_bug.cgi?id=100436
>>>>
>>>> I also spent time looking into the kernel drivers to see if they were
>>>> causing or contributing to the jumps. One of the things that I tried was
>>>> calling rmi_irq_fn() from a workqueue instead of calling
>>>> generic_handle_irq(). Originally, we were using a workqueue before interrupt
>>>> handling was added to the rmi core. I also tried moving the call to
>>>> generic_handle_irq() to a workqueue. In both cases the jumps seemed to
>>>> disappear or at least be reduced. I looked through the irq handling code and
>>>> I did not see anything which should cause an issue. The only difference
>>>> between irq thread and the workqueue that I could think of is that the irq
>>>> thread runs at a higher priority. But, I didn't really see much of a
>>>> difference between the timing of the events in the evemu-record logs.
>>> Despite libinput having issues has reported by Peter, I wonder if the
>>> priority of the IRQ thread isn't the one interfering with the data here.
>>> In the workqueue version, the processing of the events didn't interfere
>>> with the retrieval of the I2C values. But with the IRQ thread, we might
>>> be delaying the retrieval of the values, and we might not be reading the
>>> correct value at the right time (oversimplifying it, but I think you get
>>> the gist of it). The 2 IRQ threads from I2C to read the data and the
>>> other one from RMI4 might simply be interfering.
>>>
>>> I am sure you have something equivalent in your tree, but could you
>>> confirm that the following patch removes the jumps?
>> Yes, this patch does remove the jumps. My version just restored the old
>> functionality which was to call rmi_process_interrupts from a workqueue
>> inside hid-rmi. Your patch seems more complete.
>>
>> I did look to see if I could find something in the threaded IRQ code which
>> would confirm that there was some interference going on. But, I didn't find
>> anything. I also see jumps with USB devices and since USB devices do not use
>> threaded IRQs that did not seem to be the source. Looking at the call stack
>> in which rmi_input_event() gets called they seem quite different between USB
>> and I2C.
>>
>> I also tried calling generic_handle_irq() from a workqueue and that also
>> seemed to remove the jumps. That led me to look into if there were any side
>> affects from calling local_irq_save / restore or generic_handle_irq() from
>> inside the IRQ thread or IRQ handler. But, I could not find anything which
>> would indicate that doing this was unsafe.
>>
>> This is what I tried:
>> https://github.com/aduggan/linux/commit/d484e423e7375f1a6564f735f44a1246f6c0ee84
> Thanks. Your patch looks smaller than mine :)
>
> Jiri, Dmitry, which patch would you prefer having upstream?
>
> Andrew's patch is smaller but requires a workqueue in hid-rmi, which
> then reinject the IRQ in RMI4. Mine has the workqueue in RMI4 and
> ditches the IRQ in hid-rmi all together (so no need to call
> local_irq_save() anymore).
>
>>> ---
>>>
>>>   From b60c0b4f145e171e55ffd861a852a49f5104d59f Mon Sep 17 00:00:00 2001
>>> From: Benjamin Tissoires<benjamin.tissoires@...hat.com>
>>> Date: Wed, 29 Mar 2017 10:41:34 +0200
>>> Subject: [PATCH] Input: rmi4 - remove the need for artificial IRQ in case of
>>>    HID
>>>
>>> The IRQ from rmi4 may interfere with the one we currently use on i2c-hid.
>>> Given that there is already a need for an external API from rmi4 to
>>> forward the attention data, we can, in this particular case rely on a
>>> separate workqueue to prevent cursor jumps.
>>>
>>> Signed-off-by: Benjamin Tissoires<benjamin.tissoires@...hat.com>
>> Tested-by: Andrew Duggan <aduggan@...aptics.com>
> Great :)
>
> Just to be sure, does suspend/resume still works?

Suspend / resume work without any issues.

> And also, could you send to Peter a new evemu-record of the device
> without the jumps? (attaching it on the fdo bug should be sufficient I
> guess).

I attached one to the bug.

Andrew

> Cheers,
> Benjamin
>
>>> ---
>>>    drivers/hid/hid-rmi.c           |  64 ---------------------
>>>    drivers/input/rmi4/rmi_driver.c | 122 ++++++++++++++++++++++++----------------
>>>    include/linux/rmi.h             |   1 +
>>>    3 files changed, 75 insertions(+), 112 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
>>> index 5b40c26..4aa882c 100644
>>> --- a/drivers/hid/hid-rmi.c
>>> +++ b/drivers/hid/hid-rmi.c
>>> @@ -316,19 +316,12 @@ static int rmi_input_event(struct hid_device *hdev, u8 *data, int size)
>>>    {
>>>    	struct rmi_data *hdata = hid_get_drvdata(hdev);
>>>    	struct rmi_device *rmi_dev = hdata->xport.rmi_dev;
>>> -	unsigned long flags;
>>>    	if (!(test_bit(RMI_STARTED, &hdata->flags)))
>>>    		return 0;
>>> -	local_irq_save(flags);
>>> -
>>>    	rmi_set_attn_data(rmi_dev, data[1], &data[2], size - 2);
>>> -	generic_handle_irq(hdata->rmi_irq);
>>> -
>>> -	local_irq_restore(flags);
>>> -
>>>    	return 1;
>>>    }
>>> @@ -556,56 +549,6 @@ static const struct rmi_transport_ops hid_rmi_ops = {
>>>    	.reset		= rmi_hid_reset,
>>>    };
>>> -static void rmi_irq_teardown(void *data)
>>> -{
>>> -	struct rmi_data *hdata = data;
>>> -	struct irq_domain *domain = hdata->domain;
>>> -
>>> -	if (!domain)
>>> -		return;
>>> -
>>> -	irq_dispose_mapping(irq_find_mapping(domain, 0));
>>> -
>>> -	irq_domain_remove(domain);
>>> -	hdata->domain = NULL;
>>> -	hdata->rmi_irq = 0;
>>> -}
>>> -
>>> -static int rmi_irq_map(struct irq_domain *h, unsigned int virq,
>>> -		       irq_hw_number_t hw_irq_num)
>>> -{
>>> -	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static const struct irq_domain_ops rmi_irq_ops = {
>>> -	.map = rmi_irq_map,
>>> -};
>>> -
>>> -static int rmi_setup_irq_domain(struct hid_device *hdev)
>>> -{
>>> -	struct rmi_data *hdata = hid_get_drvdata(hdev);
>>> -	int ret;
>>> -
>>> -	hdata->domain = irq_domain_create_linear(hdev->dev.fwnode, 1,
>>> -						 &rmi_irq_ops, hdata);
>>> -	if (!hdata->domain)
>>> -		return -ENOMEM;
>>> -
>>> -	ret = devm_add_action_or_reset(&hdev->dev, &rmi_irq_teardown, hdata);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>> -	hdata->rmi_irq = irq_create_mapping(hdata->domain, 0);
>>> -	if (hdata->rmi_irq <= 0) {
>>> -		hid_err(hdev, "Can't allocate an IRQ\n");
>>> -		return hdata->rmi_irq < 0 ? hdata->rmi_irq : -ENXIO;
>>> -	}
>>> -
>>> -	return 0;
>>> -}
>>> -
>>>    static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>>    {
>>>    	struct rmi_data *data = NULL;
>>> @@ -677,18 +620,11 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>>    	mutex_init(&data->page_mutex);
>>> -	ret = rmi_setup_irq_domain(hdev);
>>> -	if (ret) {
>>> -		hid_err(hdev, "failed to allocate IRQ domain\n");
>>> -		return ret;
>>> -	}
>>> -
>>>    	if (data->device_flags & RMI_DEVICE_HAS_PHYS_BUTTONS)
>>>    		rmi_hid_pdata.f30_data.disable = true;
>>>    	data->xport.dev = hdev->dev.parent;
>>>    	data->xport.pdata = rmi_hid_pdata;
>>> -	data->xport.pdata.irq = data->rmi_irq;
>>>    	data->xport.proto_name = "hid";
>>>    	data->xport.ops = &hid_rmi_ops;
>>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>>> index d64fc92..5e65cba 100644
>>> --- a/drivers/input/rmi4/rmi_driver.c
>>> +++ b/drivers/input/rmi4/rmi_driver.c
>>> @@ -209,32 +209,46 @@ void rmi_set_attn_data(struct rmi_device *rmi_dev, unsigned long irq_status,
>>>    	attn_data.data = fifo_data;
>>>    	kfifo_put(&drvdata->attn_fifo, attn_data);
>>> +
>>> +	schedule_work(&drvdata->attn_work);
>>>    }
>>>    EXPORT_SYMBOL_GPL(rmi_set_attn_data);
>>> -static irqreturn_t rmi_irq_fn(int irq, void *dev_id)
>>> +static void attn_callback(struct work_struct *work)
>>>    {
>>> -	struct rmi_device *rmi_dev = dev_id;
>>> -	struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
>>> +	struct rmi_driver_data *drvdata = container_of(work,
>>> +							struct rmi_driver_data,
>>> +							attn_work);
>>>    	struct rmi4_attn_data attn_data = {0};
>>>    	int ret, count;
>>>    	count = kfifo_get(&drvdata->attn_fifo, &attn_data);
>>> -	if (count) {
>>> -		*(drvdata->irq_status) = attn_data.irq_status;
>>> -		drvdata->attn_data = attn_data;
>>> -	}
>>> +	if (!count)
>>> +		return;
>>> -	ret = rmi_process_interrupt_requests(rmi_dev);
>>> +	*(drvdata->irq_status) = attn_data.irq_status;
>>> +	drvdata->attn_data = attn_data;
>>> +
>>> +	ret = rmi_process_interrupt_requests(drvdata->rmi_dev);
>>>    	if (ret)
>>> -		rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
>>> +		rmi_dbg(RMI_DEBUG_CORE, &drvdata->rmi_dev->dev,
>>>    			"Failed to process interrupt request: %d\n", ret);
>>> -	if (count)
>>> -		kfree(attn_data.data);
>>> +	kfree(attn_data.data);
>>>    	if (!kfifo_is_empty(&drvdata->attn_fifo))
>>> -		return rmi_irq_fn(irq, dev_id);
>>> +		schedule_work(&drvdata->attn_work);
>>> +}
>>> +
>>> +static irqreturn_t rmi_irq_fn(int irq, void *dev_id)
>>> +{
>>> +	struct rmi_device *rmi_dev = dev_id;
>>> +	int ret;
>>> +
>>> +	ret = rmi_process_interrupt_requests(rmi_dev);
>>> +	if (ret)
>>> +		rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
>>> +			"Failed to process interrupt request: %d\n", ret);
>>>    	return IRQ_HANDLED;
>>>    }
>>> @@ -242,7 +256,6 @@ static irqreturn_t rmi_irq_fn(int irq, void *dev_id)
>>>    static int rmi_irq_init(struct rmi_device *rmi_dev)
>>>    {
>>>    	struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
>>> -	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>>>    	int irq_flags = irq_get_trigger_type(pdata->irq);
>>>    	int ret;
>>> @@ -260,8 +273,6 @@ static int rmi_irq_init(struct rmi_device *rmi_dev)
>>>    		return ret;
>>>    	}
>>> -	data->enabled = true;
>>> -
>>>    	return 0;
>>>    }
>>> @@ -910,23 +921,27 @@ void rmi_enable_irq(struct rmi_device *rmi_dev, bool clear_wake)
>>>    	if (data->enabled)
>>>    		goto out;
>>> -	enable_irq(irq);
>>> -	data->enabled = true;
>>> -	if (clear_wake && device_may_wakeup(rmi_dev->xport->dev)) {
>>> -		retval = disable_irq_wake(irq);
>>> -		if (retval)
>>> -			dev_warn(&rmi_dev->dev,
>>> -				 "Failed to disable irq for wake: %d\n",
>>> -				 retval);
>>> -	}
>>> +	if (irq) {
>>> +		enable_irq(irq);
>>> +		data->enabled = true;
>>> +		if (clear_wake && device_may_wakeup(rmi_dev->xport->dev)) {
>>> +			retval = disable_irq_wake(irq);
>>> +			if (retval)
>>> +				dev_warn(&rmi_dev->dev,
>>> +					 "Failed to disable irq for wake: %d\n",
>>> +					 retval);
>>> +		}
>>> -	/*
>>> -	 * Call rmi_process_interrupt_requests() after enabling irq,
>>> -	 * otherwise we may lose interrupt on edge-triggered systems.
>>> -	 */
>>> -	irq_flags = irq_get_trigger_type(pdata->irq);
>>> -	if (irq_flags & IRQ_TYPE_EDGE_BOTH)
>>> -		rmi_process_interrupt_requests(rmi_dev);
>>> +		/*
>>> +		 * Call rmi_process_interrupt_requests() after enabling irq,
>>> +		 * otherwise we may lose interrupt on edge-triggered systems.
>>> +		 */
>>> +		irq_flags = irq_get_trigger_type(pdata->irq);
>>> +		if (irq_flags & IRQ_TYPE_EDGE_BOTH)
>>> +			rmi_process_interrupt_requests(rmi_dev);
>>> +	} else {
>>> +		data->enabled = true;
>>> +	}
>>>    out:
>>>    	mutex_unlock(&data->enabled_mutex);
>>> @@ -946,20 +961,22 @@ void rmi_disable_irq(struct rmi_device *rmi_dev, bool enable_wake)
>>>    		goto out;
>>>    	data->enabled = false;
>>> -	disable_irq(irq);
>>> -	if (enable_wake && device_may_wakeup(rmi_dev->xport->dev)) {
>>> -		retval = enable_irq_wake(irq);
>>> -		if (retval)
>>> -			dev_warn(&rmi_dev->dev,
>>> -				 "Failed to enable irq for wake: %d\n",
>>> -				 retval);
>>> -	}
>>> -
>>> -	/* make sure the fifo is clean */
>>> -	while (!kfifo_is_empty(&data->attn_fifo)) {
>>> -		count = kfifo_get(&data->attn_fifo, &attn_data);
>>> -		if (count)
>>> -			kfree(attn_data.data);
>>> +	if (irq) {
>>> +		disable_irq(irq);
>>> +		if (enable_wake && device_may_wakeup(rmi_dev->xport->dev)) {
>>> +			retval = enable_irq_wake(irq);
>>> +			if (retval)
>>> +				dev_warn(&rmi_dev->dev,
>>> +					 "Failed to enable irq for wake: %d\n",
>>> +					 retval);
>>> +		}
>>> +	} else {
>>> +		/* make sure the fifo is clean */
>>> +		while (!kfifo_is_empty(&data->attn_fifo)) {
>>> +			count = kfifo_get(&data->attn_fifo, &attn_data);
>>> +			if (count)
>>> +				kfree(attn_data.data);
>>> +		}
>>>    	}
>>>    out:
>>> @@ -998,9 +1015,12 @@ EXPORT_SYMBOL_GPL(rmi_driver_resume);
>>>    static int rmi_driver_remove(struct device *dev)
>>>    {
>>>    	struct rmi_device *rmi_dev = to_rmi_device(dev);
>>> +	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>>>    	rmi_disable_irq(rmi_dev, false);
>>> +	cancel_work_sync(&data->attn_work);
>>> +
>>>    	rmi_f34_remove_sysfs(rmi_dev);
>>>    	rmi_free_function_list(rmi_dev);
>>> @@ -1230,9 +1250,15 @@ static int rmi_driver_probe(struct device *dev)
>>>    		}
>>>    	}
>>> -	retval = rmi_irq_init(rmi_dev);
>>> -	if (retval < 0)
>>> -		goto err_destroy_functions;
>>> +	if (pdata->irq) {
>>> +		retval = rmi_irq_init(rmi_dev);
>>> +		if (retval < 0)
>>> +			goto err_destroy_functions;
>>> +	}
>>> +
>>> +	data->enabled = true;
>>> +
>>> +	INIT_WORK(&data->attn_work, attn_callback);
>>>    	if (data->f01_container->dev.driver)
>>>    		/* Driver already bound, so enable ATTN now. */
>>> diff --git a/include/linux/rmi.h b/include/linux/rmi.h
>>> index 64125443..dc90178 100644
>>> --- a/include/linux/rmi.h
>>> +++ b/include/linux/rmi.h
>>> @@ -364,6 +364,7 @@ struct rmi_driver_data {
>>>    	struct rmi4_attn_data attn_data;
>>>    	DECLARE_KFIFO(attn_fifo, struct rmi4_attn_data, 16);
>>> +	struct work_struct attn_work;
>>>    };
>>>    int rmi_register_transport_device(struct rmi_transport_dev *xport);
>>> -- 2.9.3 I only tested this on a prototype attached to a cp2112 USB to
>>> I2C, so I haven't tested suspend/resume and can't check on the jumps
>>> here.
>>>> At this point I am still not sure if the issue is that the events are being
>>>> reported incorrectly by the kernel or if the new touchpad acceleration code
>>>> in libinput is just not handling the events correctly. I would appreciate
>>>> any suggestions. I'm not sure how much time we have left before we need to
>>>> decide if we need to go back to hid-multitouch or not.
>>> If we can get the confirmation that removing the IRQ handling from
>>> hid-rmi solves the issue, it would be a matter of submitting this patch
>>> to upstream. I also wonder if currently non PTP touchpads are affected
>>> by the jumps or not. If not, I'd say it's safer to delay the HID
>>> catchall for v4.12, if they are, then we should probably make sure this
>>> patch (or any fix) gets into v4.11.
>> Yes, I was able to reproduce the jumps on non PTP touchpads so all touchpads
>> seem to be affected by this.
>>
>> Andrew
>>
>>> Cheers,
>>> Benjamin
>>>
>>>> Thanks,
>>>> Andrew
>>>>
>>>>>> Hopefully, this will end up being a quick fix in the kernel and we can get
>>>>>> it applied to 4.11 without having to hold off on disabling hid-rmi for PTPs.
>>>>>>
>>>>>> Andrew
>>>>>>
>>>>>>> Cheers,
>>>>>>> Benjamin
>>>>>>>
>>>>>>>>>>>>> Just to confirm: I noticed "jumpiness during fine pointing motions" as
>>>>>>>>>>>>> well since switching to 4.11-rc.
>>>>>>>>>> One of my test systems is a XPS 13 9343 and I have not really seen any
>>>>>>>>>> jumpiness. But, based on the data I am seeing that if I lift my finger and
>>>>>>>>>> place it again in a short period of time the first event or so will be at
>>>>>>>>>> the location of the previous contact. Then it will switch over to the
>>>>>>>>>> current location. When switching over to hid-multitouch I was unable to
>>>>>>>>>> reproduce this behavior. This definitely could be the source of the jumps.
>>>>>>>>>>
>>>>>>>>> The jumpiness definitely happens without lifting my finger, but I'm
>>>>>>>>> willing
>>>>>>>>> to test any patch you think would improve the situation. Moving one finger
>>>>>>>>> slowly in a figure-8 across my touchpad shows the issue clearly for me.
>>>>>>>>> The
>>>>>>>>> small variations in speed of my finger due to the friction on the trackpad
>>>>>>>>> get magnified to relatively large jumpy pointer movements on screen. It
>>>>>>>>> seems much more noticeable in diagonal movements than completely vertical
>>>>>>>>> or horizontal movements.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Cameron
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> diff --git a/drivers/input/rmi4/rmi_f34v7.c
>>>>>>>>> b/drivers/input/rmi4/rmi_f34v7.c
>>>>>>>>> index 56c6c39..1291d9a 100644
>>>>>>>>> --- a/drivers/input/rmi4/rmi_f34v7.c
>>>>>>>>> +++ b/drivers/input/rmi4/rmi_f34v7.c
>>>>>>>>> @@ -1369,9 +1369,9 @@ int rmi_f34v7_probe(struct f34_data *f34)
>>>>>>>>>             } else if (f34->bootloader_id[1] == 7) {
>>>>>>>>>                     f34->bl_version = 7;
>>>>>>>>>             } else {
>>>>>>>>> -               dev_err(&f34->fn->dev, "%s: Unrecognized bootloader
>>>>>>>>> version\n",
>>>>>>>>> -                               __func__);
>>>>>>>>> -               return -EINVAL;
>>>>>>>>> +               dev_info(&f34->fn->dev, "%s: Unsupported bootloader
>>>>>>>>> version: %u\n",
>>>>>>>>> +                               __func__, f34->bootloader_id[1]);
>>>>>>>>> +               return -ENODEV;
>>>>>>>>>             }
>>>>>>>>>             memset(&f34->v7.blkcount, 0x00, sizeof(f34->v7.blkcount));

Powered by blists - more mailing lists