[Libva] [PATCH intel-driver v2 3/3] extbuf: add support for userptr imports.

[Chris Wilson]

[cc'ed Trvtko since he is looking into the libdrm API for userptr and
associated issues.]

On Thu, Apr 02, 2015 at 04:30:24PM +0200, Gwenole Beauchesne wrote:
> Allow creating VA surfaces with userptr allocated buffers. This requires
> a recent enough version of libdrm (>= 2.4.57), but also a kernel (>= 3.16)
> which contains appropriate fixes for userptr.
> 
> v2: only request synchronized mappings (Chris Wilson).
> 
> Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne at intel.com>
> ---
>  configure.ac         | 19 ++++++++++++
>  src/i965_drv_video.c | 81 +++++++++++++++++++++++++++++++++++++++++-----------
>  src/i965_drv_video.h |  1 +
>  src/intel_driver.h   |  1 +
>  src/intel_memman.c   | 64 ++++++++++++++++++++++++++++++++++++++++-
>  src/intel_memman.h   |  7 +++++
>  6 files changed, 156 insertions(+), 17 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index d71a3cc..3c19cd2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -76,6 +76,25 @@ LIBDRM_VERSION=libdrm_version
>  PKG_CHECK_MODULES([DRM], [libdrm >= $LIBDRM_VERSION])
>  AC_SUBST(LIBDRM_VERSION)
>  
> +AC_CACHE_CHECK([whether libdrm supports userptr allocations],
> +    [ac_cv_libdrm_has_userptr],
> +    [saved_CPPFLAGS="$CPPFLAGS"
> +     saved_CFLAGS="$CFLAGS"
> +     CPPFLAGS="$CPPFLAGS $DRM_CFLAGS"
> +     LIBS="$LIBS $DRM_LIBS -ldrm_intel"
> +     AC_TRY_LINK(
> +         [#include <intel_bufmgr.h>],
> +         [drm_intel_bo_alloc_userptr(NULL, NULL, NULL, 0, 0, 0, 0)],
> +         [ac_cv_libdrm_has_userptr="yes"],
> +         [ac_cv_libdrm_has_userptr="no"])
> +     CPPFLAGS="$saved_CPPFLAGS"
> +     LIBS="$saved_LIBS"]
> +)
> +if test "$ac_cv_libdrm_has_userptr" = "yes"; then
> +    AC_DEFINE([HAVE_DRM_INTEL_USERPTR], [1],
> +        [Defined to 1 if libdrm supports userptr allocations])
> +fi

Did you consider

if pkg-config --exists 'libdrm_intel >= 2.4.57'; then
    AC_DEFINE([HAVE_DRM_INTEL_USERPTR], [1],
        [Defined to 1 if libdrm supports userptr allocations])
fi

? The full check for the function in the library should be foolproof,
but overkill?

> +drm_intel_bo *
> +do_import_userptr(struct intel_driver_data *intel, const char *name,
> +    void *data, size_t data_size, uint32_t va_flags)
> +{
> +#ifdef HAVE_DRM_INTEL_USERPTR
> +    uint32_t page_size, tiling_mode, flags = 0;
> +    drm_intel_bo *bo;
> +
> +    /* XXX: userptr is only supported for page-aligned allocations */
> +    page_size = getpagesize();
> +    if ((uintptr_t)data & (page_size - 1))
> +        return NULL;

I would pass through the pointer alignment checks to the kernel. Then if
they are relaxed or tightened, the library doesn't need to be adapted.

> +    tiling_mode = (va_flags & VA_SURFACE_EXTBUF_DESC_ENABLE_TILING) ?
> +        I915_TILING_Y : I915_TILING_NONE;

Hmm, if you going to allow I915_TILING_Y, you are going to need to
sanity check the allocation size. There is a kernel/libdrm patch coming
to allow you to specify a padding for underallocated user buffers. Does
the client ever specify width,height,pitch for their userptr surface?

> +    bo = drm_intel_bo_alloc_userptr(intel->bufmgr, name, data, tiling_mode, 0,
> +        data_size, flags);
> +    if (bo)
> +        return bo;

pad_to_size = 0;
if (tiling_mode != I915_TILING_MODE) {
	aligned_y = ALIGN(surface_height, 16);
	pitch = ALIGN(surface_width * surface_cpp, 128);
	pad_to_size = aligned_y * pitch;
}
drm_intel_bo_pad_to_size(bo, pad_to_size);

Actually, that is probably best done inside libdrm
drm_intel_bo_pad_for_tiling(bo, width, height, cpp, I915_TILING_Y);
since libdrm already has the information about required padding for
different tile modes.

> +#endif
> +    return NULL;
> +}
> +
> +drm_intel_bo *
> +intel_memman_import_userptr(struct intel_driver_data *intel, const char *name,
> +    void *data, size_t data_size, uint32_t va_flags)
> +{
> +    if (!intel_memman_has_userptr(intel))
> +        return NULL;
> +    return do_import_userptr(intel, name, data, data_size, va_flags);
> +}
> +
> +bool
> +intel_memman_has_userptr(struct intel_driver_data *intel)
> +{
> +    drm_intel_bo *bo;
> +    size_t page_size;
> +    void *page;
> +
> +    if (intel->userptr_disabled > 1) {
> +        intel->userptr_disabled = 1;
> +
> +        page_size = getpagesize();
> +        if (posix_memalign(&page, page_size, page_size) == 0) {
> +            bo = do_import_userptr(intel, "userptr test buffer",
> +                page, page_size, 0);
> +            if (bo) {
> +                drm_intel_bo_unreference(bo);
> +                intel->userptr_disabled  = 0;
> +            }
> +            free(page);
> +        }
> +    }
> +    return !intel->userptr_disabled;

This doesn't actually serve any purpose. It is redundant as libdrm_intel
does the very same check itself, and in both cases (if libdrm_intel
userptr sanity check fails, or if !intel_memman_has_userptr fails) you
just return NULL.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre