Bug 12763 - [xrandr TV] need TV output property control
Summary: [xrandr TV] need TV output property control
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Driver/intel (show other bugs)
Version: 7.3 (2007.09)
Hardware: Other Linux (All)
: medium enhancement
Assignee: Wang Zhenyu
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 20276
  Show dependency treegraph
 
Reported: 2007-10-09 21:49 UTC by Philip Langdale
Modified: 2009-03-02 22:07 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
TV output properties (5.91 KB, patch)
2007-11-08 13:07 UTC, Jesse Barnes
no flags Details | Splinter Review
Update TV output props patch (6.10 KB, patch)
2007-11-09 13:00 UTC, Jesse Barnes
no flags Details | Splinter Review
Updated patch (8.18 KB, patch)
2009-02-22 22:59 UTC, Wang Zhenyu
no flags Details | Splinter Review

Description Philip Langdale 2007-10-09 21:49:42 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?
Comment 1 Jesse Barnes 2007-10-31 12:53:13 UTC
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
--- a/src/i830_tv.c
+++ b/src/i830_tv.c
@@ -1178,7 +1178,7 @@ i830_tv_mode_set(xf86OutputPtr output, DisplayModePtr mode
            (i830_float_to_csc(color_conversion->bv) << 16) |
            (i830_float_to_luma(color_conversion->av)));

-    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)));
     {
Comment 2 Philip Langdale 2007-10-31 12:55:37 UTC
Yes it does, I'm very happy to say.

I guess we should leave this bug open until the fix gets into the tree.
Comment 3 Gordon Jin 2007-11-06 22:38:40 UTC
Jesse, should the patch go to upstream?
Comment 4 Jesse Barnes 2007-11-08 13:07:20 UTC
Created attachment 12409 [details] [review]
TV output properties
Comment 5 Jesse Barnes 2007-11-08 13:08:07 UTC
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...

Thanks,
Jesse
Comment 6 Philip Langdale 2007-11-08 13:21:07 UTC
Jesse,

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?
Comment 7 Jesse Barnes 2007-11-08 13:45:30 UTC
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.
Comment 8 Philip Langdale 2007-11-08 13:52:54 UTC
Right - I missed that. Anyway, change looks good to me.
Comment 9 Robert Lowery 2007-11-08 16:29:39 UTC
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.


Comment 10 Jesse Barnes 2007-11-08 16:57:50 UTC
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...

Jesse
Comment 11 Robert Lowery 2007-11-09 02:44:59 UTC
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
Comment 12 Jesse Barnes 2007-11-09 09:18:42 UTC
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.
Comment 13 Jesse Barnes 2007-11-09 13:00:49 UTC
Created attachment 12432 [details] [review]
Update TV output props patch
Comment 14 Jesse Barnes 2007-11-14 11:24:40 UTC
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.
Comment 15 Gordon Jin 2008-07-02 19:18:49 UTC
Keith, how are things going on this feature? Users are asking about this.
Comment 16 Keith Packard 2008-07-02 21:20:27 UTC
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.
Comment 17 Wang Zhenyu 2008-12-08 01:03:34 UTC
This has to be reworked for different float-point format used in color knobs on 915 and 965.
Comment 18 Wang Zhenyu 2009-02-22 22:59:20 UTC
Created attachment 23198 [details] [review]
Updated patch

Here's updated one. Export integer range instead of hardware float point format to user.
Comment 19 Wang Zhenyu 2009-03-02 22:07:30 UTC
Patch is pushed upstream. Close.
commit 42e34e90e2e4048b38481cab61cef46f932eada7
Author: Zhenyu Wang <zhenyu.z.wang@intel.com>
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.



Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.