[PATCH weston 1/2] gl-renderer: Take a list of acceptable formats in create functions
Derek Foreman
derekf at osg.samsung.com
Fri May 15 09:57:16 PDT 2015
On 15/05/15 02:18 AM, Pekka Paalanen wrote:
> On Thu, 14 May 2015 20:14:53 -0700
> Bryce Harrington <bryce at osg.samsung.com> wrote:
>
>> On Mon, May 11, 2015 at 02:19:02PM -0500, Derek Foreman wrote:
>>> Currently we pass either a single format or no formats to the gl renderer
>>> create and output_create functions. We extend this to any number of
>>> formats so we can allow fallback formats if we don't get our first pick.
>>>
>>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>>
>> This all looks fine, but I have some bikesheddy comments. I'll give a
>> R-B and you can decide whether or not my suggestions have any utility.
>>
>> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
>
> Hi,
>
> I agree with Bryce's comments below, and also agree that this patch is
> also landable as is.
>
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> I have made three further notes below.
>
>>> ---
>>> src/compositor-drm.c | 5 ++--
>>> src/compositor-fbdev.c | 4 +--
>>> src/compositor-wayland.c | 12 +++++----
>>> src/compositor-x11.c | 5 ++--
>>> src/gl-renderer.c | 68 +++++++++++++++++++++++++++++++-----------------
>>> src/gl-renderer.h | 6 +++--
>>> 6 files changed, 63 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
>>> index 0cdb8f4..69bdcfd 100644
>>> --- a/src/compositor-drm.c
>>> +++ b/src/compositor-drm.c
>>> @@ -1398,7 +1398,7 @@ drm_compositor_create_gl_renderer(struct drm_compositor *ec)
>>>
>>> format = ec->format;
>>> if (gl_renderer->create(&ec->base, EGL_PLATFORM_GBM_KHR, (void *) ec->gbm,
>>> - gl_renderer->opaque_attribs, &format) < 0) {
>>> + gl_renderer->opaque_attribs, &format, 1) < 0) {
>>> return -1;
>>> }
>>>
>>> @@ -1620,7 +1620,8 @@ drm_output_init_egl(struct drm_output *output, struct drm_compositor *ec)
>>> (EGLNativeDisplayType)output->surface,
>>> output->surface,
>>> gl_renderer->opaque_attribs,
>>> - &format) < 0) {
>>> + &format,
>>> + 1) < 0) {
>>> weston_log("failed to create gl renderer output state\n");
>>> gbm_surface_destroy(output->surface);
>>> return -1;
>>> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
>>> index 7c505ce..3f3394f 100644
>>> --- a/src/compositor-fbdev.c
>>> +++ b/src/compositor-fbdev.c
>>> @@ -570,7 +570,7 @@ fbdev_output_create(struct fbdev_compositor *compositor,
>>> if (gl_renderer->output_create(&output->base,
>>> (EGLNativeWindowType)NULL, NULL,
>>> gl_renderer->opaque_attribs,
>>> - NULL) < 0) {
>>> + NULL, 0) < 0) {
>>> weston_log("gl_renderer_output_create failed.\n");
>>> goto out_shadow_surface;
>>> }
>>> @@ -871,7 +871,7 @@ fbdev_compositor_create(struct wl_display *display, int *argc, char *argv[],
>>> if (gl_renderer->create(&compositor->base, NO_EGL_PLATFORM,
>>> EGL_DEFAULT_DISPLAY,
>>> gl_renderer->opaque_attribs,
>>> - NULL) < 0) {
>>> + NULL, 0) < 0) {
>>> weston_log("gl_renderer_create failed.\n");
>>> goto out_launcher;
>>> }
>>> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
>>> index 303151c..c9983e0 100644
>>> --- a/src/compositor-wayland.c
>>> +++ b/src/compositor-wayland.c
>>> @@ -648,7 +648,8 @@ wayland_output_init_gl_renderer(struct wayland_output *output)
>>> output->gl.egl_window,
>>> output->gl.egl_window,
>>> gl_renderer->alpha_attribs,
>>> - NULL) < 0)
>>> + NULL,
>>> + 0) < 0)
>>> goto cleanup_window;
>>>
>>> return 0;
>>> @@ -1970,10 +1971,11 @@ wayland_compositor_create(struct wl_display *display, int use_pixman,
>>>
>>> if (!c->use_pixman) {
>>> if (gl_renderer->create(&c->base,
>>> - EGL_PLATFORM_WAYLAND_KHR,
>>> - c->parent.wl_display,
>>> - gl_renderer->alpha_attribs,
>>> - NULL) < 0) {
>>> + EGL_PLATFORM_WAYLAND_KHR,
>>> + c->parent.wl_display,
>>> + gl_renderer->alpha_attribs,
>>> + NULL,
>>> + 0) < 0) {
>>> weston_log("Failed to initialize the GL renderer; "
>>> "falling back to pixman.\n");
>>> c->use_pixman = 1;
>>> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
>>> index 5129e85..3565677 100644
>>> --- a/src/compositor-x11.c
>>> +++ b/src/compositor-x11.c
>>> @@ -906,7 +906,8 @@ x11_compositor_create_output(struct x11_compositor *c, int x, int y,
>>> (EGLNativeWindowType) output->window,
>>> &xid,
>>> gl_renderer->opaque_attribs,
>>> - NULL);
>>> + NULL,
>>> + 0);
>>> if (ret < 0)
>>> return NULL;
>>> }
>>> @@ -1493,7 +1494,7 @@ init_gl_renderer(struct x11_compositor *c)
>>> return -1;
>>>
>>> ret = gl_renderer->create(&c->base, EGL_PLATFORM_X11_KHR, (void *) c->dpy,
>>> - gl_renderer->opaque_attribs, NULL);
>>> + gl_renderer->opaque_attribs, NULL, 0);
>>>
>>> return ret;
>>> }
>>> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
>>> index ae3122f..6746a9b 100644
>>> --- a/src/gl-renderer.c
>>> +++ b/src/gl-renderer.c
>>> @@ -1898,14 +1898,37 @@ log_egl_config_info(EGLDisplay egldpy, EGLConfig eglconfig)
>>> }
>>>
>>> static int
>>> +match_config(EGLDisplay egl_display,
>>
>> This function name seems a bit non-descript.
>> match_config_to_visual() maybe? Or match_visual_config()?
Sure, follow up will call it match_config_to_visual()
>>> + EGLint visual_id,
>>> + EGLConfig *configs,
>>> + int count)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < count; ++i) {
>>> + EGLint id;
>>> +
>>> + if (!eglGetConfigAttrib(egl_display,
>>> + configs[i], EGL_NATIVE_VISUAL_ID,
>>> + &id))
>>> + continue;
>>
>> I gather eglGetConfigAttrib logs an error code somewhere when it fails?
>
> It doesn't, but it's also not important. If you can't query the visual
> of a config, that config is likely useless anyway. Or the whole EGL
> implementation is broken, or whatever. We don't care, because if it
> really turns out to be a problem, we will fail hard elsewhere anyway
> and this is only collateral fallout.
>
>>> + if (id == visual_id)
>>> + return i;
>>> + }
>>> +
>>
>> For future debugging purposes, might be friendly to log an error here.
Agreed, will do.
>>> + return -1;
>>> +}
>>> +
>>> +static int
>>> egl_choose_config(struct gl_renderer *gr, const EGLint *attribs,
>>> - const EGLint *visual_id,
>>> + const EGLint *visual_id, const int n_ids,
>>> EGLConfig *config_out)
>>
>> Since this appears to just return 0 or -1, would it be clearer to change
>> it to returning just a boolean?
Thought about that, but it's not really a boolean sounding function
name, and we don't generally use bool anywhere else unless the function
name reads like a boolean question...
I didn't add the function or change the prototype, I just left it as is.
>>> {
>>> EGLint count = 0;
>>> EGLint matched = 0;
>>> EGLConfig *configs;
>>> - int i;
>>> + int i, config_index = -1;
>>>
>>> if (!eglGetConfigs(gr->egl_display, NULL, 0, &count) || count < 1)
>>> return -1;
>>> @@ -1915,31 +1938,25 @@ egl_choose_config(struct gl_renderer *gr, const EGLint *attribs,
>>> return -1;
>>>
>>> if (!eglChooseConfig(gr->egl_display, attribs, configs,
>>> - count, &matched))
>>> + count, &matched) || !matched)
>>> goto out;
>>>
>>> - for (i = 0; i < matched; ++i) {
>>> - EGLint id;
>>> + if (!visual_id)
>>> + config_index = 0;
>>>
>>> - if (visual_id) {
>>> - if (!eglGetConfigAttrib(gr->egl_display,
>>> - configs[i], EGL_NATIVE_VISUAL_ID,
>>> - &id))
>>> - continue;
>>> + for (i = 0; config_index == -1 && i < n_ids; i++)
>>> + config_index = match_config(gr->egl_display, visual_id[i],
>>> + configs, matched);
>>>
>>> - if (id != 0 && id != *visual_id)
>>> - continue;
>>> - }
>>> -
>>> - *config_out = configs[i];
>>> -
>>> - free(configs);
>>> - return 0;
>>> - }
>>> + if (config_index != -1)
>>> + *config_out = configs[config_index];
>>>
>>> out:
>>> free(configs);
>>> - return -1;
>>> + if (config_index == -1)
>>> + return -1;
>>> +
>>> + return 0;
>>> }
>>
>> The logic above with the config_index seems a bit clunky to me, but I
>> can't seem to work out something less clunky. My sense is that
>> overloading the return value of match_config() to both return indexes
>> and error codes makes handling the result more difficult.
>>
>> Perhaps instead of an index, what if you had match_config() returned a
>> pointer to the appropriate configs[] element (or NULL if none were
>> found)?
It'd still be the same number of tests, just for NULL instead of -1...
> The logic as is is fine by me.
I'm going to be lazy and leave it as is.
>>> static void
>>> @@ -1976,7 +1993,8 @@ gl_renderer_output_create(struct weston_output *output,
>>> EGLNativeWindowType window_for_legacy,
>>> void *window_for_platform,
>>> const EGLint *attribs,
>>> - const EGLint *visual_id)
>>> + const EGLint *visual_id,
>>> + int n_ids)
>>> {
>>> struct weston_compositor *ec = output->compositor;
>>> struct gl_renderer *gr = get_renderer(ec);
>>> @@ -1984,7 +2002,8 @@ gl_renderer_output_create(struct weston_output *output,
>>> EGLConfig egl_config;
>>> int i;
>>>
>>> - if (egl_choose_config(gr, attribs, visual_id, &egl_config) == -1) {
>>> + if (egl_choose_config(gr, attribs, visual_id,
>>> + n_ids, &egl_config) == -1) {
>>> weston_log("failed to choose EGL config for output\n");
>>> return -1;
>>> }
>>> @@ -2260,7 +2279,7 @@ platform_to_extension(EGLenum platform)
>>> static int
>>> gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
>>> void *native_window, const EGLint *attribs,
>>> - const EGLint *visual_id)
>>> + const EGLint *visual_id, int n_formats)
>
> You call it n_ids everywhere else, would be nice to use the same here
> too.
Ouch. Will fix that.
I'll post a follow up shortly.
>>> {
>>> struct gl_renderer *gr;
>>> EGLint major, minor;
>>> @@ -2323,7 +2342,8 @@ gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
>>> goto err_egl;
>>> }
>>>
>>> - if (egl_choose_config(gr, attribs, visual_id, &gr->egl_config) < 0) {
>>> + if (egl_choose_config(gr, attribs, visual_id,
>>> + n_formats, &gr->egl_config) < 0) {
>>> weston_log("failed to choose EGL config\n");
>>> goto err_egl;
>>> }
>>> diff --git a/src/gl-renderer.h b/src/gl-renderer.h
>>> index ebc139f..ceb4206 100644
>>> --- a/src/gl-renderer.h
>>> +++ b/src/gl-renderer.h
>>> @@ -76,7 +76,8 @@ struct gl_renderer_interface {
>>> EGLenum platform,
>>> void *native_window,
>>> const EGLint *attribs,
>>> - const EGLint *visual_id);
>>> + const EGLint *visual_id,
>>> + const int n_ids);
>>>
>>> EGLDisplay (*display)(struct weston_compositor *ec);
>>>
>>> @@ -84,7 +85,8 @@ struct gl_renderer_interface {
>>> EGLNativeWindowType window_for_legacy,
>>> void *window_for_platform,
>>> const EGLint *attribs,
>>> - const EGLint *visual_id);
>>> + const EGLint *visual_id,
>>> + const int n_ids);
>>>
>>> void (*output_destroy)(struct weston_output *output);
>
>
> Thanks,
> pq
>
More information about the wayland-devel
mailing list