Re: [Mesa-dev] [PATCH] egl: Fix for not setting EGL_BAD_ALLOC on try to create multiple window surfaces on single window v2.

[Daniel Stone]

Hi Adrian,
Thanks for the patch! Some comments inline.

On 22 March 2017 at 13:40, Adrian Pielech  wrote:
> +struct list_head window_list = {NULL, NULL};
> +
> +/* Checks if a EGLWindow already have a created surface */
> +static inline bool
> +is_window_associated_with_surface(EGLNativeWindowType window)
> +{
> +   list_for_each_entry(struct window_list_item, item, _list, link) {
> +  if (item->native_window = window) {

This must be changed to == rather than =. Right now it will always
return a false positive.

> + return true;
> +  }
> +   }
> +
> +   return false;
> +}

I also can't see any locking associated with these (mutex/spinlock),
so this would in fact fail when multiple threads are
creating/destroying surfaces/displays simultaneously. Can you please
add some locking so that concurrent access succeeds?

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

Re: [Mesa-dev] [PATCH] egl: Fix for not setting EGL_BAD_ALLOC on try to create multiple window surfaces on single window v2.

[Emil Velikov]

Hi Adrian,

Welcome to Mesa !

On 22 March 2017 at 13:40, Adrian Pielech  wrote:
> According to the EGL 1.5 spec, section 3.5.1, page 34,35
> eglCreateWindowSurface should return EGL_BAD_ALLOC if there is already
> surface for given window. Similarly it is written in EGL 1.4 spec,
> section 3.5.1, page 29.
>
> I apologize for small mistakes on earlier message...
>
... and there's no need to apologize ;-)

In case Eric's reply comes a bit overwhelming:

There's nothing fundamentally wrong with your work, it just doesn't
follow the surrounding coding style/conventions.
Please give it a bit of polish for v2.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl: Fix for not setting EGL_BAD_ALLOC on try to create multiple window surfaces on single window v2.

[Eric Engestrom]

Hi Adrian,

Thanks for this patch!

I have only had a quick look at the logic itself, but it looks correct.
Inline are a few comments to address before we can merge this patch.

I also recommend reading our guidelines on submitting patches:
https://mesa3d.org/submittingpatches.html


> Subject: [Mesa-dev] [PATCH] egl: Fix for not setting EGL_BAD_ALLOC on try
>  to create multiple window surfaces on single window v2.

That's a very long title; we usually try to keep it at around 50-60 chars,
but it's not a hard limit (going over is ok).

This title could easily be shorter though, eg.:
  egl: return an error for existing surfaces in eglCreateWindowSurface


On Wednesday, 2017-03-22 14:40:40 +0100, Adrian Pielech wrote:
> According to the EGL 1.5 spec, section 3.5.1, page 34,35
> eglCreateWindowSurface should return EGL_BAD_ALLOC if there is already
> surface for given window. Similarly it is written in EGL 1.4 spec,
> section 3.5.1, page 29.

Is there a Piglit or dEQP test for this? I'm thinking of the dEQP group
dEQP-EGL.functional.negative_api.* for instance, but there might be
other tests or test suites that cover this.

If you find a test, make sure to run it, and mention it in the commit.
If there are no test that cover this case, it would be great if you
could add one, but it's of course not mandatory :)


> 
> I apologize for small mistakes on earlier message...

Not need to apologize, we all make typos ;)
If you want to add a note like this on a patch, you can add a line
containing 3 dashes (`---`), and any line after that will only appear in
the mail, not the commit. Make sure your signed-off-by line comes before
that though :)


> 
> Signed-off-by: Adrian Pielech 
> ---
>  src/egl/main/eglapi.c | 90 
> +--
>  1 file changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index 5694b5a..5736825 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -90,6 +90,7 @@
>  #include "c11/threads.h"
>  #include "GL/mesa_glinterop.h"
>  #include "eglcompiler.h"
> +#include "util/list.h"
>  
>  #include "eglglobals.h"
>  #include "eglcontext.h"
> @@ -102,6 +103,65 @@
>  #include "eglimage.h"
>  #include "eglsync.h"
>  

At the top of this file is an explanation of the naming convention.
Could you rename you structs and functions to follow this?

> +struct window_list_item

This one for example just needs an `_egl_` prefix:
  struct _egl_window_list_item

Could you also move this hunk (your struct and helper functions) after
the macro block?

Between `struct _egl_entrypoint` and the existing helper functions seems
like a good place, keeping the structs grouped and your helper functions
close to their struct.


> +{
> +struct list_head link;
> +
> +EGLNativeWindowType native_window;
> +EGLSurface attached_surface;
> +EGLDisplay attached_display;
> +};
> +
> +struct list_head window_list = {NULL, NULL};

As you may have noticed, there are no global in this file (the
entrypoint list in eglGetProcAddress being the only exception that I can
think of), everything is attached to something.
In this case, the surface is attached to a native windows.
I'm not sure if there's a way to do this without a global here, but if
you can find a way, it would probably be cleaner.
(If you can't find a better way, you can keep this in your next
revision; don't let that stop you. I couldn't find one just now.)


> +
> +/* Checks if a EGLWindow already have a created surface */
> +static inline bool
> +is_window_associated_with_surface(EGLNativeWindowType window)

As mentioned above, the name of the function should follow the naming
convention. For this function, that would be:
  _eglIsWindowAssociatedWithSurface


> +{
> +   list_for_each_entry(struct window_list_item, item, _list, link) {
> +  if (item->native_window = window) {

`==` ;)


> + return true;
> +  }
> +   }
> +
> +   return false;
> +}
> +
> +static inline void
> +associate_window_with_surface_list(EGLNativeWindowType window,
> +   EGLSurface surface,
> +   EGLDisplay display)
> +{
> +   struct window_list_item *item = malloc(sizeof(struct window_list_item));
> +   assert(item);

Asserts have their uses, but error-handling isn't one of them.
This function should return a bool, and the callers should handle this
failure.


> +
> +   item->native_window = window;
> +   item->attached_surface = surface;
> +   item->attached_display = display;
> +
> +   list_add(>link, _list);
> +}
> +
> +static inline void
> +disassociate_window_with_surface_list(EGLSurface surface)
> +{
> +   list_for_each_entry(struct window_list_item, item, _list, link) {
> +  if (item->attached_surface == surface) {
> + list_del(>link);
> + free(item);
> + break;
> +  }
> +   }
> +}
> +
> +static inline void
> 

[Mesa-dev] [PATCH] egl: Fix for not setting EGL_BAD_ALLOC on try to create multiple window surfaces on single window v2.

[Adrian Pielech]

According to the EGL 1.5 spec, section 3.5.1, page 34,35
eglCreateWindowSurface should return EGL_BAD_ALLOC if there is already
surface for given window. Similarly it is written in EGL 1.4 spec,
section 3.5.1, page 29.

I apologize for small mistakes on earlier message...

Signed-off-by: Adrian Pielech 
---
 src/egl/main/eglapi.c | 90 +--
 1 file changed, 88 insertions(+), 2 deletions(-)

diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index 5694b5a..5736825 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -90,6 +90,7 @@
 #include "c11/threads.h"
 #include "GL/mesa_glinterop.h"
 #include "eglcompiler.h"
+#include "util/list.h"
 
 #include "eglglobals.h"
 #include "eglcontext.h"
@@ -102,6 +103,65 @@
 #include "eglimage.h"
 #include "eglsync.h"
 
+struct window_list_item
+{
+struct list_head link;
+
+EGLNativeWindowType native_window;
+EGLSurface attached_surface;
+EGLDisplay attached_display;
+};
+
+struct list_head window_list = {NULL, NULL};
+
+/* Checks if a EGLWindow already have a created surface */
+static inline bool
+is_window_associated_with_surface(EGLNativeWindowType window)
+{
+   list_for_each_entry(struct window_list_item, item, _list, link) {
+  if (item->native_window = window) {
+ return true;
+  }
+   }
+
+   return false;
+}
+
+static inline void
+associate_window_with_surface_list(EGLNativeWindowType window,
+   EGLSurface surface,
+   EGLDisplay display)
+{
+   struct window_list_item *item = malloc(sizeof(struct window_list_item));
+   assert(item);
+
+   item->native_window = window;
+   item->attached_surface = surface;
+   item->attached_display = display;
+
+   list_add(>link, _list);
+}
+
+static inline void
+disassociate_window_with_surface_list(EGLSurface surface)
+{
+   list_for_each_entry(struct window_list_item, item, _list, link) {
+  if (item->attached_surface == surface) {
+ list_del(>link);
+ free(item);
+ break;
+  }
+   }
+}
+
+static inline void
+disassociate_all_window_surface_links_by_display(EGLDisplay display)
+{
+   list_for_each_entry_safe(struct window_list_item, item, _list, link) 
{
+  list_del(>link);
+  free(item);
+   }
+}
 
 /**
  * Macros to help return an API entrypoint.
@@ -623,6 +683,10 @@ eglInitialize(EGLDisplay dpy, EGLint *major, EGLint *minor)
   *minor = disp->Version % 10;
}
 
+   if (!window_list.next || !window_list.prev) {
+  list_inithead(_list);
+   }
+
RETURN_EGL_SUCCESS(disp, EGL_TRUE);
 }
 
@@ -640,6 +704,13 @@ eglTerminate(EGLDisplay dpy)
if (disp->Initialized) {
   _EGLDriver *drv = disp->Driver;
 
+  /*
+   * In corner case, user could call eglTerminate without calling
+   * eglDestroySurface, so it is wise clean up list entries and free memory
+   * after his/her activities.
+   */
+  disassociate_all_window_surface_links_by_display(dpy);
+
   drv->API.Terminate(drv, disp);
   /* do not reset disp->Driver */
   disp->ClientAPIsString[0] = 0;
@@ -897,11 +968,24 @@ eglCreateWindowSurface(EGLDisplay dpy, EGLConfig config,
EGLNativeWindowType window, const EGLint *attrib_list)
 {
_EGLDisplay *disp = _eglLockDisplay(dpy);
+   EGLSurface window_surface = NULL;
 
_EGL_FUNC_START(disp, EGL_OBJECT_DISPLAY_KHR, NULL, EGL_NO_SURFACE);
STATIC_ASSERT(sizeof(void*) == sizeof(window));
-   return _eglCreateWindowSurfaceCommon(disp, config, (void*) window,
-attrib_list);
+
+   /* According to the EGL 1.5 spec, section 3.5.1, page 34,35
+* eglCreateWindowSurface should return EGL_BAD_ALLOC if there is already
+* created a surface for given window.
+*/
+   if (is_window_associated_with_surface(window))
+  RETURN_EGL_ERROR(disp, EGL_BAD_ALLOC, EGL_NO_SURFACE);
+
+   window_surface = _eglCreateWindowSurfaceCommon(disp, config, (void*) window,
+  attrib_list);
+   if (window_surface)
+  associate_window_with_surface_list(window, window_surface, dpy);
+
+   return window_surface;
 }
 
 static void *
@@ -1099,6 +1183,8 @@ eglDestroySurface(EGLDisplay dpy, EGLSurface surface)
_eglUnlinkSurface(surf);
ret = drv->API.DestroySurface(drv, disp, surf);
 
+   disassociate_window_with_surface_list(surface);
+
RETURN_EGL_EVAL(disp, ret);
 }
 
-- 
2.7.4



Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial 
Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | 
Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i 
moze zawierac informacje poufne. W razie przypadkowego otrzymania tej 
wiadomosci, prosimy o powiadomienie nadawcy oraz trwale