Re: [PATCH] gl-renderer: Create a high priority context

[Chris Wilson]

Quoting Emil Velikov (2018-03-02 14:52:28)
> Hi Chris,
> 
> On 1 March 2018 at 08:28, Chris Wilson  wrote:
> > EGL_IMG_context_priority allows the client to request that their
> > rendering be considered high priority. For ourselves, this is important
> > as we are interactive and any delay in our rendering causes input-output
> 
> > +   if (gr->has_context_priority) {
> > +   EGLint value = EGL_CONTEXT_PRIORITY_MEDIUM_IMG;
> > +
> > +   eglQueryContext(gr->egl_display, gr->egl_context,
> > +   EGL_CONTEXT_PRIORITY_LEVEL_IMG, );
> > +
> > +   if (value != EGL_CONTEXT_PRIORITY_HIGH_IMG) {
> > +   weston_log("Failed to obtain a high priority 
> > context.\n");
> > +   /* Not an error, continue on as normal */
> > +   }
> While this (and EGL spec) says "not an error" the i965 driver will
> error out as the ioctl fails.

The high priority attribute is filtered out from the allowed set of EGL
attributes, so the request for a high priority context is silently
converted back to normal. You don't get as far as hitting the ioctl.

diff --git a/src/egl/main/eglcontext.c b/src/egl/main/eglcontext.c
index 1b03160439..8c64f9ab82 100644
--- a/src/egl/main/eglcontext.c
+++ b/src/egl/main/eglcontext.c
@@ -332,6 +332,60 @@ _eglParseContextAttribList(_EGLContext *ctx, _EGLDisplay 
*dpy,
  ctx->NoError = !!val;
  break;
 
+  case EGL_CONTEXT_PRIORITY_LEVEL_IMG:
+ /* The  EGL_IMG_context_priority spec says:
+  *
+  * "EGL_CONTEXT_PRIORITY_LEVEL_IMG determines the priority level of
+  * the context to be created. This attribute is a hint, as an
+  * implementation may not support multiple contexts at some
+  * priority levels and system policy may limit access to high
+  * priority contexts to appropriate system privilege level. The
+  * default value for EGL_CONTEXT_PRIORITY_LEVEL_IMG is
+  * EGL_CONTEXT_PRIORITY_MEDIUM_IMG."
+  */
+ {
+int bit;
+
+switch (val) {
+case EGL_CONTEXT_PRIORITY_HIGH_IMG:
+   bit = __EGL_CONTEXT_PRIORITY_HIGH_BIT;
+   break;
+case EGL_CONTEXT_PRIORITY_MEDIUM_IMG:
+   bit = __EGL_CONTEXT_PRIORITY_MEDIUM_BIT;
+   break;
+case EGL_CONTEXT_PRIORITY_LOW_IMG:
+   bit = __EGL_CONTEXT_PRIORITY_LOW_BIT;
+   break;
+default:
+   bit = -1;
+   break;
+}
+
+if (bit < 0) {
+   err = EGL_BAD_ATTRIBUTE;
+   break;
+}
+
+/* "This extension allows an EGLContext to be created with a
+ * priority hint. It is possible that an implementation will not
+ * honour the hint, especially if there are constraints on the
+ * number of high priority contexts available in the system, or
+ * system policy limits access to high priority contexts to
+ * appropriate system privilege level. A query is provided to find
+ * the real priority level assigned to the context after creation."
+ *
+ * We currently assume that the driver applies the priority hint
+ * and filters out any it cannot handle during the screen setup,
+ * e.g. dri2_setup_screen(). As such we can mask any change that
+ * the driver would fail, and ctx->ContextPriority matches the
+ * hint applied to the driver/hardware backend.
+ */
+if (dpy->Extensions.IMG_context_priority & (1 << bit))
+   ctx->ContextPriority = val;
+
+break;
+ }
+
   default:
  err = EGL_BAD_ATTRIBUTE;
  break;

and dpy->Extensions.IMG_context_priority should not have HIGH_BIT set.
-Chris
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] gl-renderer: Create a high priority context

[Emil Velikov]

Hi Chris,

On 1 March 2018 at 08:28, Chris Wilson  wrote:
> EGL_IMG_context_priority allows the client to request that their
> rendering be considered high priority. For ourselves, this is important
> as we are interactive and any delay in our rendering causes input-output

> +   if (gr->has_context_priority) {
> +   EGLint value = EGL_CONTEXT_PRIORITY_MEDIUM_IMG;
> +
> +   eglQueryContext(gr->egl_display, gr->egl_context,
> +   EGL_CONTEXT_PRIORITY_LEVEL_IMG, );
> +
> +   if (value != EGL_CONTEXT_PRIORITY_HIGH_IMG) {
> +   weston_log("Failed to obtain a high priority 
> context.\n");
> +   /* Not an error, continue on as normal */
> +   }
While this (and EGL spec) says "not an error" the i965 driver will
error out as the ioctl fails.
Say, for some reason the kernel module/HW cannot do a high-priority one ATM.

Should that be changed/fixed?

-Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] gl-renderer: Create a high priority context

[Daniel Stone]

Hi Chris,

On 1 March 2018 at 08:28, Chris Wilson  wrote:
> The previously mentioned bug affecting i965_dri.so not accepting
> CONTEXT_ATTRIB_PRIORITY should now be fixed in the stable release, so
> the window of nasty surprises should be closed. :)

Thanks for the reminder! Merged and pushed now.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] gl-renderer: Create a high priority context

[Chris Wilson]

EGL_IMG_context_priority allows the client to request that their
rendering be considered high priority. For ourselves, this is important
as we are interactive and any delay in our rendering causes input-output
jitter; a less than smooth user interactive. So if the driver supports
setting the context priority, try and create our EGLContext as high
priority. The driver may reject our request due to system restrictions,
in which case it will fallback to normal priority, but if successful it
will reschedule our rendering and all of its dependencies to execute
earlier, especially important when the GPU is being hogged by background
clients.

Signed-off-by: Chris Wilson 
Cc: Daniel Stone 
Reviewed-by: Daniel Stone 
---

The previously mentioned bug affecting i965_dri.so not accepting
CONTEXT_ATTRIB_PRIORITY should now be fixed in the stable release, so
the window of nasty surprises should be closed. :)
-Chris

---
 libweston/gl-renderer.c | 36 ++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
index d3ed4a10..a6b29a92 100644
--- a/libweston/gl-renderer.c
+++ b/libweston/gl-renderer.c
@@ -223,6 +223,8 @@ struct gl_renderer {
PFNEGLQUERYWAYLANDBUFFERWL query_buffer;
int has_bind_display;
 
+   int has_context_priority;
+
int has_egl_image_external;
 
int has_egl_buffer_age;
@@ -3189,6 +3191,9 @@ gl_renderer_setup_egl_extensions(struct weston_compositor 
*ec)
return -1;
}
 
+   if (weston_check_egl_extension(extensions, "EGL_IMG_context_priority"))
+   gr->has_context_priority = 1;
+
if (weston_check_egl_extension(extensions, 
"EGL_WL_bind_wayland_display"))
gr->has_bind_display = 1;
if (gr->has_bind_display) {
@@ -3631,10 +3636,10 @@ gl_renderer_setup(struct weston_compositor *ec, 
EGLSurface egl_surface)
EGLConfig context_config;
EGLBoolean ret;
 
-   EGLint context_attribs[] = {
+   EGLint context_attribs[16] = {
EGL_CONTEXT_CLIENT_VERSION, 0,
-   EGL_NONE
};
+   unsigned int nattr = 2;
 
if (!eglBindAPI(EGL_OPENGL_ES_API)) {
weston_log("failed to bind EGL_OPENGL_ES_API\n");
@@ -3642,6 +3647,21 @@ gl_renderer_setup(struct weston_compositor *ec, 
EGLSurface egl_surface)
return -1;
}
 
+   /*
+* Being the compositor we require minimum output latency,
+* so request a high priority context for ourselves - that should
+* reschedule all of our rendering and its dependencies to be completed
+* first. If the driver doesn't permit us to create a high priority
+* context, it will fallback to the default priority (MEDIUM).
+*/
+   if (gr->has_context_priority) {
+   context_attribs[nattr++] = EGL_CONTEXT_PRIORITY_LEVEL_IMG;
+   context_attribs[nattr++] = EGL_CONTEXT_PRIORITY_HIGH_IMG;
+   }
+
+   assert(nattr < ARRAY_LENGTH(context_attribs));
+   context_attribs[nattr] = EGL_NONE;
+
context_config = gr->egl_config;
 
if (gr->has_configless_context)
@@ -3665,6 +3685,18 @@ gl_renderer_setup(struct weston_compositor *ec, 
EGLSurface egl_surface)
}
}
 
+   if (gr->has_context_priority) {
+   EGLint value = EGL_CONTEXT_PRIORITY_MEDIUM_IMG;
+
+   eglQueryContext(gr->egl_display, gr->egl_context,
+   EGL_CONTEXT_PRIORITY_LEVEL_IMG, );
+
+   if (value != EGL_CONTEXT_PRIORITY_HIGH_IMG) {
+   weston_log("Failed to obtain a high priority 
context.\n");
+   /* Not an error, continue on as normal */
+   }
+   }
+
ret = eglMakeCurrent(gr->egl_display, egl_surface,
 egl_surface, gr->egl_context);
if (ret == EGL_FALSE) {
-- 
2.16.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] gl-renderer: Create a high priority context

[Daniel Stone]

Hi Chris,
wayland-devel@ rather than weston[sic]-dev[sic]@ ...

On Sat, 2018-01-20 at 00:39 +, Chris Wilson wrote:
> EGL_IMG_context_priority allows the client to request that their
> rendering be considered high priority. For ourselves, this is important
> as we are interactive and any delay in our rendering causes input-output
> jitter; a less than smooth user interactive. So if the driver supports
> setting the context priority, try and create our EGLContext as high
> priority. The driver may reject our request due to system restrictions,
> in which case it will fallback to normal priority, but if successful it
> will reschedule our rendering and all of its dependencies to execute
> earlier, especially important when the GPU is being hogged by background
> clients.
> 
> Signed-off-by: Chris Wilson 
> Cc: Daniel Stone 
> ---
> Note that in testing I found a regression in i965_dri.so,
> (see "i965: Accept CONTEXT_ATTRIB_PRIORITY for brwCreateContext")
> so bare that in mind if testing against Intel.

This looks good to me. My only trivial complaint is that failing to obtain
a high-priority context probably shouldn't be logged by default. However,
in the absence of any way to actually control logging verbosity, this
isn't your fault ...

I'd like the Mesa patch to land and sit for a month or so, to narrow the
odds that users are going to fail to start Weston. But once it's sat for a
bit, happy to land it and this is:
Reviewed-by: Daniel Stone 

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel