Bug 12763

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)));
     {

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...

Thanks,
Jesse

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?

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...

Jesse

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]
Updated patch

Here's updated one. Export integer range instead of hardware float point format to user.

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.