Hi,
On 12/23/23 16:01, Paul Cercueil wrote:
> Hi Hans,
>
> Le samedi 23 décembre 2023 à 15:29 +0100, Hans de Goede a écrit :
>> Hi Paul,
>>
>> On 12/20/23 14:39, Paul Cercueil wrote:
>>> Hi Dmitry,
>>>
>>> Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
>>>> Hi Paul,
>>>>
>>>> On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
>>>>> Hi Peter,
>>>>>
>>>>> Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
>>>>> écrit :
>>>>>> On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
>>>>>>> From: Chris Morgan <macromorgan@xxxxxxxxxxx>
>>>>>>>
>>>>>>> Stop checking if the minimum abs value is greater than the
>>>>>>> maximum
>>>>>>> abs
>>>>>>> value. When the axis is inverted this condition is allowed.
>>>>>>> Without
>>>>>>> relaxing this check, it is not possible to use uinput on
>>>>>>> devices in
>>>>>>> userspace with an inverted axis, such as the adc-joystick
>>>>>>> found
>>>>>>> on
>>>>>>> many handheld gaming devices.
>>>>>>
>>>>>> As mentioned in the other thread [1] a fair bit of userspace
>>>>>> relies
>>>>>> on
>>>>>> that general assumption so removing it will likely cause all
>>>>>> sorts of
>>>>>> issues.
>>>>>
>>>>> There is some userspace that works with it though, so why
>>>>> restrict
>>>>> it
>>>>> artificially?
>>>>>
>>>>> The fact that some other userspace code wouldn't work with it
>>>>> sounds a
>>>>> bit irrelevant. They just never encountered that min>max usage
>>>>> before.
>>>>>
>>>>> And removing this check won't cause all sort of issues, why
>>>>> would
>>>>> it?
>>>>> It's not like the current software actively probes min>max and
>>>>> crash
>>>>> badly if it doesn't return -EINVAL...
>>>>
>>>> It will cause weird movements because calculations expect min be
>>>> the
>>>> minimum, and max the maximum, and not encode left/right or
>>>> up/down.
>>>> Putting this into adc joystick binding was a mistake.
>>>
>>> I don't see why it was a mistake, it's only one of the ways to
>>> specify
>>> that the axis is inverted. This information is between the firmware
>>> (DT) and the kernel, that doesn't mean the information has to be
>>> relayed as-is to the userspace.
>>>
>>> Unlike what you wrote in your other answer, when talking about
>>> input
>>> the kernel doesn't really normalize anything - it gives you the
>>> min/max
>>> values, and the raw samples, not normalized samples (they don't get
>>> translated to a pre-specified range, or even clamped).
>>>
>>> I don't really like the idea of having the driver tamper with the
>>> samples, but if the specification really is that max>min, then it
>>> would
>>> be up to evdev/joydev (if the individual drivers are allowed
>>> min>max)
>>> or adc-joystick (if they are not) to process the samples.
>>
>> I don't see why a driver, especially a userspace driver which
>> then injects things back into the kernel using uinput, would
>> not take care of inverting the samples itself and then just
>> present userspace with normalized data where min is simply 0
>> (as result of normalization as part of inversion) and
>> max is (original_max - original_min).
>
> Yes, I totally agree.
>
> What I was saying is, as Chris is only "piping" events from adc-
> joystick into uinput, that the problem is more that evdev/joydev don't
> handle axis inversion and provide min>max values that most of the
> userspace (and some kernel drivers e.g. uinput) don't support.
Ah I see, that sounds like a joydev adc-joystick / driver bug
to me then.
>> Note that this is exactly what is being done for touchscreens,
>> where having the touchscreen mounted e.g. upside-down is
>> a long standing issue and this is thus also a long solved issue,
>> see: drivers/input/touchscreen.c which contains generic
>> code for parsing device-properties including swapped / inverted
>> axis as well as generic code for reporting the position to the
>> input core, where the helpers from drivers/input/touchscreen.c
>> take care of the swap + invert including normalization when
>> doing inversion.
>>
>> Specifically this contains in touchscreen_parse_properties() :
>>
>> prop->max_x = input_abs_get_max(input, axis_x);
>> prop->max_y = input_abs_get_max(input, axis_y);
>>
>> if (prop->invert_x) {
>> absinfo = &input->absinfo[axis_x];
>> absinfo->maximum -= absinfo->minimum;
>> absinfo->minimum = 0;
>> }
>>
>> if (prop->invert_y) {
>> absinfo = &input->absinfo[axis_y];
>> absinfo->maximum -= absinfo->minimum;
>> absinfo->minimum = 0;
>> }
>>
>> and then when reporting touches:
>>
>> void touchscreen_report_pos(struct input_dev *input,
>> const struct touchscreen_properties
>> *prop,
>> unsigned int x, unsigned int y,
>> bool multitouch)
>> {
>> if (prop->invert_x)
>> x = prop->max_x - x;
>>
>> if (prop->invert_y)
>> y = prop->max_y - y;
>>
>> if (prop->swap_x_y)
>> swap(x, y);
>>
>> input_report_abs(input, multitouch ? ABS_MT_POSITION_X :
>> ABS_X, x);
>> input_report_abs(input, multitouch ? ABS_MT_POSITION_Y :
>> ABS_Y, y);
>> }
>>
>> One of the tasks of a driver / the kernel is to provide some
>> level of hardware abstraction to isolate userspace from
>> hw details. IMHO taking care of the axis-inversion for userspace
>> with something like the above is part of the kernels' HAL task.
>
> Totally agree, but this is not done anywhere, is it? evdev seems to
> just pass the hardware values alongside some basic meta-data (min/max
> values, fuzz etc.), it does not tamper with the data. Should evdev
> handle axis inversion? Should it be in adc-joystick (and every other
> driver that needs that) instead?
For touchcreens we have chosen to have a set of generic helpers
and then make using those helpers the responsibility of the driver.
Part of the reason for doing this is because some touchscreen drivers
already were doing axis inversion inside the driver triggering on
things like e.g. DMI matches, or maybe custom pre standardization
device properties, etc.
So the decision was made to add a set of helpers and convert drivers
one by one. Where drivers can e.g. still set prop->invert_x manually,
but then they also need to take care of the min/max adjustments
manually (min is typically 0 for touchscreens though).
I expect that there will also be enough existing special handling
in the joystick code that piece-meal conversion using helpers
is likely best.
With that said having axis inversion support in the evdev core
does sound interesting, but that means also storing the max-value
inside the core for abs axis and this will likely be a big
change / lots of work.
Regards,
Hans
