Re: [Mesa-dev] [PATCH] egl/wayland: break double/tripple buffering feedback loops
On 1/15/19 8:02 AM, Daniel Stone wrote: > Hi, > > On Tue, 18 Dec 2018 at 17:59, Lucas Stach wrote: >> Am Dienstag, den 18.12.2018, 17:43 + schrieb Emil Velikov: On Tue, 18 Dec 2018 at 11:16, Lucas Stach wrote: if (dri2_surf->back == NULL) dri2_surf->back = &dri2_surf->color_buffers[i]; - else if (dri2_surf->back->dri_image == NULL) + else if (dri2_surf->back->dri_image == NULL && dri2_surf->color_buffers[i].dri_image) dri2_surf->back = &dri2_surf->color_buffers[i]; + age = dri2_surf->back->age; } >>> >>> AFAICT this is the wayland equivalent of >>> 4f1d27a406478d405eac6f9894ccc46a80034adb >>> Where the exact same logic/commit message applies. >> >> No it isn't. It's exactly what it says in the commit log. It's keeping >> the tripple buffer around for a bit, even if we don't strictly need it >> when the client is currently doing double buffering. I'm having a bit of a hard time following the logic in this first hunk myself... The dri2_surf->color_buffers[i].age < age check looks like it's intended to skip buffers younger than the one currently in hand - ie) pick the oldest buffer. But doing so would break the second hunk because we'd never end up with a very old buffer to trim. (It doesn't actually cause the oldest buffer to be picked though, because of the other tests involved) I would like to at least see a comment explaining what's going on, because it looks kind of like a bug on a casual read. I think I side with Emil that this is an independent functional change. The first hunk should be able to stand on its own, and having a commit log for it explaining what it does to the selection process would be helpful. > Right - the crucial part in Derek's GBM commit was removing the > 'break' and adding the extra conditional on age. > > Derek's patch stabilises the age of buffers handed back to the user, > by always returning the oldest available buffer. That slightly > pessimises rendering if there is a 'free' buffer in the queue: if four > buffers are allocated, then we will always return a buffer from three > flips ago, maybe meaning more rendering work. It means that, barring > the client holding on to one buffer for unexpectedly long, the age of > the oldest buffer in the queue will never be greater than the queue > depth. > > This patch instead relies on unbalanced ages, where older buffers in > the queue are allowed to age far beyond the queue depth if not used > during normal rendering. Yes, it's a bit more annoying to track how long a buffer is "unneeded" for when you always pick the oldest, since age becomes useless for that purpose. For this patch to work, we need a buffer to be unused until it ages beyond a threshold - intuitively it seems to me this will just naturally happen to the last buffer in the array without the first hunk at all? >> When things are on the edge between double buffering being enough and >> sometimes a third buffer being needed to avoid stalling we would >> otherwise bounce rapidly between allocating and disposing the third >> buffer. >> >> The DRM platform has no such optimization and just keeps the third >> buffer around forever. This patch keeps the optimization in the Wayland >> platform, but adds a bit of hysteresis before disposing the buffer when >> going from tripple to double buffering to see if things are settling on >> double buffering. > > Ideally we'd have globally optimal behaviour for both platforms, but > that doesn't really seem doable for now. I think this is a good > balance though. There will only be one GBM user at a time, so having > that allocate excessive buffers doesn't seem too bad, and the penalty > for doing so is your entire system stuttering as the compositor > becomes blocked. Given the general stability of compositors, if they > need a larger queue depth at some point, they are likely to need it > again in the near future. > > Conversely, there may be a great many Wayland clients, and these > clients may bounce between overlay and GPU composition. Given that, it > seems reasonable to opportunistically free up buffers, to make sure we > have enough memory available across the system. Right - to be clear, I think this is a really good idea. :) I'm just having a little trouble with the details of the implementation. >>> The age check here seems strange - both number used and it's relation >>> to double/triple buffering. >>> Have you considered tracking/checking how many buffers we have? >> >> A hysteresis value of 18 is just something that worked well in >> practice. It didn't appear to defer the buffer destruction for too long >> while keeping the feedback loop well under control. > > Yeah, having this #defined with a comment above it would be nice. > > With that, this patch is: > Reviewed-by: Daniel Stone > > Cheers, > Daniel > ___ mesa-dev mailing list mes
[Mesa-dev] [PATCH] egl/wayland: Make swrast display_sync the correct queue
commit 03dd9a88b0be17ff0ce91e92f6902a9a85ba584a introduced per surface
queues, but the display_sync for swrast_commit_backbuffer remained on
the old queue. This is likely to break when dispatching the correct
queue at the top of function (which can't dispatch the sync callback
we're waiting for).
The easiest known reproduction case is running weston-subsurfaces under
weston --use-pixman
Signed-off-by: Derek Foreman
---
src/egl/drivers/dri2/platform_wayland.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index 94f7defa65..80853ac00b 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -1751,7 +1751,7 @@ dri2_wl_swrast_commit_backbuffer(struct dri2_egl_surface
*dri2_surf)
* handle the commit and send a release event before checking for a free
* buffer */
if (dri2_surf->throttle_callback == NULL) {
- dri2_surf->throttle_callback = wl_display_sync(dri2_dpy->wl_dpy_wrapper);
+ dri2_surf->throttle_callback =
wl_display_sync(dri2_surf->wl_dpy_wrapper);
wl_callback_add_listener(dri2_surf->throttle_callback,
&throttle_listener, dri2_surf);
}
--
2.14.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] egl: fix var type
On 2017-11-16 04:16 AM, Eric Engestrom wrote:
queryImage() takes an `int*`, compiler is warning about the
signed<->unsigned pointer mismatch.
Fixes: 0db36caa192b129cb4f2 "egl/wayland: Add a fallback when fourcc
query isn't supported"
Signed-off-by: Eric Engestrom
---
src/egl/drivers/dri2/platform_wayland.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index 7010dfdcb2beeafe30d8..b844df368ff934bfb652 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -676,7 +676,7 @@ get_fourcc(struct dri2_egl_display *dri2_dpy,
__DRIimage *image, int *fourcc)
{
EGLBoolean query;
- uint32_t dri_format;
+ int dri_format;
Yup, sorry.
Reviewed-by: Derek Foreman
query = dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_FOURCC,
fourcc);
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/wayland: Add a fallback when fourcc query isn't supported
Please disregard, I misunderstood a comment in the bug tracker and this
has been posted here before.
Sorry for the line noise.
On 2017-11-14 09:39 AM, Derek Foreman wrote:
When queryImage doesn't support __DRI_IMAGE_ATTRIB_FOURCC wayland clients
will die with a NULL derefence in wl_proxy_add_listener.
Attempt to provide a simple fallback to keep ancient systems working.
Signed-off-by: Derek Foreman
---
src/egl/drivers/dri2/platform_wayland.c | 32 ++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index b38eb1c335..7010dfdcb2 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -671,6 +671,35 @@ static const struct wl_callback_listener throttle_listener
= {
.done = wayland_throttle_callback
};
+static EGLBoolean
+get_fourcc(struct dri2_egl_display *dri2_dpy,
+ __DRIimage *image, int *fourcc)
+{
+ EGLBoolean query;
+ uint32_t dri_format;
+
+ query = dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_FOURCC,
+ fourcc);
+ if (query)
+ return true;
+
+ query = dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_FORMAT,
+ &dri_format);
+ if (!query)
+ return false;
+
+ switch (dri_format) {
+ case __DRI_IMAGE_FORMAT_ARGB:
+ *fourcc = __DRI_IMAGE_FOURCC_ARGB;
+ return true;
+ case __DRI_IMAGE_FORMAT_XRGB:
+ *fourcc = __DRI_IMAGE_FOURCC_XRGB;
+ return true;
+ default:
+ return false;
+ }
+}
+
static struct wl_buffer *
create_wl_buffer(struct dri2_egl_display *dri2_dpy,
struct dri2_egl_surface *dri2_surf,
@@ -684,8 +713,7 @@ create_wl_buffer(struct dri2_egl_display *dri2_dpy,
query = dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_WIDTH,
&width);
query &= dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_HEIGHT,
&height);
- query &= dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_FOURCC,
-&fourcc);
+ query &= get_fourcc(dri2_dpy, image, &fourcc);
if (!query)
return NULL;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] egl/wayland: Add a fallback when fourcc query isn't supported
When queryImage doesn't support __DRI_IMAGE_ATTRIB_FOURCC wayland clients
will die with a NULL derefence in wl_proxy_add_listener.
Attempt to provide a simple fallback to keep ancient systems working.
Signed-off-by: Derek Foreman
---
src/egl/drivers/dri2/platform_wayland.c | 32 ++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index b38eb1c335..7010dfdcb2 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -671,6 +671,35 @@ static const struct wl_callback_listener throttle_listener
= {
.done = wayland_throttle_callback
};
+static EGLBoolean
+get_fourcc(struct dri2_egl_display *dri2_dpy,
+ __DRIimage *image, int *fourcc)
+{
+ EGLBoolean query;
+ uint32_t dri_format;
+
+ query = dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_FOURCC,
+ fourcc);
+ if (query)
+ return true;
+
+ query = dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_FORMAT,
+ &dri_format);
+ if (!query)
+ return false;
+
+ switch (dri_format) {
+ case __DRI_IMAGE_FORMAT_ARGB:
+ *fourcc = __DRI_IMAGE_FOURCC_ARGB;
+ return true;
+ case __DRI_IMAGE_FORMAT_XRGB:
+ *fourcc = __DRI_IMAGE_FOURCC_XRGB;
+ return true;
+ default:
+ return false;
+ }
+}
+
static struct wl_buffer *
create_wl_buffer(struct dri2_egl_display *dri2_dpy,
struct dri2_egl_surface *dri2_surf,
@@ -684,8 +713,7 @@ create_wl_buffer(struct dri2_egl_display *dri2_dpy,
query = dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_WIDTH,
&width);
query &= dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_HEIGHT,
&height);
- query &= dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_FOURCC,
-&fourcc);
+ query &= get_fourcc(dri2_dpy, image, &fourcc);
if (!query)
return NULL;
--
2.15.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] vc4: Don't advertise tiled dmabuf modifiers if we can't use them
If the DRM_VC4_GET_TILING ioctl isn't present then we can't tell
if a dmabuf bo is tiled or linear, so will always assume it's
linear.
By not advertising tiled formats in this situation we ensure the
assumption is correct.
This fixes a bug where most attempts to render a gl wayland client
under weston will result in a client side abort.
Signed-off-by: Derek Foreman
---
src/gallium/drivers/vc4/vc4_screen.c | 31 ++-
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/src/gallium/drivers/vc4/vc4_screen.c
b/src/gallium/drivers/vc4/vc4_screen.c
index 5743e13045..b39cc744e6 100644
--- a/src/gallium/drivers/vc4/vc4_screen.c
+++ b/src/gallium/drivers/vc4/vc4_screen.c
@@ -549,25 +549,30 @@ vc4_screen_query_dmabuf_modifiers(struct pipe_screen
*pscreen,
unsigned int *external_only,
int *count)
{
+int m, i;
+uint64_t available_modifiers[] = {
+DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED,
+DRM_FORMAT_MOD_LINEAR,
+};
+struct vc4_screen *screen = vc4_screen(pscreen);
+int num_modifiers = screen->has_tiling_ioctl ? 2 : 1;
+
if (!modifiers) {
-*count = 2;
+*count = num_modifiers;
return;
}
-*count = MIN2(max, 2);
-
+*count = MIN2(max, num_modifiers);
+m = screen->has_tiling_ioctl ? 0 : 1;
/* We support both modifiers (tiled and linear) for all sampler
- * formats.
+ * formats, but if we don't have the DRM_VC4_GET_TILING ioctl
+ * we shouldn't advertise the tiled formats.
*/
-modifiers[0] = DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
-if (external_only)
-external_only[0] = false;
-if (max < 2)
-return;
-
-modifiers[1] = DRM_FORMAT_MOD_LINEAR;
-if (external_only)
-external_only[1] = false;
+for (i = 0; i < *count; i++) {
+modifiers[i] = available_modifiers[m++];
+if (external_only)
+external_only[i] = false;
+ }
}
#define PTR_TO_UINT(x) ((unsigned)((intptr_t)(x)))
--
2.14.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] wayland-drm: constify the callbacks struct, take 2
On 2017-09-27 01:49 PM, Emil Velikov wrote:
From: Emil Velikov
Now that wayland-drm (correctly) keeps a local copy of the callbacks,
this should not longer cause explosions.
After all the symbol is a local, constant data.
Cc: Daniel Stone
Cc: Derek Foreman
Signed-off-by: Emil Velikov
---
Derek, can you please check the series on your end.
I've ran this through wayland (xwayland and drm) + mpv and the simple
weston demos (simple-egl, simple-damage, flowers, smoke, editor)
of the weston demos, I think only simple-egl is actually an egl user,
but thanks for being thorough. :)
The series looks good to me, and functions well in my testing too.
Tested-by: Derek Foreman
---
src/egl/drivers/dri2/egl_dri2.c | 14 +-
src/egl/wayland/wayland-drm/wayland-drm.c | 2 +-
src/egl/wayland/wayland-drm/wayland-drm.h | 2 +-
3 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index adcaae0bab7..a0e8b0be5b0 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -2730,17 +2730,16 @@ dri2_wl_release_buffer(void *user_data, struct
wl_drm_buffer *buffer)
dri2_dpy->image->destroyImage(buffer->driver_buffer);
}
-static struct wayland_drm_callbacks wl_drm_callbacks = {
-.authenticate = NULL,
-.reference_buffer = dri2_wl_reference_buffer,
-.release_buffer = dri2_wl_release_buffer
-};
-
static EGLBoolean
dri2_bind_wayland_display_wl(_EGLDriver *drv, _EGLDisplay *disp,
struct wl_display *wl_dpy)
{
struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
+ const struct wayland_drm_callbacks wl_drm_callbacks = {
+ .authenticate = (int(*)(void *, uint32_t)) dri2_dpy->vtbl->authenticate,
+ .reference_buffer = dri2_wl_reference_buffer,
+ .release_buffer = dri2_wl_release_buffer
+ };
int flags = 0;
uint64_t cap;
@@ -2749,9 +2748,6 @@ dri2_bind_wayland_display_wl(_EGLDriver *drv, _EGLDisplay *disp,
if (dri2_dpy->wl_server_drm)
return EGL_FALSE;
- wl_drm_callbacks.authenticate =
- (int(*)(void *, uint32_t)) dri2_dpy->vtbl->authenticate;
-
if (drmGetCap(dri2_dpy->fd, DRM_CAP_PRIME, &cap) == 0 &&
cap == (DRM_PRIME_CAP_IMPORT | DRM_PRIME_CAP_EXPORT) &&
dri2_dpy->image->base.version >= 7 &&
diff --git a/src/egl/wayland/wayland-drm/wayland-drm.c
b/src/egl/wayland/wayland-drm/wayland-drm.c
index 0f0a2317c7e..73dfba9600e 100644
--- a/src/egl/wayland/wayland-drm/wayland-drm.c
+++ b/src/egl/wayland/wayland-drm/wayland-drm.c
@@ -259,7 +259,7 @@ wayland_drm_buffer_get(struct wl_drm *drm, struct
wl_resource *resource)
struct wl_drm *
wayland_drm_init(struct wl_display *display, char *device_name,
- struct wayland_drm_callbacks *callbacks, void *user_data,
+ const struct wayland_drm_callbacks *callbacks, void
*user_data,
uint32_t flags)
{
struct wl_drm *drm;
diff --git a/src/egl/wayland/wayland-drm/wayland-drm.h
b/src/egl/wayland/wayland-drm/wayland-drm.h
index 77e8d273042..111383ff1d6 100644
--- a/src/egl/wayland/wayland-drm/wayland-drm.h
+++ b/src/egl/wayland/wayland-drm/wayland-drm.h
@@ -34,7 +34,7 @@ wayland_drm_buffer_get(struct wl_drm *drm, struct wl_resource
*resource);
struct wl_drm *
wayland_drm_init(struct wl_display *display, char *device_name,
-struct wayland_drm_callbacks *callbacks, void *user_data,
+const struct wayland_drm_callbacks *callbacks, void *user_data,
uint32_t flags);
void
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] ttn: Fix out-of-bounds accesses since the always-2D-constants change.
This fixes a crash from glTexImage2D() at startup for all EFL
applications on rpi3.
Thanks,
Tested-by: Derek Foreman
On 2017-09-07 12:17 PM, Eric Anholt wrote:
Only one of the three checks for dim was updated, so we would try to set a
UBO buffer index source value on a nir_load_uniform, and wouldn't actually
declare non-UBO uniforms.
Fixes: 37dd8e8dee1d ("gallium: all drivers should accept two-dimensional constant
buffer indexing")
---
src/gallium/auxiliary/nir/tgsi_to_nir.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
b/src/gallium/auxiliary/nir/tgsi_to_nir.c
index aa715dcae2db..1b630096ffaa 100644
--- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
+++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
@@ -314,7 +314,8 @@ ttn_emit_declaration(struct ttn_compile *c)
file == TGSI_FILE_CONSTANT);
/* nothing to do for UBOs: */
- if ((file == TGSI_FILE_CONSTANT) && decl->Declaration.Dimension) {
+ if ((file == TGSI_FILE_CONSTANT) && decl->Declaration.Dimension &&
+ decl->Dim.Index2D != 0) {
b->shader->info.num_ubos =
MAX2(b->shader->info.num_ubos, decl->Dim.Index2D);
return;
@@ -638,7 +639,7 @@ ttn_src_for_file_and_index(struct ttn_compile *c, unsigned
file, unsigned index,
load = nir_intrinsic_instr_create(b->shader, op);
load->num_components = 4;
- if (dim) {
+ if (dim && (dim->Index > 0 || dim->Indirect)) {
if (dimind) {
load->src[srcn] =
ttn_src_for_file_and_index(c, dimind->File, dimind->Index,
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/wayland: resolve quirky try_damage_buffer() implementation
On 13/01/17 11:27 AM, Emil Velikov wrote:
From: Emil Velikov
The implementation was added with commit d085a5dff5b and effectively
provided a hidden dependency.
Namely: the codepath used was determined solely during build time. Thus
if we built again new wayland and then run against older (yet still
within the requirements, as per the configure) one will get undefined
symbols.
indeed. :(
As of earlier commit 36b9976e1f9 "egl/wayland: Avoid race conditions
when on non-main thread" the required version was bumped to one which
provides the API, thus we can drop the quirky solution.
Cc: Derek Foreman
Signed-off-by: Emil Velikov
I'd forgotten this was still in place - thanks for coming back to it.
Reviewed-by: Derek Foreman
---
One way to avoid the issue w/o bumping the requirement (for -stable) is
to add fall-back define alongside weak implementation of the functions.
The latter should "return false" and will get automatically overridden
if new enough wayland is used.
>
Not sure how much one should care - just thinking out loud.
If one should care, that sounds like a good way to go about it...
Let me know if you want me to care, and I'll write it up.
Thanks,
Derek
---
src/egl/drivers/dri2/platform_wayland.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index 4009cc9691..3057604d3c 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -669,14 +669,6 @@ try_damage_buffer(struct dri2_egl_surface *dri2_surf,
const EGLint *rects,
EGLint n_rects)
{
-/* The WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION macro and
- * wl_proxy_get_version() were both introduced in wayland 1.10.
- * Instead of bumping our wayland dependency we just make this
- * function conditional on the required 1.10 features, falling
- * back to old (correct but suboptimal) behaviour for older
- * wayland.
- */
-#ifdef WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION
int i;
if (wl_proxy_get_version((struct wl_proxy *) dri2_surf->wl_win->surface)
@@ -692,8 +684,6 @@ try_damage_buffer(struct dri2_egl_surface *dri2_surf,
rect[2], rect[3]);
}
return EGL_TRUE;
-#endif
- return EGL_FALSE;
}
/**
* Called via eglSwapBuffers(), drv->API.SwapBuffers().
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] egl/dri2: add image_loader_extension back into loader extensions for wayland
before commit f871946594129500a67c05a6d9fe99db54b4bb64
image_loader_extension was always present in dri2_dpy->extensions,
after that commit it is only present for render nodes.
Its removal broke partial render based on buffer age on (at least)
raspberry pi.
Signed-off-by: Derek Foreman
---
I'm no expert on these bits of code, I simply found this by bisecting and
looking at the differences.
To see the problem, if one has a raspberry pi 3, one can run weston under
weston (or any efl applications using the GL back-end on a wayland
compositor).
Frames become black in areas that aren't updated, instead of displaying the
content implied by the queried buffer age.
src/egl/drivers/dri2/platform_wayland.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index 395f1e181d..fd4812a4fe 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -1080,6 +1080,7 @@ static struct dri2_egl_display_vtbl dri2_wl_display_vtbl
= {
static const __DRIextension *dri2_loader_extensions[] = {
&dri2_loader_extension.base,
+ &image_loader_extension.base,
&image_lookup_extension.base,
&use_invalidate.base,
NULL,
--
2.11.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i915: Add XRGB8888 format to intel_screen_make_configs
On 24/11/16 08:53 AM, Emil Velikov wrote:
On 24 November 2016 at 06:22, Boyan Ding wrote:
2016-11-24 13:29 GMT+08:00 Derek Foreman :
On 23/11/16 07:18 PM, Boyan Ding wrote:
2016-11-24 7:01 GMT+08:00 Derek Foreman :
This is a copy of commit 536003c11e4cb1172c540932ce3cce06f03bf44e
except for i915.
Original log for the i965 commit follows:
Some application, such as drm backend of weston, uses XRGB config as
default. i965 doesn't provide this format, but before commit 65c8965d,
the drm platform of EGL takes ARGB as XRGB. Now that commit
65c8965d makes EGL recognize format correctly so weston won't start
because it can't find XRGB. Add XRGB format to i965 just as
other drivers do.
Signed-off-by: Derek Foreman
---
src/mesa/drivers/dri/i915/intel_screen.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/mesa/drivers/dri/i915/intel_screen.c
b/src/mesa/drivers/dri/i915/intel_screen.c
index 1b80df0..5c7c06a 100644
--- a/src/mesa/drivers/dri/i915/intel_screen.c
+++ b/src/mesa/drivers/dri/i915/intel_screen.c
@@ -1044,7 +1044,8 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
{
static const mesa_format formats[] = {
MESA_FORMAT_B5G6R5_UNORM,
- MESA_FORMAT_B8G8R8A8_UNORM
+ MESA_FORMAT_B8G8R8A8_UNORM,
+ MESA_FORMAT_B8G8R8X8_UNORM
};
/* GLX_SWAP_COPY_OML is not supported due to page flipping. */
--
2.10.2
Hi Derek,
I sent exactly the same patch one and half years ago at [1], but
withdrew it because it seems no one got interested in that and I don't
have the hardware to test. If you're sure it is correct, this gets my
Acked-by: Boyan Ding
I'm sorry, I didn't see your patch. It makes more sense to me that I give
you my RB on that patch in place of your Ack on mine. I don't want to take
credit for a problem you solved over a year ago. :)
I don't have appropriate hardware but this has been tested for me by an
Enlightenment user who was unable to use our GL backend because it's trying
to use XRGB.
Weston ran for him but logged a warning about falling back to an ARGB
visual, which led me to the discovery that this had only been changed for
i965.
I'll try to get him to reply with a "Tested-by" tomorrow.
Thanks for the kind reply, but I wonder if a more thorough testing
should be done before applying this. I remember its i965 counterpart
(commit 28090b30d) did cause some problem (Bug 90791) although it seems
not the fault on its own.
We're facing the same problem I faced last year when posting this
patch -- I didn't have the authority to say that it was okay, and I
didn't have the hardware to test on as Emil once suggested[1].
Precisely - the patch on it's own was perfectly reasonable but it will
likely cause issues like i965 one.
Yikes, that's a long list of side effects. This is something I can work
around in enlightenment anyway, by doing the same manner of fallback
weston does (fall back to an ARGB visual if an XRGB is unavailable).
Please let me know if it's preferred that I just do it there instead.
Thanks for taking to time to compile that list.
Derek
Derek, skimming through the i965 history a very lengthy list of
regressions/fixes comes up[1] as the format was advertised as
supported.
Considering we don't get much testing, it would be great to avoid
breaking the world by fixing EGL/drm users (Wayland compositors,
other).
Ian, Mark Janes,
Gents, do you have the hardware/chance to test this patch ?
Thanks
Emil
[1]
28090b30dd6b5977de085f48c620574214b6b4ba i965: Add XRGB format to
intel_screen_make_configs
* Adds the format since things broke as EGL/drm was fixed to correctly
honour the formats commit 65c8965d033 (egl: Take alpha bits into
account when selecting GBM formats)
8da79b8378ae87474d8c47ad955e4833edf98359 i965: Fix HW blitter pitch limits
c2d0606827412b710dcaed80268fc665de8c9c5d i915: Blit RGBX<->RGBA
drawpixels // should read i965
922c0c9fd526ce19b87bc74a3159dec7705c1de1 i965: Export format
comparison for blitting between miptrees
* Fixes serious performance issue due to the extra format.
bd38f91f8d80897ca91979962d80d4bc0acef586 i965: do_blit_drawpixels:
decode array formats
* Fixes the last commit above
2cebaac479d49cd6df4e97b466ba14bab3f30db1 i965: Don't use tiled_memcpy
to download from RGBX or BGRX surfaces
* Disable those since it causes regressions
3f10774cbab1e803f8aa3d6d24f8f6f98b8128c3 i965: Check base format to
determine whether to use tiled memcpy
* Aims to rework/fix the above.
c769efda939e06338d41e1046a5f954c690951d5 i965: Add
MESA_FORMAT_B8G8R8X8_SRGB to brw_format_for_mesa_format
43f4be5f06b7a96b96a3a7b43f5112139a1f423a i965: Add B8G8R8X8_SRGB to
the alpha format override
839793680f99b8387bee9489733d5071c10f3ace i965: Use
MESA_FORMAT_B8G8R8X8_SRGB for RGB visuals
* Aim to rework/fix the mesa <> hardware format mappings as a side
effect
Re: [Mesa-dev] [PATCH] i915: Add XRGB8888 format to intel_screen_make_configs
On 23/11/16 07:18 PM, Boyan Ding wrote:
2016-11-24 7:01 GMT+08:00 Derek Foreman :
This is a copy of commit 536003c11e4cb1172c540932ce3cce06f03bf44e
except for i915.
Original log for the i965 commit follows:
Some application, such as drm backend of weston, uses XRGB config as
default. i965 doesn't provide this format, but before commit 65c8965d,
the drm platform of EGL takes ARGB as XRGB. Now that commit
65c8965d makes EGL recognize format correctly so weston won't start
because it can't find XRGB. Add XRGB format to i965 just as
other drivers do.
Signed-off-by: Derek Foreman
---
src/mesa/drivers/dri/i915/intel_screen.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/mesa/drivers/dri/i915/intel_screen.c
b/src/mesa/drivers/dri/i915/intel_screen.c
index 1b80df0..5c7c06a 100644
--- a/src/mesa/drivers/dri/i915/intel_screen.c
+++ b/src/mesa/drivers/dri/i915/intel_screen.c
@@ -1044,7 +1044,8 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
{
static const mesa_format formats[] = {
MESA_FORMAT_B5G6R5_UNORM,
- MESA_FORMAT_B8G8R8A8_UNORM
+ MESA_FORMAT_B8G8R8A8_UNORM,
+ MESA_FORMAT_B8G8R8X8_UNORM
};
/* GLX_SWAP_COPY_OML is not supported due to page flipping. */
--
2.10.2
Hi Derek,
I sent exactly the same patch one and half years ago at [1], but
withdrew it because it seems no one got interested in that and I don't
have the hardware to test. If you're sure it is correct, this gets my
Acked-by: Boyan Ding
I'm sorry, I didn't see your patch. It makes more sense to me that I
give you my RB on that patch in place of your Ack on mine. I don't want
to take credit for a problem you solved over a year ago. :)
I don't have appropriate hardware but this has been tested for me by an
Enlightenment user who was unable to use our GL backend because it's
trying to use XRGB.
Weston ran for him but logged a warning about falling back to an ARGB
visual, which led me to the discovery that this had only been changed
for i965.
I'll try to get him to reply with a "Tested-by" tomorrow.
Regards,
Boyan Ding
[1] https://lists.freedesktop.org/archives/mesa-dev/2015-April/081501.html
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i915: Add XRGB8888 format to intel_screen_make_configs
This is a copy of commit 536003c11e4cb1172c540932ce3cce06f03bf44e
except for i915.
Original log for the i965 commit follows:
Some application, such as drm backend of weston, uses XRGB config as
default. i965 doesn't provide this format, but before commit 65c8965d,
the drm platform of EGL takes ARGB as XRGB. Now that commit
65c8965d makes EGL recognize format correctly so weston won't start
because it can't find XRGB. Add XRGB format to i965 just as
other drivers do.
Signed-off-by: Derek Foreman
---
src/mesa/drivers/dri/i915/intel_screen.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/mesa/drivers/dri/i915/intel_screen.c
b/src/mesa/drivers/dri/i915/intel_screen.c
index 1b80df0..5c7c06a 100644
--- a/src/mesa/drivers/dri/i915/intel_screen.c
+++ b/src/mesa/drivers/dri/i915/intel_screen.c
@@ -1044,7 +1044,8 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
{
static const mesa_format formats[] = {
MESA_FORMAT_B5G6R5_UNORM,
- MESA_FORMAT_B8G8R8A8_UNORM
+ MESA_FORMAT_B8G8R8A8_UNORM,
+ MESA_FORMAT_B8G8R8X8_UNORM
};
/* GLX_SWAP_COPY_OML is not supported due to page flipping. */
--
2.10.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] gbm/drm: Pick the oldest available buffer in get_back_bo
Applications may query the back buffer age to efficiently perform
partial updates. Generally the application will keep a fixed length
damage history, and use this to calculate what needs to be redrawn
based on the age of the back buffer it's about to render to.
If presented with a buffer that has an age greater than the
length of the damage history, the application will likely have
to completely repaint the buffer.
Our current buffer selection strategy is to pick the first available
buffer without considering its age. If an application frequently
manages to fit within two buffers but occasionally requires a third,
this extra buffer will almost always be old enough to fall outside
of a reasonably long damage history, and require a full repaint.
This patch changes the buffer selection behaviour to prefer the oldest
available buffer.
By selecting the oldest available buffer, the application will likely
always be able to use its damage history, at a cost of having to
perform slightly more work every frame. This is an improvement if
the cost of a full repaint is heavy, and the surface damage between
frames is relatively small.
It should be noted that since we don't currently trim our queue in
any way, an application that briefly needs a large number of buffers
will continue to receive older buffers than it would if it only ever
needed two buffers.
Reviewed-by: Daniel Stone
Signed-off-by: Derek Foreman
---
The only changes are in the commit log, which hopefully better
expresses the rationale for the change.
src/egl/drivers/dri2/platform_drm.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_drm.c
b/src/egl/drivers/dri2/platform_drm.c
index 2099314..f812ab5 100644
--- a/src/egl/drivers/dri2/platform_drm.c
+++ b/src/egl/drivers/dri2/platform_drm.c
@@ -215,13 +215,15 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
struct dri2_egl_display *dri2_dpy =
dri2_egl_display(dri2_surf->base.Resource.Display);
struct gbm_dri_surface *surf = dri2_surf->gbm_surf;
+ int age = 0;
unsigned i;
if (dri2_surf->back == NULL) {
for (i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
-if (!dri2_surf->color_buffers[i].locked) {
+if (!dri2_surf->color_buffers[i].locked &&
+ dri2_surf->color_buffers[i].age >= age) {
dri2_surf->back = &dri2_surf->color_buffers[i];
- break;
+ age = dri2_surf->color_buffers[i].age;
}
}
}
--
2.10.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] gbm/drm: Pick the oldest available buffer in get_back_bo
We find the oldest backbuffer we can, on the grounds that clients are
only going to keep a fixed history queue, so this gives them the
greatest chance of being able to use that queue via making sure
the age is ~always less than the depth of the swapchain
Reviewed-by: Daniel Stone
Signed-off-by: Derek Foreman
---
src/egl/drivers/dri2/platform_drm.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_drm.c
b/src/egl/drivers/dri2/platform_drm.c
index 2099314..f812ab5 100644
--- a/src/egl/drivers/dri2/platform_drm.c
+++ b/src/egl/drivers/dri2/platform_drm.c
@@ -215,13 +215,15 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
struct dri2_egl_display *dri2_dpy =
dri2_egl_display(dri2_surf->base.Resource.Display);
struct gbm_dri_surface *surf = dri2_surf->gbm_surf;
+ int age = 0;
unsigned i;
if (dri2_surf->back == NULL) {
for (i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
-if (!dri2_surf->color_buffers[i].locked) {
+if (!dri2_surf->color_buffers[i].locked &&
+ dri2_surf->color_buffers[i].age >= age) {
dri2_surf->back = &dri2_surf->color_buffers[i];
- break;
+ age = dri2_surf->color_buffers[i].age;
}
}
}
--
2.10.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] Revert "wayland: Block for the frame callback in get_back_bo not dri2_swap_buffers"
On 31/10/16 09:30 AM, Daniel Stone wrote: On 24 October 2016 at 20:42, Daniel Stone wrote: This reverts commit 25cc889004aad6d1cab9edd76db898658e347b97, though since the code has changed, it was applied manually. The intent of moving blocking from SwapBuffers to get_back_bo, was to avoid unnecessary triple-buffering by ensuring that the compositor had fully processed the previous frame before we started rendering. This means that the only time we would have to resort to triple-buffering would be when the buffer is directly scanned out, thus saving an extra buffer for composition anyway. The 'repaint window' changes introduced in Weston since then, however, have narrowed the window of time between the frame event being sent and the repaint loop needing to conclude, to 7ms by default, in order to reduce latency. This means however that blocking in get_back_bo gives a maximum of 7ms for the entire GL submission to begin and complete. Not only this, but if a client is using buffer_age to avoid full repaints, the buffer-age request will stall in get_back_bo until the frame callback completes, meaning that the client cannot even calculate the repaint area before the 7ms window. The combination of the two meant that WebKit-GTK+ was failing to achieve full framerate on a Minnowboard, due to spending a great deal of its time attempting to query the age of the next buffer before redraw. Revert to the previous behaviour of allowing rendering to begin but delaying SwapBuffers, unless and until we can find a more gentle behaviour. Ping - adding a couple more CCs. This looks correct to me. I've done some testing with weston and enlightenment and various EFL clients, and note no regressions. Reviewed-by: Derek Foreman Tested-by: Derek Foreman Thanks, Derek Cheers, Daniel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] egl/wayland: Try to use wl_surface.damage_buffer for SwapBuffersWithDamage
On 16/02/16 10:37 AM, Daniel Stone wrote:
> Hi,
>
> On 16 February 2016 at 16:34, Derek Foreman wrote:
>> +try_damage_buffer(struct dri2_egl_surface *dri2_surf,
>> + const EGLint *rects,
>> + EGLint n_rects)
>> +{
>> +/* The WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION macro and
>> + * wl_proxy_get_version() were both introduced in wayland 1.10.
>> + * Instead of bumping our wayland dependency we just make this
>> + * function conditional on the required 1.10 features, falling
>> + * back to old (correct but suboptimal) behaviour for older
>> + * wayland.
>> + */
>> +#ifdef WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION
>
> It still bumps the runtime requirement, i.e. once built against >=1.10
> it can only ever be run against >= 1.10. Maybe dlsym is overkill, but
> OTOH maybe not ...
Yup, that's true.
I kind of just assumed distros would set their own dependency
information to whatever libwayland they actually built against.
I hadn't given much thought to building mesa against a new libwayland
then downgrading libwayland or transplanting that mesa to a system with
older wayland.
I can re-do this with runtime dlsym checking for wl_proxy_get_version()
if that's preferred - someone else can make that decision because I
don't have a strong opinion either way. :)
Thanks,
Derek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] egl/wayland: Try to use wl_surface.damage_buffer for SwapBuffersWithDamage
Since commit d1314de293e9e4a63c35f094c3893aaaed8580b4 we ignore
damage passed to SwapBuffersWithDamage.
Wayland 1.10 now has functionality that allows us to properly
process those damage rectangles, and a way to query if it's
available.
Now we can use wl_surface.damage_buffer and interpret the incoming
damage as being in buffer co-ordinates.
Reviewed-by: Jason Ekstrand
Reviewed-by: Pekka Paalanen
Signed-off-by: Derek Foreman
---
Changes from v1:
Add comment explaining why the call to wl_proxy_get_version() is hidden
by the seemingly unrelated macro.
src/egl/drivers/dri2/platform_wayland.c | 39 ++---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index c2438f7..341acb7 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -653,6 +653,37 @@ create_wl_buffer(struct dri2_egl_surface *dri2_surf)
&wl_buffer_listener, dri2_surf);
}
+static EGLBoolean
+try_damage_buffer(struct dri2_egl_surface *dri2_surf,
+ const EGLint *rects,
+ EGLint n_rects)
+{
+/* The WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION macro and
+ * wl_proxy_get_version() were both introduced in wayland 1.10.
+ * Instead of bumping our wayland dependency we just make this
+ * function conditional on the required 1.10 features, falling
+ * back to old (correct but suboptimal) behaviour for older
+ * wayland.
+ */
+#ifdef WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION
+ int i;
+
+ if (wl_proxy_get_version((struct wl_proxy *) dri2_surf->wl_win->surface)
+ < WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION)
+ return EGL_FALSE;
+
+ for (i = 0; i < n_rects; i++) {
+ const int *rect = &rects[i * 4];
+
+ wl_surface_damage_buffer(dri2_surf->wl_win->surface,
+ rect[0],
+ dri2_surf->base.Height - rect[1] - rect[3],
+ rect[2], rect[3]);
+ }
+ return EGL_TRUE;
+#endif
+ return EGL_FALSE;
+}
/**
* Called via eglSwapBuffers(), drv->API.SwapBuffers().
*/
@@ -703,10 +734,12 @@ dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
dri2_surf->dx = 0;
dri2_surf->dy = 0;
- /* We deliberately ignore the damage region and post maximum damage, due to
+ /* If the compositor doesn't support damage_buffer, we deliberately
+* ignore the damage region and post maximum damage, due to
* https://bugs.freedesktop.org/78190 */
- wl_surface_damage(dri2_surf->wl_win->surface,
- 0, 0, INT32_MAX, INT32_MAX);
+ if (!n_rects || !try_damage_buffer(dri2_surf, rects, n_rects))
+ wl_surface_damage(dri2_surf->wl_win->surface,
+0, 0, INT32_MAX, INT32_MAX);
if (dri2_dpy->is_different_gpu) {
_EGLContext *ctx = _eglGetCurrentContext();
--
2.7.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/wayland: Try to use wl_surface.damage_buffer for SwapBuffersWithDamage
On 12/02/16 04:46 PM, Emil Velikov wrote:
> On 11 February 2016 at 16:34, Derek Foreman wrote:
>> Since commit d1314de293e9e4a63c35f094c3893aaaed8580b4 we ignore
>> damage passed to SwapBuffersWithDamage.
>>
>> Wayland 1.10 now has functionality that allows us to properly
>> process those damage rectangles, and a way to query if it's
>> available.
>>
>> Now we can use wl_surface.damage_buffer and interpret the incoming
>> damage as being in buffer co-ordinates.
>>
>> Signed-off-by: Derek Foreman
>> ---
>> src/egl/drivers/dri2/platform_wayland.c | 32
>> +---
>> 1 file changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/platform_wayland.c
>> b/src/egl/drivers/dri2/platform_wayland.c
>> index c2438f7..b5a5b59 100644
>> --- a/src/egl/drivers/dri2/platform_wayland.c
>> +++ b/src/egl/drivers/dri2/platform_wayland.c
>> @@ -653,6 +653,30 @@ create_wl_buffer(struct dri2_egl_surface *dri2_surf)
>>&wl_buffer_listener, dri2_surf);
>> }
>>
>> +static EGLBoolean
>> +try_damage_buffer(struct dri2_egl_surface *dri2_surf,
>> + const EGLint *rects,
>> + EGLint n_rects)
>> +{
>> +#ifdef WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION
>> + int i;
>> +
>> + if (wl_proxy_get_version((struct wl_proxy *) dri2_surf->wl_win->surface)
>> + < WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION)
>> + return EGL_FALSE;
>> +
>> + for (i = 0; i < n_rects; i++) {
>> + const int *rect = &rects[i * 4];
>> +
>> + wl_surface_damage_buffer(dri2_surf->wl_win->surface,
>> + rect[0],
>> + dri2_surf->base.Height - rect[1] - rect[3],
>> + rect[2], rect[3]);
>> + }
>> + return EGL_TRUE;
>> +#endif
>
> I'm slightly worried about keeping this compile time. For example if
> we compile against old wayland, and then run against a capable one...
> this code won't exist thus will never get executed. Thus leading to a
> handful of "wtf" moments.
>
> I would just ensure it's defined at the top.
>
> #ifndef WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION
> #define WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION foo
> #endif
If your wayland headers don't have
WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION then compilation will fail
because wl_proxy_get_version() isn't present.
wl_proxy_get_version() will be formally introduced in the same wayland
release (next Tuesday's 1.10 release) as wl_surface_damage_buffer(), so
I'm intentionally using that as a tricky way to avoid the check
entirely. Too tricky?
If you prefer I can work out some autoconfy stuff to determine if
wl_proxy_get_version() is available?
> Cheers,
> Emil
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] egl/wayland: Try to use wl_surface.damage_buffer for SwapBuffersWithDamage
Since commit d1314de293e9e4a63c35f094c3893aaaed8580b4 we ignore
damage passed to SwapBuffersWithDamage.
Wayland 1.10 now has functionality that allows us to properly
process those damage rectangles, and a way to query if it's
available.
Now we can use wl_surface.damage_buffer and interpret the incoming
damage as being in buffer co-ordinates.
Signed-off-by: Derek Foreman
---
src/egl/drivers/dri2/platform_wayland.c | 32 +---
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index c2438f7..b5a5b59 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -653,6 +653,30 @@ create_wl_buffer(struct dri2_egl_surface *dri2_surf)
&wl_buffer_listener, dri2_surf);
}
+static EGLBoolean
+try_damage_buffer(struct dri2_egl_surface *dri2_surf,
+ const EGLint *rects,
+ EGLint n_rects)
+{
+#ifdef WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION
+ int i;
+
+ if (wl_proxy_get_version((struct wl_proxy *) dri2_surf->wl_win->surface)
+ < WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION)
+ return EGL_FALSE;
+
+ for (i = 0; i < n_rects; i++) {
+ const int *rect = &rects[i * 4];
+
+ wl_surface_damage_buffer(dri2_surf->wl_win->surface,
+ rect[0],
+ dri2_surf->base.Height - rect[1] - rect[3],
+ rect[2], rect[3]);
+ }
+ return EGL_TRUE;
+#endif
+ return EGL_FALSE;
+}
/**
* Called via eglSwapBuffers(), drv->API.SwapBuffers().
*/
@@ -703,10 +727,12 @@ dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
dri2_surf->dx = 0;
dri2_surf->dy = 0;
- /* We deliberately ignore the damage region and post maximum damage, due to
+ /* If the compositor doesn't support damage_buffer, we deliberately
+* ignore the damage region and post maximum damage, due to
* https://bugs.freedesktop.org/78190 */
- wl_surface_damage(dri2_surf->wl_win->surface,
- 0, 0, INT32_MAX, INT32_MAX);
+ if (!n_rects || !try_damage_buffer(dri2_surf, rects, n_rects))
+ wl_surface_damage(dri2_surf->wl_win->surface,
+0, 0, INT32_MAX, INT32_MAX);
if (dri2_dpy->is_different_gpu) {
_EGLContext *ctx = _eglGetCurrentContext();
--
2.7.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/4] loader: Rename drm_open_device() to loader_open_device() and share it
This is already our common idiom for opening files with CLOEXEC and
it's a little ugly, so let's share this one implementation.
Signed-off-by: Derek Foreman
---
src/loader/loader.c | 6 +++---
src/loader/loader.h | 3 +++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/loader/loader.c b/src/loader/loader.c
index 17bf133..fc46815 100644
--- a/src/loader/loader.c
+++ b/src/loader/loader.c
@@ -314,8 +314,8 @@ get_id_path_tag_from_fd(struct udev *udev, int fd)
return id_path_tag;
}
-static int
-drm_open_device(const char *device_name)
+int
+loader_open_device(const char *device_name)
{
int fd;
#ifdef O_CLOEXEC
@@ -404,7 +404,7 @@ int loader_get_user_preferred_fd(int default_fd, int
*different_device)
goto default_device_clean;
}
- fd = drm_open_device(device_name);
+ fd = loader_open_device(device_name);
if (fd >= 0) {
close(default_fd);
} else {
diff --git a/src/loader/loader.h b/src/loader/loader.h
index 60c58f2..055dc78 100644
--- a/src/loader/loader.h
+++ b/src/loader/loader.h
@@ -37,6 +37,9 @@ extern "C" {
#define _LOADER_GALLIUM (1 << 1)
int
+loader_open_device(const char *);
+
+int
loader_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id);
char *
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/4] glx: Use loader_open_device() helper
We've moved the open with CLOEXEC idiom into a helper function, so
call it instead of duplicating the code here.
Signed-off-by: Derek Foreman
---
src/glx/dri2_glx.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 538cf1a..27ea952 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -1183,15 +1183,7 @@ dri2CreateScreen(int screen, struct glx_display * priv)
return NULL;
}
-#ifdef O_CLOEXEC
- psc->fd = open(deviceName, O_RDWR | O_CLOEXEC);
- if (psc->fd == -1 && errno == EINVAL)
-#endif
- {
- psc->fd = open(deviceName, O_RDWR);
- if (psc->fd != -1)
- fcntl(psc->fd, F_SETFD, fcntl(psc->fd, F_GETFD) | FD_CLOEXEC);
- }
+ psc->fd = loader_open_device(deviceName);
if (psc->fd < 0) {
ErrorMessageF("failed to open drm device: %s\n", strerror(errno));
goto handle_error;
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/4] egl/drm: Duplicate fd with F_DUPFD_CLOEXEC to prevent leak
Replacing dup() with fcntl F_DUPFD_CLOEXEC creates the duplicate
file descriptor with CLOEXEC so it won't be leaked to child
processes if the process fork()s later.
Signed-off-by: Derek Foreman
---
src/egl/drivers/dri2/platform_drm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/egl/drivers/dri2/platform_drm.c
b/src/egl/drivers/dri2/platform_drm.c
index 3391afc..c97c54f 100644
--- a/src/egl/drivers/dri2/platform_drm.c
+++ b/src/egl/drivers/dri2/platform_drm.c
@@ -632,7 +632,7 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
}
if (fd < 0) {
- fd = dup(gbm_device_get_fd(gbm));
+ fd = fcntl(gbm_device_get_fd(gbm), F_DUPFD_CLOEXEC, 3);
if (fd < 0) {
free(dri2_dpy);
return EGL_FALSE;
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 4/4] egl: Use the loader_open_device() helper to do open with CLOEXEC
We've moved the open with CLOEXEC idiom into a helper function, so
call it instead of duplicating the code.
This also replaces a couple of opens that didn't properly do CLOEXEC.
Signed-off-by: Derek Foreman
---
src/egl/drivers/dri2/platform_drm.c | 4 ++--
src/egl/drivers/dri2/platform_surfaceless.c | 11 +--
src/egl/drivers/dri2/platform_wayland.c | 11 +--
src/egl/drivers/dri2/platform_x11.c | 12 ++--
4 files changed, 6 insertions(+), 32 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_drm.c
b/src/egl/drivers/dri2/platform_drm.c
index c97c54f..a62da41 100644
--- a/src/egl/drivers/dri2/platform_drm.c
+++ b/src/egl/drivers/dri2/platform_drm.c
@@ -611,9 +611,9 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
char buf[64];
int n = snprintf(buf, sizeof(buf), DRM_DEV_NAME, DRM_DIR_NAME, 0);
if (n != -1 && n < sizeof(buf))
- fd = open(buf, O_RDWR);
+ fd = loader_open_device(buf);
if (fd < 0)
- fd = open("/dev/dri/card0", O_RDWR);
+ fd = loader_open_device("/dev/dri/card0");
dri2_dpy->own_device = 1;
gbm = gbm_create_device(fd);
if (gbm == NULL)
diff --git a/src/egl/drivers/dri2/platform_surfaceless.c
b/src/egl/drivers/dri2/platform_surfaceless.c
index 30cea36..48f15df 100644
--- a/src/egl/drivers/dri2/platform_surfaceless.c
+++ b/src/egl/drivers/dri2/platform_surfaceless.c
@@ -97,16 +97,7 @@ dri2_initialize_surfaceless(_EGLDriver *drv, _EGLDisplay
*disp)
if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base + i) <
0)
continue;
-#ifdef O_CLOEXEC
- dri2_dpy->fd = open(card_path, O_RDWR | O_CLOEXEC);
- if (dri2_dpy->fd < 0 && errno == EINVAL)
-#endif
- {
- dri2_dpy->fd = open(card_path, O_RDWR);
- if (dri2_dpy->fd >= 0)
-fcntl(dri2_dpy->fd, F_SETFD, fcntl(dri2_dpy->fd, F_GETFD) |
- FD_CLOEXEC);
- }
+ dri2_dpy->fd = loader_open_device(card_path);
free(card_path);
if (dri2_dpy->fd < 0)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index ea2f9f2..1c98552 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -891,16 +891,7 @@ drm_handle_device(void *data, struct wl_drm *drm, const
char *device)
if (!dri2_dpy->device_name)
return;
-#ifdef O_CLOEXEC
- dri2_dpy->fd = open(dri2_dpy->device_name, O_RDWR | O_CLOEXEC);
- if (dri2_dpy->fd == -1 && errno == EINVAL)
-#endif
- {
- dri2_dpy->fd = open(dri2_dpy->device_name, O_RDWR);
- if (dri2_dpy->fd != -1)
- fcntl(dri2_dpy->fd, F_SETFD, fcntl(dri2_dpy->fd, F_GETFD) |
-FD_CLOEXEC);
- }
+ dri2_dpy->fd = loader_open_device(dri2_dpy->device_name);
if (dri2_dpy->fd == -1) {
_eglLog(_EGL_WARNING, "wayland-egl: could not open %s (%s)",
dri2_dpy->device_name, strerror(errno));
diff --git a/src/egl/drivers/dri2/platform_x11.c
b/src/egl/drivers/dri2/platform_x11.c
index e0d0fdc..e010800 100644
--- a/src/egl/drivers/dri2/platform_x11.c
+++ b/src/egl/drivers/dri2/platform_x11.c
@@ -43,6 +43,7 @@
#include "egl_dri2.h"
#include "egl_dri2_fallbacks.h"
+#include "loader.h"
static EGLBoolean
dri2_x11_swap_interval(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf,
@@ -1230,16 +1231,7 @@ dri2_initialize_x11_dri2(_EGLDriver *drv, _EGLDisplay
*disp)
if (!dri2_load_driver(disp))
goto cleanup_conn;
-#ifdef O_CLOEXEC
- dri2_dpy->fd = open(dri2_dpy->device_name, O_RDWR | O_CLOEXEC);
- if (dri2_dpy->fd == -1 && errno == EINVAL)
-#endif
- {
- dri2_dpy->fd = open(dri2_dpy->device_name, O_RDWR);
- if (dri2_dpy->fd != -1)
- fcntl(dri2_dpy->fd, F_SETFD, fcntl(dri2_dpy->fd, F_GETFD) |
-FD_CLOEXEC);
- }
+ dri2_dpy->fd = loader_open_device(dri2_dpy->device_name);
if (dri2_dpy->fd == -1) {
_eglLog(_EGL_WARNING,
"DRI2: could not open %s (%s)", dri2_dpy->device_name,
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/4] CLOEXEC fix-ups
This series catches a couple of places where we forget to set CLOEXEC on file descriptors, and makes a helper function for the necessarily ugly way we have to open fds to make sure CLOEXEC is set. Derek Foreman (4): egl/drm: Duplicate fd with F_DUPFD_CLOEXEC to prevent leak loader: Rename drm_open_device() to loader_open_device() and share it glx: Use loader_open_device() helper egl: Use the loader_open_device() helper to do open with CLOEXEC src/egl/drivers/dri2/platform_drm.c | 6 +++--- src/egl/drivers/dri2/platform_surfaceless.c | 11 +-- src/egl/drivers/dri2/platform_wayland.c | 11 +-- src/egl/drivers/dri2/platform_x11.c | 12 ++-- src/glx/dri2_glx.c | 10 +- src/loader/loader.c | 6 +++--- src/loader/loader.h | 3 +++ 7 files changed, 14 insertions(+), 45 deletions(-) -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl/glx: Use helper function to open devices to ensure CLOEXEC is set
On 12/06/15 06:04 PM, Emil Velikov wrote: > On 12/06/15 19:07, Derek Foreman wrote: >> We do this right almost everywhere already, but the idiom is a little >> gruesome. This moves it into loader.c and fixes dri2_initialize_drm() >> which could open without CLOEXEC in some cases. >> >> Signed-off-by: Derek Foreman >> --- >> src/egl/drivers/dri2/platform_drm.c | 4 ++-- >> src/egl/drivers/dri2/platform_wayland.c | 11 +-- >> src/egl/drivers/dri2/platform_x11.c | 12 ++-- >> src/glx/dri2_glx.c | 10 +- >> src/loader/loader.c | 6 +++--- >> src/loader/loader.h | 3 +++ >> 6 files changed, 12 insertions(+), 34 deletions(-) > > Despite that it's on the trivial side, can we please split this into 3 > patches: > 1) s/static int drm_open_device/int loader_drm_open_device/ > 2) Use in libEGL (new EGL platform - "surfaceless" will land on Monday, > so better wait for that one to land as well) > 3) Use in libGL > > I'll throw in with similar patches for VL (for the video > targets/drivers) and pipe-loader (OpenCL). Sounds good, I've done that now - I'll wait til monday to handle platform_surfaceless and send the lot. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/drm: Try to use CLOEXEC for drm fds
On 12/06/15 05:52 PM, Emil Velikov wrote:
> On 12/06/15 19:05, Derek Foreman wrote:
>> On 12/06/15 11:29 AM, Emil Velikov wrote:
>>> Hi Derek,
>>
>> Hi, thanks for looking at this. :)
>>
> Props goes to Matt, for the reminder. It kind of fell of my radar.
>
>>> On 1 May 2015 at 18:34, Derek Foreman wrote:
>>>> These fds can propagate to child processes if we don't set CLOEXEC,
>>>> so make a best effort to do that.
>>>>
>>>> Signed-off-by: Derek Foreman
>>>> ---
>>>> Noticed this when fixing up similar problems in weston - weston's
>>>> drm fd gets dup()ed here and loses CLOEXEC and ends up in every child
>>>> process the shell launches.
>>>>
>>>> src/egl/drivers/dri2/platform_drm.c | 14 +++---
>>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/egl/drivers/dri2/platform_drm.c
>>>> b/src/egl/drivers/dri2/platform_drm.c
>>>> index 486b003..c326d6c 100644
>>>> --- a/src/egl/drivers/dri2/platform_drm.c
>>>> +++ b/src/egl/drivers/dri2/platform_drm.c
>>>> @@ -596,7 +596,11 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay
>>>> *disp)
>>>> struct dri2_egl_display *dri2_dpy;
>>>> struct gbm_device *gbm;
>>>> int fd = -1;
>>>> - int i;
>>>> + int i, flags = O_RDWR;
>>>> +
>>>> +#ifdef O_CLOEXEC
>>>> + flags |= O_CLOEXEC;
>>>> +#endif
>>>>
>>>> loader_set_logger(_eglLog);
>>>>
>>>> @@ -611,9 +615,9 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>>>>char buf[64];
>>>>int n = snprintf(buf, sizeof(buf), DRM_DEV_NAME, DRM_DIR_NAME, 0);
>>>>if (n != -1 && n < sizeof(buf))
>>>> - fd = open(buf, O_RDWR);
>>>> + fd = open(buf, flags);
>>>>if (fd < 0)
>>>> - fd = open("/dev/dri/card0", O_RDWR);
>>>> + fd = open("/dev/dri/card0", flags);
>>>>dri2_dpy->own_device = 1;
>>>>gbm = gbm_create_device(fd);
>>>>if (gbm == NULL)
>>>> @@ -639,6 +643,10 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay
>>>> *disp)
>>>>}
>>>> }
>>>>
>>>> + flags = fcntl(dri2_dpy->fd, F_GETFD);
>>>> + if (flags >= 0 && !(flags & FD_CLOEXEC))
>>>> + fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>>>> +
>>> In other places in mesa we explicitly check for EINVAL if open()
>>> fails, and use that as indication that O_CLOEXEC is not supported
>>> (note that we only use the O_RDWR and O_CLOEXEC flags). Curious which
>>> would be the better approach to use through mesa ?
>>
>> Oops - I think that way is better.
>>
>> There's a simple function in loader.c called drm_open_device() that does
>> the open that way, could we just rename that to loader_open_device() and
>> make it non-static and call it from dri2_initialize_drm()?
>>
> That's exactly what I was thinking actually :-)
>
>> That said...
>>
>> Unfortunately, it appears my previous testing was insufficient and
>> there's still a leaked fd - dup() creates fds without CLOEXEC set.
>>
> Seems that we ignore that in a few places through mesa - how about we
> replace dup() + fcntl(F_SETFD, fcntl(F_GETFD)|FD_CLOEXEC) with
> fcntl(F_DUPFD_CLOEXEC) ?
>
> Its requirements are almost the same as O_CLOEXEC (kernel 2.6.{23,24}
> and glibc 2.7) as opposed to dup2/dup3 which requires glibc 2.9 (not
> sure about the kernel).
I like it - do you want me to do that?
> Cheers,
> Emil
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] egl/drm: set CLOEXEC on fd after dup()
Prevents fd leak if the caller forks later. Signed-off-by: Derek Foreman --- src/egl/drivers/dri2/platform_drm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c index 3391afc..5c356c4 100644 --- a/src/egl/drivers/dri2/platform_drm.c +++ b/src/egl/drivers/dri2/platform_drm.c @@ -637,6 +637,7 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp) free(dri2_dpy); return EGL_FALSE; } + fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) | FD_CLOEXEC); } dri2_dpy->fd = fd; -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] egl/glx: Use helper function to open devices to ensure CLOEXEC is set
We do this right almost everywhere already, but the idiom is a little
gruesome. This moves it into loader.c and fixes dri2_initialize_drm()
which could open without CLOEXEC in some cases.
Signed-off-by: Derek Foreman
---
src/egl/drivers/dri2/platform_drm.c | 4 ++--
src/egl/drivers/dri2/platform_wayland.c | 11 +--
src/egl/drivers/dri2/platform_x11.c | 12 ++--
src/glx/dri2_glx.c | 10 +-
src/loader/loader.c | 6 +++---
src/loader/loader.h | 3 +++
6 files changed, 12 insertions(+), 34 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_drm.c
b/src/egl/drivers/dri2/platform_drm.c
index 5c356c4..bb6afee 100644
--- a/src/egl/drivers/dri2/platform_drm.c
+++ b/src/egl/drivers/dri2/platform_drm.c
@@ -611,9 +611,9 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
char buf[64];
int n = snprintf(buf, sizeof(buf), DRM_DEV_NAME, DRM_DIR_NAME, 0);
if (n != -1 && n < sizeof(buf))
- fd = open(buf, O_RDWR);
+ fd = loader_open_device(buf);
if (fd < 0)
- fd = open("/dev/dri/card0", O_RDWR);
+ fd = loader_open_device("/dev/dri/card0");
dri2_dpy->own_device = 1;
gbm = gbm_create_device(fd);
if (gbm == NULL)
diff --git a/src/egl/drivers/dri2/platform_wayland.c
b/src/egl/drivers/dri2/platform_wayland.c
index ea2f9f2..1c98552 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -891,16 +891,7 @@ drm_handle_device(void *data, struct wl_drm *drm, const
char *device)
if (!dri2_dpy->device_name)
return;
-#ifdef O_CLOEXEC
- dri2_dpy->fd = open(dri2_dpy->device_name, O_RDWR | O_CLOEXEC);
- if (dri2_dpy->fd == -1 && errno == EINVAL)
-#endif
- {
- dri2_dpy->fd = open(dri2_dpy->device_name, O_RDWR);
- if (dri2_dpy->fd != -1)
- fcntl(dri2_dpy->fd, F_SETFD, fcntl(dri2_dpy->fd, F_GETFD) |
-FD_CLOEXEC);
- }
+ dri2_dpy->fd = loader_open_device(dri2_dpy->device_name);
if (dri2_dpy->fd == -1) {
_eglLog(_EGL_WARNING, "wayland-egl: could not open %s (%s)",
dri2_dpy->device_name, strerror(errno));
diff --git a/src/egl/drivers/dri2/platform_x11.c
b/src/egl/drivers/dri2/platform_x11.c
index e0d0fdc..e010800 100644
--- a/src/egl/drivers/dri2/platform_x11.c
+++ b/src/egl/drivers/dri2/platform_x11.c
@@ -43,6 +43,7 @@
#include "egl_dri2.h"
#include "egl_dri2_fallbacks.h"
+#include "loader.h"
static EGLBoolean
dri2_x11_swap_interval(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf,
@@ -1230,16 +1231,7 @@ dri2_initialize_x11_dri2(_EGLDriver *drv, _EGLDisplay
*disp)
if (!dri2_load_driver(disp))
goto cleanup_conn;
-#ifdef O_CLOEXEC
- dri2_dpy->fd = open(dri2_dpy->device_name, O_RDWR | O_CLOEXEC);
- if (dri2_dpy->fd == -1 && errno == EINVAL)
-#endif
- {
- dri2_dpy->fd = open(dri2_dpy->device_name, O_RDWR);
- if (dri2_dpy->fd != -1)
- fcntl(dri2_dpy->fd, F_SETFD, fcntl(dri2_dpy->fd, F_GETFD) |
-FD_CLOEXEC);
- }
+ dri2_dpy->fd = loader_open_device(dri2_dpy->device_name);
if (dri2_dpy->fd == -1) {
_eglLog(_EGL_WARNING,
"DRI2: could not open %s (%s)", dri2_dpy->device_name,
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 538cf1a..27ea952 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -1183,15 +1183,7 @@ dri2CreateScreen(int screen, struct glx_display * priv)
return NULL;
}
-#ifdef O_CLOEXEC
- psc->fd = open(deviceName, O_RDWR | O_CLOEXEC);
- if (psc->fd == -1 && errno == EINVAL)
-#endif
- {
- psc->fd = open(deviceName, O_RDWR);
- if (psc->fd != -1)
- fcntl(psc->fd, F_SETFD, fcntl(psc->fd, F_GETFD) | FD_CLOEXEC);
- }
+ psc->fd = loader_open_device(deviceName);
if (psc->fd < 0) {
ErrorMessageF("failed to open drm device: %s\n", strerror(errno));
goto handle_error;
diff --git a/src/loader/loader.c b/src/loader/loader.c
index 17bf133..fc46815 100644
--- a/src/loader/loader.c
+++ b/src/loader/loader.c
@@ -314,8 +314,8 @@ get_id_path_tag_from_fd(struct udev *udev, int fd)
return id_path_tag;
}
-static int
-drm_open_device(const char *device_name)
+int
+loader_open_device(const char *device_name)
{
int fd;
#ifdef O_CLOEXEC
@@ -404,7 +404,7 @@ int loader_get_user_preferred_fd(int default_fd, int
*different_device)
goto default_device_clean;
}
- fd = drm_open_device(device_name);
+ fd = loader_open_device(device_name);
if (fd >= 0) {
close(default_fd);
} else {
diff --git a/src/loader/loader.h b/src/loader/loader.h
index 60c58f2..055dc78 100644
--- a/src/loader/loader.h
+++
Re: [Mesa-dev] [PATCH] egl/drm: Try to use CLOEXEC for drm fds
On 12/06/15 11:29 AM, Emil Velikov wrote:
> Hi Derek,
Hi, thanks for looking at this. :)
> On 1 May 2015 at 18:34, Derek Foreman wrote:
>> These fds can propagate to child processes if we don't set CLOEXEC,
>> so make a best effort to do that.
>>
>> Signed-off-by: Derek Foreman
>> ---
>> Noticed this when fixing up similar problems in weston - weston's
>> drm fd gets dup()ed here and loses CLOEXEC and ends up in every child
>> process the shell launches.
>>
>> src/egl/drivers/dri2/platform_drm.c | 14 +++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/platform_drm.c
>> b/src/egl/drivers/dri2/platform_drm.c
>> index 486b003..c326d6c 100644
>> --- a/src/egl/drivers/dri2/platform_drm.c
>> +++ b/src/egl/drivers/dri2/platform_drm.c
>> @@ -596,7 +596,11 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>> struct dri2_egl_display *dri2_dpy;
>> struct gbm_device *gbm;
>> int fd = -1;
>> - int i;
>> + int i, flags = O_RDWR;
>> +
>> +#ifdef O_CLOEXEC
>> + flags |= O_CLOEXEC;
>> +#endif
>>
>> loader_set_logger(_eglLog);
>>
>> @@ -611,9 +615,9 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>>char buf[64];
>>int n = snprintf(buf, sizeof(buf), DRM_DEV_NAME, DRM_DIR_NAME, 0);
>>if (n != -1 && n < sizeof(buf))
>> - fd = open(buf, O_RDWR);
>> + fd = open(buf, flags);
>>if (fd < 0)
>> - fd = open("/dev/dri/card0", O_RDWR);
>> + fd = open("/dev/dri/card0", flags);
>>dri2_dpy->own_device = 1;
>>gbm = gbm_create_device(fd);
>>if (gbm == NULL)
>> @@ -639,6 +643,10 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>>}
>> }
>>
>> + flags = fcntl(dri2_dpy->fd, F_GETFD);
>> + if (flags >= 0 && !(flags & FD_CLOEXEC))
>> + fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>> +
> In other places in mesa we explicitly check for EINVAL if open()
> fails, and use that as indication that O_CLOEXEC is not supported
> (note that we only use the O_RDWR and O_CLOEXEC flags). Curious which
> would be the better approach to use through mesa ?
Oops - I think that way is better.
There's a simple function in loader.c called drm_open_device() that does
the open that way, could we just rename that to loader_open_device() and
make it non-static and call it from dri2_initialize_drm()?
That said...
Unfortunately, it appears my previous testing was insufficient and
there's still a leaked fd - dup() creates fds without CLOEXEC set.
I've sent another couple of patches to this effect...
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] egl/drm: Try to use CLOEXEC for drm fds
These fds can propagate to child processes if we don't set CLOEXEC,
so make a best effort to do that.
Signed-off-by: Derek Foreman
---
Noticed this when fixing up similar problems in weston - weston's
drm fd gets dup()ed here and loses CLOEXEC and ends up in every child
process the shell launches.
src/egl/drivers/dri2/platform_drm.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_drm.c
b/src/egl/drivers/dri2/platform_drm.c
index 486b003..c326d6c 100644
--- a/src/egl/drivers/dri2/platform_drm.c
+++ b/src/egl/drivers/dri2/platform_drm.c
@@ -596,7 +596,11 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
struct dri2_egl_display *dri2_dpy;
struct gbm_device *gbm;
int fd = -1;
- int i;
+ int i, flags = O_RDWR;
+
+#ifdef O_CLOEXEC
+ flags |= O_CLOEXEC;
+#endif
loader_set_logger(_eglLog);
@@ -611,9 +615,9 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
char buf[64];
int n = snprintf(buf, sizeof(buf), DRM_DEV_NAME, DRM_DIR_NAME, 0);
if (n != -1 && n < sizeof(buf))
- fd = open(buf, O_RDWR);
+ fd = open(buf, flags);
if (fd < 0)
- fd = open("/dev/dri/card0", O_RDWR);
+ fd = open("/dev/dri/card0", flags);
dri2_dpy->own_device = 1;
gbm = gbm_create_device(fd);
if (gbm == NULL)
@@ -639,6 +643,10 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
}
}
+ flags = fcntl(dri2_dpy->fd, F_GETFD);
+ if (flags >= 0 && !(flags & FD_CLOEXEC))
+ fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
+
dri2_dpy->fd = fd;
dri2_dpy->device_name = loader_get_device_name_for_fd(dri2_dpy->fd);
dri2_dpy->driver_name = strdup(dri2_dpy->gbm_dri->base.driver_name);
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
