[PATCH] render: Don't generate invalid pixman format when using a 24bpp framebuffer with a 32bit depth visual.

[Takashi Iwai]

At Fri, 15 Aug 2014 12:01:53 +0200,
Takashi Iwai wrote:
> 
> At Tue, 12 Aug 2014 16:03:54 -0700,
> Keith Packard wrote:
> > 
> > Robert Ancell <robert.ancell at canonical.com> writes:
> > 
> > > When using the fb backend at 24bpp it allows a visual with 32 bit
> > > depth.
> > 
> > That's not valid; depth must never be larger than bits per pixel.
> 
> Can we put some assert to catch such a bug?
> 
> > Please
> > fix your driver, don't break the insides of the X server.
> 
> Which driver do you mean, X or KMS?  It seems happening on both fbdev
> and modesetting X drivers when cirrus/mgag200 KMS is used with 24bpp.

Looking further down, I believe it's rather a X server problem, but
the real cause is in a different place.

The KMS driver advertises properly depth 24 and bpp 24.  However,
fbFinishScreenInit() has the following code:

    /*
     * By default, a 24bpp screen will use 32bpp images, this avoids
     * problems with many applications which just can't handle packed
     * pixels.  If you want real 24bit images, include a 24bpp
     * format in the pixmap formats
     */
    if (bpp == 24) {
        int f;

        imagebpp = 32;
        /*
         * Check to see if we're advertising a 24bpp image format,
         * in which case windows will use it in preference to a 32 bit
         * format.
         */
        for (f = 0; f < screenInfo.numPixmapFormats; f++) {
            if (screenInfo.formats[f].bitsPerPixel == 24) {
                imagebpp = 24;
                break;
            }
        }
    }
    if (imagebpp == 32) {
        fbGetScreenPrivate(pScreen)->win32bpp = bpp;
        fbGetScreenPrivate(pScreen)->pix32bpp = bpp;
    }
    else {
        fbGetScreenPrivate(pScreen)->win32bpp = 32;
        fbGetScreenPrivate(pScreen)->pix32bpp = 32;
    }

Since bpp = 24 and there is no format providing 24 bpp (fbdev takes
the default screen pixmap formats), imagebpp above is 32, thus
*->wind32bpp is set to bpp, i.e. 24.

This value is referred in fbCreateWindow():

    if (pWin->drawable.bitsPerPixel == 32)
        pWin->drawable.bitsPerPixel =
            fbGetScreenPrivate(pWin->drawable.pScreen)->win32bpp;

That is, bitsPerPixel is replaced from 32 to 24 in CreateWindow().
Hence this results in the combination of depth=32/bpp=24.  Ouch.

So, either we have to kill the hack in fb/* above, or accept that the
combo "depth 32 / bpp 24" can happen and fix the codes appropriately.
Or another better alternative?


Takashi