Created attachment 279628 [details] [review]
wayland: Implement unredirection

When running on native KMS we can avoid costly copies for fullscreen drm surfaces
by directly scanning them out, similar to unredirection whe use in X to bypass the
compositor.

Unredirection on wayland does not have any side effects so we can avoid having complex
heuristics like we have in X.

Review of attachment 279628 [details] [review]:

This needs to go through MetaMonitorManagerNative.

(In reply to comment #2)
> Review of attachment 279628 [details] [review]:
> 
> This needs to go through MetaMonitorManagerNative.

That doesn't exists ... (for some reason its called KMS while everything else is native) ... but that makes sense. So should I move the scanout stuff into something like meta_monitor_manager_kms_scanout_buffer() ?

Yeah, that's fine with me. I just want all this stuff to be in one place.

Created attachment 279657 [details] [review]
wayland: Implement unredirection

When running on native KMS we can avoid costly copies for fullscreen drm surfaces
by directly scanning them out, similar to unredirection whe use in X to bypass the
compositor.

Unredirection on wayland does not have any side effects so we can avoid having complex
heuristics like we have in X.


--

Moved scanout code to MonitorManagerKms

Review of attachment 279657 [details] [review]:

::: src/backends/native/meta-monitor-manager-kms.c
@@ +950,3 @@
+  for (i = 0; i < manager->n_outputs; i++)
+    {
+      if (manager->outputs[i].output_id == output_id)

Just use find_output_by_id?

@@ +962,3 @@
+    return FALSE;
+
+  bo = gbm_bo_import (gbm, GBM_BO_IMPORT_WL_BUFFER, buffer->resource, GBM_BO_USE_SCANOUT);

You're leaking BOs here. You need to call gbo_bo_destroy when it's no longer a valid BO.

@@ +974,3 @@
+  handle = gbm_bo_get_handle (bo).u32;
+
+  if (drmModeAddFB (drm_fd, width, height, 24, 32, stride, handle, &fb_id))

And you're leaking FBs. You need to call drmModeRmFB.

@@ +982,3 @@
+  cogl_kms_display_set_ignore_crtc (cogl_context_get_display (ctx), *crtc_id, TRUE);
+
+  if (drmModeSetCrtc (drm_fd,

Do we expect this to be called at 60fps? If so, we should *probably* use drmModePageFlip here instead so it's synchronized to vblank.

::: src/compositor/meta-surface-actor-wayland.c
@@ +110,3 @@
+#else
+  if (!clutter_check_windowing_backend (CLUTTER_WINDOWING_EGL))
+    return FALSE;

I'd prefer if this was:

    MetaMonitorManager *manager = meta_monitor_manager_get_default ();
    if (META_IS_MONITOR_MANAGER_KMS (kms))
      {
        ...
      }

Reminder, we want to make our lives the easiest if we ever add an EGL backend for e.g. NVIDIA. Checking EGLNATIVE just isn't good enough.

@@ +114,3 @@
+
+  if (!priv->buffer || !window)
+    return FALSE;

Can !window ever happen?

@@ +116,3 @@
+    return FALSE;
+
+  /* We can only scanout non shm (drm) buffers */

That's a lie. We could create dumb buffers, and do the copy ourselves. It would actually be fast enough.

@@ +131,3 @@
+      cairo_region_overlap_t overlap;
+
+      meta_window_get_outer_rect (window, &rect);

This is wrong. get_outer_rect is in root window coordinates, and is about the frame rect, not the buffer rect. Just use a rect of { 0, 0, buffer_width, buffer_height }.

Additionally, if the window is of an RGB24 surface, it is also fully opaque. I don't know if set_opaque_region should be done on RGB24 or other non-alpha surfaces.

You should check meta_surface_actor_is_argb32. I'd also move the checking for the opaque region into another method. I'd create a meta_surface_actor_is_fully_opaque which is:

    if (argb32)
      return check_opaque_region ());
    else
      return FALSE;

@@ +137,3 @@
+    }
+
+  return meta_window_is_monitor_sized (window);

This seems wrong to me. The opaque_region is an optimization: it's not guaranteed to be there and valid on all ARGB32 windows.

The absense of it alongside an ARGB32 means that we shouldn't be able to scan it out.

@@ +160,3 @@
+  if (unredirected)
+      priv->last_crtc_id = 0;
+  else if (!unredirected)

This can just be an else.

@@ +162,3 @@
+  else if (!unredirected)
+    {
+      cogl_kms_display_set_ignore_crtc (cogl_context_get_display (ctx), priv->last_crtc_id, FALSE);

I'd prefer if this was handled by calling meta_monitor_manager_kms_scanout_buffer with a NULL buffer.

@@ +167,3 @@
+    }
+
+  priv->unredirected = unredirected;

I don't like that we wait until the next attach before starting to scan out.

@@ +354,3 @@
+          uint32_t crtc_id;
+
+          if (meta_monitor_manager_kms_scanout_buffer (meta_monitor_manager_get (), priv->buffer, priv->surface->window->monitor->output_id, &crtc_id))

Can you put some of these on their own line?

    uint32_t output_id = priv->surface->window->monitor->output_id;

Also, if we remove all the Cogl calls from this file, we don't need to track the CRTC ID.

@@ +361,3 @@
+                priv->last_crtc_id = crtc_id;
+
+              return;

We should probably keep the texture up to date. Otherwise, if we unredirect for whatever reason (Alt+Tab), and the client doesn't attach a new buffer, then we don't want to show stale buffer contents.

(In reply to comment #6)
> Review of attachment 279657 [details] [review]:
> 
> ::: src/backends/native/meta-monitor-manager-kms.c
> @@ +950,3 @@
> +  for (i = 0; i < manager->n_outputs; i++)
> +    {
> +      if (manager->outputs[i].output_id == output_id)
> 
> Just use find_output_by_id?

OK.

> @@ +962,3 @@
> +    return FALSE;
> +
> +  bo = gbm_bo_import (gbm, GBM_BO_IMPORT_WL_BUFFER, buffer->resource,
> GBM_BO_USE_SCANOUT);
> 
> You're leaking BOs here. You need to call gbo_bo_destroy when it's no longer a
> valid BO.

When is it no longer valid when the buffer gets destroyed?

> @@ +974,3 @@
> +  handle = gbm_bo_get_handle (bo).u32;
> +
> +  if (drmModeAddFB (drm_fd, width, height, 24, 32, stride, handle, &fb_id))
> 
> And you're leaking FBs. You need to call drmModeRmFB.

OK.

> @@ +982,3 @@
> +  cogl_kms_display_set_ignore_crtc (cogl_context_get_display (ctx), *crtc_id,
> TRUE);
> +
> +  if (drmModeSetCrtc (drm_fd,
> 
> Do we expect this to be called at 60fps? If so, we should *probably* use
> drmModePageFlip here instead so it's synchronized to vblank.

We expect that to be called as often as the app wants ... I don't like the idea of forcing vblank sync on apps (that's also what weston does).

> ::: src/compositor/meta-surface-actor-wayland.c
> @@ +110,3 @@
> +#else
> +  if (!clutter_check_windowing_backend (CLUTTER_WINDOWING_EGL))
> +    return FALSE;
> 
> I'd prefer if this was:
> 
>     MetaMonitorManager *manager = meta_monitor_manager_get_default ();
>     if (META_IS_MONITOR_MANAGER_KMS (kms))
>       {
>         ...
>       }
> 
> Reminder, we want to make our lives the easiest if we ever add an EGL backend
> for e.g. NVIDIA. Checking EGLNATIVE just isn't good enough.

OK good point.

> @@ +114,3 @@
> +
> +  if (!priv->buffer || !window)
> +    return FALSE;
> 
> Can !window ever happen?

You told me yes i.e that surfaces could be window less ... but we wouldn't unredirect them so probably no.

> @@ +116,3 @@
> +    return FALSE;
> +
> +  /* We can only scanout non shm (drm) buffers */
> 
> That's a lie. We could create dumb buffers, and do the copy ourselves. It would
> actually be fast enough.

Avoiding a copy by doing another copy instead does not make much sense to me.

> @@ +131,3 @@
> +      cairo_region_overlap_t overlap;
> +
> +      meta_window_get_outer_rect (window, &rect);
> 
> This is wrong. get_outer_rect is in root window coordinates, and is about the
> frame rect, not the buffer rect. Just use a rect of { 0, 0, buffer_width,
> buffer_height }.
> 
> Additionally, if the window is of an RGB24 surface, it is also fully opaque. I
> don't know if set_opaque_region should be done on RGB24 or other non-alpha
> surfaces.
> 
> You should check meta_surface_actor_is_argb32. I'd also move the checking for
> the opaque region into another method. I'd create a
> meta_surface_actor_is_fully_opaque which is:
> 
>     if (argb32)
>       return check_opaque_region ());
>     else
>       return FALSE;

OK.

> @@ +137,3 @@
> +    }
> +
> +  return meta_window_is_monitor_sized (window);
> 
> This seems wrong to me. The opaque_region is an optimization: it's not
> guaranteed to be there and valid on all ARGB32 windows.

Why is this wrong? This is just about the size of the window i.e if the window is monitor sized we want so unredirect it otherwise no (same on as on X).

> The absense of it alongside an ARGB32 means that we shouldn't be able to scan
> it out.

This also does not make sense in this context did you mean the if (window->opaque_region) check?

> @@ +160,3 @@
> +  if (unredirected)
> +      priv->last_crtc_id = 0;
> +  else if (!unredirected)
> 
> This can just be an else.
> 
> @@ +162,3 @@
> +  else if (!unredirected)
> +    {
> +      cogl_kms_display_set_ignore_crtc (cogl_context_get_display (ctx),
> priv->last_crtc_id, FALSE);
> 
> I'd prefer if this was handled by calling
> meta_monitor_manager_kms_scanout_buffer with a NULL buffer.

That's what I wanted to do before but monitor manager does not know which crtc_id it ought to stop ignoring.

> @@ +167,3 @@
> +    }
> +
> +  priv->unredirected = unredirected;
> 
> I don't like that we wait until the next attach before starting to scan out.

Why ? This is is a performance optimization one extra copy for the first frame wouldn't hurt you much it wouldn't even show up in any profiling given that apps draw at 60fps+ (i.e the ones where it matters at all).

> @@ +354,3 @@
> +          uint32_t crtc_id;
> +
> +          if (meta_monitor_manager_kms_scanout_buffer
> (meta_monitor_manager_get (), priv->buffer,
> priv->surface->window->monitor->output_id, &crtc_id))
> 
> Can you put some of these on their own line?
> 
>     uint32_t output_id = priv->surface->window->monitor->output_id;

Sure.

> Also, if we remove all the Cogl calls from this file, we don't need to track
> the CRTC ID.

Then we need to track it somewhere else ... otherwise you can have this:

window on crtc_id 1 gets unredirected; we ignore crtc_id 1
window gets moved to crtc_id 2; we ignore both crtc_id and 2 so we have to do the tracking somewhere.

> @@ +361,3 @@
> +                priv->last_crtc_id = crtc_id;
> +
> +              return;
> 
> We should probably keep the texture up to date. Otherwise, if we unredirect for
> whatever reason (Alt+Tab), and the client doesn't attach a new buffer, then we
> don't want to show stale buffer contents.

Does updating the texture imply a copy? If yes then it would defeat the whole purpose.

(In reply to comment #7)
> When is it no longer valid when the buffer gets destroyed?

I'd just destroy the bo when it's no longer in use. I have no idea what happens if you destroy the image that's currently used for scanout.

> We expect that to be called as often as the app wants ... I don't like the idea
> of forcing vblank sync on apps (that's also what weston does).

Then how can apps get vblank sync when unredirected?

> You told me yes i.e that surfaces could be window less ... but we wouldn't
> unredirect them so probably no.

No surface that is windowless will ever have any of these methods called on it, given that they're called by the MetaWindowActor.

> Avoiding a copy by doing another copy instead does not make much sense to me.

There's a very big difference between doing a simple memcpy which is quite fast and running the entire GL pipeline and doing full compositing, which isn't. It's not just a simple copy, it involves running the whole GPU.

> This also does not make sense in this context did you mean the if
> (window->opaque_region) check?

I was reviewing the end of the block, not the "is_monitor_sized" call. The bug is that if window->opaque_region is NULL, then we simply go ahead even if the window is monitor-sized.

> That's what I wanted to do before but monitor manager does not know which
> crtc_id it ought to stop ignoring.

But you pass it the output_id, right?

> Then we need to track it somewhere else ... otherwise you can have this:
> 
> window on crtc_id 1 gets unredirected; we ignore crtc_id 1
> window gets moved to crtc_id 2; we ignore both crtc_id and 2 so we have to do
> the tracking somewhere.

You can track the output_id instead of the crtc_id.

> Does updating the texture imply a copy? If yes then it would defeat the whole
> purpose.

Then at the very least update the texture when undirecting.

(In reply to comment #8)
> There's a very big difference between doing a simple memcpy which is quite fast
> and running the entire GL pipeline and doing full compositing, which isn't.
> It's not just a simple copy, it involves running the whole GPU.

Well doing a copy a screen sized copy for every frame is more expensive (on my laptop that be ~1.3GB/s at 60fps) and noticeable in profiles then simply drawing an unblended texture (which is very fast). So while yes it can be done it does not give you much. When running gnome-shell under llvmpipe the most expensive operations are the large copies (that's why fixing glxCopySubBuffer helped a lot).

> > Does updating the texture imply a copy? If yes then it would defeat the whole
> > purpose.
> 
> Then at the very least update the texture when undirecting.

OK that makes sense.

Created attachment 279794 [details] [review]
wayland: Implement unredirection

When running on native KMS we can avoid costly copies for fullscreen drm surfaces
by directly scanning them out, similar to unredirection whe use in X to bypass the
compositor.

Unredirection on wayland does not have any side effects so we can avoid having complex
heuristics like we have in X.


---

Fixed bo and fb leaks and done suggested refactorings, also fixed the "lying" comment to be more accurate.

Review of attachment 279794 [details] [review]:

::: src/backends/native/meta-monitor-manager-kms.c
@@ +929,3 @@
+ *
+ * Scans out @buffer on the output specified by @output_id.
+ * When @buffer is %NULL the output's state is restored.

Overrides the scanout buffer on the output specified by @output_id with @buffer. If @buffer is %NULL, this restores the scanout buffer to be the default scanout buffer, the Clutter stage.

@@ +931,3 @@
+ * When @buffer is %NULL the output's state is restored.
+ *
+ * Return value: Whether the operation was successfull or not (boolean)

Returns: %TRUE if the operation was successful, otherwise %FALSE.

@@ +1003,3 @@
+  manager_kms->last_scanned_crtc = crtc_id;
+
+  if (drmModeSetCrtc (drm_fd,

I really don't like that we have this without any way for apps to get vblank syncing. We *really* should be using drmModePageFlip by default, but Cogl is going to be really confused by the event that comes back.

::: src/compositor/meta-surface-actor-wayland.c
@@ +126,3 @@
+      cairo_region_overlap_t overlap;
+
+      overlap = cairo_region_contains_rectangle (opaque_region, (const cairo_rectangle_int_t *)&rect);

What's the cast for?

@@ +182,3 @@
+      if (!unredirected)
+        {
+          meta_monitor_manager_kms_scanout_buffer (meta_monitor_manager_get (), NULL, 0);

This should be META_MONITOR_MANAGER_KMS (manager), I would think.

@@ +373,3 @@
+          uint32_t output_id = priv->surface->window->monitor->output_id;
+
+          if (meta_monitor_manager_kms_scanout_buffer (meta_monitor_manager_get (), priv->buffer, output_id))

It seems like an odd place to have this bypass in set_buffer. If an app uses eglSwapBuffersWithDamage, we'll continue to batch up redraws. We don't stop queueing redraws in meta-wayland-surface.c. I'm not sure if we should or not, or if there's some code in here that would make me happy.

But as-is, I'm not too fond of it. Having it in set_buffer is really outside of where it should be.

The reason we don't paint the stage is that Cogl marks the CRTC as ignored, which means it doesn't do drmPageFlipCRTC, which means that it doesn't get the swap event, which means that Clutter never pushes its master clock forward. As far as I can tell. It's a giant wishy-washy chain of events, and I'd like to not rely on it.

But if we have an external monitor attached, and that is composited, then we'll paint the entire stage, and I assume fill in junk for what's supposed to be undirected.

In the SHM case we also have the thing where we can attach the same buffer multiple times, and just mark the regions that were damaged as places that we should copy. I don't know if this is the case for any importable buffer, but if so, we won't get a set_buffer, so this will just fall down.

@@ +384,3 @@
+
+      if (priv->unredirected)
+        meta_monitor_manager_kms_scanout_buffer (meta_monitor_manager_get (), NULL, 0);

Do we need this? should_unredirect already checks for this case, which means that later down the line we should get a set_unredirected (FALSE), right?

::: src/meta/meta-shaped-texture.h
@@ +79,3 @@
                                             cairo_region_t    *opaque_region);
 
+cairo_region_t * meta_shaped_texture_get_opaque_region (MetaShapedTexture *stex);

Can you put this in meta-shaped-texture-private.h instead?

(In reply to comment #11)
> Review of attachment 279794 [details] [review]:
> 
> ::: src/backends/native/meta-monitor-manager-kms.c
> @@ +929,3 @@
> + *
> + * Scans out @buffer on the output specified by @output_id.
> + * When @buffer is %NULL the output's state is restored.
> 
> Overrides the scanout buffer on the output specified by @output_id with
> @buffer. If @buffer is %NULL, this restores the scanout buffer to be the
> default scanout buffer, the Clutter stage.

OK.

> @@ +931,3 @@
> + * When @buffer is %NULL the output's state is restored.
> + *
> + * Return value: Whether the operation was successfull or not (boolean)
> 
> Returns: %TRUE if the operation was successful, otherwise %FALSE.

OK.

> @@ +1003,3 @@
> +  manager_kms->last_scanned_crtc = crtc_id;
> +
> +  if (drmModeSetCrtc (drm_fd,
> 
> I really don't like that we have this without any way for apps to get vblank
> syncing. We *really* should be using drmModePageFlip by default, but Cogl is
> going to be really confused by the event that comes back.

Not only that the app may want do draw without vsync specifically games tend to do that to get low latency input. Weston does the same.

> ::: src/compositor/meta-surface-actor-wayland.c
> @@ +126,3 @@
> +      cairo_region_overlap_t overlap;
> +
> +      overlap = cairo_region_contains_rectangle (opaque_region, (const
> cairo_rectangle_int_t *)&rect);
> 
> What's the cast for?

dunno left over from something will remove.

> @@ +182,3 @@
> +      if (!unredirected)
> +        {
> +          meta_monitor_manager_kms_scanout_buffer (meta_monitor_manager_get
> (), NULL, 0);
> 
> This should be META_MONITOR_MANAGER_KMS (manager), I would think.
> 
> @@ +373,3 @@
> +          uint32_t output_id = priv->surface->window->monitor->output_id;
> +
> +          if (meta_monitor_manager_kms_scanout_buffer
> (meta_monitor_manager_get (), priv->buffer, output_id))
> 
> It seems like an odd place to have this bypass in set_buffer. If an app uses
> eglSwapBuffersWithDamage, we'll continue to batch up redraws. We don't stop
> queueing redraws in meta-wayland-surface.c. I'm not sure if we should or not,
> or if there's some code in here that would make me happy.
> 
> But as-is, I'm not too fond of it. Having it in set_buffer is really outside of
> where it should be.

Where do you want it to be? When the app gives us a new buffer we scan that one out. That's the only sane way to do it. Could move this to a lower level i.e not involving the actor at all ... but when you split out the surface actors we ended up with this interface where we have the X implementation in the surface-actor and an empty one for the wayland-surface-actor so it made sense to me to put it there.

> The reason we don't paint the stage is that Cogl marks the CRTC as ignored,
> which means it doesn't do drmPageFlipCRTC, which means that it doesn't get the
> swap event, which means that Clutter never pushes its master clock forward. As
> far as I can tell. It's a giant wishy-washy chain of events, and I'd like to
> not rely on it.

It will fake a swap event at the end of the swap https://git.gnome.org/browse/cogl/commit/?h=cogl-1.18&id=22378d572b039e4fe6ceb56e56492470bb461d48 ... actually without that everything comes to halt even for the unredirected window.

> But if we have an external monitor attached, and that is composited, then we'll
> paint the entire stage, and I assume fill in junk for what's supposed to be
> undirected.

No. That's not any different from what happens on X. What gets scanned out has nothing to do with what you actually paint. Painting happens offscreen then we tell the gpu "please put this on screen" .... we would just not do the later for the crtc where the window is unredirected. 

> In the SHM case we also have the thing where we can attach the same buffer
> multiple times, and just mark the regions that were damaged as places that we
> should copy. I don't know if this is the case for any importable buffer, but if
> so, we won't get a set_buffer, so this will just fall down.

That does not make any sense for DRM buffers.

> @@ +384,3 @@
> +
> +      if (priv->unredirected)
> +        meta_monitor_manager_kms_scanout_buffer (meta_monitor_manager_get (),
> NULL, 0);
> 
> Do we need this? should_unredirect already checks for this case, which means
> that later down the line we should get a set_unredirected (FALSE), right?

Yeah we indeed can remove this.

> ::: src/meta/meta-shaped-texture.h
> @@ +79,3 @@
>                                              cairo_region_t    *opaque_region);
> 
> +cairo_region_t * meta_shaped_texture_get_opaque_region (MetaShapedTexture
> *stex);
> 
> Can you put this in meta-shaped-texture-private.h instead?

Yes.

How's the "destroy Cogl" coming along, Adel?

I have a new set of patches for cogl that calls a callback when a flip for a crtc is done so I don't have to fight the cogl polling loop. There is still some flickering that I have to fix ... should get back to that soon.

Anyone here still working on getting this bug fixed? I mean it's been over one year now without any apparent activity to warrent this still being "OPEN" and not "CLOSED" bug.

This has to wait until we move cogl into mutter which is being done now. To avoid the hacky mess.

GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/mutter/-/issues/

Thank you for your understanding and your help.