After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 721450 - Re-enable swap_region for mesa 10.1+ llvmpipe / swrast
Re-enable swap_region for mesa 10.1+ llvmpipe / swrast
Status: RESOLVED FIXED
Product: cogl
Classification: Platform
Component: GLX
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Cogl maintainer(s)
Cogl maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-01-04 12:31 UTC by drago01
Modified: 2014-03-11 17:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
winsys-glx: Reenable swap_region for llvmpipe and swrast (2.12 KB, patch)
2014-01-06 14:32 UTC, drago01
none Details | Review
winsys-glx: Reenable swap_region for llvmpipe and swrast (2.30 KB, patch)
2014-01-06 16:23 UTC, drago01
committed Details | Review

Description drago01 2014-01-04 12:31:43 UTC
Currently we have it disabled due to a bug in current and older mesa versions: https://git.gnome.org/browse/cogl/tree/cogl/winsys/cogl-winsys-glx.c?id=6b14fa222bafb7e67c4845b26439322b34ac26a2#n459

But Dave finally fixed it in mesa git: http://cgit.freedesktop.org/mesa/mesa/commit/?id=ba00f2f6f54cbc5ffdb0f0b94bcd672d147cdc36

So we should enable it it should help gnome-shell on llvmpipe a lot because the large copies really hurt.

Now the question is how to do it. Adding the mesa version to context->gpu sounds wrong to me also this check should be in cogl-winsys-glx-feature-functions.h but we can't really do a version check there either.

Do we have other places where we need the mesa version outside of gpu info? Any suggestion on where / how to fix that check (we could simply remove it but would break with older mesa versions so I rather not).
Comment 1 Robert Bragg 2014-01-06 13:50:31 UTC
The GPU info api we have is meant to be a general facility for use anywhere within Cogl where structured version, vendor and architecture information is useful. The plan was even to make something like that api public at some point too.

Although the -feature-functions.h only a basic mechanism for automatically enabling certain features according to extensions which handles common cases, when that isn't flexible enough we can do more special case checks in cogl-winsys-glx.c:update_base_winsys_features() to enable/disable features.

Since the GpuInfo can't be initialized until we have a GL context (until then we can't call glGetString) we can also add special case checks in cogl-winsys-glx.c:update_winsys_features() which lets us hook in when there is a context and the gpu info is initialized by the call to _cogl_context_update_features() at the top of that function.

My guess a.t.m is that we should update the checks where we currently apply the workaround in update_winsys_features() so it goes something like like:

if (glx_renderer->pf_glXCopySubBuffer || context->glBlitFramebuffer)
{
  CoglGpuInfo *info = &context->gpu;
  CoglGpuInfoArchitecture arch = info->architecture;

  COGL_FLAGS_SET (context->winsys_features, COGL_WINSYS_FEATURE_SWAP_REGION, TRUE);
  if (info->driver_package == COGL_GPU_INFO_DRIVER_PACKAGE_MESA &&
      info->driver_package_version < COGL_VERSION_ENCODE (10, 1, 0) &&
      (arch == COGL_GPU_INFO_ARCHITECTURE_LLVMPIPE ||
       arch == COGL_GPU_INFO_ARCHITECTURE_SOFTPIPE ||
       arch == COGL_GPU_INFO_ARCHITECTURE_SWRAST))
    {
      COGL_FLAGS_SET (context->winsys_features, COGL_WINSYS_FEATURE_SWAP_REGION, FALSE);
    }
}

The GpuInfo should already be detecting when you are running with a Mesa driver and its version so hopefully you don't need to change anything there.
Comment 2 drago01 2014-01-06 14:32:30 UTC
Created attachment 265440 [details] [review]
winsys-glx: Reenable swap_region for llvmpipe and swrast

The bug that prevented MESA_copy_sub_buffer to work for
swrast / llvmpipe got fixed in mesa 10.1 git so enable
it for mesa 10.1+.

---

OK, so lets do it this way then.
Comment 3 Robert Bragg 2014-01-06 15:44:47 UTC
(In reply to comment #2)
> Created an attachment (id=265440) [details] [review]
> winsys-glx: Reenable swap_region for llvmpipe and swrast
> 
> The bug that prevented MESA_copy_sub_buffer to work for
> swrast / llvmpipe got fixed in mesa 10.1 git so enable
> it for mesa 10.1+.
> 
> ---
> 
> OK, so lets do it this way then.

+          (arch == COGL_GPU_INFO_ARCHITECTURE_LLVMPIPE &&
+           arch == COGL_GPU_INFO_ARCHITECTURE_SOFTPIPE &&
+           arch == COGL_GPU_INFO_ARCHITECTURE_SWRAST))

I think this should be using || instead of && here.

I think it should be ok also to remove the following marker in the comment now that the workaround has essentially been resolved:

/* XXX: ONGOING BUG:
 * (Don't change the line above since we use this to grep for
 * un-resolved bug workarounds as part of the release process.)
Comment 4 drago01 2014-01-06 16:23:02 UTC
Created attachment 265451 [details] [review]
winsys-glx: Reenable swap_region for llvmpipe and swrast

The bug that prevented MESA_copy_sub_buffer to work for
swrast / llvmpipe got fixed in mesa 10.1 git so enable
it for mesa 10.1+.
Comment 5 Robert Bragg 2014-01-06 17:22:52 UTC
Comment on attachment 265451 [details] [review]
winsys-glx: Reenable swap_region for llvmpipe and swrast

Thanks I have pushed the patch to master and the cogl-1.18 branch. I made one tweak to #include "cogl-version.h" since I got a warning about COGL_VERSION_ENCODE being implicitly defined when building.
Comment 6 Federico Mena Quintero 2014-03-11 17:15:15 UTC
For reference, the original is bug #674208.