On Tue, 26 Sep 2017 18:15:38 +0100 Daniel Stone <dani...@collabora.com> wrote:
> Use a real drm_plane to back the scanout plane, displacing
> output->fb_{last,cur,pending} to their plane-tracked equivalents.
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
> libweston/compositor-drm.c | 167
> +++++++++++++++++++++++++++------------------
> 1 file changed, 101 insertions(+), 66 deletions(-)
Hi Daniel,
my comments from last time are mostly fixed.
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 33555d00..23ac2dec 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -2565,10 +2605,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 know this path is all kinds of broken, but how about at least setting
output->state_invalid = true;
here to guarantee we hit the SetCrtc path the repaint?
>
> if (b->use_pixman) {
> drm_output_fini_pixman(output);
> @@ -2909,14 +2945,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) {
> found_elsewhere = true;
> break;
> }
> @@ -3839,8 +3877,6 @@ drm_output_enable(struct weston_output *base)
> output->connector->connector_type == DRM_MODE_CONNECTOR_eDP)
> output->base.connection_internal = true;
>
> - weston_plane_init(&output->scanout_plane, b->compositor, 0, 0);
> -
> if (output->cursor_plane)
> weston_compositor_stack_plane(b->compositor,
> &output->cursor_plane->base,
> @@ -3848,7 +3884,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);
>
> weston_log("Output %s, (connector %d, crtc %d)\n",
> @@ -3877,20 +3914,11 @@ 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_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);
> - drm_fb_unref(output->fb_current);
> - output->fb_current = NULL;
> -
> if (b->use_pixman)
> drm_output_fini_pixman(output);
> else
> drm_output_fini_egl(output);
>
> - weston_plane_release(&output->scanout_plane);
> -
I would assume this has the same issues as the cursor plane patch does.
Deinit should undo weston_compositor_stack_plane().
It probably has the same shutdown problem as well: destroy_sprites()
destroying the plane, then output destruction hitting use-after-free.
> if (output->cursor_plane) {
> /* Turn off hardware cursor */
> drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
> @@ -3960,10 +3988,6 @@ drm_output_disable(struct weston_output *base)
> if (output->base.enabled)
> drm_output_deinit(&output->base);
>
> - assert(!output->fb_last);
> - assert(!output->fb_current);
> - assert(!output->fb_pending);
> -
> output->disable_pending = 0;
>
> weston_log("Disabling output %s\n", output->base.name);
> @@ -4058,6 +4082,16 @@ create_output_for_connector(struct drm_backend *b,
> }
> }
>
> + 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);
> + drm_output_destroy(&output->base);
> + return -1;
> + }
> +
> /* Failing to find a cursor plane is not fatal, as we'll fall back
> * to software cursor. */
> output->cursor_plane =
Other than those, seems fine to me.
I also re-read your scene-graph planes vs. KMS planes comment from last
time, and yeah, agreed. It's a little funny that primary_plane (by
libweston core) is used with renderer damage tracking even when
scanout_plane is used for scanning it out. Maybe when I or someone gets
to fixing the renderer-based clone mode it will get cleaned up.
Thanks,
pq
pgpxzmUyGnrHX.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
