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

[GBM] support 10bit output modes and other fixes #14515

Merged
merged 4 commits into from Oct 24, 2018

Conversation

@lrusak
Copy link
Contributor

commented Oct 3, 2018

This is to go alongside #14514

Much of the configuration should now happen automatically as hardcoded formats are removed.
I have tested this on a AMD Vega m GPU however I don't have a monitor that can support 10 bit output.

This should be tested across all platforms (including older intel platforms cc @wsnipex)

Also @Kwiboo please test that this does not break things for you. The only issue I see may be that you don't support gbm_bo_get_format but I'm not sure.

If anyone has issues please upload the output of the modetest utility from libdrm

@lrusak lrusak added this to the Leia 18.0-beta4 milestone Oct 3, 2018
@lrusak lrusak requested a review from a1rwulf Oct 3, 2018
@lrusak lrusak force-pushed the lrusak:drm-10 branch 3 times, most recently from cd741b4 to e2491a6 Oct 3, 2018
Copy link
Member

left a comment

Other than the naming minor, code looks good to me.

@@ -89,6 +90,9 @@ class CDRMUtils
virtual bool AddProperty(struct drm_object *object, const char *name, uint64_t value) { return false; }
virtual bool SetProperty(struct drm_object *object, const char *name, uint64_t value) { return false; }

static uint32_t ToAlpha(uint32_t fourcc);
static uint32_t FromAlpha(uint32_t fourcc);

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Oct 5, 2018

Member

I think @pkerling mentioned it already in the linked PR and I agree with him.
Those methods need a better name, they are part of the public api and DRMUtils is not only about planes.
So the name should make it more clear that you have to feed a plane and what the result is.
Actually I think we should start to use proper method comments in the header for public methods (not needed in this PR but we should do it at some point).

This comment has been minimized.

Copy link
@notspiff

notspiff Oct 5, 2018

Contributor

i disagree. doxy. always. not later. now.

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Oct 5, 2018

Member

I'll come up with a PR to add the doxy then.

@wsnipex

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

this breaks on my ivy i3: http://paste.ubuntu.com/p/DtM6XgFKFs/

@lrusak lrusak force-pushed the lrusak:drm-10 branch from e2491a6 to d28796d Oct 7, 2018
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

@wsnipex sorry I didn't explain myself. For using 10bit planes with this PR you will also need to include #14514

@wsnipex

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

this PR doesn't apply on top of master + #14514

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

It's because they share a commit. Either resolve the conflicts or wait till #14514 is in 😁

@lrusak lrusak force-pushed the lrusak:drm-10 branch from d28796d to 0689c95 Oct 8, 2018
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

#14514 is in so this can be tested directly on top now @wsnipex

@wsnipex

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

doesn't work on my i3225: http://paste.ubuntu.com/p/qmYw63X7Pr/

@lrusak lrusak force-pushed the lrusak:drm-10 branch from 0689c95 to b3de51c Oct 13, 2018
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

@pkerling can I get a sign-off on the EGL changes?

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

todo: move c3ca0a7 to the end of the commits after squashing

@wsnipex

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

this works now on my ivy bridge i3 👍

CWinSystemGbm::GetDrm()->GetOverlayPlane()->useFallbackFormat = true;
visualId = CDRMUtils::FourCCWithAlpha(CWinSystemGbm::GetDrm()->GetOverlayPlane()->GetFormat());

if (!m_eglContext.ChooseConfig(renderableType, visualId))

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 16, 2018

Member

Actually at this point you cannot be sure whether you even have a display, so we should either break up initialization into multiple steps or you check before calling ChooseConfig

@@ -185,6 +185,7 @@ class CEGLContextUtils final
bool CreateSurface(EGLNativeWindowType nativeWindow);
bool CreatePlatformSurface(void* nativeWindow, EGLNativeWindowType nativeWindowLegacy);
bool CreateContext(CEGLAttributesVec contextAttribs);
bool ChooseConfig(EGLint renderableType, EGLint visualId);

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 16, 2018

Member

If you make it public, add a check for preconditions (m_eglDisplay) like all other public functions

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

@pkerling does the latest fixup solve your issue?

@pkerling

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@lrusak Not really? The problem stays that when CreatePlatformDisplay() fails, you don't know why (e.g. maybe it couldn't bind the API), and consequently cannot assume you can call ChooseConfig(). If I'm not mistaken, with the flow like it is now you would try to continue without a bound API in the example.

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

@pkerling ah yes ok. I see the whole picture now. I'm not really sure how to solve it though. I'll have to think about it and revisit this.

@MartijnKaijser MartijnKaijser removed this from the Leia 18.0-beta4 milestone Oct 23, 2018
lrusak added 4 commits Oct 2, 2018
…is found
@lrusak lrusak force-pushed the lrusak:drm-10 branch from b31341e to 7a5de9f Oct 24, 2018
@lrusak lrusak merged commit 5b7080a into xbmc:master Oct 24, 2018
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@Rechi Rechi added this to the Leia 18.0-beta5 milestone Oct 24, 2018
@Kwiboo Kwiboo referenced this pull request Nov 3, 2018
3 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.