Bugzilla – Bug 12763
[xrandr TV] need TV output property control
Last modified: 2009-03-02 22:07:30 UTC
I've just got myself a 965 based aopen mini-pc for a mythtv setup and I've got it plugged into my TV and it's way too bright. The only knob I seem to have is gamma and that's not really the problem. This brightness problem is especially bad in Xv where things are clearly oversaturated.
Currently, the driver does not implement Xv brightness and contrast control, but is there something I can do in the short term? Are there are hard coded values somewhere that I can tweak and then rebuild the driver?
Rob posted a potential fix for this issue, does this patch help?
diff --git a/src/i830_tv.c b/src/i830_tv.c
index ee2538a..e6d2ee5 100644
@@ -1178,7 +1178,7 @@ i830_tv_mode_set(xf86OutputPtr output, DisplayModePtr mode
(i830_float_to_csc(color_conversion->bv) << 16) |
- OUTREG(TV_CLR_KNOBS, 0x10606000);
+ OUTREG(TV_CLR_KNOBS, 0x00404000);
OUTREG(TV_CLR_LEVEL, ((video_levels->black << TV_BLACK_LEVEL_SHIFT) |
(video_levels->blank << TV_BLANK_LEVEL_SHIFT)));
Yes it does, I'm very happy to say.
I guess we should leave this bug open until the fix gets into the tree.
Jesse, should the patch go to upstream?
Created attachment 12409 [details] [review]
TV output properties
Philip & Rob, does something like the attached patch look ok to you? It should expose some new output properties for the TV output, which you can control using the xrandr command. It also changes the defaults to the ones Rob found worked best for him...
That seems the right way to handle this. I'd just observe that you didn't change the initial value, but removed the code that sets it - ie: this means we'll be using BIOS values by default, which is probably the right thing to do.
My only concern would be that the old value was put there deliberately so I'd figure that there's some configuration out there where it's the right thing to do. Does the effect of the register depend on the specific encoder chip being used or the specific intel graphics part?
No, we'll just initialize it in a different place now--when the TV output is initially set up.
There's actually one thing about the register I should have documented: if the output is RGB, the saturation and contrast values have to be identical. Should be an easy fix though.
I'm not sure how the values will affect different machines.
Right - I missed that. Anyway, change looks good to me.
Patch looks good to me, I'll test it out tonight.
A couple of comments
1) the patch treats all of this variables as simple 8 bit integers, however contrast and saturation are actually 2.6 unsigned floating point numbers. This means the user of xrandr will need to do the float->integer conversion themselves. Is that ok?
2) What are the limits of valid values in each case. I assume it is not 0-255? eg hue is an integer phase angle in degrees.
Yeah, I just represented the actual register fields directly, which isn't the most user-friendly approach.
Hue is an 8 bit value representing the phase angle from 0 to 360 degrees.
Saturation & contrast are 2.6 fixed point values as you say.
And Brightness is a 2's complement value.
Changing the Atoms to make that clearer is probably the right thing to do...
Patch defaults ok, but am I using xrandr correctly when changing values?
xrandr --output TV --set TV_BRIGHTNESS 0
X Error of failed request: BadValue (integer parameter out of range for operation)
Major opcode of failed request: 156 (RANDR)
Minor opcode of failed request: 13 ()
Value in failed request: 0x6f
Serial number of failed request: 18
Current serial number in output stream: 19
Well, either xrandr doesn't know how to handle these resources, or there's a bug somewhere in my code. :)
I'll have to try and setup my 915 with TV out support to test it out.
Created attachment 12432 [details] [review]
Update TV output props patch
Keith says we can do output properties in a better way with Randr 1.3, so I'm punting this bug over to him.
For 2.2, I'll just apply something like Rob's patch.
Keith, how are things going on this feature? Users are asking about this.
The only thing we'd do in RandR 1.3 is standardize on the names of these properties. The AR to do that was accepted by SuSE engineering, but I haven't heard anything for quite a while about it.
I'd say we should just publish our own names for these properties and not worry about RandR 1.3.
Btw, 'float 2.6' is not an acceptable format to export to users. Can we make this a 32-bit integer please? Make all of the others 32-bit integers as well.
This has to be reworked for different float-point format used in color knobs on 915 and 965.
Created attachment 23198 [details] [review]
Here's updated one. Export integer range instead of hardware float point format to user.
Patch is pushed upstream. Close.
Author: Zhenyu Wang <firstname.lastname@example.org>
Date: Tue Mar 3 22:55:35 2009 +0800
TV: add property control for TV attributes
This is based on Jesse's origin patch for bug #12763.
But export integer range to user instead of hardware float
point format, and fix different real format on 965G and 945G
for contrast and saturation.