[PATCH weston v10 13/61] compositor-drm: Introduce fb_last member

[Daniel Stone]

Hi,
Thanks for the review, and earlier merges too!

On 7 April 2017 at 13:57, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue,  4 Apr 2017 17:54:31 +0100
> Daniel Stone <daniels at collabora.com> 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 <pekka.paalanen at collabora.co.uk>
>
> 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