[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 <adrian.pielech at intel.com>
> ---
>  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, &window_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(&item->link, &window_list);
> +}
> +
> +static inline void
> +disassociate_window_with_surface_list(EGLSurface surface)
> +{
> +   list_for_each_entry(struct window_list_item, item, &window_list, link) {
> +      if (item->attached_surface == surface) {
> +         list_del(&item->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, &window_list, link) {
> +      list_del(&item->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(&window_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.

"their" is the gender-neutral pronoun, but you can drop that last line.
This also isn't a corner case, so you can just say:

  User can call eglTerminate() without calling eglDestroySurface(), so
  let's clean up the list entries.


> +       */
> +      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,

The spec you quoted covers eglCreatePlatformWindowSurface(). As the
behaviour of eglCreateWindowSurface() is meant to be identical (more or
less, see first paragraph of page 35), this isn't wrong, but it isn't
what the commit message says.

I would suggest moving that logic to _eglCreateWindowSurfaceCommon(), so
that all the eglCreate*WindowSurface*() functions use it, making their
behaviour identical in this respect.


>                         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 jej usuniecie; jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.
> 

IANAL, but I'm pretty sure we can't take code that contains such
a notice. Can you please make sure this doesn't get added next time you
send a patch?

Cheers,
  Eric