[PATCH v2 weston] libinput: normalize WL_CALIBRATION before passing it to libinput
Jonas Ådahl
jadahl at gmail.com
Tue Sep 9 13:42:26 PDT 2014
On Tue, Sep 09, 2014 at 01:15:42PM +1000, Peter Hutterer wrote:
> On Mon, Sep 08, 2014 at 08:16:07PM +0200, Jonas Ådahl wrote:
> > On Fri, Sep 05, 2014 at 11:25:25AM +1000, Peter Hutterer wrote:
> > > WL_CALIBRATION, introduced in weston-1.1, requires the translation component
> > > of the calibration matrix to be in screen coordinates. libinput does not have
> > > access to this and it's not a very generic way to do this anyway. So with
> > > the libinput backend, WL_CALIBRATION support is currently broken (#82742).
> > > This cannot be fixed in libinput without changing its API for this specific
> > > use-case.
> > >
> > > This patch lets weston take care of WL_CALIBRATION. It takes the original
> > > format and normalizes it before passing it to libinput. This way libinput
> > > still does the coordinate transformation, weston just needs to provide the
> > > initial configuration.
> > >
> > > Note that this needs an updated libinput, otherwise libinput will try to
> > > transform coordinates as well.
> >
> > Should we also deprecate the WL_CALIBRATION udev parameter and warn
> > about when using it, as well as having weston-calibrator output the
> > normalized calibration matrix?
>
> tbh, I'd rather leave it as-is. I'm not a big fan of the configuration
> back-channel in libinput, I'd prefer this to be set by the compositor as
> required. with WL_CALIBRATION as-is that works fine, having a weston tool
> spit out a udev configuration that is then read by libinput to give weston
> calibrated values seems a bit all over the place to me.
Are you saying you want to get rid of LIBINPUT_CALIBRATION_MATRIX,
keep having the compositors read the udev configuration unconditionally?
At least we should not have both LIBINPUT_CALIBRATION_MATRIX *and*
WL_CALIBRATION around forever.
Regarding where to put it, if a calibration tool outputs data to be read
by libinput, it makes more sense to move it there. Whether its good or
not to have a back-channel (i.e. udev configuration reading), I'm not
sure what I think is better. Calibration is not really configuration
that you change, its something that is always system wide, either
bundled by the OEM or set up once and never touched again, so it's not
really that bad to have it set silently IMHO.
On a side note, there is still a problem with the current format of
WL_CALIBRATION and that is that it's translation part is still in pixels,
and outputs don't necessarily always have the same resolution, which
would make the calibration off when not using the resolution used when
calibrating.
Jonas
>
> > We could also move weston-calibrator to
> > libinput, but then we'd need to port it to something else than toytoolkit
> > (or by not using a toolkit), which naturally is less trivial. Keeping
> > weston-calibrator that outputs the non-normalized matrix, at least after
> > the release, is probably not a good idea.
> >
> > In anyway, this patch can be considered
> > Reviewed-by: Jonas Ådahl <jadahl at gmail.com> with some minor style nits
> > below.
>
> thanks for the review, updated patch is on the list.
>
> Cheers,
> Peter
> >
> >
> > Jonas
> >
> > > ---
> > > Changes to v1:
> > > - fix copy/paste error assigning height to the screen width
> > >
> > > https://bugs.freedesktop.org/show_bug.cgi?id=82742 has positive test results
> > > so we don't break exisint setups with WL_CALIBRATION set.
> > >
> > > Can be merged once libinput 0.6 is released.
> > >
> > > configure.ac | 2 +-
> > > src/libinput-device.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 88 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/configure.ac b/configure.ac
> > > index bc5c88a..fb19fb2 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -160,7 +160,7 @@ AC_ARG_ENABLE(libinput-backend, [ --disable-libinput-backend],,
> > > AM_CONDITIONAL([ENABLE_LIBINPUT_BACKEND], [test x$enable_libinput_backend = xyes])
> > > if test x$enable_libinput_backend = xyes; then
> > > AC_DEFINE([BUILD_LIBINPUT_BACKEND], [1], [Build the libinput input device backend])
> > > - PKG_CHECK_MODULES(LIBINPUT_BACKEND, [libinput >= 0.5.0])
> > > + PKG_CHECK_MODULES(LIBINPUT_BACKEND, [libinput >= 0.6.0])
> > > fi
> > >
> > >
> > > diff --git a/src/libinput-device.c b/src/libinput-device.c
> > > index 6e50eeb..fded34c 100644
> > > --- a/src/libinput-device.c
> > > +++ b/src/libinput-device.c
> > > @@ -282,6 +282,90 @@ notify_output_destroy(struct wl_listener *listener, void *data)
> > > }
> > > }
> > >
> > > +/**
> > > + * The WL_CALIBRATION property requires a pixel-specific matrix to be
> > > + * applied after scaling device coordinates to screen coordinates. libinput
> > > + * can't do that, so we need to convert the calibration to the normalized
> > > + * format libinput expects.
> > > + */
> > > +static void
> > > +evdev_device_set_calibration(struct evdev_device *device)
> > > +{
> > > + struct udev *udev;
> > > + struct udev_device *udev_device = NULL;
> > > + const char *sysname = libinput_device_get_sysname(device->device);
> > > + const char *calibration_values;
> > > + uint32_t width, height;
> > > + float calibration[6];
> > > + enum libinput_config_status status;
> > > +
> > > + if (!device->output)
> > > + return;
> > > +
> > > + width = device->output->width;
> > > + height = device->output->height;
> > > + if (width == 0 || height == 0)
> > > + return;
> > > +
> > > + /* If libinput has a pre-set calibration matrix, don't override it */
> > > + if (!libinput_device_config_calibration_has_matrix(device->device) ||
> > > + libinput_device_config_calibration_get_default_matrix(
> > > + device->device,
> > > + calibration) != 0)
> > > + return;
> > > +
> > > + udev = udev_new();
> > > + if (!udev)
> > > + return;
> > > +
> > > + udev_device = udev_device_new_from_subsystem_sysname(udev,
> > > + "input",
> > > + sysname);
> > > + if (!udev_device)
> > > + goto out;
> > > +
> > > + calibration_values =
> > > + udev_device_get_property_value(udev_device,
> > > + "WL_CALIBRATION");
> >
> > These two lines should be indented.
> >
> > > +
> > > + if (!calibration_values || sscanf(calibration_values,
> > > + "%f %f %f %f %f %f",
> > > + &calibration[0],
> > > + &calibration[1],
> > > + &calibration[2],
> > > + &calibration[3],
> > > + &calibration[4],
> > > + &calibration[5]) != 6)
> > > + goto out;
> > > +
> > > + weston_log("Applying calibration: %f %f %f %f %f %f "
> > > + "(normalized %f %f)\n",
> > > + calibration[0],
> > > + calibration[1],
> > > + calibration[2],
> > > + calibration[3],
> > > + calibration[4],
> > > + calibration[5],
> > > + calibration[2]/width,
> > > + calibration[5]/height);
> >
> > There should be spaces between the binary operators.
> >
> > > +
> > > + /* normalize to a format libinput can use. There is a chance of
> > > + this being wrong if the width/height don't match the device
> > > + width/height but I'm not sure how to fix that */
> > > + calibration[2] /= width;
> > > + calibration[5] /= height;
> > > +
> > > + status = libinput_device_config_calibration_set_matrix(device->device,
> > > + calibration);
> > > + if (status != LIBINPUT_CONFIG_STATUS_SUCCESS)
> > > + weston_log("Failed to apply calibration.\n");
> > > +
> > > +out:
> > > + if (udev_device)
> > > + udev_device_unref(udev_device);
> > > + udev_unref(udev);
> > > +}
> > > +
> > > void
> > > evdev_device_set_output(struct evdev_device *device,
> > > struct weston_output *output)
> > > @@ -295,6 +379,7 @@ evdev_device_set_output(struct evdev_device *device,
> > > device->output_destroy_listener.notify = notify_output_destroy;
> > > wl_signal_add(&output->destroy_signal,
> > > &device->output_destroy_listener);
> > > + evdev_device_set_calibration(device);
> > > }
> > >
> > > static void
> > > @@ -318,6 +403,8 @@ configure_device(struct evdev_device *device)
> > > libinput_device_config_tap_set_enabled(device->device,
> > > enable_tap);
> > > }
> > > +
> > > + evdev_device_set_calibration(device);
> > > }
> > >
> > > struct evdev_device *
> > > --
> > > 1.9.3
> > >
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list