Hi, Thanks for the review, and earlier merges too! On 7 April 2017 at 13:57, Pekka Paalanen <[email protected]> wrote: > On Tue, 4 Apr 2017 17:54:31 +0100 > Daniel Stone <[email protected]> wrote: >> @@ -1593,10 +1619,16 @@ drm_output_switch_mode(struct weston_output >> *output_base, struct weston_mode *mo >> output->base.current_mode->flags = >> WL_OUTPUT_MODE_CURRENT | WL_OUTPUT_MODE_PREFERRED; >> >> - /* reset rendering stuff. */ >> + /* XXX: This drops our current buffer too early, before we've started >> + * displaying it. Ideally this should be much more atomic and >> + * integrated with a full repaint cycle, rather than doing a >> + * sledgehammer modeswitch first, and only later showing new >> + * content. >> + */ >> drm_fb_unref(output->fb_current); >> - drm_fb_unref(output->fb_pending); >> - output->fb_current = output->fb_pending = NULL; >> + assert(!output->fb_last); >> + assert(!output->fb_pending); > > I was about to complain that these asserts could be randomly hit by > clients that invoke temporary video mode changes through fullscreen > scaling modes. But, that code has not existed for some time now. > > The only user left for the mode switching API is the fullscreen shell, > which calls weston_output_mode_switch_to_{native,temporary}(). > > There is potential here to randomly break fullscreen shell and external > plugins that might be using this API. I'm not sure I care though.
I agree, it's not nice. I decided not to route around it because it's so viciously broken anyway (cf. comment and https://phabricator.freedesktop.org/T7621); before this series it would've dropped the framebuffer, turned the monitor off, allocated new buffers, and if that failed, you would've been left staring at a blank screen until you somehow managed to kill the app. We really should've got T7621 in GSoC this year. :( But hopefully your clone-mode stuff leads us much closer towards something sensible/supportable there! >> + output->fb_last = output->fb_current = NULL; >> >> if (b->use_pixman) { >> drm_output_fini_pixman(output); >> @@ -2632,8 +2664,9 @@ drm_output_deinit(struct weston_output *base) >> struct drm_output *output = to_drm_output(base); >> struct drm_backend *b = to_drm_backend(base->compositor); >> >> - /* output->fb_pending must not be set here; >> + /* output->fb_last and output->fb_pending must not be set here; >> * destroy_pending/disable_pending exist to guarantee exactly this. */ >> + assert(!output->fb_last); >> assert(!output->fb_pending); > > First I was a little bit worried about these asserts in case we shut > down the compositor while a pageflip is pending, but it's harmless. > drm_output_destroy() just returns early, we never service any events > again, and exit the process. I tested and it doesn't abort. They can't be hit, because the only two callers (destroy and disable) bail early if the *_pending flags are set. That's why we have {page_flip,vblank,atomic_complete}_pending. >> @@ -2854,8 +2888,9 @@ destroy_sprites(struct drm_backend *backend) >> sprite->plane_id, >> output->crtc_id, 0, 0, >> 0, 0, 0, 0, 0, 0, 0, 0); >> + assert(!sprite->fb_last); >> + assert(!sprite->fb_pending); >> drm_fb_unref(sprite->fb_current); >> - drm_fb_unref(sprite->fb_pending); >> weston_plane_release(&sprite->plane); >> free(sprite); >> } > > Reviewed-by: Pekka Paalanen <[email protected]> > > I shall wait till Monday until pushing this in case someone wants to > scream about the remote possibility of breaking video mode switching. Thankyou! Cheers, Daniel _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
