GNOME Bugzilla – Bug 676006
If EDID mapping fails, try to find a built-in output
Last modified: 2012-06-19 15:47:36 UTC
Created attachment 213979 [details] [review] 0001-wacom-add-helper-function-to-get-output-list.patch serial devices with no useful EDID cannot be calibrated, we just see the error message below: (gnome-control-center:11071): wacom-cc-panel-WARNING **: No fuzzy match based on heuristics was found. (gnome-control-center:11071): wacom-cc-panel-CRITICAL **: Output associated with the tablet is not connected. Unable to calibrate. Attached patches add output name matching. If no edid mapping was found, search for eDP or LVDS outputs, they're built-in. The first patch is just a cleanup to re-use some code.
Created attachment 213980 [details] [review] 0002-wacom-if-edid-matching-fails-search-for-eDP-LVDS-bui.patch
Review of attachment 213979 [details] [review]: I would put those 3 items into their own structure, and have its own free/destroy function. This would also make keeping the data around for 2 consecutive calls easier.
Review of attachment 213980 [details] [review]: As mentioned in the other patch's review, you're poking the hardware once already, then destroying everything, and building it again. Keep the screen data around for long enough to do what you need to do.
Created attachment 215111 [details] [review] 0001-wacom-add-helper-function-to-get-output-list.patch Updated. Changes to v1: get output list once in find_output, then re-use across find_output_*
Created attachment 215112 [details] [review] 0001-wacom-if-edid-matching-fails-search-for-eDP-LVDS-bui.patch Updated. Changes to v1: use pre-filled output list
Review of attachment 215112 [details] [review]: ::: panels/wacom/gsd-wacom-device.c @@ +591,3 @@ + name = gnome_rr_output_info_get_name (*rr_output_info); + if (g_str_has_prefix (name, "eDP") || + g_str_has_prefix (name, "LVDS")) The misnamed "gnome_rr_output_is_laptop()" function does that, and it even handles connector types from RANDR 1.3
Review of attachment 215111 [details] [review]: ::: panels/wacom/gsd-wacom-device.c @@ +495,3 @@ +static void +gsd_wacom_output_list_finalize (GsdWacomRROutputList *ol) finalize() is for objects. Use _free() @@ +498,3 @@ +{ + if (ol->rr_screen) + g_object_unref (ol->rr_screen); g_clear_object() @@ +500,3 @@ + g_object_unref (ol->rr_screen); + if (ol->rr_screen) + g_object_unref (ol->rr_config); Ditto.
(In reply to comment #6) > Review of attachment 215112 [details] [review]: > > ::: panels/wacom/gsd-wacom-device.c > @@ +591,3 @@ > + name = gnome_rr_output_info_get_name (*rr_output_info); > + if (g_str_has_prefix (name, "eDP") || > + g_str_has_prefix (name, "LVDS")) > > The misnamed "gnome_rr_output_is_laptop()" function does that, and it even > handles connector types from RANDR 1.3 how do I convert from GnomeRROutputInfo to GnomeRROutput?
(In reply to comment #8) > (In reply to comment #6) > > Review of attachment 215112 [details] [review] [details]: > > > > ::: panels/wacom/gsd-wacom-device.c > > @@ +591,3 @@ > > + name = gnome_rr_output_info_get_name (*rr_output_info); > > + if (g_str_has_prefix (name, "eDP") || > > + g_str_has_prefix (name, "LVDS")) > > > > The misnamed "gnome_rr_output_is_laptop()" function does that, and it even > > handles connector types from RANDR 1.3 > > how do I convert from GnomeRROutputInfo to GnomeRROutput? Sigh, yes. The whole code in GsdWacomDevice uses GnomeRROutputInfo, which is a flattened version of the current XRandR configuration, usually for storage into config files. We shouldn't be using GnomeRROutputInfo at all...
We can now use GnomeRROutput after bug 677032 gets merged. This makes the laptop detection much better too, as we can rely on the connector type. Sorry about the churn.
Created attachment 216726 [details] [review] Updated and consolidated patch Now that the patch for bug 677032 got merged, we really do not need to create a new structure, passing "GnomeRRScreen *" is enough. This new patch removes the now unnecessary GsdWacomRROutputList structure added by the previous versions of the same and uses GnomeRRScreen to avoid querying the hardware multiple times. It also uses gnome_rr_output_is_laptop() to identify the built-in monitor.
_Note_: all this really belongs to gnome-settings-daemon and not gnome-control-center, so the proposed patch (attachment 216726 [details] [review]) is meant to be applied to gnome-settings-daemon.
Review of attachment 216726 [details] [review]: Looks fine otherwise. ::: plugins/wacom/gsd-wacom-device.c @@ +754,3 @@ + } + + rr_output = find_output_by_display (rr_screen, device); Are you leaking rr_output if it's not a screen tablet?
(In reply to comment #13) > Are you leaking rr_output if it's not a screen tablet? No leak there, it's the returned value. That value is unref'ed later by the caller (gsd_wacom_device_get_display_monitor())
Committed in gnome-settings-daemon and pulled in gnome-control-center