[kmscube] Search for a suitable config

Submitted by Drew DeVault on July 2, 2019, 11:57 p.m.

Details

Message ID 20190702235735.11801-1-sir@cmpwn.com
State New
Headers show
Series "Search for a suitable config" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Drew DeVault July 2, 2019, 11:57 p.m.
Instead of assuming the first will be suitable. kmscube fails to start
for me without this change.
---
 common.c | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
 common.h |  1 +
 2 files changed, 46 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/common.c b/common.c
index f9bd280..45c074d 100644
--- a/common.c
+++ b/common.c
@@ -24,11 +24,47 @@ 
 #include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
+#include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
 
 #include "common.h"
 
+static bool egl_get_config(EGLDisplay disp, EGLint *attribs,
+		EGLConfig *out, EGLint visual_id) {
+	EGLint count = 0, matched = 0, ret;
+
+	ret = eglGetConfigs(disp, NULL, 0, &count);
+	if (ret == EGL_FALSE || count == 0) {
+		printf("eglGetConfigs returned no configs\n");
+		return false;
+	}
+
+	EGLConfig configs[count];
+
+	ret = eglChooseConfig(disp, attribs, configs, count, &matched);
+	if (ret == EGL_FALSE) {
+		printf("eglChooseConfig failed\n");
+		return false;
+	}
+
+	for (int i = 0; i < matched; ++i) {
+		EGLint visual;
+		if (!eglGetConfigAttrib(disp, configs[i],
+				EGL_NATIVE_VISUAL_ID, &visual)) {
+			continue;
+		}
+
+		if (!visual_id || visual == visual_id) {
+			*out = configs[i];
+			return true;
+		}
+	}
+
+	printf("no valid egl config found\n");
+	return false;
+}
+
 struct gbm * init_gbm(int drm_fd, int w, int h)
 {
         struct gbm *gbm = calloc(1, sizeof (struct gbm));
@@ -59,7 +95,7 @@  int init_egl(struct egl *egl, const struct gbm *gbm)
 		EGL_NONE
 	};
 
-	static const EGLint config_attribs[] = {
+	static EGLint config_attribs[] = {
 		EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
 		EGL_RED_SIZE, 1,
 		EGL_GREEN_SIZE, 1,
@@ -81,9 +117,10 @@  int init_egl(struct egl *egl, const struct gbm *gbm)
 	get_proc(eglDestroySyncKHR);
 	get_proc(eglWaitSyncKHR);
 	get_proc(eglDupNativeFenceFDANDROID);
+	get_proc(eglCreatePlatformWindowSurfaceEXT);
 
 	if (egl->eglGetPlatformDisplayEXT) {
-		egl->display = egl->eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_KHR,
+		egl->display = egl->eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_MESA,
 				gbm->dev, NULL);
 	} else {
 		egl->display = eglGetDisplay((void *)gbm->dev);
@@ -106,8 +143,9 @@  int init_egl(struct egl *egl, const struct gbm *gbm)
 		return -1;
 	}
 
-	if (!eglChooseConfig(egl->display, config_attribs, &egl->config, 1, &n) || n != 1) {
-		printf("failed to choose config: %d\n", n);
+	if (!egl_get_config(egl->display, config_attribs,
+				&egl->config, GBM_FORMAT_XRGB8888)) {
+		printf("Failed to get EGL config\n");
 		return -1;
 	}
 
@@ -118,10 +156,10 @@  int init_egl(struct egl *egl, const struct gbm *gbm)
 		return -1;
 	}
 
-	egl->surface = eglCreateWindowSurface(egl->display, egl->config,
-			(EGLNativeWindowType)gbm->surface, NULL);
+	egl->surface = egl->eglCreatePlatformWindowSurfaceEXT(
+			egl->display, egl->config, gbm->surface, NULL);
 	if (egl->surface == EGL_NO_SURFACE) {
-		printf("failed to create egl surface\n");
+		printf("failed to create egl surface: %d\n", eglGetError());
 		return -1;
 	}
 
diff --git a/common.h b/common.h
index 1ddf04b..1675f98 100644
--- a/common.h
+++ b/common.h
@@ -69,6 +69,7 @@  struct egl {
 	EGLSurface surface;
 
 	PFNEGLGETPLATFORMDISPLAYEXTPROC eglGetPlatformDisplayEXT;
+	PFNEGLCREATEPLATFORMWINDOWSURFACEEXTPROC eglCreatePlatformWindowSurfaceEXT;
 	PFNEGLCREATEIMAGEKHRPROC eglCreateImageKHR;
 	PFNEGLDESTROYIMAGEKHRPROC eglDestroyImageKHR;
 	PFNGLEGLIMAGETARGETTEXTURE2DOESPROC glEGLImageTargetTexture2DOES;

Comments

Hi Drew,

On Wed, 3 Jul 2019 at 08:16, Drew DeVault <sir@cmpwn.com> wrote:
> Instead of assuming the first will be suitable. kmscube fails to start
> for me without this change.

There are a couple of unrelated changes combined in here, but I think
the core one is good.

eglChooseConfig has some really useful properties, where it strictly
specifies a sort order, does not include EGL_NATIVE_VISUAL_ID in that
sort order, and silently accepts attributes which are not relevant for
the sort. Your change which takes all possible configs and then
queries them to make sure that the native visual ID is actually
correct is the right thing to do.

Cheers,
Daniel
On Wed, 3 Jul 2019 at 08:16, Drew DeVault <sir@cmpwn.com> wrote:
>
> Instead of assuming the first will be suitable. kmscube fails to start
> for me without this change.

Yes please. Picking the first one is rarely the correct thing to do.
Especially when we have platform specific attributes which are exempt
from the sorting rules.

> ---
>  common.c | 52 +++++++++++++++++++++++++++++++++++++++++++++-------
>  common.h |  1 +
>  2 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/common.c b/common.c
> index f9bd280..45c074d 100644
> --- a/common.c
> +++ b/common.c
> @@ -24,11 +24,47 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <stdio.h>
> +#include <stdbool.h>
>  #include <stdlib.h>
>  #include <string.h>
>
>  #include "common.h"
>
> +static bool egl_get_config(EGLDisplay disp, EGLint *attribs,
Nit: "const EGLint *attribs" and keep the const notation in the parent function.

> +               EGLConfig *out, EGLint visual_id) {
Nit: { on the next line

> +       EGLint count = 0, matched = 0, ret;
> +
> +       ret = eglGetConfigs(disp, NULL, 0, &count);
> +       if (ret == EGL_FALSE || count == 0) {
> +               printf("eglGetConfigs returned no configs\n");
> +               return false;
> +       }
> +
> +       EGLConfig configs[count];
> +
VLAs are convenient, although they lead to messy binary, are not
supported on some platforms etc.
How about:

EGLConfig configs[128];
assert(count...)

> +       ret = eglChooseConfig(disp, attribs, configs, count, &matched);
> +       if (ret == EGL_FALSE) {
> +               printf("eglChooseConfig failed\n");
> +               return false;
> +       }
> +
> +       for (int i = 0; i < matched; ++i) {
> +               EGLint visual;
> +               if (!eglGetConfigAttrib(disp, configs[i],
> +                               EGL_NATIVE_VISUAL_ID, &visual)) {
> +                       continue;
> +               }
> +
> +               if (!visual_id || visual == visual_id) {
> +                       *out = configs[i];
> +                       return true;
> +               }
> +       }
> +
> +       printf("no valid egl config found\n");
> +       return false;
> +}
> +
>  struct gbm * init_gbm(int drm_fd, int w, int h)
>  {
>          struct gbm *gbm = calloc(1, sizeof (struct gbm));
> @@ -59,7 +95,7 @@ int init_egl(struct egl *egl, const struct gbm *gbm)
>                 EGL_NONE
>         };
>
> -       static const EGLint config_attribs[] = {
> +       static EGLint config_attribs[] = {
Nit: Keep the const.

>                 EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
>                 EGL_RED_SIZE, 1,
>                 EGL_GREEN_SIZE, 1,
> @@ -81,9 +117,10 @@ int init_egl(struct egl *egl, const struct gbm *gbm)
>         get_proc(eglDestroySyncKHR);
>         get_proc(eglWaitSyncKHR);
>         get_proc(eglDupNativeFenceFDANDROID);
> +       get_proc(eglCreatePlatformWindowSurfaceEXT);
Move this and associated changes to a separate commit, preserving the
fallback to eglCreateWindowSurface().

>
>         if (egl->eglGetPlatformDisplayEXT) {
> -               egl->display = egl->eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_KHR,
> +               egl->display = egl->eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_MESA,
Out of curiosity: Both constants are numerically the same. Why the change?

>                                 gbm->dev, NULL);
>         } else {
>                 egl->display = eglGetDisplay((void *)gbm->dev);
> @@ -106,8 +143,9 @@ int init_egl(struct egl *egl, const struct gbm *gbm)
>                 return -1;
>         }
>
> -       if (!eglChooseConfig(egl->display, config_attribs, &egl->config, 1, &n) || n != 1) {
> -               printf("failed to choose config: %d\n", n);
> +       if (!egl_get_config(egl->display, config_attribs,
> +                               &egl->config, GBM_FORMAT_XRGB8888)) {
Let's use gbm.format instead of open-coding it.

Thanks
Emil