[PATCH] [RFC] xinerama: attempt to unify the two protocol implementations.

[Adam Jackson]

On Tue, 2010-10-26 at 14:46 +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> randr and panoramiX have had two separate copies of this code for long enough,
> 
> this patch sets up a separate xinerama protocol that both randr and panoramix
> call into to configure what is sent on the wire.
> 
> this needs a lot more testing and fair bit of review, but this is a first
> cut to see if people agree with the idea.

Looks like a good start, but.

> +struct XineramaProtoRec {
> +    int num_screens;
> +    int ids[MAXSCREENS];
> +    xXineramaScreenInfo info[MAXSCREENS];
> +    Bool primary[MAXSCREENS];
> +};

I'm not a huge fan of this, on an Evergreen this means you don't even
scale to three cards.

> +int XineramaProtoInit(void (*reset_proc)(ExtensionEntry *extEntry))
> +{
> +    ExtensionEntry 	*extEntry;
> +
> +    extEntry = AddExtension(PANORAMIX_PROTOCOL_NAME, 0, 0,
> +			    ProcXineramaProtoDispatch,
> +			    SProcXineramaProtoDispatch,
> +			    reset_proc,
> +			    StandardMinorOpcode);
> +
> +    if (!extEntry)
> +	return -1;
> +    xineramaProtoEnabled = TRUE;
> +    return 0;
> +}

This is broken if both Xinerama and RANDR are active, AddExtension isn't
prepared to be called multiple times for the same protocol name.  More
like:

    static ExtensionEntry *extEntry;

    if (!extEntry)
        extEntry = AddExtension();
    if (!extEntry)
        return -1;
    /* ... */

> +int XineramaProtoRegisterScreen(int id, xXineramaScreenInfo *info, Bool primary)

Nitpicking: This should probably come with documentation that 'id'
should be an XID so as to be unique across backends (assuming we want
TwinView and RANDR drivers to have consistent Xinerama geometry, which,
we might as well).  Also, no need for it to return int if it's never
going to fail.

Design: I'm pretty sure this approach is broken.  Imagine having
PanoramiXExtensionInit called, and then RANDR active (on one or more of
the backend ScreenRecs). You'll have panoramix-level boxes for each
ScreenRec, and then randr-level boxes for each CRTC on each randrful
ScreenRec.  That ain't right.

The way I'd tried this before was by adding a GetScreenBoxes hook to the
ScreenRec and then having the Xin protocol pull information out of that
(with an mi version that just returns the dumb screen geometry).  Which
is irritating for a couple of reasons, you have to loop over all
ScreenRecs to build them all up, and then loop over them all again
looking for the primary (and then again for the boring ones).  It's also
not terribly efficient, but queries are rare so maybe it doesn't matter.

I think I still have the code for that, I'll try to dig it up.

- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-devel/attachments/20101028/d6b74bfb/attachment.pgp>