[Mesa-dev] [PATCH kmscube 5/6] common: Use libdrm AddFB with modifiers

[Emil Velikov]

On 13 April 2017 at 19:22, Ben Widawsky <ben at bwidawsk.net> wrote:
> Note: nothing happens here yet since LINEAR == 0.

Suggestion for the subject

common: use drmModeAddFB2* API over the legacy drmModeAddFB one


> ---
>  configure.ac |  2 +-
>  drm-common.c | 37 +++++++++++++++++++++++++++++++++----
>  2 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 33167e4..f564ef3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -35,7 +35,7 @@ AC_PROG_CC
>  m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])])
>
>  # Obtain compiler/linker options for depedencies
> -PKG_CHECK_MODULES(DRM, libdrm)
> +PKG_CHECK_MODULES(DRM, [libdrm >= 2.4.71])
>  PKG_CHECK_MODULES(GBM, gbm >= 13.0)
>  PKG_CHECK_MODULES(EGL, egl)
>  PKG_CHECK_MODULES(GLES2, glesv2)
> diff --git a/drm-common.c b/drm-common.c
> index b69ed70..eb460df 100644
> --- a/drm-common.c
> +++ b/drm-common.c
> @@ -46,7 +46,7 @@ struct drm_fb * drm_fb_get_from_bo(struct gbm_bo *bo)
>  {
>         int drm_fd = gbm_device_get_fd(gbm_bo_get_device(bo));
>         struct drm_fb *fb = gbm_bo_get_user_data(bo);
> -       uint32_t width, height, stride, handle;
> +       uint32_t width, height, strides[4]={0}, handles[4] = {0}, offsets[4] = {0}, flags = 0;
Nit: Add spaces around = for strides[].

>         int ret;
>
>         if (fb)
> @@ -57,10 +57,39 @@ struct drm_fb * drm_fb_get_from_bo(struct gbm_bo *bo)
>
>         width = gbm_bo_get_width(bo);
>         height = gbm_bo_get_height(bo);
> -       stride = gbm_bo_get_stride(bo);
> -       handle = gbm_bo_get_handle(bo).u32;
>
> -       ret = drmModeAddFB(drm_fd, width, height, 24, 32, stride, handle, &fb->fb_id);
> +#ifndef HAVE_GBM_MODIFIERS
> +       strides[0] = gbm_bo_get_stride(bo);
> +       handles[0] = gbm_bo_get_handle(bo).u32;
These two should go in the fallback path.

> +       ret = -1;
> +#else
> +       uint64_t modifiers[4] = {0};
> +       modifiers[0] = gbm_bo_get_modifier(bo);
> +       const int num_planes = gbm_bo_get_plane_count(bo);
> +       for (int i = 0; i < num_planes; i++) {
> +               strides[i] = gbm_bo_get_stride_for_plane(bo, i);
> +               handles[i] = gbm_bo_get_handle(bo).u32;
> +               offsets[i] = gbm_bo_get_offset(bo, i);
> +               modifiers[i] = modifiers[0];
> +       }
> +
> +       if (modifiers[0]) {
> +               flags = DRM_MODE_FB_MODIFIERS;
> +               printf("Using modifier %lx\n", modifiers[0]);
> +       }
> +
> +       ret = drmModeAddFB2WithModifiers(drm_fd, width, height,
> +                       DRM_FORMAT_XRGB8888, handles, strides, offsets,
> +                       modifiers, &fb->fb_id, flags);
> +#endif
> +       if (ret) {
> +               if (flags)
> +                       fprintf(stderr, "Modifiers failed!\n");
> +               flags = 0;
Drop this line or use it in drmModeAddFB2?

Here we'd want to correctly initialise all of strides[] handles[],
since they may contain the 'wrong' values from above.
it's a bit pedantic I admit, but should make the code easier to read
and will prevent explosions in [buggy] kernel modules.

-Emil