Make the compositor render software cursors

[emersion]

Making wlroots responsible for rendering them causes some issues. It adds rendering code when swapping buffers, after wlr_renderer_end. It causes issues like #1291, because wlr_output is not presentation-time-aware (and shouldn't be).

We should probably remove the rendering code from wlr_output_swap_buffers and make the compositor do it. We could provide a helper function to render software cursors that could be called by the compositor in its rendering process.

Same applies to fullscreen surfaces -- but we should remove wlr_output_set_fullscreen_surface altogether, replacing it with something else.

@ascent12 How does this interact with the renderer redesign?

---

wlroots has migrated to gitlab.freedesktop.org. This issue has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/1363

[ascent12]

+1
Rendering it for the user was a naive decision I made before we had things like damage handling and redrew the entire frame every time. It doesn't interact nicely now that we actually have those extra features.

How does this interact with the renderer redesign?

It's one of those things I need to look into. My knowledge of how X11 actually handles cursors isn't great, so I'll have to see what's actually possible which would work over every backend.

Ideally we'd create another wlr_image that you could draw to with normal renderer commands.

[emersion]

My knowledge of how X11 actually handles cursors isn't great, so I'll have to see what's actually possible which would work over every backend.

On X11 we only support software cursors. I don't think it's worth it to do anything more complicated.

[ascent12]

True. Since with wayland and DRM, we can basically just use normal buffers (although allocated with a special parameter for DRM), it should be able to fit pretty easily into wlr_allocator.
I'll see if X11 can take an xcb_pixmap_t and directly use it as a cursor or not. If it can't we'll just keep ignoring it.
Edit: Yeah, I decided I'm not even going to try and work with the weird way X11 does cursors.

[ammen99]

I think this is a good idea. Having the compositor draw software cursors has another advantage as well - wlroots won't be adding damage (AFAIK) so that the compositor is free to track damage in any way it wants to. Currently wlroots forces me to use scaled coordinates for damage which I find, well, quite inconvenient and complicates damage handling in wayfire.

[emersion]

Currently wlroots forces me to use scaled coordinates for damage which I find, well, quite inconvenient and complicates damage handling in wayfire.

After this change wlroots would take damage in renderer coordinates (scaled + transformed), so that it can provide it directly to the backend.

[ammen99]

Currently wlroots forces me to use scaled coordinates for damage which I find, well, quite inconvenient and complicates damage handling in wayfire.

After this change wlroots would take damage in renderer coordinates (scaled + transformed), so that it can provide it directly to the backend.

You mean when swapping buffers? If yes, that's not a problem, I need to implement transforming from layout to scaled+transformed coordinates anyway for the final phase of drawing, but coordinate systems will be consistent throughout the rest of the code.

[emersion]

Remaining issues to get this closed:

• wlroots sends frame_done events to cursor surfaces in wlr_output_swap_buffers. This makes it impossible for compositors to implement repaint scheduling for cursor surfaces.
• wlroots still damages software cursors when moved (see output_cursor_damage_whole, we probably want to remove wlr_output_damage_whole too)

[ascent12]

I think that we need to remove handling of software cursors entirely from wlroots, including any damage tracking for them.
Basically, we aren't the ones in control of the wlr_renderer and the scene graph, so us performing any rendering really ends up causing issues and requires wlr_output to be a very complex interface for giving damage information etc. back to the user.
Compositors should draw the cursors themselves with the renderer functions directly, and opportunistically make use of hardware cursors.

• Remove struct wlr_output_cursor and all related functions.
• Remove any functions that cause rendering from struct wlr_cursor. I still think it's a useful interface for tracking the cursor position on struct wlr_output_layout, but should not actually render anything.
• Remove wlr_xcursor_manager_set_cursor_image.
• Work out how "cursor locks" are going to work with the screen recording interfaces. Probably just involves the compositor listening to the signals themselves.
• Maybe some other stuff I'm forgetting

The wlr_output_cursor_set_* function needs to be rethought, which is dependent on how we'd add support for planes. Maybe that could be left until later, and just revert to being a function that works directly on the wlr_output.

[emersion]

+1 overall. We might want to keep some helpers for cursors though, but definitely not in wlr_output.

Additionally it's not clear how to handle cursors in the Wayland backend, especially with multi-seat.

revert to being a function that works directly on the wlr_output

That could be a good step to push things forwards, yes.

[ddevault]

I agree that we should offer a helper, outside of wlr_output.

[ascent12]

Time for another one of my text dumps:

Hardware cursors

In order to remove a whole bunch of rendering crap from the backends, I think it should be changed to not automatically scale or rotate cursors. When we get a plane interface, I would expect it to not do any of that too, unless it's exposed as a DRM property or wl_surface.set_buffer_transform.

Ideally in the future, the interface would be something like

void wlr_output_set_cursor_dmabuf(struct wlr_output, struct wlr_dmabuf_attribs *buffer);

The DRM backend could scan it out directly on the cursor plane, and the wayland backend can give it straight to the parent compositor with wp_linux_dmabuf, and have no extra copies.
But this is not something the backends are currently capable of doing, and requires a lot of infrastructure (which I want to do in renderer v6).
With a plane interface, we could even remove the concept of hardware cursors and only have planes, but this is way off in the future. Also, the way SHM would work also needs thought, but that's getting off of the point.

An issue currently is that compositors don't have a way to render to an off-screen buffer (to do rotations and scaling) with the current wlr_renderer interface. Again, this is something I want in renderer v6, and would be too much to add now.
So the current interface taking a wlr_texture or even a uint8_t * of pixels won't work.

A possible intermediate interface could be:

bool wlr_output_cursor_attach_render(struct wlr_output *output, int *buffer_age);

which would work exactly the same as wlr_output_attach_render, but instead works with the cursor's buffer instead of the main buffer. The backend just needs to keep an EGLSurface internally, which it already does.

The DRM backend has limitations on buffer sizes, but the Wayland backend doesn't. It's not obvious as to how this should be handled.
Maybe something like

bool wlr_output_cursor_try_set_size(struct wlr_output *output, int *x, int *y);

which will attempt to set the cursor size to *x and *y, and depending on the backend limitations it will leave the actual size used in *x and *y, which the compositor would need to know for their rendering calls.

Or perhaps I can just leave this alone for now, and keep the rendering mess inside of the backends.
I just really want

wlroots/backend/drm/drm.c

Lines 652 to 789 in bf90474

	static bool drm_connector_set_cursor(struct wlr_output *output,
	struct wlr_texture *texture, int32_t scale,
	enum wl_output_transform transform,
	int32_t hotspot_x, int32_t hotspot_y, bool update_texture) {
	struct wlr_drm_connector *conn = get_drm_connector_from_output(output);
	struct wlr_drm_backend *drm = get_drm_backend_from_backend(output->backend);
	struct wlr_drm_crtc *crtc = conn->crtc;
	if (!crtc) {
	return false;
	}
	struct wlr_drm_plane *plane = crtc->cursor;
	if (!plane) {
	// We don't have a real cursor plane, so we make a fake one
	plane = calloc(1, sizeof(*plane));
	if (!plane) {
	wlr_log_errno(WLR_ERROR, "Allocation failed");
	return false;
	}
	crtc->cursor = plane;
	}
	if (!plane->surf.gbm) {
	int ret;
	uint64_t w, h;
	ret = drmGetCap(drm->fd, DRM_CAP_CURSOR_WIDTH, &w);
	w = ret ? 64 : w;
	ret = drmGetCap(drm->fd, DRM_CAP_CURSOR_HEIGHT, &h);
	h = ret ? 64 : h;
	if (!drm->parent) {
	if (!init_drm_surface(&plane->surf, &drm->renderer, w, h,
	drm->renderer.gbm_format, GBM_BO_USE_LINEAR | GBM_BO_USE_SCANOUT)) {
	wlr_log(WLR_ERROR, "Cannot allocate cursor resources");
	return false;
	}
	} else {
	if (!init_drm_surface(&plane->surf, &drm->parent->renderer, w, h,
	drm->parent->renderer.gbm_format, GBM_BO_USE_LINEAR)) {
	wlr_log(WLR_ERROR, "Cannot allocate cursor resources");
	return false;
	}
	if (!init_drm_surface(&plane->mgpu_surf, &drm->renderer, w, h,
	drm->renderer.gbm_format, GBM_BO_USE_LINEAR | GBM_BO_USE_SCANOUT)) {
	wlr_log(WLR_ERROR, "Cannot allocate cursor resources");
	return false;
	}
	}
	}
	wlr_matrix_projection(plane->matrix, plane->surf.width,
	plane->surf.height, output->transform);
	struct wlr_box hotspot = { .x = hotspot_x, .y = hotspot_y };
	wlr_box_transform(&hotspot, &hotspot,
	wlr_output_transform_invert(output->transform),
	plane->surf.width, plane->surf.height);
	if (plane->cursor_hotspot_x != hotspot.x ||
	plane->cursor_hotspot_y != hotspot.y) {
	// Update cursor hotspot
	conn->cursor_x -= hotspot.x - plane->cursor_hotspot_x;
	conn->cursor_y -= hotspot.y - plane->cursor_hotspot_y;
	plane->cursor_hotspot_x = hotspot.x;
	plane->cursor_hotspot_y = hotspot.y;
	if (!drm->iface->crtc_move_cursor(drm, conn->crtc, conn->cursor_x,
	conn->cursor_y)) {
	return false;
	}
	wlr_output_update_needs_frame(output);
	}
	if (!update_texture) {
	// Don't update cursor image
	return true;
	}
	plane->cursor_enabled = false;
	if (texture != NULL) {
	int width, height;
	wlr_texture_get_size(texture, &width, &height);
	width = width * output->scale / scale;
	height = height * output->scale / scale;
	if (width > (int)plane->surf.width || height > (int)plane->surf.height) {
	wlr_log(WLR_ERROR, "Cursor too large (max %dx%d)",
	(int)plane->surf.width, (int)plane->surf.height);
	return false;
	}
	make_drm_surface_current(&plane->surf, NULL);
	struct wlr_renderer *rend = plane->surf.renderer->wlr_rend;
	struct wlr_box cursor_box = { .width = width, .height = height };
	float matrix[9];
	wlr_matrix_project_box(matrix, &cursor_box, transform, 0, plane->matrix);
	wlr_renderer_begin(rend, plane->surf.width, plane->surf.height);
	wlr_renderer_clear(rend, (float[]){ 0.0, 0.0, 0.0, 0.0 });
	wlr_render_texture_with_matrix(rend, texture, matrix, 1.0);
	wlr_renderer_end(rend);
	swap_drm_surface_buffers(&plane->surf, NULL);
	plane->cursor_enabled = true;
	}
	if (!drm->session->active) {
	return true; // will be committed when session is resumed
	}
	struct gbm_bo *bo = plane->cursor_enabled ? plane->surf.back : NULL;
	if (bo && drm->parent) {
	bo = copy_drm_surface_mgpu(&plane->mgpu_surf, bo);
	}
	if (bo) {
	// workaround for nouveau
	// Buffers created with GBM_BO_USER_LINEAR are placed in NOUVEAU_GEM_DOMAIN_GART.
	// When the bo is attached to the cursor plane it is moved to NOUVEAU_GEM_DOMAIN_VRAM.
	// However, this does not wait for the render operations to complete, leaving an empty surface.
	// see https://bugs.freedesktop.org/show_bug.cgi?id=109631
	// The render operations can be waited for using:
	glFinish();
	}
	bool ok = drm->iface->crtc_set_cursor(drm, crtc, bo);
	if (ok) {
	wlr_output_update_needs_frame(output);
	}
	return ok;
	}

gone; it's some of my least favourite code in the DRM backend.

Hardware+Software rendering helpers

(I'm talking about a function that tries to set a hardware cursor and falls back to rendering one if it fails)

I've thought about this a bit, but I haven't come up with anything good yet.
I thought about maybe putting a helper in wlr_cursor, but that has issues because multiple cursors can exist and they each don't know if another one tried to set a hardware cursor before it.

Maybe wlr_output_layout? But that's honestly a weird place to put it.

As long as we don't have a helper for a scene graph, there really is no good place to put it. I still think leaving it completely up to compositors is the best bet.

Although, any suggestions are welcome.

On a related note, I think making wlr_xcursor_manager optionally keep wlr_textures for cursors at different sizes could be helpful.

[ddevault]

Rather than extending wlr_cursor or wlr_output_layout, I would like to have a third interface, which just concerns itself with presenting cursors on wlr_outputs.