Weird glamor screen pixmap API

[Keith Packard]

Looking at Daniel's patches to fix screen resize in the modesetting
driver, I've come across an oddity in the glamor API which deserves some
exploration. glamor_egl_create_textured_screen_ext has this weird extra
'back_pixmap' argument. What the heck does it do? Let's see...

In the glamor egl code:

 glamor_egl_create_textured_screen_ext(... PixmapPtr *back_pixmap)

        This stores back_pixmap in glamor_egl, and then passes it to glamor_set_screen_pixmap


	glamor_egl->back_pixmap = back_pixmap;
        glamor_egl_create_textured_screen()
                glamor_set_screen_pixmap(screen_pixmap, glamor_egl->back_pixmap)
                
        
 glamor_egl_close_screen()

    if (glamor_egl->back_pixmap && *glamor_egl->back_pixmap) {
        pixmap_priv = glamor_get_pixmap_private(*glamor_egl->back_pixmap);
        if (pixmap_priv->base.image) {
            eglDestroyImageKHR(glamor_egl->display, pixmap_priv->base.image);
            pixmap_priv->base.image = NULL;
        }
    }


Ok, so the EGL code in glamor essentially holds a pointer to a pixmap
pointer, and when the screen is closed, destroys the EGL image
referenced by that pixmap. This seems pointless to me; the EGL image is
going to be destroyed when the pixmap is eventually destroyed in any
case, unless that pixmap is completely broken.

Now, what does glamor_set_screen_pixmap do with back_pixmap?

    glamor_priv->back_pixmap = back_pixmap;

Awesome! it also holds a pointer to the same thing. We'd hate to lose
track of it... Now, the only thing the upper level glamor code does with
this fine pointer is also in close_screen:

    if (glamor_priv->back_pixmap && *glamor_priv->back_pixmap)
        glamor_set_pixmap_private(*glamor_priv->back_pixmap, NULL);

So, at close screen time, glamor first destroys any egl image structure,
then clears the pixmap private.

The other place where eglDestroyImageKHR gets called is from
glamor_destroy_pixmap when dri3 is enabled. Why this call is conditional
on dri3 is a mystery.

Of the five users of glamor_egl_create_textured_screen_ext (intel,
modesetting, wayland, radeon and nouveau), only the intel driver passes
a pixmap pointer reference to it, so let's go look and see what it does.

The intel driver can have two scanout buffers allocated at a time; one
currently displayed, and one queued in the kernel to flip to. It's this
queued buffer which gets attached to the back_pixmap. And, of course,
that pixmap needs to get cleaned up on server reset, including freeing
the EGL image structure. Before the DRI3 code was added to glamor, it
seems pretty clear that the only way to get the EGL image freed was to
have  glamor do that, and the only way for that to happen was for glamor
to know that this pixmap was 'magic'. Now that glamor_destroy_pixmap has
a path to destroy the EGL image, the right approach is to just use it.

Ok, so I'm convinced that the right thing to do is:

 1) In glamor_destroy_pixmap, always destroy any EGL image.

 2) Ignore the back_pixmap argument to
    glamor_egl_create_textured_screen_ext.

Now, as to Daniel's question in his patch to make resize work:

 First, where I'm unsure. I haven't found a ...destroy... function, so we
 just always create?

First, an aside on the functions in play here:

-------------------

Note that a pile of glamor 'create' functions are misnamed/broken:

        glamor_egl_create_textured_pixmap

This one doesn't create a pixmap, nor take a texture as a parameter,
instead, it:

 * Wraps the object referenced by the provided handle (the
   interpretation of which depends on whether you have GEM or not) in an
   EGL image structure.

   For GEM backends, it does this by creating a global name for the
   object using flink (yay for security), and then passing that in as
   the EGL_DRM_BUFFER_MESA pointer to eglCreateImageKHR.

   For non-GEM backends, I suspect it just falls over as it hands the
   handle to eglCreateImageKHR as the EGL_DRM_BUFFER_MESA pointer.

 * Creates a texture object from the image

 * Creates an FBO for the texture

 * Sets that FBO as the pixmap storage, but only if the pixmap doesn't
   already have an FBO. If the pixmap has an FBO, the newly created FBO
   is dropped on the floor (there is a comment about not having another
   FBO attached to the pixmap, so at least you should have been warned).

 * Sets the pixmap image to the EGL image structure so that when you
   destroy the pixmap, you'll have a reference to the image and can
   destroy it as well.

	glamor_egl_create_textured_pixmap_from_gbm_bo

This is much the same as glamor_egl_create_textured_pixmap, but takes a
EGL_NATIVE_PIXMAP_KHR pointer, making the 'gbm_bo' part of the name a
lie when not using gbm, of course.

        glamor_egl_create_textured_screen
        glamor_egl_create_textured_screen_ext

These two don't create a screen, or even a pixmap. Instead, they use
glamor_egl_create_textured_pixmap to take the provided buffer handle and
set that as the buffer for the current screen pixmap.

        glamor_set_screen_pixmap

This one has nothing to do with setting the current screen pixmap (in
fact, we never change the screen pixmap in the current server
code). Instead, all it does is copy the size of the current screen
pixmap into the underlying pixmap private FBO size.

--------------------

Ok, so now that we know that glamor_egl_create_textured_screen_ext has
nothing to do with actually creating any storage, and is more about
hooking an externally created buffer to the existing screen pixmap, we
can start to reason about what it would mean when we do this multiple
times.

glamor_egl_create_textured_screen_ext doesn't do anything
interesting. glamor_egl_create_textured_pixmap (which it calls), creates
the EGL image and FBO then assigns them to the pixmap. The FBO
assignment checks for an existing FBO and destroys it. The EGL image
assignment doesn't.

I've got a series of three patches which fix this stuff; review
encouraged so that we can go ahead and get Daniel's patches merged and
then get the GBM stuff landed.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20141208/3f9c7975/attachment.sig>