Bug 91541

I might have stripped to much from the data. To make it more obvious:

(In reply to Daniel Martin from comment #0)
> t440p (2013_12)
>         Touchpad model: 1, fw: 8.1, id: 0x1e2b1, caps:
> 0xd001a3/0x940300/0x127c00, board id: 2722, fw id: 1484859

... from dmesg

>         PNP: LEN0036 PNP0f13

... from /sys/devices/platform/i8042/serio1/firmware_id

>         Kernel says:    x [1024..5112], y [2024..4832]
>         Touchpad sends: x [1024..5079], y [2049..4831]

... captured with touchpad-edge-detector (libevdev)

UGh.

Double Ugh.

Still flabbergasted....

Ok, so looking at  the capability fields the firmware reports, and esp. the 3th field, all the 2013_xx models have: 0x127c00 where as the 2014_10 models all have 0x12f800 or 0x12e800, so we can use that to differentiate.

No turning that into a kernel patch is going to get a bit ugly I'm afraid, but I do not see much other choice.

Regards,

Hans

fwiw, dmesg, evtest and edge from a X1 Carbon 3rd is here:
https://bugs.freedesktop.org/show_bug.cgi?id=88609

x [1266..5676], y [1096..4758]

This looks like the new generation touchpads listed here. Daniel, did you confirm whether the advertised range is incorrect without the quirks? Or are they correct if you take the quirks out?

(In reply to Peter Hutterer from comment #3)
> fwiw, dmesg, evtest and edge from a X1 Carbon 3rd is here:
> https://bugs.freedesktop.org/show_bug.cgi?id=88609
> 
> x [1266..5676], y [1096..4758]
> 
> This looks like the new generation touchpads listed here. Daniel, did you
> confirm whether the advertised range is incorrect without the quirks? Or are
> they correct if you take the quirks out?

Just finished to change the code to query and log the dimensions before applying (and logging) quirk values. And I can confirm that the queried dimensions don't need to be quirked on newer models (if we ignore a small difference):
    x [1266..5674], y [1170..4684]

(In reply to Daniel Martin from comment #4)
> (In reply to Peter Hutterer from comment #3)
> > fwiw, dmesg, evtest and edge from a X1 Carbon 3rd is here:
> > https://bugs.freedesktop.org/show_bug.cgi?id=88609
> > 
> > x [1266..5676], y [1096..4758]
> > 
> > This looks like the new generation touchpads listed here. Daniel, did you
> > confirm whether the advertised range is incorrect without the quirks? Or
> are
> > they correct if you take the quirks out?
> 
> Just finished to change the code to query and log the dimensions before
> applying (and logging) quirk values. And I can confirm that the queried
> dimensions don't need to be quirked on newer models (if we ignore a small
> difference):
>     x [1266..5674], y [1170..4684]

Interesting, IIRC then the older models did not give us complete min/max values (I think the min values were missing), so we would use the hardcoded fallback min values, if I remember this right, then we could change the min_max quirk handling code to only apply the quirk if the firmware did not give us both valid min and max values, and then we should be good to go, this sounds like a much better solution then checking the capabilities flags.

(In reply to Hans de Goede from comment #5)
> (In reply to Daniel Martin from comment #4)
> > (In reply to Peter Hutterer from comment #3)
> > > fwiw, dmesg, evtest and edge from a X1 Carbon 3rd is here:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=88609
> > > 
> > > x [1266..5676], y [1096..4758]
> > > 
> > > This looks like the new generation touchpads listed here. Daniel, did you
> > > confirm whether the advertised range is incorrect without the quirks? Or
> are
> > > they correct if you take the quirks out?
> > 
> > Just finished to change the code to query and log the dimensions before
> > applying (and logging) quirk values. And I can confirm that the queried
> > dimensions don't need to be quirked on newer models (if we ignore a small
> > difference):
> >     x [1266..5674], y [1170..4684]
> 
> Interesting, IIRC then the older models did not give us complete min/max
> values (I think the min values were missing),

If I remove this statement from the check before querying the min dimensions:
    SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
I'm able to query the min dimensions (i.e. on the old T540p) and I end up with:
    x [1024..5112], y [1024..3834]
Though, what we want is something like (x looks good, y doesn't):
    x [1024..5062], y [2075..4832]

> so we would use the hardcoded
> fallback min values, if I remember this right, then we could change the
> min_max quirk handling code to only apply the quirk if the firmware did not
> give us both valid min and max values, and then we should be good to go,
> this sounds like a much better solution then checking the capabilities flags.

Atm. I think/hope we end up with something like:
    priv->ext_cap_0c == 0x127c00 ... old model, we need a quirk; return otherwise

To confirm this (partly) I'll re-run all models I have and take notes of the queried dimensions.

Created attachment 164151 [details]
3 patches to log queried values

(Sorry, not yet available on a branch.)

The patches do:
- remove checks before min/max dimension queries
- re-order code to query before applying a quirk
- log queried and quirked dimensions

Hi Daniel,

Thanks for you work on this! About removing the SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 and similar checks. We've discussed this with synaptics (as in the manufacturer i the past), and that check is necessarry, the fact that the older T540 / T440, etc. laptops fail this check is a bug in their firmware, just like them reporting the wrong min/max value even when the check is omitted is a bug.

But now we can use this to our advantage. I like the second patch in your set.

We can track in a flags field which min/max value's we've successfully gotten from the firmware while keeping the SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 check in place, then we will not get a
complete set of min/max values on the older 540 / T440, etc. and we can change the quirks applying code to only apply the quirk when the firmware did not give a complete set of min/max values, and then we should have both the old and the new 540 / T440, etc. models working.

Regards,

Hans

(In reply to Hans de Goede from comment #8)
> Hi Daniel,
> 
> Thanks for you work on this! About removing the
> SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 and similar checks. We've
> discussed this with synaptics (as in the manufacturer i the past), and that
> check is necessarry, the fact that the older T540 / T440, etc. laptops fail
> this check is a bug in their firmware, just like them reporting the wrong
> min/max value even when the check is omitted is a bug.

The new T540p has the same priv->capabilities as the old one, so the min dimensions query would be skipped too. :/

(In reply to Daniel Martin from comment #9)
> (In reply to Hans de Goede from comment #8)
> > Hi Daniel,
> > 
> > Thanks for you work on this! About removing the
> > SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 and similar checks. We've
> > discussed this with synaptics (as in the manufacturer i the past), and that
> > check is necessarry, the fact that the older T540 / T440, etc. laptops fail
> > this check is a bug in their firmware, just like them reporting the wrong
> > min/max value even when the check is omitted is a bug.
> 
> The new T540p has the same priv->capabilities as the old one, so the min
> dimensions query would be skipped too. :/

Argh.

So we could do something like this:

1) If the touchpad pnp_id is in the min_max quirk list skip the
    SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 check for getting min / max
    as that seems to be safe to do on these models
2) If y_min == 1024 assume old version where they forgot to actually program the y_min value, use
    values from quirk
3) else use the value reported by the touchpad

I think that the above might be (somewhat) better then looking at the firmware caps as I initially suggested.

I have send the first version of my patches to the list:
    http://www.spinics.net/lists/linux-input/msg36092.html

My changes basicly do:
- query min/max dimensions before looking out for a quirk
- query the min dimensions even if
    SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
  fails, but the firmware version has proven to be safe for
  this query (atm. this is verion 8.1 only)
- skip quirks if we find a new model - the dimensions are
    x [1266..5674, y [1170..4684]

(In reply to Daniel Martin from comment #11)
> I have send the first version of my patches to the list:
>     http://www.spinics.net/lists/linux-input/msg36092.html
> 
> My changes basicly do:
> - query min/max dimensions before looking out for a quirk
> - query the min dimensions even if
>     SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
>   fails, but the firmware version has proven to be safe for
>   this query (atm. this is verion 8.1 only)
> - skip quirks if we find a new model - the dimensions are
>     x [1266..5674, y [1170..4684]

Thanks for the patch-set. Overall the patch-set looks good, I do have some minor comments. I'm not subscribed to linux-input, so I'm going to leave some comments here instead, when you do a v2 of the set please Cc me.

[PATCH 1/6] Input: synaptics - Split synaptics_resolution(), query first :

The:

	if (SYN_ID_MAJOR(priv->identity) < 4)
		return 0;

In  synaptics_quirks() seems unnecessary to me. I understand that you're doing this to avoid this patch causing any functional differences, but I think it is safe and sensible to drop this.

[PATCH 2/6] Input: synaptics - Log queried and quirked dimension values

Perhaps log all of min/max as reported by the touchpad in one go when all the probing is done, like you do for the quirk case.

[PATCH 3/6] Input: synaptics - Query min dimensions when safe firmware

Do you happen to have a x230, t430 or t530 available for testing IIRC the y_min value eported by the kernel is incorrect there too, we've never spend time fixing this because without top area soft buttons y_min is much less important. But since you're looking into this now, it would be good to see if:
1) My memory is correct that y_min is wrong on the X230 & friends (run edge-detector to check)
2) If this can be fixed by removing the SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 check
3) If 1 and 2 are both true change this patch to also skip the check on thr x230 & friends

Other then that the above would be nice to have, this patch looks good.

[PATCH 4/6] Input: synaptics - Skip quirks when post-2013 dimensions

Does this set of min/max values also get reported by newer x240-s ? That would be weird as the
older x240-s have a different range then the T440 / T550. Likewise e.g. the Helix has a different range, and will likely at one point start showing the same problem.

I think it would be better to not apply the quirks if y_min != 1024, I believe that all models which do not have the right values for y_min / y_max report 1024 for y_min.

Ah I see that in patch 6/6 you remove the x240 from the quirks list, as with the
SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 fix the older model already reports the right ranges itself. I can see how the check you add is the safest way to go then wrt not causing any regressions.

So ack, this patch looks good.

[PATCH 5/6] Input: synaptics - Add PnP id for X1 Carbon 1nd

Didn't the  X1 Carbon 1st (note not 1nd) have physical trackpoint buttons rather then the soft topbuttons? Only devices with the software top buttons should be in this list.

[PATCH 6/6] Input: synaptics - Remove obsolete min/max quirk for X240

Ack, this patch looks good, but it MUST only be applied after applying patch 3.

Most recent patch series:
http://www.spinics.net/lists/linux-input/msg36493.html

The patches are in v4.0 and have been applied to stable branches.

That's the trigger to close this bug or should it stay open until new versions of the stable branches are released?

No, I believe it is time to close this one.