X server 1.3 release status, plea for driver developer comments

[Keith Packard]

On Wed, 2007-02-21 at 01:18 -0800, Andy Ritger wrote:
> Sorry I've not provided feedback sooner, Keith.  I'm still digesting
> everything.
> 
> My biggest concern for the NVIDIA driver was the ability to provide
> backwards compatible behavior when the driver advertises "fake" modes to
> XF86VidMode/RandR that are really the bounding box of a TwinView/MergedFB
> MetaMode.  It looks like that backwards compatible behavior should
> be fine.  The NVIDIA driver will most likely give users the option of
> either the old TwinView behavior (in which case the driver won't call
> xf86RandR12Init()), or the RandR 1.2 behavior, in which case the old
> TwinView behavior will be replaced with RandR 1.2 behavior.  That much
> looks like it should be OK.

Right, that's one of the nicest parts of the XFree86 DDX architecture --
nothing is imposed on the drivers, most of the DDX just offers functions
for the driver to use; we now have two parallel mode setting
architetures which drivers should be able to select on the fly. We
should plan on deprecating the old code at some point, but there's no
particular hurry.

> A few minor things I've noticed:
> 
>      - when a non-1.2 RandR driver is used, I see a SEGV when
>        ProcRRSetScreenConfig() calls:
> 
>              if (!RRCrtcSet (pScrPriv->crtcs[c], NULL, 0, 0, RR_Rotate_0, 0, NULL))
> 
>        but RRCrtcSet() dereferences the passed-in NULL pointer 'mode' argument:
> 
>              size.width = mode->mode.width;
> 
>        I'm not sure how much of RRCrtcSet() should be skipped if the mode is NULL.

Ok, I'll treat this as a bug.

>      - in rrmode.c, the list is stored globally, rather than per X screen
>        (i.e., 'modes' and 'num_modes' are static global variables);
>        shouldn't these be stored instead in, say, the rrScrPrivRec?

It shouldn't really matter; the RandR spec doesn't say that modes are
per-screen, and I can't think of any particular reason to make them
per-screen. Having additional sharing just means less memory usage in
multi-screen environments.

>      - in xf86CrtcSetMode(), it would make sense for the call to
>        output->funcs->mode_set() to be wrapped with a more generic
>        "pre_mode_set/post_mode_set", rather than crtc->funcs->dpms(Off/On)
>        calls.

Right, you mentioned this; I'd like to see some kind of proposed API
change that makes it work for you; building an API without any reference
hardware seems even worse than building it based on just the intel
driver.

> In terms of the actual driver API for RandR 1.2, I don't have much to
> say, yet.  It might be good to document the API in the XFree86 DESIGN
> document (xserver/hw/xfree86/doc/DESIGN.sgml), for use as a reference
> for others.

Right, documentation always lags implementation in this kind of code as
we slowly converge on the desired API over a period of interaction
between various parties.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg/attachments/20070221/6cacacec/attachment.pgp>