winlayer.c fails to build

[Harold Hunt]

Alexander,

Oops, I put a little more thought into what you said, and here's what I've
come up with:

> A debug session showed, that PScreen->CloseScreen is called for
> all initialized
> components. The crash ocurs in layerinit.c in layerCloseScreen:350
> "xfree(shadowGetScrPriv(pScreen))" since the data was already freed in
> shadowCloseScreen. It seem's they wanted to catch this with the
> if statement
> "if (kind != LAYER_SHADOW)". LAYER_SHADOW is defined in layer.h as 1,
> while LAYER_FB is defined as 0. In the debug session, kind is always 0 -
> which equals to LAYER_FB.

Yes, the "layer kind" is just an index into an array of the layer kinds that
have been created.  We only create one layer kind, so our index has to be 0
(which, as you mentioned, is LAYER_FB).  Layer was built with the assumption
that every screen type it would be used on would have a pointer to the
primary display framebuffer so that updates can be written directly to the
screen.  However, we all know that Cygwin/XFree86 uses a shadow framebuffer
as the primary display framebuffer and we depend on the shadow layer to
batch blit the updates to the screen.  The #ifdef __CYGWIN__'s that I added
to layerinit.c attempt to remove the aforementioned assumption when building
for Cygwin.

> It seems they expect the fb-layer to be initialized before the
> shadow layer.
> The fb initialization was commented out by your patch. What was
> the reason for
> this.

Ah, because we already initialize the fb system in winscrinit.c.  We can't
have an fb layer because updates written directly to an fb layer would not
make it to the screen in a predictable fashion.  We can only have one layer:
a shadow based layer.

> - let shadowCloseScreen set the privPtr to NULL after freeing it
> (xfree will
>         ignore such pointers)
>
>     xfree (pScrPriv);
> +   shadowGetScrPriv(pScreen) = NULL;
>     return (*pScreen->CloseScreen) (i, pScreen);

Hmm... I thought I liked this one for a minute, but it likely wouldn't take
long until somebody came along and removed that statement in the interest
of, "saving code size"  :)

> - let LayerStartInit initialize the fb screen.
>     server start - close cycle worked for engine 1,2 and 4.
>     Engine 8 failed with winAllocateFBPrimaryDD-Could not lock
> primary surface
>     But it also does with the fb layer initialization commented out

In light of what I mentioned above, we really can't allow Layer to think
that it has an fb-based layer.  We can only have the shadow layer, so the
above patch won't work.

I think the safest way to fix the crash on shutdown is to just add another
Cygwinism to layerinit.c:

#if !defined(__CYGWIN__)
    /*
     * make sure the shadow layer is cleaned up as well
     */
    if (kind != LAYER_SHADOW)
	xfree (shadowGetScrPriv (pScreen));
#endif

As you mentioned, the CloseScreen procedure that layerCloseScreen wraps is
shadowCloseScreen (rather than the assumed fbCloseScreen), so it makes sense
that we would not want to try to free the shadow privates again.  I like
this patch because it will keep random block-heads from changing the
functionality on our platform.  What do you think?

On a side note, the ultimate fix would be to nag Keith Packard and Jim
Gettys (he's had something to do with RandR, so I suspect he may have helped
on layer) into removing the assumption that layer will always have two layer
types.  I don't seem to get a lot of mileage out of mailing the XFree86
devel list, so perhaps someone else would like to try...

Harold