[PATCH weston v11 10/13] compositor-drm: Use drm_plane for scanout plane

[Daniel Stone]

Hi Pekka,

On 21 July 2017 at 14:59, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue, 18 Jul 2017 14:14:32 +0100
> Daniel Stone <daniels at collabora.com> wrote:
>> @@ -1424,15 +1419,29 @@ drm_output_prepare_scanout_view(struct drm_output_state *output_state,
>>               return NULL;
>>       }
>>
>> -     output->fb_pending = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT);
>> -     if (!output->fb_pending) {
>> +     state = drm_output_state_get_plane(output_state, scanout_plane);
>
> Is it not possible that state->fb is already non-NULL? I suppose not,
> because drm_output_state_duplicate() is always called with
> DRM_OUTPUT_STATE_CLEAR_PLANES in any path that may lead here.
>
> Maybe an assert just in case?

Actually, we should return here if there is, because that means we
already have a client scanout view. Matt Hoosier noticed this and sent
a patch for the pre-stateful path; catching it here would have the
same effect.

>> +     state->fb = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT);
>> +     if (!state->fb) {
>> +             drm_plane_state_put_back(state);
>>               gbm_bo_destroy(bo);
>>               return NULL;
>>       }
>>
>> -     drm_fb_set_buffer(output->fb_pending, buffer);
>> +     drm_fb_set_buffer(state->fb, buffer);
>> +
>> +     state->output = output;
>> +
>> +     state->src_x = 0;
>> +     state->src_y = 0;
>> +     state->src_w = ev->surface->width << 16;
>> +     state->src_h = ev->surface->height << 16;
>
> You need to use buffer size here, not the surface size. We allow
> non-identity scale and transform, as long as they are the same between
> the buffer and the output. Surface size would be incorrect.

Sure.

>> -     return &output->scanout_plane;
>> +     state->dest_x = 0;
>> +     state->dest_y = 0;
>> +     state->dest_w = output->base.width;
>> +     state->dest_h = output->base.height;
>
> Destination size should probably be taken from current_mode, in case we
> are temporarily in a non-native video mode. That would make the output
> size different from the mode. And the output size is wrong here anyway,
> if output transform and scale are not identity.

Done.

>> @@ -1607,14 +1635,29 @@ drm_output_repaint(struct weston_output *output_base,
>>       }
>>
>>       drm_output_render(state, damage);
>> -     if (!output->fb_pending)
>> +     scanout_state = drm_output_state_get_plane(state, scanout_plane);
>> +     if (!scanout_state || !scanout_state->fb)
>>               goto err;
>>
>> +     /* The legacy SetCrtc API doesn't allow us to do scaling, and the
>> +      * legacy PageFlip API doesn't allow us to do clipping either. */
>> +     assert(scanout_state->src_x == 0);
>> +     assert(scanout_state->src_y == 0);
>> +     assert(scanout_state->src_w ==
>> +             (unsigned) (output->base.current_mode->width << 16));
>> +     assert(scanout_state->src_h ==
>> +             (unsigned) (output->base.current_mode->height << 16));
>> +     assert(scanout_state->dest_x == 0);
>> +     assert(scanout_state->dest_y == 0);
>> +     assert(scanout_state->src_w == scanout_state->dest_w << 16);
>> +     assert(scanout_state->src_h == scanout_state->dest_h << 16);
>
> The two asserts you said you already fixed. Technically I think the
> code was correct, but definitely not easy to read right. :-D

Really fixed now!

>> @@ -2536,10 +2567,6 @@ drm_output_switch_mode(struct weston_output *output_base, struct weston_mode *mo
>>        *      sledgehammer modeswitch first, and only later showing new
>>        *      content.
>>        */
>> -     drm_fb_unref(output->fb_current);
>> -     assert(!output->fb_last);
>> -     assert(!output->fb_pending);
>> -     output->fb_last = output->fb_current = NULL;
>
> I think the comment above might need updating, but I suppose it's safe
> to say the switch_mode hook is just plain broken for now, to be fixed
> in the future. Or is it broken?
>
> We choose a new mode, we reinitialize the renderer... maybe it works
> most of the time by luck? OTOH, not sure we even have a way to trigger
> this to begin with.

You need a fullscreen_shell client. But yeah, it is just savagely
broken at the moment. :( I'm almost tempted to remove the code to do
it in fullscreen_shell until we have something more workable in the
backends.

>>       if (b->use_pixman) {
>>               drm_output_fini_pixman(output);
>> @@ -2870,14 +2897,16 @@ drm_output_find_special_plane(struct drm_backend *b, struct drm_output *output,
>>                * same plane for two outputs. */
>>               wl_list_for_each(tmp, &b->compositor->pending_output_list,
>>                                base.link) {
>> -                     if (tmp->cursor_plane == plane) {
>> +                     if (tmp->cursor_plane == plane ||
>> +                         tmp->scanout_plane == plane) {
>>                               found_elsewhere = true;
>>                               break;
>>                       }
>>               }
>>               wl_list_for_each(tmp, &b->compositor->output_list,
>>                                base.link) {
>> -                     if (tmp->cursor_plane == plane) {
>> +                     if (tmp->cursor_plane == plane ||
>> +                         tmp->scanout_plane == plane) {
>
> Ah, here they are.

?

>> @@ -3755,6 +3784,15 @@ drm_output_enable(struct weston_output *base)
>>       if (b->pageflip_timeout)
>>               drm_output_pageflip_timer_create(output);
>>
>> +     output->scanout_plane =
>> +             drm_output_find_special_plane(b, output,
>> +                                           WDRM_PLANE_TYPE_PRIMARY);
>> +     if (!output->scanout_plane) {
>> +             weston_log("Failed to find primary plane for output %s\n",
>> +                        output->base.name);
>> +             goto err;
>
> The error path (not this jump but the later ones) forgets to clean up
> scanout_plane.

Fixed, by moving it to output creation and not making the same mistake there.

>> @@ -3804,7 +3840,8 @@ drm_output_enable(struct weston_output *base)
>>       else
>>               b->cursors_are_broken = 1;
>>
>> -     weston_compositor_stack_plane(b->compositor, &output->scanout_plane,
>> +     weston_compositor_stack_plane(b->compositor,
>> +                                   &output->scanout_plane->base,
>>                                     &b->compositor->primary_plane);
>
> The scanout plane is a little strange. It is used for both fullscreen
> direct scanout as well as the renderer output for the primary plane.
> But, I guess that makes no harm. It's probably just another rabbit hole.

I juggled a few different ways around that, and none of them made me
happy. I couldn't really find a good way around scene-graph planes and
KMS planes being totally exactly the same thing, except when we
promote a fullscreen client buffer to the KMS primary plane, at which
point Weston's plane stops being the KMS primary plane and just
temporarily disappears.

Cheers,
Daniel