Hi Pekka,
On 21 July 2017 at 14:59, Pekka Paalanen <ppaala...@gmail.com> wrote:
> On Tue, 18 Jul 2017 14:14:32 +0100
> Daniel Stone <dani...@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
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
