Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

[WIP] Renderer v6 #1355

Closed
wants to merge 23 commits into from
Closed

[WIP] Renderer v6 #1355

wants to merge 23 commits into from

Conversation

ascent12
Copy link
Member

@ascent12 ascent12 commented Nov 3, 2018

Closes #1352

Just to be clear, this is still VERY much a work-in-progress. If I were to guess, I would say that it's only about 5% 10% 20% done.
I'm posting this now just to let people see what I'm doing and get feedback before I go too deep into changing everything.

The code compiles, but that's about it. At this point, the entirety of the backends, examples, and rootston have been removed, just until they're updated. There are several #if 0 blocks around, as a "I'll get to it later" thing.

I'm intending to build a mailbox swapchain (Vulkan terminology), which arises quite naturally from using dmabufs, and can allow for people who want to write renderers that go faster than the display's refresh rate. Here is a pretty good explanation of different swapchain types.

@ascent12
Copy link
Member Author

ascent12 commented Nov 6, 2018

I've gotten to the point where examples/simple works with the wayland backend. So some of the overall changes could be reviewed or commented on.

@emersion
Copy link
Member

emersion commented Nov 6, 2018

Reminder that this design:

  • Doesn't work if the parent Wayland compositor doesn't support the linux-dmabuf protocol (not sure what the situation on X11 is)
  • Makes it impossible to have a software renderer
  • Ties wlroots to GBM, making it impossible to use another allocator (ie. the future Unix allocator, though I'm not holding my breath)

BTW, here is a WIP patch that advertises devices in linux-dmabuf: https://lists.freedesktop.org/archives/wayland-devel/2018-November/039577.html (note that devices are advertised per surface, not globally)

@ascent12
Copy link
Member Author

ascent12 commented Nov 6, 2018

Doesn't work if the parent Wayland compositor doesn't support the linux-dmabuf protocol (not sure what the situation on X11 is)

This is a use case I'm really not interested in. Considering there are no known (relevant) compositors that don't support this, it's a lot of effort for nobody.
It's not an inherent flaw of this design, though. It's just creating a wl_shm buffer and performing a copy into that.

Makes it impossible to have a software renderer

Not impossible; just not 100% guaranteed to work. gbm_bos are mmapable in most cases.
Again, a lot of effort for basically nobody.

Ties wlroots to GBM, making it impossible to use another allocator (ie. the future Unix allocator, though I'm not holding my breath)

Considering it hasn't been touched in over 8 months, I've given up on it for the time being. Right now, GBM is the only viable option, so I'm completely fine with depending on that.

@emersion
Copy link
Member

emersion commented Nov 6, 2018

This is a use case I'm really not interested in. Considering there are no known (relevant) compositors that don't support this, it's a lot of effort for nobody.
It's not an inherent flaw of this design, though. It's just creating a wl_shm buffer and performing a copy into that.

You still need to get a render node. This isn't even in the current linux-dmabuf protocol.

Not impossible; just not 100% guaranteed to work. gbm_bos are mmapable in most cases. On DRM, it would be possible to use dumb-buffers too.
Again, a lot of effort for basically nobody.

This would not be viable, as you'll end up doing roundtrips between the GPU and the main memory all the time.

I'd like to implement a software renderer for a display manager.

(note that devices are advertised per surface, not globally)

This means the FD needs to be per-output, not per-backend.

@ascent12
Copy link
Member Author

ascent12 commented Nov 6, 2018

You still need to get a render node. This isn't even in the current linux-dmabuf protocol.

Sure. Obviously I'll add that when it's actually made part of the protocol.

This would not be viable, as you'll end up doing roundtrips between the GPU and the main memory all the time.

I suppose. The allocation model for software renderers is completely different than what's here (backend allocated instead of renderer allocated), and it seems like trying to cram the allocations into the same API would be a mistake. I have an idea about how that may work, but I'll think about it some more.

This means the FD needs to be per-output, not per-backend.

That certainly causes some issues. That basically mandates that multiple renderers becomes a solved problem.
I am going to wait for the next linux-dmabuf version to see what actually gets merged.

return -1;
}

struct wlr_format_set *wlr_backend_get_formats(struct wlr_backend *backend) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably be const

return false;
}
if (output->scheduled) {
struct wlr_wl_buffer *buffer = gbm_bo_get_user_data(output->scheduled);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty weird. The frame event means "please render", not "please commit". Instead we should call wlr_output_send_frame here, and let the compositor swap buffers to commit.

* our rendering loop, so we start it again by sending a frame event.
*/
if (!output->frame_callback) {
wlr_output_send_frame(&output->wlr_output);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty fragile.

free(buffer);
}

static bool output_schedule_frame(struct wlr_output *wlr_output, struct gbm_bo *bo,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

output_schedule_frame should schedule a frame event, and should not submit a frame.

@ascent12
Copy link
Member Author

Now I've gotten the X11 backend to successfully render examples/simple. Now that we're using the present extension, we can get rid of that god-awful way we were scheduling frames by using a timer.
I got a bit carried away with the changes, and started adding Xinput2 support, but I haven't gotten around to finishing that yet. There are still some other X11 changes that need to be made too.

I also "ported" the multi backend to this, but it made me realise that it REALLY doesn't play nicely with this interface. We need to direct allocations to a specific backend, and the multi backend just can't know who it's supposed to be destined for.
I think the multi backend needs to be massively overhauled. I haven't thought of how yet.

@emersion
Copy link
Member

I got a bit carried away with the changes, and started adding Xinput2 support, but I haven't gotten around to finishing that yet. There are still some other X11 changes that need to be made too.

Please don't make so massive that it becomes impossible to review (it's already). You can open another PR if you want to do this. ;_;

I think the multi backend needs to be massively overhauled. I haven't thought of how yet.

Yeah I'm not sure how we can fix this.

@emersion
Copy link
Member

emersion commented Nov 10, 2018

Speaking of the size of this PR, what's your plan to get this merged? Do you just use this as a experimentation playground and intend to break it into smaller chunks? (I can help doing that)

I'm afraid about what happened to the other "renderer redesign" PRs, last time they were just closed and it took quite some time to gradually port changes.

@ascent12
Copy link
Member Author

I'll probably rebase it pretty soon so there is a sane commit history to review and work off of. Perhaps I could also change things so it's more of a "introduce new thing -> gradually change everything to use it -> remove old thing" instead of "change everything at once".

@emersion
Copy link
Member

emersion commented Nov 10, 2018

This means the FD needs to be per-output, not per-backend.

That certainly causes some issues. That basically mandates that multiple renderers becomes a solved problem.
I am going to wait for the next linux-dmabuf version to see what actually gets merged.

Note that this doesn't necessarily mean multiple renderers. The compositor could still use only one GPU for rendering, and the others just for direct scan-out.

We still need to advertise multiple GPUs to clients.

This was referenced Nov 11, 2018
@emersion
Copy link
Member

Mesa uses drmGetDevices2 and DRM_NODE_RENDER to get a render node (see loader_open_render_node in src/loader/loader.c).

render/egl.c Outdated
};
eglDebugMessageControlKHR(egl_log, debug_attribs);
// This implies EGL_EXT_platform_base
if (!strstr(client_exts, "EGL_MESA_platform_gbm")) {
Copy link
Member

@emersion emersion Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use strstr, some extension names are prefixes of other extensions, this has caused issues in the past.

.vert_src = quad_vertex_src,
.frag_src = quad_fragment_src,
.num_uniforms = 2,
.uniforms = (struct uniform []) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? All elements aren't of the same size anymore…

@emersion
Copy link
Member

Have you designed the API such that the DRM IN_FORMATS property can be supported later?

@ascent12
Copy link
Member Author

Mesa uses drmGetDevices2 and DRM_NODE_RENDER to get a render node (see loader_open_render_node in src/loader/loader.c).

This is something I was planning to do. We don't need to expose our modesetting FD over the API. at least not over that function.

Have you designed the API such that the DRM IN_FORMATS property can be supported later?

Yes, I believe that would work. It was also something I was thinking about doing as part of this PR.

@ascent12
Copy link
Member Author

ascent12 commented Nov 18, 2018

I've rebased this, but I haven't gotten around to doing all of it yet. Some of the old work is still sitting on https://github.com/ascent12/wlroots/tree/renderer_v6_save.

It no longer removes old interfaces, which will instead be delayed until the very end.

@ascent12
Copy link
Member Author

I've added some functions to wlr_output related to presentation. They're so that supporting things like wp_viewporter and zwp_fullscreen_shell_v1 are possible in the future.
The functions accumulate state and are all presented in one go. This is so it's easier to add more of these types of functions in the future and not have to keep breaking the API.

* This is not applied until the next call to wlr_output_present.
*
* Not every backend supports this.
* TODO: Add a way to query if it's supported ahead of time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should probably return a bool

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was originally thinking that, but if this ends up being not supported, this affects how you're going to render things, and you'd probably call this function after you've done that.
I think the way to check needs to be done earlier and without changing the output state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Then just add a function that checks that impl->set_viewport != NULL and assert this in wlr_output_set_viewport?

@emersion
Copy link
Member

emersion commented Nov 23, 2018

Maybe we could rename present to commit and set_image to attach.

The functions accumulate state and are all presented in one go. This is so it's easier to add more of these types of functions in the future and not have to keep breaking the API.

Yeah that sounds good.

@emersion emersion added the breaking Breaking change in public API label Nov 27, 2018
@ascent12 ascent12 force-pushed the renderer_v6 branch 2 times, most recently from c4cb676 to 4e2e5e8 Compare December 9, 2018 09:10
@emersion
Copy link
Member

emersion commented Apr 9, 2019

Needs rebase

@panaman67
Copy link

@ascent12 Any chance at a rebase my friend?

@ascent12
Copy link
Member Author

ascent12 commented May 9, 2019

@panaman67 I intend to try and get most of this merged through separate PRs; it'll be too monstrously massive doing it this way. Maybe when I get to the point of actually changing wlr_renderer after laying all of the groundwork elsewhere, I could do that here to tie everything in a nice little bow, but this is mainly just sitting here for me to cherry-pick commits off.
I can't really be bothered with a non-trivial rebase for this branch, because it's probably not going to help very much right now, and is just going to get a bunch of new conflicts before it's ready.

I've been very fickle about what I've been working on, and have only been working on this on-and-off in my own spare time mainly, so it's been slow progress. I'd certainly accept any help from anybody who is interested in getting their hands dirty in wlroots internals and wants to get work like this done more quickly.

@emersion
Copy link
Member

Hmm. https://gitlab.freedesktop.org/wayland/weston/merge_requests/183#note_177014

@emersion emersion mentioned this pull request Jun 1, 2020
9 tasks
@emersion emersion marked this pull request as draft June 9, 2020 16:51
@emersion
Copy link
Member

Almost all of this has now been merged, incrementally and slightly updated (mostly to accommodate for the software renderer). Thanks for working on this PR, it's been instrumental for the renderer v6 refactoring!

@emersion emersion closed this Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking Breaking change in public API
Development

Successfully merging this pull request may close these issues.

Renderer redesign v6
3 participants