Hi Joe,

Thank you for the patch.

On Monday 31 Jul 2017 11:29:13 Joe Kniss wrote:
> New getfb2 functionality uses drm_mode_fb_cmd2 struct to be symmetric
> with addfb2.

What's the use case for this ? We haven't needed such an ioctl for so long
that it seemed to me that userspace doesn't really need it, but I could be
wrong.

> Also modifies *_fb_create_handle() calls to accept a
> format_plane_index so that handles for each plane can be generated.
> Previously, many *_fb_create_handle() calls simply defaulted to plane 0
> only.

And with this patch the amd/amdgpu, armada, gma500, i915, mediatek, msm,
nouveau and radeon drivers still do. Do none of them support multi-planar
formats ?

> Signed-off-by: Joe Kniss <djmk@google.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c |  5 +-
>  drivers/gpu/drm/armada/armada_fb.c          |  1 +
>  drivers/gpu/drm/drm_crtc_internal.h         |  2 +
>  drivers/gpu/drm/drm_fb_cma_helper.c         | 11 ++--
>  drivers/gpu/drm/drm_framebuffer.c           | 79 +++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_ioctl.c                 |  1 +
>  drivers/gpu/drm/exynos/exynos_drm_fb.c      |  7 ++-
>  drivers/gpu/drm/gma500/framebuffer.c        |  2 +
>  drivers/gpu/drm/i915/intel_display.c        |  1 +
>  drivers/gpu/drm/mediatek/mtk_drm_fb.c       |  1 +
>  drivers/gpu/drm/msm/msm_fb.c                |  5 +-
>  drivers/gpu/drm/nouveau/nouveau_display.c   |  1 +
>  drivers/gpu/drm/omapdrm/omap_fb.c           |  5 +-
>  drivers/gpu/drm/radeon/radeon_display.c     |  5 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  6 ++-
>  drivers/gpu/drm/tegra/fb.c                  |  9 +++-
>  include/drm/drm_framebuffer.h               |  1 +
>  include/uapi/drm/drm.h                      |  2 +
>  18 files changed, 127 insertions(+), 17 deletions(-)

[snip]

> diff --git a/drivers/gpu/drm/drm_framebuffer.c
> b/drivers/gpu/drm/drm_framebuffer.c index 28a0108a1ab8..67b3be1bedbc 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -24,6 +24,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_auth.h>
>  #include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem.h>
>
>  #include "drm_crtc_internal.h"

[snip]

> +/**
> + * drm_mode_getfb2 - get FB info
> + * @dev: drm device for the ioctl
> + * @data: data pointer for the ioctl
> + * @file_priv: drm file for the ioctl call
> + *
> + * Lookup the FB given its ID and return info about it.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_mode_getfb2(struct drm_device *dev,
> +                void *data, struct drm_file *file_priv)
> +{
> +     struct drm_mode_fb_cmd2 *r = data;
> +     struct drm_framebuffer *fb;
> +     int ret, i;
> +
> +     if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +             return -EINVAL;
> +
> +     fb = drm_framebuffer_lookup(dev, r->fb_id);
> +     if (!fb)
> +             return -ENOENT;
> +
> +     r->height = fb->height;
> +     r->width = fb->width;
> +     r->pixel_format = fb->format->format;
> +     for (i = 0; i < 4; ++i) {
> +             r->pitches[i] = fb->pitches[i];
> +             r->offsets[i] = fb->offsets[i];
> +             r->modifier[i] = fb->modifier;
> +             r->handles[i] = 0;
> +     }
> +
> +     for (i = 0; i < fb->format->num_planes; ++i) {
> +             if (fb->funcs->create_handle) {
> +                     if (drm_is_current_master(file_priv) ||
> +                         capable(CAP_SYS_ADMIN) ||
> +                         drm_is_control_client(file_priv)) {
> +                             ret = fb->funcs->create_handle(fb, i,
> file_priv,
> +
> &r->handles[i]);
> +                             if (ret)
> +                                     break;
> +                     } else {
> +                             /* GET_FB() is an unprivileged ioctl so we
> must
> +                              * not return a buffer-handle to non-master
> +                              * processes! For backwards-compatibility
> +                              * reasons, we cannot make GET_FB()
> privileged,
> +                              * so just return an invalid handle for
> +                              * non-masters. */

There's no backward compatibility to handle here, just make it privileged if
it has to be.

> +                             r->handles[i] = 0;
> +                             ret = 0;
> +                     }
> +             } else {
> +                     ret = -ENODEV;
> +                     break;
> +             }
> +     }
> +
> +     /* If handle creation failed, delete/dereference any that were made.
*/
> +     if (ret) {
> +             for (i = 0; i < 4; ++i) {
> +                     if (r->handles[i])
> +                             drm_gem_handle_delete(file_priv, r-
>handles[i]);

My initial reaction to this was to reply that not all drivers use GEM, but
after some investigation it seems they actually do. If that's really the case,
we could get rid of the .create_handle() operation completely, which should
simplify the implementation of both DRM_IOCTL_MODE_GETFB and
DRM_IOCTL_MODE_GETFB2.

> +                     r->handles[i] = 0;
> +             }
> +     }
> +
> +     drm_framebuffer_unreference(fb);
> +
> +     return ret;
> +}

[snip]

> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> b/drivers/gpu/drm/omapdrm/omap_fb.c index 29dc677dd4d3..a982c72a773e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -90,12 +90,15 @@ struct omap_framebuffer {
>  };
>
>  static int omap_framebuffer_create_handle(struct drm_framebuffer *fb,
> +             unsigned int format_plane_index,
>               struct drm_file *file_priv,
>               unsigned int *handle)
>  {
>       struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
> +     if (format_plane_index >= 4 || !omap_fb->planes[format_plane_index])

The first check shouldn't be needed, the core should never call this operation
with an index >= 4.

> +             return -ENOENT;
>       return drm_gem_handle_create(file_priv,
> -                     omap_fb->planes[0].bo, handle);
> +                     omap_fb->planes[format_plane_index].bo, handle);
>  }

[snip]

> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c52843bc70..c81c75335cca 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -814,6 +814,8 @@ extern "C" {
>  #define DRM_IOCTL_MODE_CREATEPROPBLOB        DRM_IOWR(0xBD, struct
> drm_mode_create_blob)
> #define DRM_IOCTL_MODE_DESTROYPROPBLOB        DRM_IOWR(0xBE, struct
> drm_mode_destroy_blob)
>
> +#define DRM_IOCTL_MODE_GETFB2  DRM_IOWR(0xC4, struct drm_mode_fb_cmd2)
> +

Which tree is this based on ? The last defined ioctl
(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE) is 0xC2 in drm-next and drm-misc-next.

>  /**
>   * Device specific ioctls should only be in their respective headers
>   * The device specific ioctl range is from 0x40 to 0x9f.

--
Regards,

Laurent Pinchart