xserver: Branch 'master'
Eric Anholt
eric at anholt.net
Tue Jan 30 12:06:49 PST 2007
On Tue, 2007-01-30 at 13:15 +0100, Luc Verhaegen wrote:
> On Mon, Jan 29, 2007 at 09:55:54AM -0800, Eric Anholt wrote:
> > hw/xfree86/common/xf86Mode.c | 39 ---------------------------------------
> > 1 files changed, 39 deletions(-)
> >
> > New commits:
> > diff-tree 31f2d4a57e04f5ea635fbb50c508405c4fc37b65 (from 1627af54497bee659ea30f2850b39cbbf576e22d)
> > Author: Eric Anholt <eric at anholt.net>
> > Date: Mon Jan 29 09:39:33 2007 -0800
> >
> > Bug #9680: Remove bogus blank length limiting in xf86SetModeCrtc().
> >
> > Our modes typically come from EDID or default modes, and when the monitor
> > asks for a specific mode, deciding to tweak it usually results in incorrect
> > display. And if the user is specifying a mode by hand, tweaking it then is
> > still pretty rude.
> >
> > Reviewed by: ajax
> >
> > diff --git a/hw/xfree86/common/xf86Mode.c b/hw/xfree86/common/xf86Mode.c
> > index 3cebac7..7f5ae36 100644
> > --- a/hw/xfree86/common/xf86Mode.c
> > +++ b/hw/xfree86/common/xf86Mode.c
> > @@ -727,45 +727,6 @@ xf86SetModeCrtc(DisplayModePtr p, int ad
> > }
> > p->CrtcHAdjusted = FALSE;
> > p->CrtcVAdjusted = FALSE;
> > -
> > - /*
> > - * XXX
> > - *
> > - * The following is taken from VGA, but applies to other cores as well.
> > - */
> > - p->CrtcVBlankStart = min(p->CrtcVSyncStart, p->CrtcVDisplay);
> > - p->CrtcVBlankEnd = max(p->CrtcVSyncEnd, p->CrtcVTotal);
> > - if ((p->CrtcVBlankEnd - p->CrtcVBlankStart) >= 127) {
> > - /*
> > - * V Blanking size must be < 127.
> > - * Moving blank start forward is safer than moving blank end
> > - * back, since monitors clamp just AFTER the sync pulse (or in
> > - * the sync pulse), but never before.
> > - */
> > - p->CrtcVBlankStart = p->CrtcVBlankEnd - 127;
> > - /*
> > - * If VBlankStart is now > VSyncStart move VBlankStart
> > - * to VSyncStart using the maximum width that fits into
> > - * VTotal.
> > - */
> > - if (p->CrtcVBlankStart > p->CrtcVSyncStart) {
> > - p->CrtcVBlankStart = p->CrtcVSyncStart;
> > - p->CrtcVBlankEnd = min(p->CrtcHBlankStart + 127, p->CrtcVTotal);
> > - }
> > - }
> > - p->CrtcHBlankStart = min(p->CrtcHSyncStart, p->CrtcHDisplay);
> > - p->CrtcHBlankEnd = max(p->CrtcHSyncEnd, p->CrtcHTotal);
> > -
> > - if ((p->CrtcHBlankEnd - p->CrtcHBlankStart) >= 63 * 8) {
> > - /*
> > - * H Blanking size must be < 63*8. Same remark as above.
> > - */
> > - p->CrtcHBlankStart = p->CrtcHBlankEnd - 63 * 8;
> > - if (p->CrtcHBlankStart > p->CrtcHSyncStart) {
> > - p->CrtcHBlankStart = p->CrtcHSyncStart;
> > - p->CrtcHBlankEnd = min(p->CrtcHBlankStart + 63 * 8, p->CrtcHTotal);
> > - }
> > - }
> > }
> >
> > /**
> This is _very_ wrong.
>
> You've subtly broken many drivers now.
>
> I used this code in my EXDC slides to show an example of what things
> were still present in xf86Mode.c, and I've known about the effects of
> this code since the spring of 2005, yet I didn't dream of removing
> this before fixing things properly.
>
> Almost all hardware has a, sometimes highly, limited blanking range,
> inherited from VGA. The work-around is to use overscan, and this is
> exactly what this code does. Most modern hardware adds one or several
> bits to the VGA blankend, but that's not always enough. Overscan is
> never a problem, CRTCs just need to have their LUTs set up correctly.
>
> All drivers have been tested with these VGA limitations, plainly
> removing this code will break many on some modes.
>
> You need to either stop or work around wide blanking in _each_ drivers
> validmode. It's easy to provide a small helper (or a pair of), taking in
> the mode and the maximum blanking. Feel free to, once again (but do try
> to admit at this time), look at my unichrome code for a suited
> implementation, where i do handle this correctly.
>
> So if you want to remove this, you need to go over each driver and find
> out about the device specific limitations. You're also able to easily
> work around the VGA style blanking in your specific driver's ValidMode.
Sigh. So I reviewed the specs for a bunch of hardware I have laying
around:
- mach64 doesn't have this limitation.
- g200 doesn't have this limitation
- g400 doesn't have this limitation
- savage4 has a much larger limitation, such that it won't be hit by any
monitor I've encountered.
- intel doesn't have this limitation
- sis305 does have this limitation(1)
- sis530 has a larger limitation, such that it won't be hit by any
monitor I've encountered
- rendition v2200 does have this limitation, but doesn't appear to
support a high enough pixel clock
- ple133 does have this limitation, and appears to support a high enough
pixel clock
- smi731 doesn't appear to have this limitation
(1) The docs aren't quite clear -- while it's a standard VGA register
set, it also mentions the extra overflow register like on the 530,
without documenting it.
So, in a review of a variety of hardware I have available to me, not
counting generations of Intel chips separately, and not considering many
more modern cards, 1 or 2 out of 10 chips I looked at might be able to
encounter this limitation. This is considering the 1680x1050
non-reduced-blanking mode as the smallest mode I know of which would hit
these limitations.
The generic code is enforcing chip-specific limitations that are,
optimistically, applicable to 1 or 2 out of 10 chips. For those chips
not affected by this limit, they get to go to extra work to undo what
this generic code helpfully tried to do, and may experience bugs in the
process (we hit this on intel because our undo code stopped working as
we were developing randr, and we apparently have issues with nonzero
borders on SDVO and this code is the only cause of nonzero borders we've
found).
As such, I'm quite inclined to leave my change as-is, and let the
drivers for those few chips which have these strict requirements do the
working around themselves.
Also, for the record, I don't recall looking at unichrome code other
than looking at filenames to see what encoders you support, and
providing review of EXA code. Sorry if that offends.
--
Eric Anholt anholt at FreeBSD.org
eric at anholt.net eric.anholt at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 187 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg/attachments/20070130/0dc09032/attachment.pgp>
More information about the xorg
mailing list