On Wed, Sep 10, 2014 at 09:34:52AM +1000, Peter Hutterer wrote: > On Tue, Sep 09, 2014 at 10:42:26PM +0200, Jonas Ådahl wrote: > > 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. > > yeah, good point. > > > 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. > > There are two kinds of calibrations, one is for 'wrongly' mounted screens > and usually requires some simple rotation matrix. The other one is for > on-the-fly calibration for touchscreens that are just off by a bit. Old > camera-based ones suffered from that more than new devices. Those > touchscreens sometimes need re-calibration after each boot, but they are > reasonably rare these days. IIRC Win8 specs require that the touchscreens > work without calibration anyway, so most common devices don't need this at > all but I wouldn't be suprised if use cases like IVI see more of the odd > ones though.
Sounds like the camera based ones seems like they need special support in the compositor to do the post-boot calibration and as such would not need any permanent calibration matrix at all. And yes it seems, as there are already users of it, that a calibration matrix is indeed needed. > > As much as I dislike the backchannel for calibration configuration there is > a need for it to be permanently assigned to a device. so > LIBINPUT_CALIBRATION_MATRIX should stay. Though we should document it > properly and make it part of the API, and provide examples enough that a > user knows what values to put in for the various basic calibrations > (rotated, flipped, upside-down). I'll get patches on the list for that in a > tick. > > > 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. > > yes. without breaking existing use-cases we can't really change that > though. We'd eventually fix it by deprecating and finally removing WL_CALIBRATION though. Probably not for 1.6 though since it's a bit to close to release for making such changes, but maybe for 1.7? Jonas > > Cheers, > Peter > > > > > 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 <[email protected]> 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 > > > > > [email protected] > > > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
