[Mesa-dev] [PATCH kmscube] Search for a suitable config

[Emil Velikov]

On Wed, 3 Jul 2019 at 08:16, Drew DeVault <sir at 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