[RFC xserver v4 12/14] glamor: Implement GetSupportedModifiers

[Michel Dänzer]

On 16/10/17 07:02 AM, Louis-Francis Ratté-Boulianne wrote:
> Implement function added in DRI3 v1.1.
> 
> A newest version of libepoxy (>= 1.4.4) is required as earlier
> versions use a problematic version of Khronos
> EXT_image_dma_buf_import_modifiers spec.

It might be better to split the Present, modesetting and Xwayland
changes into separate patches.


> v4: Only send scanout-supported modifiers if flipping is possible

What's the reason for that? Scanout compatible tiling is probably
better than linear even if the buffer isn't scanned out directly,
isn't it?


> +_X_EXPORT Bool
> +glamor_get_drawable_modifiers(DrawablePtr draw, CARD32 format,
> +                              CARD32 *num_modifiers, uint64_t **modifiers)
> +{
> +    struct glamor_screen_private *glamor_priv =
> +        glamor_get_screen_private(draw->pScreen);
> +
> +    if (glamor_priv->get_drawable_modifiers) {
> +        return glamor_priv->get_drawable_modifiers(draw, format,
> +                                                   num_modifiers, modifiers);
> +    }
> +    *num_modifiers = 0;
> +    return TRUE;
> +}

I think the logic would be clearer as:

    if (!glamor_priv->get_drawable_modifiers) {
        *num_modifiers = 0;
        return TRUE;
    }

    return glamor_priv->get_drawable_modifiers(draw, format,
                                               num_modifiers, modifiers);
}


> diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
> index cb1f03dc9..4e60569a1 100644
> --- a/glamor/glamor_egl.c
> +++ b/glamor/glamor_egl.c
> @@ -488,6 +488,95 @@ glamor_pixmap_from_fds(ScreenPtr screen,
>      return pixmap;
>  }
>  
> +_X_EXPORT Bool
> +glamor_get_formats(ScreenPtr screen,
> +                   CARD32 *num_formats, CARD32 **formats)
> +{
> +#ifdef GLAMOR_HAS_EGL_QUERY_DMABUF
> +    struct glamor_egl_screen_private *glamor_egl;
> +    EGLint num;
> +
> +    glamor_egl = glamor_egl_get_screen_private(xf86ScreenToScrn(screen));
> +
> +    if (!glamor_egl->dmabuf_capable)
> +        return FALSE;
> +
> +    if (!eglQueryDmaBufFormatsEXT(glamor_egl->display, 0, NULL, &num)) {
> +        *num_formats = 0;
> +        return FALSE;
> +    }
> +
> +    if (num == 0) {
> +        *num_formats = 0;
> +        return TRUE;
> +    }
> +
> +    *formats = calloc(num, sizeof(CARD32));
> +    if (*formats == NULL) {
> +        *num_formats = 0;
> +        return FALSE;
> +    }
> +
> +    if (!eglQueryDmaBufFormatsEXT(glamor_egl->display, num,
> +                                  (EGLint *) *formats, &num)) {
> +        *num_formats = 0;
> +        free(*formats);
> +        return FALSE;
> +    }
> +
> +    *num_formats = num;
> +    return TRUE;
> +#else
> +    *num_formats = 0;
> +    return TRUE;
> +#endif
> +}

Seems weird to return TRUE from glamor_get_formats/modifiers when
GLAMOR_HAS_EGL_QUERY_DMABUF isn't defined. It would probably be better
not to set those hooks in dri3_screen_info_rec in the first place in that
case.


> diff --git a/hw/xfree86/common/xf86Mode.c b/hw/xfree86/common/xf86Mode.c
> index 3053e53f1..f11c545e2 100644
> --- a/hw/xfree86/common/xf86Mode.c
> +++ b/hw/xfree86/common/xf86Mode.c
> @@ -86,6 +86,7 @@
>  
>  #include <X11/X.h>
>  #include "xf86Modes.h"
> +#include "xf86Crtc.h"
>  #include "os.h"
>  #include "servermd.h"
>  #include "globals.h"

It's odd to add this #include without any other changes in this file.
Why is it needed?


> diff --git a/present/present.c b/present/present.c
> index 4d38d2d84..3efae348b 100644
> --- a/present/present.c
> +++ b/present/present.c
> @@ -611,6 +611,44 @@ present_check_flip_window (WindowPtr window)
>      }
>  }
>  
> +Bool
> +present_can_window_flip(WindowPtr window)
> +{
> +    ScreenPtr                   screen = window->drawable.pScreen;
> +    PixmapPtr                   window_pixmap;
> +    WindowPtr                   root = screen->root;
> +    present_screen_priv_ptr     screen_priv = present_screen_priv(screen);
> +
> +    if (!screen_priv)
> +        return FALSE;
> +
> +    if (!screen_priv->info)
> +        return FALSE;
> +
> +    /* Check to see if the driver supports flips at all */
> +    if (!screen_priv->info->flip)
> +        return FALSE;
> +
> +    /* Make sure the window hasn't been redirected with Composite */
> +    window_pixmap = screen->GetWindowPixmap(window);
> +    if (window_pixmap != screen->GetScreenPixmap(screen) &&
> +        window_pixmap != screen_priv->flip_pixmap &&
> +        window_pixmap != present_flip_pending_pixmap(screen))
> +        return FALSE;

Could short-cut this a little if the window is already flipping:

    window_pixmap = screen->GetWindowPixmap(window);
    if (window_pixmap == screen_priv->flip_pixmap ||
        window_pixmap == present_flip_pending_pixmap(screen))
        return TRUE;

    if (window_pixmap != screen->GetScreenPixmap(screen))
        return FALSE;

    [...]


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer