Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EGLUtils: rework the way we choose the egl config #14514

Merged
merged 2 commits into from Oct 8, 2018

Conversation

@lrusak
Copy link
Contributor

commented Oct 3, 2018

This changes the egl config selection to be more inline with what upstream projects do (read weston). This will also allow us more control in the future to select different contexts.

This is needed to allow for more features in the future like 10bit output and MSAA selection.

Only tested on GBM but it shouldn't break other systems due to the default visual id = 0

see, wayland-project/weston@77c1a5b
and, https://cgit.freedesktop.org/mesa/kmscube/commit/?id=56c3917ffd1f05942246e2532ca4a5707554a2fc

{EGL_GREEN_SIZE, 8},
{EGL_BLUE_SIZE, 8},
{EGL_ALPHA_SIZE, 8},
{EGL_DEPTH_SIZE, 16},

This comment has been minimized.

Copy link
@koenkooi

koenkooi Oct 4, 2018

Contributor

EGL_DEPTH_SIZE=16 is problematic with some implementations, e.g. lima only supports '8', do we really need 16 bits for the z position?
In the past I just patched out the line in offending apps (e.g. kmscube) and that seemed to work fine.

This comment has been minimized.

Copy link
@lrusak

lrusak Oct 4, 2018

Author Contributor

@koenkooi That is something I can explore but I do net yet understand the implications of changing this value. I have tried to google to see how to determine what this value should be but so far have come up short.

This comment has been minimized.

Copy link
@koenkooi

koenkooi Oct 5, 2018

Contributor

Running your small test app I get only 0 or 24 as depth size on lima:

matched configs: 12

config: 0
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 0 (0x0000)
  EGL_DEPTH_SIZE: 0 (0x0000)
  EGL_NATIVE_VISUAL_ID: 875713112 (0x34325258)
  EGL_SAMPLES: 0 (0x0000)

config: 1
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 0 (0x0000)
  EGL_DEPTH_SIZE: 24 (0x0018)
  EGL_NATIVE_VISUAL_ID: 875713112 (0x34325258)
  EGL_SAMPLES: 0 (0x0000)

config: 2
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 0 (0x0000)
  EGL_DEPTH_SIZE: 24 (0x0018)
  EGL_NATIVE_VISUAL_ID: 875713112 (0x34325258)
  EGL_SAMPLES: 0 (0x0000)

config: 3
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 0 (0x0000)
  EGL_DEPTH_SIZE: 0 (0x0000)
  EGL_NATIVE_VISUAL_ID: 875713112 (0x34325258)
  EGL_SAMPLES: 4 (0x0004)

config: 4
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 0 (0x0000)
  EGL_DEPTH_SIZE: 24 (0x0018)
  EGL_NATIVE_VISUAL_ID: 875713112 (0x34325258)
  EGL_SAMPLES: 4 (0x0004)

config: 5
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 0 (0x0000)
  EGL_DEPTH_SIZE: 24 (0x0018)
  EGL_NATIVE_VISUAL_ID: 875713112 (0x34325258)
  EGL_SAMPLES: 4 (0x0004)

config: 6
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 8 (0x0008)
  EGL_DEPTH_SIZE: 0 (0x0000)
  EGL_NATIVE_VISUAL_ID: 875713089 (0x34325241)
  EGL_SAMPLES: 0 (0x0000)

config: 7
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 8 (0x0008)
  EGL_DEPTH_SIZE: 24 (0x0018)
  EGL_NATIVE_VISUAL_ID: 875713089 (0x34325241)
  EGL_SAMPLES: 0 (0x0000)

config: 8
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 8 (0x0008)
  EGL_DEPTH_SIZE: 24 (0x0018)
  EGL_NATIVE_VISUAL_ID: 875713089 (0x34325241)
  EGL_SAMPLES: 0 (0x0000)

config: 9
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 8 (0x0008)
  EGL_DEPTH_SIZE: 0 (0x0000)
  EGL_NATIVE_VISUAL_ID: 875713089 (0x34325241)
  EGL_SAMPLES: 4 (0x0004)

config: 10
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 8 (0x0008)
  EGL_DEPTH_SIZE: 24 (0x0018)
  EGL_NATIVE_VISUAL_ID: 875713089 (0x34325241)
  EGL_SAMPLES: 4 (0x0004)

config: 11
  EGL_RED_SIZE: 8 (0x0008)
  EGL_GREEN_SIZE: 8 (0x0008)
  EGL_BLUE_SIZE: 8 (0x0008)
  EGL_ALPHA_SIZE: 8 (0x0008)
  EGL_DEPTH_SIZE: 24 (0x0018)
  EGL_NATIVE_VISUAL_ID: 875713089 (0x34325241)
  EGL_SAMPLES: 4 (0x0004)
Copy link
Member

left a comment

Some minors

In general: We have to think about the direction this will take. If at some point we want to have 10-bit on another EGL platform (Wayland?), the code will have to be modified again. Can we make this more general? Or move the decision logic to the actual platform? Matching visual ID is already gbm-specific.

xbmc/windowing/gbm/DRMUtils.h Outdated Show resolved Hide resolved
xbmc/utils/EGLUtils.cpp Outdated Show resolved Hide resolved
xbmc/utils/EGLUtils.cpp Outdated Show resolved Hide resolved
xbmc/utils/EGLUtils.cpp Outdated Show resolved Hide resolved
xbmc/utils/EGLUtils.cpp Outdated Show resolved Hide resolved
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

@pkerling please take a look at how weston is doing it. I'm not sure how the selection will work for the egl config when 10 bit modes become more available.

https://cgit.freedesktop.org/wayland/weston/tree/libweston/gl-renderer.c#n2928

@pkerling

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

I assume for GBM you'll still just match on the visual ID and use a 10-bit FourCC - that's your plan anyway right?

Actually, for everything non-GBM, the obvious solution would be to just take the first config. It is supposed to be the one with the most precision in the color buffers. Or do we want to take a less precise one on purpose?

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

Yes, for GBM It's already working in combination with #14515 to use 10bit output.

I would also agree that we should just take the config with the most precision. I think it is just the way it is for historical reasons. In this case the egl attributes should be the minimum required to run kodi, then the configs are returned in a most to least manner and the first one is chosen (like what is done in weston) https://github.com/wayland-project/weston/blob/master/libweston/gl-renderer.c#L2952

@pkerling

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

OK then I'd say we set EGL_RED_SIZE etc. to 8 (as min. requirement) and if visualId is zero just take the first one, no need to check anything then.

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

I will have to set EGL_ALPHA_SIZE to 2 otherwise the the 10bit color modes will be filtered out.

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

@pkerling I added a fixup to simplify the config selection. I'll double check when I'm home that it works on various platforms.

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

and another fixup to just allocate for the matched configs

if (visualId == id)
break;
}

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 5, 2018

Member

Do we consider it a problem if we do not find a config with matching visual ID? We should at least log, right?

This comment has been minimized.

Copy link
@lrusak

lrusak Oct 5, 2018

Author Contributor

I don't think that will ever happen unless the egl config attributes are too strict.

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 5, 2018

Member

I don't think that will ever happen

famous last words ;-)

logging doesn't hurt imo

@pkerling

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

I will have to set EGL_ALPHA_SIZE to 2 otherwise the the 10bit color modes will be filtered out.

Strange, it shouldn't actually do that. I can see how it might change the priorities, but if EGL_ALPHA_SIZE is not specified it should still contain the 10bit modes :-/

lrusak added 2 commits Oct 3, 2018
…ormats
This changes the egl config selection to be more inline with
what upstream projects do (read weston) This will also allow us
more control in the future to select different contexts.
@lrusak lrusak force-pushed the lrusak:egl-choose-config branch from 85a8363 to b902f8b Oct 5, 2018
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

Strange, it shouldn't actually do that. I can see how it might change the priorities, but if EGL_ALPHA_SIZE is not specified it should still contain the 10bit modes :-/

yes, I could remove EGL_ALPHA_SIZE completely but then we may get configs that don't include alpha at all (which we don't want)

@pkerling

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

yes, I could remove EGL_ALPHA_SIZE completely but then we may get configs that don't include alpha at all (which we don't want)

Shouldn't they not end up getting selected anyway for GBM due to the fourcc filter?

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

Shouldn't they not end up getting selected anyway for GBM due to the fourcc filter?

It's not GBM that I'm worried about :) it's introducing configs into the mix that aren't desired for other platforms. Not sure if this is a valid concern though

@pkerling

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Why should other platforms need an alpha component?

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

Why should other platforms need an alpha component?

You tell me 😸

@pkerling

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

I think you only need alpha if you have a translucent GUI plane above a video plane. I don't know about egl configs on all the linux embedded platforms, but they are going to go be replaced by gbm at some point anyway. But maybe you're right, for Wayland for example it would also theoretically be possible to have a separate video surface.

Maybe add a parameter to allow the platform to decide whether it wants alpha? This looks like one more reason to move the egl config selection algorithm into the actual platforms.

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

I think you only need alpha if you have a translucent GUI plane above a video plane. I don't know about egl configs on all the linux embedded platforms, but they are going to go be replaced by gbm at some point anyway. But maybe you're right, for Wayland for example it would also theoretically be possible to have a separate video surface.

so currently 3/5 linux platforms need alpha and then android probably does too

Maybe add a parameter to allow the platform to decide whether it wants alpha? This looks like one more reason to move the egl config selection algorithm into the actual platforms.

I am going to leave it as is for now and hopefully we can test on every platform to make sure it is still working (I don't see why it wouldn't be)

Copy link
Member

left a comment

looks fine now for the stuff we actually use at the moment :-)

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2018

I tried a test build with the latest version of this PR and the crashes on Kodi start affecting RPi are now resolved, although I haven't been able to test any more than that (no monitor connected to RPis). No more test builds now for about a week, build server is taking a break.

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

@pkerling

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

works for me 😁

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

everything seems to be fine here on amlogic

@lrusak lrusak merged commit cb213fc into xbmc:master Oct 8, 2018
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

Thanks all for testing!

@Rechi Rechi added this to the Leia 18.0-beta4 milestone Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.