Sync bit bug in drivers/video/radeonfb.c ?

Sync bit bug in drivers/video/radeonfb.c ?

[Kai Engert]

I suspect the line
         v_sync_pol = sync & FB_SYNC_VERT_HIGH_ACT ? 0 : 1;
is incorrect, and probably also line
         h_sync_pol = sync & FB_SYNC_HOR_HIGH_ACT ? 0 : 1;

If I use "fbset -vsync low" the uppermost line is only halfway displayed 
on my system. With "fbset -vsync high" the uppermost line is fully visible.

I'm using 2.4.19-rc3-ac4 which has an entry in drivers/video/modedb.c to 
support a 1280x600 screen resolution on Sony C1M*. That entry defines
   fb_videomode->sync = FB_SYNC_VERT_HIGH_ACT

However, after booting the top level line is not fully visible.
I have to correct that manually with "fbset -vsync high".

The patch below makes it work on boot.

Also note, the variables vSyncPol and hSyncPol use the same assignments, 
but seem to be unused, and should probably get removed.

Kai

PS: I'm not subscribed, please cc me on replys. Thanks.


--- bad/drivers/video/radeonfb.c        Tue Jul 30 14:38:29 2002
+++ good/drivers/video/radeonfb.c       Tue Jul 30 14:39:10 2002
@@ -2401,8 +2401,8 @@
         }

         sync = mode->sync;
-       h_sync_pol = sync & FB_SYNC_HOR_HIGH_ACT ? 0 : 1;
-       v_sync_pol = sync & FB_SYNC_VERT_HIGH_ACT ? 0 : 1;
+       h_sync_pol = sync & FB_SYNC_HOR_HIGH_ACT ? 1 : 0;
+       v_sync_pol = sync & FB_SYNC_VERT_HIGH_ACT ? 1 : 0;

         RTRACE("hStart = %d, hEnd = %d, hTotal = %d\n",
                 hSyncStart, hSyncEnd, hTotal);

Re: Sync bit bug in drivers/video/radeonfb.c ?

[James Mayer]

Hi Kai, 

Oddly enough, if I apply this patch on my C1MV/M, I lose the upper
half of my uppermost line, and must use fbset -vsync low to fix it.

I determined the 1200x600 modedb entry using the existing radeonfb
driver, and it appears our hardware is exactly opposite on this issue.

Which C1M* do you have?

  James

> --- bad/drivers/video/radeonfb.c        Tue Jul 30 14:38:29 2002
> +++ good/drivers/video/radeonfb.c       Tue Jul 30 14:39:10 2002
> @@ -2401,8 +2401,8 @@
>          }
> 
>          sync = mode->sync;
> -       h_sync_pol = sync & FB_SYNC_HOR_HIGH_ACT ? 0 : 1;
> -       v_sync_pol = sync & FB_SYNC_VERT_HIGH_ACT ? 0 : 1;
> +       h_sync_pol = sync & FB_SYNC_HOR_HIGH_ACT ? 1 : 0;
> +       v_sync_pol = sync & FB_SYNC_VERT_HIGH_ACT ? 1 : 0;
> 
>          RTRACE("hStart = %d, hEnd = %d, hTotal = %d\n",
>                  hSyncStart, hSyncEnd, hTotal);

Re: Sync bit bug in drivers/video/radeonfb.c ?

[Kai Engert]

That's strange! I'm using a C1MGP, german version.

See http://www.kuix.de/sonyc1/lspci for lspci output.

However, if you apply the patch below, and you have to manually set it 
to vsync low, I guess this means your on boot mode was set to high, 
which is what is requested in modedb.c, and it sounds like the patch 
below is correct. But we probably need more entries in modedb.c to 
support all models of the C1M.

Kai


James Mayer wrote:
> Hi Kai, 
> 
> Oddly enough, if I apply this patch on my C1MV/M, I lose the upper
> half of my uppermost line, and must use fbset -vsync low to fix it.
> 
> I determined the 1200x600 modedb entry using the existing radeonfb
> driver, and it appears our hardware is exactly opposite on this issue.
> 
> Which C1M* do you have?
> 
>   James
> 
> 
>>--- bad/drivers/video/radeonfb.c        Tue Jul 30 14:38:29 2002
>>+++ good/drivers/video/radeonfb.c       Tue Jul 30 14:39:10 2002
>>@@ -2401,8 +2401,8 @@
>>         }
>>
>>         sync = mode->sync;
>>-       h_sync_pol = sync & FB_SYNC_HOR_HIGH_ACT ? 0 : 1;
>>-       v_sync_pol = sync & FB_SYNC_VERT_HIGH_ACT ? 0 : 1;
>>+       h_sync_pol = sync & FB_SYNC_HOR_HIGH_ACT ? 1 : 0;
>>+       v_sync_pol = sync & FB_SYNC_VERT_HIGH_ACT ? 1 : 0;
>>
>>         RTRACE("hStart = %d, hEnd = %d, hTotal = %d\n",
>>                 hSyncStart, hSyncEnd, hTotal);

Re: Sync bit bug in drivers/video/radeonfb.c ?

[Russell King]

On Tue, Jul 30, 2002 at 04:54:24PM +0200, Kai Engert wrote:
> That's strange! I'm using a C1MGP, german version.
> 
> See http://www.kuix.de/sonyc1/lspci for lspci output.
> 
> However, if you apply the patch below, and you have to manually set it 
> to vsync low, I guess this means your on boot mode was set to high, 
> which is what is requested in modedb.c, and it sounds like the patch 
> below is correct. But we probably need more entries in modedb.c to 
> support all models of the C1M.

Maybe the real answer is for someone to scope the sync signals and
work out whether the original or the patched version is correct.
Then someone's left with a buggy monitor (which, quote honestly
wouldn't surprise me in the least.)

If a buggy monitor is the problem, it might be an idea to introduce
generic "invert_vsync=1" and "invert_hsync=1" options to the kernel
(since a buggy monitor will affect _all_ cards, not just radeon)

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Sync bit bug in drivers/video/radeonfb.c ?

[James Mayer]

On Tue, Jul 30, 2002 at 04:54:24PM +0200, Kai Engert wrote:
> That's strange! I'm using a C1MGP, german version.
> 
> See http://www.kuix.de/sonyc1/lspci for lspci output.
> 
> However, if you apply the patch below, and you have to manually set it 
> to vsync low, I guess this means your on boot mode was set to high, 
> which is what is requested in modedb.c, and it sounds like the patch 
> below is correct. But we probably need more entries in modedb.c to 
> support all models of the C1M.

I was the one who determined the modedb.c entry for the C1MV, using my
laptop's hardware, and the existing (original) radeonfb driver.

I don't have the equipment to open the laptop and scope the DFP's sync
line, so I opened up the XFree86 4.2.0 source and looked at their
Radeon driver instead:

xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_driver.c, line 3774:

 save->crtc_v_sync_strt_wid = (((mode->CrtcVSyncStart - 1) & 0xfff)
				 | (vsync_wid << 16)
				 | ((mode->Flags & V_NVSYNC)
				    ? RADEON_CRTC_V_SYNC_POL
				    : 0));

RADEON_CRTC_V_SYNC_POL is defined as (1 << 23) in radeon_reg.h. 

I think when vertical sync is low (the V_NVSYNC flag is set), the
register bit is set to 1.  When vertical sync is high, the V_NVSYNC
flag is false, and the register bit is zero.

Looking at Ani's radeonfb code, we have two spots to consider:

drivers/video/radeonfb.c, line 2421:

sync = mode->sync;
h_sync_pol = sync & FB_SYNC_HOR_HIGH_ACT ? 0 : 1;
v_sync_pol = sync & FB_SYNC_VERT_HIGH_ACT ? 0 : 1;

and drivers/video/radeonfb.c, line 2472:

newmode.crtc_v_sync_strt_wid = (((vSyncStart - 1) & 0xfff) |
				 (vsync_wid << 16) | (v_sync_pol  << 23));

Looking at this, I think if vertical sync is low, v_sync_pol is 1, and
v_sync_pol << 23 sets the register bit to 1.  If vertical sync is
high, v_sync_pol is 0, and the register bit is cleared.


The XFree86 4.2.0 code seems to agree with the existing radeonfb
driver, with the sense of the tests being inverted.


I couldn't find a programming manual for the card on ATI's site (might
not know where to look).  XFree86 4.2.0 worked for me before I tried
the framebuffer, and it configured itself just fine.

Hopefully Ani will have an answer...

  James