Need a drmModeGetFB2() or similar query

Need a drmModeGetFB2() or similar query

[Joe Kniss]

[-- Attachment #1.1: Type: text/plain, Size: 923 bytes --]

Hi,
  I am a recent addition to Google's ChromeOS gfx team.   I am currently
working on display testing and reporting.   An important part of this is
our screen capture tool, which works by querying drm for crtcs, planes, and
fbs.  Unfortunately, there is only limited information available via
drmModeGetFB(), which often wrong information when drmModeAddFB2() was used
to create the fbs.   For example, if the pixel format is NV12 or YUV420,
the fb returned knows nothing about the additional buffer planes required
by these formats.   Ideally, we would like a function (e.g. drmModeGetFB2)
to return information symmetric with drmModeAddFB2 including the pixel
format id, buffer plane information etc.

I am curious what would be the best way to make this work.  I'm happy to
create the patch, but need some guidance first.

cheers,
-j

-- 
Dr. Joe Michael Kniss |  Google ChromeOS |  djmk@google.com   |
1-801-898-7977

[-- Attachment #1.2: Type: text/html, Size: 2480 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

[Joe Kniss]

New getfb2 functionality uses drm_mode_fb_cmd2 struct to be symmetric
with addfb2.   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.

Signed-off-by: Joe Kniss <djmk-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 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(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 39fc388f222a..c77c1cd265a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -566,8 +566,9 @@ static void amdgpu_user_framebuffer_destroy(struct drm_framebuffer *fb)
 }
 
 static int amdgpu_user_framebuffer_create_handle(struct drm_framebuffer *fb,
-						  struct drm_file *file_priv,
-						  unsigned int *handle)
+						 unsigned int plane_index,
+						 struct drm_file *file_priv,
+						 unsigned int *handle)
 {
 	struct amdgpu_framebuffer *amdgpu_fb = to_amdgpu_framebuffer(fb);
 
diff --git a/drivers/gpu/drm/armada/armada_fb.c b/drivers/gpu/drm/armada/armada_fb.c
index 2a7eb6817c36..9f237544f6c5 100644
--- a/drivers/gpu/drm/armada/armada_fb.c
+++ b/drivers/gpu/drm/armada/armada_fb.c
@@ -23,6 +23,7 @@ static void armada_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int armada_fb_create_handle(struct drm_framebuffer *fb,
+	unsigned int format_plane_index,
 	struct drm_file *dfile, unsigned int *handle)
 {
 	struct armada_framebuffer *dfb = drm_fb_to_armada_fb(fb);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 955c5690bf64..ec8d913240fe 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -170,6 +170,8 @@ int drm_mode_rmfb(struct drm_device *dev,
 		  void *data, struct drm_file *file_priv);
 int drm_mode_getfb(struct drm_device *dev,
 		   void *data, struct drm_file *file_priv);
+int drm_mode_getfb2(struct drm_device *dev,
+		   void *data, struct drm_file *file_priv);
 int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
 			   void *data, struct drm_file *file_priv);
 
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 596fabf18c3e..5fd7bcc2c6d1 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -110,13 +110,16 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb)
 }
 EXPORT_SYMBOL(drm_fb_cma_destroy);
 
-int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
-	struct drm_file *file_priv, unsigned int *handle)
+static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+				    unsigned int format_plane_index,
+				    struct drm_file *file_priv,
+				    unsigned int *handle)
 {
 	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
-
+	if (format_plane_index >= 4 || !fb_dma->obj[format_plane_index])
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-			&fb_cma->obj[0]->base, handle);
+			&fb_cma->obj[format_plane_index]->base, handle);
 }
 EXPORT_SYMBOL(drm_fb_cma_create_handle);
 
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"
 
@@ -438,7 +439,7 @@ int drm_mode_getfb(struct drm_device *dev,
 	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, file_priv,
+			ret = fb->funcs->create_handle(fb, 0, file_priv,
 						       &r->handle);
 		} else {
 			/* GET_FB() is an unprivileged ioctl so we must not
@@ -458,6 +459,82 @@ int drm_mode_getfb(struct drm_device *dev,
 	return ret;
 }
 
+/**
+ * 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. */
+				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]);
+			r->handles[i] = 0;
+		}
+	}
+
+	drm_framebuffer_unreference(fb);
+
+	return ret;
+}
+
 /**
  * drm_mode_dirtyfb_ioctl - flush frontbuffer rendering on an FB
  * @dev: drm device for the ioctl
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a7c61c23685a..a9b578dc5d17 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -627,6 +627,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETPROPERTY, drm_mode_connector_property_set_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPBLOB, drm_mode_getblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB, drm_mode_getfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB2, drm_mode_getfb2, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB2, drm_mode_addfb2, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index c77a5aced81a..5e8b774dc1af 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -88,13 +88,16 @@ static void exynos_drm_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int exynos_drm_fb_create_handle(struct drm_framebuffer *fb,
+				       unsigned int format_plane_index,
 					struct drm_file *file_priv,
 					unsigned int *handle)
 {
 	struct exynos_drm_fb *exynos_fb = to_exynos_fb(fb);
-
+	if (format_plane_index >= MAX_FB_BUFFER ||
+	    !exynos_fb->exynos_gem[format_plane_index])
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-				     &exynos_fb->exynos_gem[0]->base, handle);
+		     &exynos_fb->exynos_gem[format_plane_index]->base, handle);
 }
 
 static const struct drm_framebuffer_funcs exynos_drm_fb_funcs = {
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index ffe6b4ffa1a8..c221544b4c6a 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -42,6 +42,7 @@
 
 static void psb_user_framebuffer_destroy(struct drm_framebuffer *fb);
 static int psb_user_framebuffer_create_handle(struct drm_framebuffer *fb,
+					      unsigned int format_plane_index,
 					      struct drm_file *file_priv,
 					      unsigned int *handle);
 
@@ -619,6 +620,7 @@ static void psbfb_output_poll_changed(struct drm_device *dev)
  *	the work for us
  */
 static int psb_user_framebuffer_create_handle(struct drm_framebuffer *fb,
+					      unsigned int plane_index,
 					      struct drm_file *file_priv,
 					      unsigned int *handle)
 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3282b0f4b134..54fdd30b0598 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15853,6 +15853,7 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 }
 
 static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
+						unsigned int format_plane_index,
 						struct drm_file *file,
 						unsigned int *handle)
 {
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
index d4246c9dceae..8343144ba1cd 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
@@ -44,6 +44,7 @@ struct drm_gem_object *mtk_fb_get_gem_obj(struct drm_framebuffer *fb)
 }
 
 static int mtk_drm_fb_create_handle(struct drm_framebuffer *fb,
+				    unsigned int format_plane_index,
 				    struct drm_file *file_priv,
 				    unsigned int *handle)
 {
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 5cf165c9c3a9..de92fc8a7a1c 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -30,12 +30,15 @@ struct msm_framebuffer {
 
 
 static int msm_framebuffer_create_handle(struct drm_framebuffer *fb,
+		unsigned int format_plane_index,
 		struct drm_file *file_priv,
 		unsigned int *handle)
 {
 	struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
+	if (format_plane_index >= MAX_PLANE)
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-			msm_fb->planes[0], handle);
+			msm_fb->planes[format_plane_index], handle);
 }
 
 static void msm_framebuffer_destroy(struct drm_framebuffer *fb)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 72fdba1a1c5d..bc30c0d3d9cd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -237,6 +237,7 @@ nouveau_user_framebuffer_destroy(struct drm_framebuffer *drm_fb)
 
 static int
 nouveau_user_framebuffer_create_handle(struct drm_framebuffer *drm_fb,
+				       unsigned int format_plane_index,
 				       struct drm_file *file_priv,
 				       unsigned int *handle)
 {
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])
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-			omap_fb->planes[0].bo, handle);
+			omap_fb->planes[format_plane_index].bo, handle);
 }
 
 static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index aea8b62835a4..2188e6341cd9 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1306,8 +1306,9 @@ static void radeon_user_framebuffer_destroy(struct drm_framebuffer *fb)
 }
 
 static int radeon_user_framebuffer_create_handle(struct drm_framebuffer *fb,
-						  struct drm_file *file_priv,
-						  unsigned int *handle)
+						 unsigned int plane_index,
+						 struct drm_file *file_priv,
+						 unsigned int *handle)
 {
 	struct radeon_framebuffer *radeon_fb = to_radeon_framebuffer(fb);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index c9ccdf8f44bb..206d93249519 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -55,13 +55,15 @@ static void rockchip_drm_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int rockchip_drm_fb_create_handle(struct drm_framebuffer *fb,
+					 unsigned int format_plane_index,
 					 struct drm_file *file_priv,
 					 unsigned int *handle)
 {
 	struct rockchip_drm_fb *rockchip_fb = to_rockchip_fb(fb);
-
+	if (format_plane_index >= ROCKCHIP_MAX_FB_BUFFER)
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-				     rockchip_fb->obj[0], handle);
+				  rockchip_fb->obj[format_plane_index], handle);
 }
 
 static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
index f142f6a4db25..ff206b143503 100644
--- a/drivers/gpu/drm/tegra/fb.c
+++ b/drivers/gpu/drm/tegra/fb.c
@@ -81,11 +81,16 @@ static void tegra_fb_destroy(struct drm_framebuffer *framebuffer)
 }
 
 static int tegra_fb_create_handle(struct drm_framebuffer *framebuffer,
+				  unsigned int format_plane_index,
 				  struct drm_file *file, unsigned int *handle)
 {
 	struct tegra_fb *fb = to_tegra_fb(framebuffer);
-
-	return drm_gem_handle_create(file, &fb->planes[0]->gem, handle);
+	if (format_plane_index >= fb->num_planes ||
+	    !fb->planes[format_plane_index])
+		return -ENOENT;
+	return drm_gem_handle_create(file,
+				     &fb->planes[format_plane_index]->gem,
+				     handle);
 }
 
 static const struct drm_framebuffer_funcs tegra_fb_funcs = {
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index dd1e3e99dcff..2fb398cb4646 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -66,6 +66,7 @@ struct drm_framebuffer_funcs {
 	 * 0 on success or a negative error code on failure.
 	 */
 	int (*create_handle)(struct drm_framebuffer *fb,
+			     unsigned int plane_index,
 			     struct drm_file *file_priv,
 			     unsigned int *handle);
 	/**
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)
+
 /**
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

[Joe Kniss]

New getfb2 functionality uses drm_mode_fb_cmd2 struct to be symmetric
with addfb2.   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.

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(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 39fc388f222a..c77c1cd265a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -566,8 +566,9 @@ static void amdgpu_user_framebuffer_destroy(struct drm_framebuffer *fb)
 }
 
 static int amdgpu_user_framebuffer_create_handle(struct drm_framebuffer *fb,
-						  struct drm_file *file_priv,
-						  unsigned int *handle)
+						 unsigned int plane_index,
+						 struct drm_file *file_priv,
+						 unsigned int *handle)
 {
 	struct amdgpu_framebuffer *amdgpu_fb = to_amdgpu_framebuffer(fb);
 
diff --git a/drivers/gpu/drm/armada/armada_fb.c b/drivers/gpu/drm/armada/armada_fb.c
index 2a7eb6817c36..9f237544f6c5 100644
--- a/drivers/gpu/drm/armada/armada_fb.c
+++ b/drivers/gpu/drm/armada/armada_fb.c
@@ -23,6 +23,7 @@ static void armada_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int armada_fb_create_handle(struct drm_framebuffer *fb,
+	unsigned int format_plane_index,
 	struct drm_file *dfile, unsigned int *handle)
 {
 	struct armada_framebuffer *dfb = drm_fb_to_armada_fb(fb);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 955c5690bf64..ec8d913240fe 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -170,6 +170,8 @@ int drm_mode_rmfb(struct drm_device *dev,
 		  void *data, struct drm_file *file_priv);
 int drm_mode_getfb(struct drm_device *dev,
 		   void *data, struct drm_file *file_priv);
+int drm_mode_getfb2(struct drm_device *dev,
+		   void *data, struct drm_file *file_priv);
 int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
 			   void *data, struct drm_file *file_priv);
 
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 596fabf18c3e..5fd7bcc2c6d1 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -110,13 +110,16 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb)
 }
 EXPORT_SYMBOL(drm_fb_cma_destroy);
 
-int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
-	struct drm_file *file_priv, unsigned int *handle)
+static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+				    unsigned int format_plane_index,
+				    struct drm_file *file_priv,
+				    unsigned int *handle)
 {
 	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
-
+	if (format_plane_index >= 4 || !fb_dma->obj[format_plane_index])
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-			&fb_cma->obj[0]->base, handle);
+			&fb_cma->obj[format_plane_index]->base, handle);
 }
 EXPORT_SYMBOL(drm_fb_cma_create_handle);
 
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"
 
@@ -438,7 +439,7 @@ int drm_mode_getfb(struct drm_device *dev,
 	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, file_priv,
+			ret = fb->funcs->create_handle(fb, 0, file_priv,
 						       &r->handle);
 		} else {
 			/* GET_FB() is an unprivileged ioctl so we must not
@@ -458,6 +459,82 @@ int drm_mode_getfb(struct drm_device *dev,
 	return ret;
 }
 
+/**
+ * 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. */
+				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]);
+			r->handles[i] = 0;
+		}
+	}
+
+	drm_framebuffer_unreference(fb);
+
+	return ret;
+}
+
 /**
  * drm_mode_dirtyfb_ioctl - flush frontbuffer rendering on an FB
  * @dev: drm device for the ioctl
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a7c61c23685a..a9b578dc5d17 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -627,6 +627,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETPROPERTY, drm_mode_connector_property_set_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPBLOB, drm_mode_getblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB, drm_mode_getfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB2, drm_mode_getfb2, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB2, drm_mode_addfb2, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index c77a5aced81a..5e8b774dc1af 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -88,13 +88,16 @@ static void exynos_drm_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int exynos_drm_fb_create_handle(struct drm_framebuffer *fb,
+				       unsigned int format_plane_index,
 					struct drm_file *file_priv,
 					unsigned int *handle)
 {
 	struct exynos_drm_fb *exynos_fb = to_exynos_fb(fb);
-
+	if (format_plane_index >= MAX_FB_BUFFER ||
+	    !exynos_fb->exynos_gem[format_plane_index])
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-				     &exynos_fb->exynos_gem[0]->base, handle);
+		     &exynos_fb->exynos_gem[format_plane_index]->base, handle);
 }
 
 static const struct drm_framebuffer_funcs exynos_drm_fb_funcs = {
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index ffe6b4ffa1a8..c221544b4c6a 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -42,6 +42,7 @@
 
 static void psb_user_framebuffer_destroy(struct drm_framebuffer *fb);
 static int psb_user_framebuffer_create_handle(struct drm_framebuffer *fb,
+					      unsigned int format_plane_index,
 					      struct drm_file *file_priv,
 					      unsigned int *handle);
 
@@ -619,6 +620,7 @@ static void psbfb_output_poll_changed(struct drm_device *dev)
  *	the work for us
  */
 static int psb_user_framebuffer_create_handle(struct drm_framebuffer *fb,
+					      unsigned int plane_index,
 					      struct drm_file *file_priv,
 					      unsigned int *handle)
 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3282b0f4b134..54fdd30b0598 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15853,6 +15853,7 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 }
 
 static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
+						unsigned int format_plane_index,
 						struct drm_file *file,
 						unsigned int *handle)
 {
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
index d4246c9dceae..8343144ba1cd 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
@@ -44,6 +44,7 @@ struct drm_gem_object *mtk_fb_get_gem_obj(struct drm_framebuffer *fb)
 }
 
 static int mtk_drm_fb_create_handle(struct drm_framebuffer *fb,
+				    unsigned int format_plane_index,
 				    struct drm_file *file_priv,
 				    unsigned int *handle)
 {
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 5cf165c9c3a9..de92fc8a7a1c 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -30,12 +30,15 @@ struct msm_framebuffer {
 
 
 static int msm_framebuffer_create_handle(struct drm_framebuffer *fb,
+		unsigned int format_plane_index,
 		struct drm_file *file_priv,
 		unsigned int *handle)
 {
 	struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
+	if (format_plane_index >= MAX_PLANE)
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-			msm_fb->planes[0], handle);
+			msm_fb->planes[format_plane_index], handle);
 }
 
 static void msm_framebuffer_destroy(struct drm_framebuffer *fb)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 72fdba1a1c5d..bc30c0d3d9cd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -237,6 +237,7 @@ nouveau_user_framebuffer_destroy(struct drm_framebuffer *drm_fb)
 
 static int
 nouveau_user_framebuffer_create_handle(struct drm_framebuffer *drm_fb,
+				       unsigned int format_plane_index,
 				       struct drm_file *file_priv,
 				       unsigned int *handle)
 {
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])
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-			omap_fb->planes[0].bo, handle);
+			omap_fb->planes[format_plane_index].bo, handle);
 }
 
 static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index aea8b62835a4..2188e6341cd9 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1306,8 +1306,9 @@ static void radeon_user_framebuffer_destroy(struct drm_framebuffer *fb)
 }
 
 static int radeon_user_framebuffer_create_handle(struct drm_framebuffer *fb,
-						  struct drm_file *file_priv,
-						  unsigned int *handle)
+						 unsigned int plane_index,
+						 struct drm_file *file_priv,
+						 unsigned int *handle)
 {
 	struct radeon_framebuffer *radeon_fb = to_radeon_framebuffer(fb);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index c9ccdf8f44bb..206d93249519 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -55,13 +55,15 @@ static void rockchip_drm_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int rockchip_drm_fb_create_handle(struct drm_framebuffer *fb,
+					 unsigned int format_plane_index,
 					 struct drm_file *file_priv,
 					 unsigned int *handle)
 {
 	struct rockchip_drm_fb *rockchip_fb = to_rockchip_fb(fb);
-
+	if (format_plane_index >= ROCKCHIP_MAX_FB_BUFFER)
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-				     rockchip_fb->obj[0], handle);
+				  rockchip_fb->obj[format_plane_index], handle);
 }
 
 static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
index f142f6a4db25..ff206b143503 100644
--- a/drivers/gpu/drm/tegra/fb.c
+++ b/drivers/gpu/drm/tegra/fb.c
@@ -81,11 +81,16 @@ static void tegra_fb_destroy(struct drm_framebuffer *framebuffer)
 }
 
 static int tegra_fb_create_handle(struct drm_framebuffer *framebuffer,
+				  unsigned int format_plane_index,
 				  struct drm_file *file, unsigned int *handle)
 {
 	struct tegra_fb *fb = to_tegra_fb(framebuffer);
-
-	return drm_gem_handle_create(file, &fb->planes[0]->gem, handle);
+	if (format_plane_index >= fb->num_planes ||
+	    !fb->planes[format_plane_index])
+		return -ENOENT;
+	return drm_gem_handle_create(file,
+				     &fb->planes[format_plane_index]->gem,
+				     handle);
 }
 
 static const struct drm_framebuffer_funcs tegra_fb_funcs = {
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index dd1e3e99dcff..2fb398cb4646 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -66,6 +66,7 @@ struct drm_framebuffer_funcs {
 	 * 0 on success or a negative error code on failure.
 	 */
 	int (*create_handle)(struct drm_framebuffer *fb,
+			     unsigned int plane_index,
 			     struct drm_file *file_priv,
 			     unsigned int *handle);
 	/**
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)
+
 /**
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

[Joe Kniss]

New getfb2 functionality uses drm_mode_fb_cmd2 struct to be symmetric
with addfb2.   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.

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(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 39fc388f222a..c77c1cd265a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -566,8 +566,9 @@ static void amdgpu_user_framebuffer_destroy(struct drm_framebuffer *fb)
 }
 
 static int amdgpu_user_framebuffer_create_handle(struct drm_framebuffer *fb,
-						  struct drm_file *file_priv,
-						  unsigned int *handle)
+						 unsigned int plane_index,
+						 struct drm_file *file_priv,
+						 unsigned int *handle)
 {
 	struct amdgpu_framebuffer *amdgpu_fb = to_amdgpu_framebuffer(fb);
 
diff --git a/drivers/gpu/drm/armada/armada_fb.c b/drivers/gpu/drm/armada/armada_fb.c
index 2a7eb6817c36..9f237544f6c5 100644
--- a/drivers/gpu/drm/armada/armada_fb.c
+++ b/drivers/gpu/drm/armada/armada_fb.c
@@ -23,6 +23,7 @@ static void armada_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int armada_fb_create_handle(struct drm_framebuffer *fb,
+	unsigned int format_plane_index,
 	struct drm_file *dfile, unsigned int *handle)
 {
 	struct armada_framebuffer *dfb = drm_fb_to_armada_fb(fb);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 955c5690bf64..ec8d913240fe 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -170,6 +170,8 @@ int drm_mode_rmfb(struct drm_device *dev,
 		  void *data, struct drm_file *file_priv);
 int drm_mode_getfb(struct drm_device *dev,
 		   void *data, struct drm_file *file_priv);
+int drm_mode_getfb2(struct drm_device *dev,
+		   void *data, struct drm_file *file_priv);
 int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
 			   void *data, struct drm_file *file_priv);
 
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 596fabf18c3e..5fd7bcc2c6d1 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -110,13 +110,16 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb)
 }
 EXPORT_SYMBOL(drm_fb_cma_destroy);
 
-int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
-	struct drm_file *file_priv, unsigned int *handle)
+static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+				    unsigned int format_plane_index,
+				    struct drm_file *file_priv,
+				    unsigned int *handle)
 {
 	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
-
+	if (format_plane_index >= 4 || !fb_dma->obj[format_plane_index])
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-			&fb_cma->obj[0]->base, handle);
+			&fb_cma->obj[format_plane_index]->base, handle);
 }
 EXPORT_SYMBOL(drm_fb_cma_create_handle);
 
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"
 
@@ -438,7 +439,7 @@ int drm_mode_getfb(struct drm_device *dev,
 	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, file_priv,
+			ret = fb->funcs->create_handle(fb, 0, file_priv,
 						       &r->handle);
 		} else {
 			/* GET_FB() is an unprivileged ioctl so we must not
@@ -458,6 +459,82 @@ int drm_mode_getfb(struct drm_device *dev,
 	return ret;
 }
 
+/**
+ * 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. */
+				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]);
+			r->handles[i] = 0;
+		}
+	}
+
+	drm_framebuffer_unreference(fb);
+
+	return ret;
+}
+
 /**
  * drm_mode_dirtyfb_ioctl - flush frontbuffer rendering on an FB
  * @dev: drm device for the ioctl
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a7c61c23685a..a9b578dc5d17 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -627,6 +627,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETPROPERTY, drm_mode_connector_property_set_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPBLOB, drm_mode_getblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB, drm_mode_getfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB2, drm_mode_getfb2, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB2, drm_mode_addfb2, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index c77a5aced81a..5e8b774dc1af 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -88,13 +88,16 @@ static void exynos_drm_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int exynos_drm_fb_create_handle(struct drm_framebuffer *fb,
+				       unsigned int format_plane_index,
 					struct drm_file *file_priv,
 					unsigned int *handle)
 {
 	struct exynos_drm_fb *exynos_fb = to_exynos_fb(fb);
-
+	if (format_plane_index >= MAX_FB_BUFFER ||
+	    !exynos_fb->exynos_gem[format_plane_index])
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-				     &exynos_fb->exynos_gem[0]->base, handle);
+		     &exynos_fb->exynos_gem[format_plane_index]->base, handle);
 }
 
 static const struct drm_framebuffer_funcs exynos_drm_fb_funcs = {
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index ffe6b4ffa1a8..c221544b4c6a 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -42,6 +42,7 @@
 
 static void psb_user_framebuffer_destroy(struct drm_framebuffer *fb);
 static int psb_user_framebuffer_create_handle(struct drm_framebuffer *fb,
+					      unsigned int format_plane_index,
 					      struct drm_file *file_priv,
 					      unsigned int *handle);
 
@@ -619,6 +620,7 @@ static void psbfb_output_poll_changed(struct drm_device *dev)
  *	the work for us
  */
 static int psb_user_framebuffer_create_handle(struct drm_framebuffer *fb,
+					      unsigned int plane_index,
 					      struct drm_file *file_priv,
 					      unsigned int *handle)
 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3282b0f4b134..54fdd30b0598 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15853,6 +15853,7 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 }
 
 static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
+						unsigned int format_plane_index,
 						struct drm_file *file,
 						unsigned int *handle)
 {
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
index d4246c9dceae..8343144ba1cd 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
@@ -44,6 +44,7 @@ struct drm_gem_object *mtk_fb_get_gem_obj(struct drm_framebuffer *fb)
 }
 
 static int mtk_drm_fb_create_handle(struct drm_framebuffer *fb,
+				    unsigned int format_plane_index,
 				    struct drm_file *file_priv,
 				    unsigned int *handle)
 {
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 5cf165c9c3a9..de92fc8a7a1c 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -30,12 +30,15 @@ struct msm_framebuffer {
 
 
 static int msm_framebuffer_create_handle(struct drm_framebuffer *fb,
+		unsigned int format_plane_index,
 		struct drm_file *file_priv,
 		unsigned int *handle)
 {
 	struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
+	if (format_plane_index >= MAX_PLANE)
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-			msm_fb->planes[0], handle);
+			msm_fb->planes[format_plane_index], handle);
 }
 
 static void msm_framebuffer_destroy(struct drm_framebuffer *fb)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 72fdba1a1c5d..bc30c0d3d9cd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -237,6 +237,7 @@ nouveau_user_framebuffer_destroy(struct drm_framebuffer *drm_fb)
 
 static int
 nouveau_user_framebuffer_create_handle(struct drm_framebuffer *drm_fb,
+				       unsigned int format_plane_index,
 				       struct drm_file *file_priv,
 				       unsigned int *handle)
 {
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])
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-			omap_fb->planes[0].bo, handle);
+			omap_fb->planes[format_plane_index].bo, handle);
 }
 
 static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index aea8b62835a4..2188e6341cd9 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1306,8 +1306,9 @@ static void radeon_user_framebuffer_destroy(struct drm_framebuffer *fb)
 }
 
 static int radeon_user_framebuffer_create_handle(struct drm_framebuffer *fb,
-						  struct drm_file *file_priv,
-						  unsigned int *handle)
+						 unsigned int plane_index,
+						 struct drm_file *file_priv,
+						 unsigned int *handle)
 {
 	struct radeon_framebuffer *radeon_fb = to_radeon_framebuffer(fb);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index c9ccdf8f44bb..206d93249519 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -55,13 +55,15 @@ static void rockchip_drm_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int rockchip_drm_fb_create_handle(struct drm_framebuffer *fb,
+					 unsigned int format_plane_index,
 					 struct drm_file *file_priv,
 					 unsigned int *handle)
 {
 	struct rockchip_drm_fb *rockchip_fb = to_rockchip_fb(fb);
-
+	if (format_plane_index >= ROCKCHIP_MAX_FB_BUFFER)
+		return -ENOENT;
 	return drm_gem_handle_create(file_priv,
-				     rockchip_fb->obj[0], handle);
+				  rockchip_fb->obj[format_plane_index], handle);
 }
 
 static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
index f142f6a4db25..ff206b143503 100644
--- a/drivers/gpu/drm/tegra/fb.c
+++ b/drivers/gpu/drm/tegra/fb.c
@@ -81,11 +81,16 @@ static void tegra_fb_destroy(struct drm_framebuffer *framebuffer)
 }
 
 static int tegra_fb_create_handle(struct drm_framebuffer *framebuffer,
+				  unsigned int format_plane_index,
 				  struct drm_file *file, unsigned int *handle)
 {
 	struct tegra_fb *fb = to_tegra_fb(framebuffer);
-
-	return drm_gem_handle_create(file, &fb->planes[0]->gem, handle);
+	if (format_plane_index >= fb->num_planes ||
+	    !fb->planes[format_plane_index])
+		return -ENOENT;
+	return drm_gem_handle_create(file,
+				     &fb->planes[format_plane_index]->gem,
+				     handle);
 }
 
 static const struct drm_framebuffer_funcs tegra_fb_funcs = {
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index dd1e3e99dcff..2fb398cb4646 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -66,6 +66,7 @@ struct drm_framebuffer_funcs {
 	 * 0 on success or a negative error code on failure.
 	 */
 	int (*create_handle)(struct drm_framebuffer *fb,
+			     unsigned int plane_index,
 			     struct drm_file *file_priv,
 			     unsigned int *handle);
 	/**
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)
+
 /**
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.14.0.rc0.400.g1c36432dff-goog

Re: [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

[Laurent Pinchart]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

[Joe Kniss]

[-- Attachment #1.1: Type: text/plain, Size: 9806 bytes --]

Thanks for the quick review!

On Tue, Aug 1, 2017 at 5:09 AM, Laurent Pinchart <laurent.pinchart@
ideasonboard.com> wrote:

> 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.
>
> Sorry, I failed to reference the original email.  Here it is:
<snip>
 I am a recent addition to Google's ChromeOS gfx team.   I am currently
working on display testing and reporting.   An important part of this is
our screen capture tool, which works by querying drm for crtcs, planes, and
fbs.  Unfortunately, there is only limited information available via
drmModeGetFB(), which often wrong information when drmModeAddFB2() was used
to create the fbs.   For example, if the pixel format is NV12 or YUV420,
the fb returned knows nothing about the additional buffer planes required
by these formats.   Ideally, we would like a function (e.g. drmModeGetFB2)
to return information symmetric with drmModeAddFB2 including the pixel
format id, buffer plane information etc.
</snip>
ChromeOS has needed this functionality from the start, for both testing and
error reporting.  We got away with guessing the buffer's format (32bit
xrgb) until now.  We are now enabling overlays and more formats including
multi-planar (e.g. NV12).  Current getfb() reports neither the pixel format
nor  planar information.  Without this information, going forward, our gfx
testing is going to break.  It would be great if we had access to higher
level buffer structs (like gbm), but we generally don't since they would be
created by other apps (chrome browser, android apps, etc...).


> > 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 ?
>
> I would imagine that some of these do support multi-planar formats, but
they don't appear to have them implemented yet (except perhaps as offsets
into a single buffer).  I will certainly be looking into this soon, but any
changes will come in future patches.


> > 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.
>
> Sure thing, sounds good.



> > +                             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.
>
> Deleting the handle is the same in all cases, but creating the handle
isn't consistent across the drivers.  I didn't see a clean way to simplify
this.


> > +                     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.
>
> I'll remove the check then.


> > +             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.
>
> I just did a quick search for "DRM_IOWR 0xC*" and picked the first unused
one, there was a patch in-flight from last month that used 0xC3 (
https://lists.freedesktop.org/archives/amd-gfx/2017-June/010153.html).
Naturally, I'll set this to whatever is appropriate.


> >  /**
> >   * Device specific ioctls should only be in their respective headers
> >   * The device specific ioctl range is from 0x40 to 0x9f.
>
> --
> Regards,
>
> Laurent Pinchart
>
>
Thanks!
-j


-- 
Dr. Joe Michael Kniss |  Google ChromeOS |  djmk@google.com   |
1-801-898-7977 <(801)%20898-7977>

[-- Attachment #1.2: Type: text/html, Size: 15140 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

[Laurent Pinchart]

Hi Joe,

On Tuesday 01 Aug 2017 10:24:25 Joe Kniss wrote:
> On Tue, Aug 1, 2017 at 5:09 AM, Laurent Pinchart wrote:
> > 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.
> 
> Sorry, I failed to reference the original email.  Here it is:
> 
> <snip>
> I am a recent addition to Google's ChromeOS gfx team.   I am currently
> working on display testing and reporting.   An important part of this is
> our screen capture tool, which works by querying drm for crtcs, planes, and
> fbs.  Unfortunately, there is only limited information available via
> drmModeGetFB(), which often wrong information when drmModeAddFB2() was used
> to create the fbs.   For example, if the pixel format is NV12 or YUV420,
> the fb returned knows nothing about the additional buffer planes required
> by these formats.   Ideally, we would like a function (e.g. drmModeGetFB2)
> to return information symmetric with drmModeAddFB2 including the pixel
> format id, buffer plane information etc.
> </snip>
> 
> ChromeOS has needed this functionality from the start, for both testing and
> error reporting.  We got away with guessing the buffer's format (32bit
> xrgb) until now.  We are now enabling overlays and more formats including
> multi-planar (e.g. NV12).  Current getfb() reports neither the pixel format
> nor  planar information.  Without this information, going forward, our gfx
> testing is going to break.  It would be great if we had access to higher
> level buffer structs (like gbm), but we generally don't since they would be
> created by other apps (chrome browser, android apps, etc...).

How is screen capture implemented ? Do you enumerate the framebuffers being 
scanned out, dump their contents and compose them manually based on the active 
plane configuration ? If so, isn't this very race-prone ?

> >> 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 ?
>  
> I would imagine that some of these do support multi-planar formats, but
> they don't appear to have them implemented yet (except perhaps as offsets
> into a single buffer).  I will certainly be looking into this soon, but any
> changes will come in future patches.
> 
> >> 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 @@

[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.
> 
> Sure thing, sounds good.
> 
> > > +                             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.
> 
> Deleting the handle is the same in all cases, but creating the handle
> isn't consistent across the drivers.  I didn't see a clean way to simplify
> this.

Creating the handle seems to be fairly consistent, except for a very small 
number of special cases. If we stored the GEM object pointers in the 
drm_framebuffer structure, the common case would be simplified. Or maybe I've 
missed something :-)

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

[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.
> 
> I just did a quick search for "DRM_IOWR 0xC*" and picked the first unused
> one, there was a patch in-flight from last month that used 0xC3 (
> https://lists.freedesktop.org/archives/amd-gfx/2017-June/010153.html).
> Naturally, I'll set this to whatever is appropriate.

The best is to base your patch on top of the drm-misc-next branch of the drm-
misc tree. Whoever is merged first will win the ioctl number :-) Of course it 
means rebasing if someone else wins the race, but that shouldn't be a big 
deal.

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

-- 
Regards,

Laurent Pinchart

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

[Joe Kniss]

[-- Attachment #1.1: Type: text/plain, Size: 10870 bytes --]

On Tue, Aug 1, 2017 at 1:46 PM, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Joe,
>
> On Tuesday 01 Aug 2017 10:24:25 Joe Kniss wrote:
> > On Tue, Aug 1, 2017 at 5:09 AM, Laurent Pinchart wrote:
> > > 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.
> >
> > Sorry, I failed to reference the original email.  Here it is:
> >
> > <snip>
> > I am a recent addition to Google's ChromeOS gfx team.   I am currently
> > working on display testing and reporting.   An important part of this is
> > our screen capture tool, which works by querying drm for crtcs, planes,
> and
> > fbs.  Unfortunately, there is only limited information available via
> > drmModeGetFB(), which often wrong information when drmModeAddFB2() was
> used
> > to create the fbs.   For example, if the pixel format is NV12 or YUV420,
> > the fb returned knows nothing about the additional buffer planes required
> > by these formats.   Ideally, we would like a function (e.g.
> drmModeGetFB2)
> > to return information symmetric with drmModeAddFB2 including the pixel
> > format id, buffer plane information etc.
> > </snip>
> >
> > ChromeOS has needed this functionality from the start, for both testing
> and
> > error reporting.  We got away with guessing the buffer's format (32bit
> > xrgb) until now.  We are now enabling overlays and more formats including
> > multi-planar (e.g. NV12).  Current getfb() reports neither the pixel
> format
> > nor  planar information.  Without this information, going forward, our
> gfx
> > testing is going to break.  It would be great if we had access to higher
> > level buffer structs (like gbm), but we generally don't since they would
> be
> > created by other apps (chrome browser, android apps, etc...).
>
> How is screen capture implemented ? Do you enumerate the framebuffers being
> scanned out, dump their contents and compose them manually based on the
> active
> plane configuration ? If so, isn't this very race-prone ?
>
>
Yes, this is basically it.  We identify the crtc to capture, query the
planes associated with it and their properties.  For each plane, we get the
fb, then a FD that we use to import a GBM buffer, which we map and
composite.  It's not ideal, but it's the only thing that will work across
all of our platforms and configs.   We either wait or sleep for consistent
testing captures.  I'm not sure what we can do about the race condition
without a much more substantial change...  We are currently looking into
some platform specific methods, but the current approach won't be going
away anytime soon.


> > >> 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 ?
> >
> > I would imagine that some of these do support multi-planar formats, but
> > they don't appear to have them implemented yet (except perhaps as offsets
> > into a single buffer).  I will certainly be looking into this soon, but
> any
> > changes will come in future patches.
> >
> > >> 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 @@
>
> [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.
> >
> > Sure thing, sounds good.
> >
> > > > +                             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.
> >
> > Deleting the handle is the same in all cases, but creating the handle
> > isn't consistent across the drivers.  I didn't see a clean way to
> simplify
> > this.
>
> Creating the handle seems to be fairly consistent, except for a very small
> number of special cases. If we stored the GEM object pointers in the
> drm_framebuffer structure, the common case would be simplified. Or maybe
> I've
> missed something :-)
>
>
I am sure this would be fine, I'd be happy to get rid of it.  I'll put
together a separate patch for this.



> > >> +                     r->handles[i] = 0;
> > >> +             }
> > >> +     }
> > >> +
> > >> +     drm_framebuffer_unreference(fb);
> > >> +
> > >> +     return ret;
> > >> +}
>
> [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.
> >
> > I just did a quick search for "DRM_IOWR 0xC*" and picked the first unused
> > one, there was a patch in-flight from last month that used 0xC3 (
> > https://lists.freedesktop.org/archives/amd-gfx/2017-June/010153.html).
> > Naturally, I'll set this to whatever is appropriate.
>
> The best is to base your patch on top of the drm-misc-next branch of the
> drm-
> misc tree. Whoever is merged first will win the ioctl number :-) Of course
> it
> means rebasing if someone else wins the race, but that shouldn't be a big
> deal.
>
>
Will do.


> > > >  /**
> > > >
> > > >   * Device specific ioctls should only be in their respective headers
> > > >   * The device specific ioctl range is from 0x40 to 0x9f.
>
> --
> Regards,
>
> Laurent Pinchart
>
>
I'll respond with an updated patch ASAP.

-- 
Dr. Joe Michael Kniss |  Google ChromeOS |  djmk@google.com   |
1-801-898-7977 <(801)%20898-7977>

[-- Attachment #1.2: Type: text/html, Size: 16939 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx