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

return HDMI physical address using EDID data retrieved from X11 randr ex... #6

Closed
wants to merge 2 commits into from

Conversation

smithereens
Copy link

...tension

@malard
Copy link
Member

malard commented Feb 8, 2014

thanks for this patch! can you tell us if you have submitted the contributor agreement to us?

use_x11_xrandr="yes"
AC_CHECK_HEADER(X11/Xlib.h,,[use_x11_xrandr="no"])
AC_CHECK_HEADER(X11/Xatom.h,,[use_x11_xrandr="no"])
AC_CHECK_HEADER(X11/extensions/Xrandr.h,,[use_xrandr="no"])
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo here: use_xrandr -> use_x11_xrandr

@smithereens
Copy link
Author

how do I see/submit the contributor agreement ?

@malard
Copy link
Member

malard commented Feb 8, 2014

Please go here: http://www.pulse-eight.net/contributors

@opdenkamp
Copy link
Contributor

nice work, thanks!
out of curiosity, which display driver are you using? last time i checked, ~2 years ago, only the first 512 bytes were exposed with nvidia and amd drivers via this extension

@smithereens
Copy link
Author

I've tested this code using nvidia-304 proprietary driver.

@opdenkamp
Copy link
Contributor

great that this is exposed now, since this is the one setting that users can mess up and that breaks cec support. i suppose it's been added to the driver in the mean time, or to xrandr.

do you happen to know if hotplug detect is exposed too?

@smithereens
Copy link
Author

@malard I don't have access to a scanner at present, it'll be a few days before I can return the paperwork.

@malard
Copy link
Member

malard commented Feb 8, 2014

Okay, we can merge it as soon as we have that back, it is for both parties protection

Cheers

From: smithereens [mailto:notifications@github.com]
Sent: 08 February 2014 12:15
To: Pulse-Eight/libcec
Cc: Martin Ellis
Subject: Re: [libcec] return HDMI physical address using EDID data retrieved from X11 randr ex... (#6)

@malardhttps://github.com/malard I don't have access to a scanner at present, it'll be a few days before I can return the paperwork.


Reply to this email directly or view it on GitHubhttps://github.com//pull/6#issuecomment-34542460.

@smithereens
Copy link
Author

I think monitor hotplug is handled by DRM not Xrandr, and nvidia driver doesn't support DRM AFAIK.

@opdenkamp
Copy link
Contributor

Alright, i'll have a look at that one later, thanks for this

@smithereens
Copy link
Author

no problem, glad to help.

@smithereens
Copy link
Author

agreement sent

XRRScreenResources *rsrc = NULL;
Window root = RootWindow(disp, screen);

#if (RANDR_MAJOR > 1) || (RANDR_MAJOR == 1 && RANDR_MINOR >=2)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test should be RANDR_MINOR >=3, XRRGetScreenResourcesCurrent() only exists for RandR 1.3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. could you update the check, then i'll merge it in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i also just noticed a little cosmetics issue:
could you use 2 spaces instead of 4 please. i could update it after the pull if your ide can't do that automatically.

@opdenkamp
Copy link
Contributor

i've squashed the commits into one, added a couple of other cosmetic changes, and will include this in the next major release.

when i said i only got 512 bytes earlier, i mixed things up. i actually only got 128 bytes. now i'm getting 256 bytes on hdmi ports with nvidia drivers, containing the hdmi vsdb, so it's working great.

@opdenkamp
Copy link
Contributor

this PR will be included in the next major release of libCEC

@smithereens
Copy link
Author

cool, any idea when the release will be out ?

@opdenkamp
Copy link
Contributor

sorry for the (very long) delay. all I can say right now is "soon". I've registered this in our internal bugtracker as ticket 4470.

AC_CHECK_LIB(Xrandr,XRRGetScreenResources,,[use_x11_xrandr="no"])
if test "x$use_x11_xrandr" = "xyes"; then
AC_DEFINE([HAVE_RANDR],[1],[Define to 1 to include support for the X11 randr extension])
AM_CONDITIONAL(USE_X11_RANDR, true)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building this on OpenELEC for R-Pi (ie. without X11) results in the following build error:

checking for bcm_host_init in -lbcm_host... yes
checking X11/Xlib.h usability... no
checking X11/Xlib.h presence... no
checking for X11/Xlib.h... no
checking X11/Xatom.h usability... no
checking X11/Xatom.h presence... no
checking for X11/Xatom.h... no
checking X11/extensions/Xrandr.h usability... no
checking X11/extensions/Xrandr.h presence... no
checking for X11/extensions/Xrandr.h... no
checking for XOpenDisplay in -lX11... no
checking for XRRGetScreenResources in -lXrandr... no
checking algorithm usability... yes
...
checking that generated files are newer than configure... done
configure: error: conditional "USE_X11_RANDR" was never defined.
Usually this means the macro was only invoked conditionally.
make: *** [release] Error 1

The following patch seems to do the trick, is this the correct fix?

--- a/configure.ac  2014-10-25 15:40:49.972645020 +0100
+++ b/configure.ac  2014-10-25 15:41:12.904644410 +0100
@@ -226,6 +226,7 @@
       AM_CONDITIONAL(USE_X11_RANDR, true)
     else
       LIBS="$libs_tmp"
+      AM_CONDITIONAL(USE_X11_RANDR, false)
     fi
     ;;
   *-apple-darwin*)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, that looks correct, thanks!

@opdenkamp
Copy link
Contributor

Will be merge in and released soon. See #41. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants