GNOME Bugzilla – Bug 746042
mutter wayland doesn't work on machines with drivers that don't handle page flips
Last modified: 2015-03-23 14:12:07 UTC
Apparently the mgag200 driver doesn't currently work with mutter on wayland. This is because the driver doesn't support page flipping. See https://plus.google.com/107928060492923463788/posts/ETAePqZTxPj For more details.
Created attachment 299121 [details] [review] kms-winsys: try to hobble along if driver doesn't support page flips Some drivers ( like mgag200 ) don't yet support drmModePageFlip. This commit tries to emulate the functionality using drmWaitVBlank and drmModeSetCrtc in those cases. Failing all else, it just falls back to flipping right away.
Just to follow up, Richard says the patch fixes things for him (although he gets no mouse cursor, which is probably a separate issue)
Review of attachment 299121 [details] [review]: ::: cogl/winsys/cogl-winsys-egl-kms.c @@ +679,3 @@ + watch_for_next_vblank_operation.request.signal = (gulong) flip; + + ret = drmWaitVBlank (kms_renderer->fd, &watch_for_next_vblank_operation); does drmWaitVBlank() work like drmModePageFlip() in that we get one event per crtc? Because, flip->pending is incremented below for each crtc meaning that if we only get one event (as opposed to one per crtc) then we'll never reach the if (flip->pending == 1) condition in process_vblank() when there's > 1 crtc, and even then we'd have to decrement it process_vblank() instead of process_flip() @@ +692,3 @@ + kms_renderer->vblank_not_supported) + { + g_idle_add (fake_vblank_timeout_handler, flip); why an idle and not immediately here? I'm afraid that the condition while (kms_onscreen->next_fb_id != 0) handle_drm_event (kms_renderer); at the beginning of _cogl_winsys_onscreen_swap_buffers_with_damage() will just hang in this case if we reach it before the idle is processed
Hi, (In reply to Rui Matos from comment #4) > does drmWaitVBlank() work like drmModePageFlip() in that we get one event > per crtc? I presume you get one event per request for an event (though I didn't verify this assumption). We request the event one time for each crtc, so it should be safe if my assumptions hold...Looking now that does indeed seem to be the case, there's vblank_event_list that all queued events get added to, they aren't filtered for dupes or anything like that. One thing I don't do is directly associate a crtc with a given vblank event. I didn't think it was possible, but why I was checking into the above for you I noticed it is possible using a trick with high bits on the type field of the request. I'll update the patch to leverage that. > @@ +692,3 @@ > + kms_renderer->vblank_not_supported) > + { > + g_idle_add (fake_vblank_timeout_handler, flip); > > why an idle and not immediately here? I'm afraid that the condition The other two cases happen after going back to the main loop. As a general rule, whenever I have some cases happen after going back to the main loop, i make all cases happen after going back to main loop, so I don't end up with weird ordering bugs. Anyway, g_idle_add is clearly wrong, regardless. At a minimum it should be using _cogl_poll_renderer_add_idle or similar. > while (kms_onscreen->next_fb_id != 0) > handle_drm_event (kms_renderer); > > at the beginning of _cogl_winsys_onscreen_swap_buffers_with_damage() will > just hang in this case if we reach it before the idle is processed Ugh, I didn't notice that drmHandleEvent was getting called outside of its dispatch handler. I guess it's not a immediately visible problem in practice since we don't swap buffers in inconvenient places. Still, a bad problem that should be fixed. So our options are (I guess): of are: 1) flip right away like you suggest 2) remove the idle source and dispatch right away at the time of the handle_drm_event call there instead of calling handle_drm_event I'll poke the patch and see what I come up with.
Created attachment 299351 [details] [review] kms-winsys: try to hobble along if driver doesn't support page flips Some drivers ( like mgag200 ) don't yet support drmModePageFlip. This commit tries to emulate the functionality using drmWaitVBlank and drmModeSetCrtc in those cases. Failing all else, it just falls back to flipping right away.
so attachment 299351 [details] [review] tries to be more careful about matching each vblank handler to a specific crtc. It also deals with the handle_drm_event() problem you brought up by using your solution of flipping right away.
Review of attachment 299351 [details] [review]: so this still isn't quite right, I think, reading through the code I noticed... ::: cogl/winsys/cogl-winsys-egl-kms.c @@ +650,1 @@ + for (l = kms_display->crtcs; l; l = l->next, crtc_index++) kms_display->crtcs isn't necessarily populated in the same order as drmModeGetResources returns them, so crtc_index isn't necessarily associated with crtc->id
Created attachment 299955 [details] [review] kms-winsys: don't leak drm mode resources object The code calls drmModeGetResources and never frees the result.
Created attachment 299956 [details] [review] kms-winsys: try to hobble along if driver doesn't support page flips Some drivers ( like mgag200 ) don't yet support drmModePageFlip. This commit tries to emulate the functionality using drmWaitVBlank and drmModeSetCrtc in those cases. Failing all else, it just falls back to flipping right away.
So this one has a problem, too. rtcm pointed out that mutter allocates its own CoglKmsCrtc objects with g_slice_new0 and passes them to cogl on monitor hotplug. That means the resources structure isn't necessarily kept up to date on hotplug events. we could fix that by adding more complexity to cogl and mutter to pass fresh resources structs around, or by calling drmModeGetResources on layout changes, but the amount of code needed to support the vblank fallback case is approaching absurd levels. Instead, I think we should say "driver support page flip requests or user gets tearing" and skip the vblank request hail mary. will attach a patch that does that, which is significantly less lines of code.
Created attachment 299976 [details] [review] kms-winsys: fix leaks in _cogl_winsys_egl_display_setup The code fails to free up the display when setup fails.
Created attachment 299977 [details] [review] kms-winsys: fix another leak in _cogl_winsys_egl_display_setup The code fails to free up the drmModeRes structure it queries for.
Created attachment 299978 [details] [review] kms-winsys: try to hobble along if driver doesn't support page flips Some drivers ( like mgag200 ) don't yet support drmModePageFlip. This commit tries to emulate the functionality using drmWaitVBlank and drmModeSetCrtc in those cases. Failing all else, it just falls back to flipping right away.
(In reply to Ray Strode [halfline] from comment #14) > This commit tries to emulate the functionality using drmWaitVBlank > and drmModeSetCrtc in those cases. ^ need to fix commit message
Created attachment 299980 [details] [review] kms-winsys: try to hobble along if driver doesn't support page flips Some drivers ( like mgag200 ) don't yet support drmModePageFlip. This commit tries forgoes waiting for vblank and flips right away in those cases. That prevents this hardware from freezing right away, but does mean there will be visible tearing on screen.
Review of attachment 299980 [details] [review]: This should work ... as said on IRC not sure whether "EBUSY == not supported" is a correct assumption but in practice it probably is. ::: cogl/winsys/cogl-winsys-egl-kms.c @@ +608,3 @@ + { + g_warning ("Failed to flip: %m"); + kms_renderer->page_flips_not_supported = TRUE; Is there any point in not just aborting the loop (i.e "break;") here? If it doesn't support it for one crtc it does not support it at all.
Created attachment 299999 [details] [review] kms-winsys: try to hobble along if driver doesn't support page flips Some drivers ( like mgag200 ) don't yet support drmModePageFlip. This commit forgoes waiting for vblank and flips right away in those cases. That prevents the hardware from freezing up the screen, but does mean there will be some visible tearing.
Review of attachment 299976 [details] [review]: What I'd really like to see is removing the ad-hoc output initialization (mirror, etc) that's not really needed. ::: cogl/winsys/cogl-winsys-egl-kms.c @@ +655,2 @@ if (!output0) + goto out; This one was clearly wrong since we would crash later in output_free() when called from _display_destroy() @@ +744,3 @@ + output_free (kms_renderer->fd, output1); + + _cogl_winsys_egl_display_destroy (display); This call seems wrong though. If this function returns FALSE, the calling code in cogl_context_new() does cogl_object_unref (display) which, IIUC, ends up doing winsys->display_destroy(), i.e. the calling code is responsible to destroy this object, not us.
Review of attachment 299977 [details] [review]: right
Review of attachment 299999 [details] [review]: looks good and seems to work fine in testing on both flip code paths. I think it's safe to include for 3.16.0. Unsure if cogl follows/should follow the code freeze though
(In reply to Rui Matos from comment #19) > Review of attachment 299976 [details] [review] [review]: > > What I'd really like to see is removing the ad-hoc output initialization > (mirror, etc) that's not really needed. Alright let's move discussion to a new bug, the only reason these fixes snuck into this bug was because I was going to use the leaked resources object for vblank handling. now we're not doing the vblank handling, so the leak fixes are pretty independent of this bug. will clone. > This call seems wrong though. If this function returns FALSE, the calling > code in cogl_context_new() does cogl_object_unref (display) which, IIUC, > ends up doing winsys->display_destroy(), i.e. the calling code is > responsible to destroy this object, not us. Indeed, reading fail there. before deciding to put the destroy call in I read the top half of the function, and decided it was idempotent. of course the bottom half of the function isn't...
okay i cloned the leak fixes off into bug 746647
Hi, (In reply to Rui Matos from comment #21) > looks good and seems to work fine in testing on both flip code paths. I > think it's safe to include for 3.16.0. Unsure if cogl follows/should follow > the code freeze though I guess it doesn't since the last release was 1.20.0 in Feb. will push now.
Attachment 299999 [details] pushed as 68d9ba3 - kms-winsys: try to hobble along if driver doesn't support page flips