[PATCH 0/8] Add GetFB2 ioctl

[PATCH 0/8] Add GetFB2 ioctl

[Daniel Stone]

Hi,
AddFB is single-planar, with no offset and only depth/bpp rather than
an explicit format, so we now have AddFB2 which can carry multiple
planes, an explicit format and also a modifier. At the time we never
actually did GetFB2 to go with GetFB.

This submission rights that historical wrong, which allows Xorg
-background none to continue to work in the face of exotic buffers.
I've written patches to Xorg to use this as UABI verification, and
I'll post a link to that very shortly.

This is also available in git form, in the wip/2018-03/getfb2 branches of:
    https://gitlab.collabora.com/daniels/linux
    https://gitlab.collabora.com/daniels/libdrm
    https://gitlab.collabora.com/daniels/igt-gpu-tools
    https://gitlab.collabora.com/daniels/xserver

Note that the kernel tree is based on top of drm-tip, and has a bunch
of patches to convert drivers to placing objects (rather than creating
handles). At the moment this is just missing amdgpu and nouveau to
complete the set of drivers, and also any testing whatsoever; I'm
sending a patch to convert Intel as that's what I've actually tested
on.

The conversion is necessary since GetFB2 ensures that it does _not_
return duplicate handles if multiple planes of a single buffer share
the same BO. This is impossible if we just use create_handle from the
driver, so instead we just rely on drivers placing the GEM object
pointers in the drm_framebuffer directly. This has the added advantage
of cleaning up a ton of unnecessary code in drivers.

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

[igt-dev] [PATCH 0/8] Add GetFB2 ioctl

[Daniel Stone]

Hi,
AddFB is single-planar, with no offset and only depth/bpp rather than
an explicit format, so we now have AddFB2 which can carry multiple
planes, an explicit format and also a modifier. At the time we never
actually did GetFB2 to go with GetFB.

This submission rights that historical wrong, which allows Xorg
-background none to continue to work in the face of exotic buffers.
I've written patches to Xorg to use this as UABI verification, and
I'll post a link to that very shortly.

This is also available in git form, in the wip/2018-03/getfb2 branches of:
    https://gitlab.collabora.com/daniels/linux
    https://gitlab.collabora.com/daniels/libdrm
    https://gitlab.collabora.com/daniels/igt-gpu-tools
    https://gitlab.collabora.com/daniels/xserver

Note that the kernel tree is based on top of drm-tip, and has a bunch
of patches to convert drivers to placing objects (rather than creating
handles). At the moment this is just missing amdgpu and nouveau to
complete the set of drivers, and also any testing whatsoever; I'm
sending a patch to convert Intel as that's what I've actually tested
on.

The conversion is necessary since GetFB2 ensures that it does _not_
return duplicate handles if multiple planes of a single buffer share
the same BO. This is impossible if we just use create_handle from the
driver, so instead we just rely on drivers placing the GEM object
pointers in the drm_framebuffer directly. This has the added advantage
of cleaning up a ton of unnecessary code in drivers.

Cheers,
Daniel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[PATCH 1/4] drm/i915: Use intel_fb_obj() everywhere

[Daniel Stone]

We already have a macro to pull the GEM object from a FB, so use it
everywhere. We'll make use of this later to move the object storage.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  4 ++--
 drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++---------
 drivers/gpu/drm/i915/intel_fbdev.c   |  9 +++++----
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7816cd53100a..68541df6f601 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1893,7 +1893,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 			   fbdev_fb->base.format->cpp[0] * 8,
 			   fbdev_fb->base.modifier,
 			   drm_framebuffer_read_refcount(&fbdev_fb->base));
-		describe_obj(m, fbdev_fb->obj);
+		describe_obj(m, intel_fb_obj(&fbdev_fb->base));
 		seq_putc(m, '\n');
 	}
 #endif
@@ -1911,7 +1911,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 			   fb->base.format->cpp[0] * 8,
 			   fb->base.modifier,
 			   drm_framebuffer_read_refcount(&fb->base));
-		describe_obj(m, fb->obj);
+		describe_obj(m, intel_fb_obj(&fb->base));
 		seq_putc(m, '\n');
 	}
 	mutex_unlock(&dev->mode_config.fb_lock);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1b16b534871a..49b0772e9abc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2478,6 +2478,7 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
 {
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct intel_rotation_info *rot_info = &intel_fb->rot_info;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	u32 gtt_offset_rotated = 0;
 	unsigned int max_size = 0;
 	int i, num_planes = fb->format->num_planes;
@@ -2542,7 +2543,7 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
 		 * fb layout agrees with the fence layout. We already check that the
 		 * fb stride matches the fence stride elsewhere.
 		 */
-		if (i == 0 && i915_gem_object_is_tiled(intel_fb->obj) &&
+		if (i == 0 && i915_gem_object_is_tiled(obj) &&
 		    (x + width) * cpp > fb->pitches[i]) {
 			DRM_DEBUG_KMS("bad fb plane %d offset: 0x%x\n",
 				      i, fb->offsets[i]);
@@ -2627,9 +2628,9 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
 		max_size = max(max_size, offset + size);
 	}
 
-	if (max_size * tile_size > intel_fb->obj->base.size) {
+	if (max_size * tile_size > obj->base.size) {
 		DRM_DEBUG_KMS("fb too big for bo (need %u bytes, have %zu bytes)\n",
-			      max_size * tile_size, intel_fb->obj->base.size);
+			      max_size * tile_size, obj->base.size);
 		return -EINVAL;
 	}
 
@@ -13910,14 +13911,15 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 {
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 
 	drm_framebuffer_cleanup(fb);
 
-	i915_gem_object_lock(intel_fb->obj);
-	WARN_ON(!intel_fb->obj->framebuffer_references--);
-	i915_gem_object_unlock(intel_fb->obj);
+	i915_gem_object_lock(obj);
+	WARN_ON(!obj->framebuffer_references--);
+	i915_gem_object_unlock(obj);
 
-	i915_gem_object_put(intel_fb->obj);
+	i915_gem_object_put(obj);
 
 	kfree(intel_fb);
 }
@@ -13926,8 +13928,7 @@ static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
 						struct drm_file *file,
 						unsigned int *handle)
 {
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 
 	if (obj->userptr.mm) {
 		DRM_DEBUG("attempting to use a userptr for a framebuffer, denied\n");
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 65a3313723c9..fdef18488fb1 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -47,7 +47,7 @@
 
 static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
 {
-	struct drm_i915_gem_object *obj = ifbdev->fb->obj;
+	struct drm_i915_gem_object *obj = intel_fb_obj(&ifbdev->fb->base);
 	unsigned int origin =
 		ifbdev->vma_flags & PLANE_HAS_FENCE ? ORIGIN_GTT : ORIGIN_CPU;
 
@@ -193,7 +193,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		drm_framebuffer_put(&intel_fb->base);
 		intel_fb = ifbdev->fb = NULL;
 	}
-	if (!intel_fb || WARN_ON(!intel_fb->obj)) {
+	if (!intel_fb || WARN_ON(!intel_fb_obj(&intel_fb->base))) {
 		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
 		ret = intelfb_alloc(helper, sizes);
 		if (ret)
@@ -265,7 +265,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	 * If the object is stolen however, it will be full of whatever
 	 * garbage was left in there.
 	 */
-	if (intel_fb->obj->stolen && !prealloc)
+	if (intel_fb_obj(fb)->stolen && !prealloc)
 		memset_io(info->screen_base, 0, info->screen_size);
 
 	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
@@ -792,7 +792,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
 	 * been restored from swap. If the object is stolen however, it will be
 	 * full of whatever garbage was left in there.
 	 */
-	if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
+	if (state == FBINFO_STATE_RUNNING &&
+	    intel_fb_obj(&ifbdev->fb->base)->stolen)
 		memset_io(info->screen_base, 0, info->screen_size);
 
 	drm_fb_helper_set_suspend(&ifbdev->helper, state);
-- 
2.16.2

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

[PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer

[Daniel Stone]

Since drm_framebuffer can now store GEM objects directly, place them
there rather than in our own subclass.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  3 +--
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49b0772e9abc..e8100a4fc877 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13916,7 +13916,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 	drm_framebuffer_cleanup(fb);
 
 	i915_gem_object_lock(obj);
-	WARN_ON(!obj->framebuffer_references--);
+	WARN_ON(obj->framebuffer_references < fb->format->num_planes);
+	obj->framebuffer_references -= fb->format->num_planes;
 	i915_gem_object_unlock(obj);
 
 	i915_gem_object_put(obj);
@@ -14176,9 +14177,9 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 				      i, fb->pitches[i], stride_alignment);
 			goto err;
 		}
-	}
 
-	intel_fb->obj = obj;
+		fb->obj[i] = &obj->base;
+	}
 
 	ret = intel_fill_fb_info(dev_priv, fb);
 	if (ret)
@@ -14190,6 +14191,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		goto err;
 	}
 
+	/* We already hold one reference to the fb, but in case it's
+	 * multi-planar and we've placed multiple pointers in
+	 * drm_framebuffer, hold extra refs.
+	 */
+	i915_gem_object_lock(obj);
+	obj->framebuffer_references += fb->format->num_planes - 1;
+	i915_gem_object_unlock(obj);
+
 	return 0;
 
 err:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 570f89b31544..d93ed9e59867 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -186,7 +186,6 @@ enum intel_output_type {
 
 struct intel_framebuffer {
 	struct drm_framebuffer base;
-	struct drm_i915_gem_object *obj;
 	struct intel_rotation_info rot_info;
 
 	/* for each plane in the normal GTT view */
@@ -985,7 +984,7 @@ struct cxsr_latency {
 #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
 #define to_intel_plane(x) container_of(x, struct intel_plane, base)
 #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
-#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
+#define intel_fb_obj(x) (((x) && (x)->obj[0]) ? to_intel_bo((x)->obj[0]) : NULL)
 
 struct intel_hdmi {
 	i915_reg_t hdmi_reg;
-- 
2.16.2

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

[PATCH 3/4] drm: Reshuffle getfb error returns

[Daniel Stone]

Make it a little more clear what's going on inside of getfb, and also
make it easier to add alternate paths to get a handle in future.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/drm_framebuffer.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index ad67203de715..6d5ff541225a 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -468,29 +468,30 @@ int drm_mode_getfb(struct drm_device *dev,
 		goto out;
 	}
 
+	if (!fb->funcs->create_handle) {
+		ret = -ENODEV;
+		goto out;
+	}
+
 	r->height = fb->height;
 	r->width = fb->width;
 	r->depth = fb->format->depth;
 	r->bpp = fb->format->cpp[0] * 8;
 	r->pitch = fb->pitches[0];
-	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,
-						       &r->handle);
-		} 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->handle = 0;
-			ret = 0;
-		}
-	} else {
-		ret = -ENODEV;
+
+	/* 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. */
+	if (!drm_is_current_master(file_priv) && !capable(CAP_SYS_ADMIN) &&
+	    !drm_is_control_client(file_priv)) {
+		r->handle = 0;
+		ret = 0;
+		goto out;
 	}
 
+	ret = fb->funcs->create_handle(fb, file_priv, &r->handle);
+
 out:
 	drm_framebuffer_put(fb);
 
-- 
2.16.2

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

[PATCH 4/4] drm: Add getfb2 ioctl

[Daniel Stone]

getfb2 allows us to pass multiple planes and modifiers, just like addfb2
over addfb.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/drm_crtc_internal.h |   2 +
 drivers/gpu/drm/drm_framebuffer.c   | 109 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_ioctl.c         |   2 +
 include/uapi/drm/drm.h              |   1 +
 4 files changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 3c2b82865ad2..0cd02f3d203d 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -173,6 +173,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_ioctl(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_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 6d5ff541225a..f1cfb6ddc776 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/drm_atomic.h>
 #include <drm/drm_print.h>
 
@@ -494,7 +495,115 @@ int drm_mode_getfb(struct drm_device *dev,
 
 out:
 	drm_framebuffer_put(fb);
+	return ret;
+}
+
+/**
+ * drm_mode_getfb2 - get extended 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_ioctl(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv)
+{
+	struct drm_mode_fb_cmd2 *r = data;
+	struct drm_framebuffer *fb;
+	unsigned int i;
+	int ret;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
 
+	fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
+	if (!fb)
+		return -ENOENT;
+
+	/* For multi-plane framebuffers, we require the driver to place the
+	 * GEM objects directly in the drm_framebuffer. For single-plane
+	 * framebuffers, we can fall back to create_handle.
+	 */
+	if (!fb->obj[0] &&
+	    (fb->format->num_planes > 1 || !fb->funcs->create_handle)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	r->height = fb->height;
+	r->width = fb->width;
+	r->pixel_format = fb->format->format;
+
+	r->flags = 0;
+	if (dev->mode_config.allow_fb_modifiers)
+		r->flags |= DRM_MODE_FB_MODIFIERS;
+
+	for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
+		r->handles[i] = 0;
+		r->pitches[i] = 0;
+		r->offsets[i] = 0;
+		r->modifier[i] = DRM_FORMAT_MOD_INVALID;
+	}
+
+	for (i = 0; i < fb->format->num_planes; i++) {
+		int j;
+
+		r->pitches[i] = fb->pitches[i];
+		r->offsets[i] = fb->offsets[i];
+		if (dev->mode_config.allow_fb_modifiers)
+			r->modifier[i] = fb->modifier;
+
+		/* If we reuse the same object for multiple planes, also
+		 * return the same handle.
+		 */
+		for (j = 0; j < i; j++) {
+			if (fb->obj[i] == fb->obj[j]) {
+				r->handles[i] = r->handles[j];
+				break;
+			}
+		}
+
+		if (r->handles[i])
+			continue;
+
+		if (fb->obj[i]) {
+			ret = drm_gem_handle_create(file_priv, fb->obj[i],
+						    &r->handles[i]);
+		} else {
+			WARN_ON(i > 0);
+			ret = fb->funcs->create_handle(fb, file_priv,
+						       &r->handles[i]);
+		}
+
+		if (ret != 0)
+			goto out;
+	}
+
+out:
+	if (ret != 0) {
+		/* Delete any previously-created handles on failure. */
+		for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
+			int j;
+
+			if (r->handles[i])
+				drm_gem_handle_delete(file_priv, r->handles[i]);
+
+			/* Zero out any handles identical to the one we just
+			 * deleted. */
+			for (j = i + 1; j < ARRAY_SIZE(r->handles); j++) {
+				if (r->handles[j] == r->handles[i])
+					r->handles[j] = 0;
+			}
+		}
+	}
+
+	drm_framebuffer_put(fb);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index af782911c505..b5896e3615e5 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -669,6 +669,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB2, drm_mode_getfb2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 6fdff5945c8a..9a33613394a9 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -892,6 +892,7 @@ extern "C" {
 #define DRM_IOCTL_MODE_LIST_LESSEES	DRM_IOWR(0xC7, struct drm_mode_list_lessees)
 #define DRM_IOCTL_MODE_GET_LEASE	DRM_IOWR(0xC8, struct drm_mode_get_lease)
 #define DRM_IOCTL_MODE_REVOKE_LEASE	DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
+#define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCA, struct drm_mode_fb_cmd2)
 
 /**
  * Device specific ioctls should only be in their respective headers
-- 
2.16.2

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

[PATCH libdrm] NOMERGE: Add drmModeGetFB2

[Daniel Stone]

Add a wrapper around the getfb2 ioctl, which returns extended
framebuffer information mirroring addfb2, including multiple planes and
modifiers.

This depends on unmerged kernel API so should not be merged.
---
 include/drm/drm.h |  1 +
 xf86drmMode.c     | 40 ++++++++++++++++++++++++++++++++++++++++
 xf86drmMode.h     | 15 +++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/include/drm/drm.h b/include/drm/drm.h
index f0bd91de..c0a35878 100644
--- a/include/drm/drm.h
+++ b/include/drm/drm.h
@@ -886,6 +886,7 @@ extern "C" {
 #define DRM_IOCTL_MODE_LIST_LESSEES	DRM_IOWR(0xC7, struct drm_mode_list_lessees)
 #define DRM_IOCTL_MODE_GET_LEASE	DRM_IOWR(0xC8, struct drm_mode_get_lease)
 #define DRM_IOCTL_MODE_REVOKE_LEASE	DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
+#define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCA, struct drm_mode_fb_cmd2)
 
 /**
  * Device specific ioctls should only be in their respective headers
diff --git a/xf86drmMode.c b/xf86drmMode.c
index cf8e1eac..d8cf6a0c 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -1582,3 +1582,43 @@ drmModeRevokeLease(int fd, uint32_t lessee_id)
 		return 0;
 	return -errno;
 }
+
+drmModeFB2Ptr
+drmModeGetFB2(int fd, uint32_t fb_id)
+{
+	struct drm_mode_fb_cmd2 get;
+	drmModeFB2Ptr ret;
+	int err;
+
+	memclear(get);
+	get.fb_id = fb_id;
+
+	err = DRM_IOCTL(fd, DRM_IOCTL_MODE_GETFB2, &get);
+	if (err != 0)
+		return NULL;
+
+	ret = drmMalloc(sizeof(drmModeFB2));
+	if (!ret)
+		return NULL;
+
+	ret->fb_id = fb_id;
+	ret->width = get.width;
+	ret->height = get.height;
+	ret->pixel_format = get.pixel_format;
+	ret->flags = get.flags;
+	ret->modifier = get.modifier[0];
+	memcpy(ret->handles, get.handles, sizeof(uint32_t) * 4);
+	memcpy(ret->pitches, get.pitches, sizeof(uint32_t) * 4);
+	memcpy(ret->offsets, get.offsets, sizeof(uint32_t) * 4);
+
+	return ret;
+}
+
+void drmModeFreeFB2(drmModeFB2Ptr ptr)
+{
+	if (!ptr)
+		return;
+
+	/* we might add more frees later. */
+	drmFree(ptr);
+}
diff --git a/xf86drmMode.h b/xf86drmMode.h
index 3cd27aee..70741c77 100644
--- a/xf86drmMode.h
+++ b/xf86drmMode.h
@@ -223,6 +223,19 @@ typedef struct _drmModeFB {
 	uint32_t handle;
 } drmModeFB, *drmModeFBPtr;
 
+typedef struct _drmModeFB2 {
+	uint32_t fb_id;
+	uint32_t width, height;
+	uint32_t pixel_format; /* fourcc code from drm_fourcc.h */
+	uint32_t modifier; /* applies to all buffers */
+	uint32_t flags;
+
+	/* per-plane GEM handle; may be duplicate entries for multiple planes */
+	uint32_t handles[4];
+	uint32_t pitches[4]; /* bytes */
+	uint32_t offsets[4]; /* bytes */
+} drmModeFB2, *drmModeFB2Ptr;
+
 typedef struct drm_clip_rect drmModeClip, *drmModeClipPtr;
 
 typedef struct _drmModePropertyBlob {
@@ -341,6 +354,7 @@ typedef struct _drmModePlaneRes {
 extern void drmModeFreeModeInfo( drmModeModeInfoPtr ptr );
 extern void drmModeFreeResources( drmModeResPtr ptr );
 extern void drmModeFreeFB( drmModeFBPtr ptr );
+extern void drmModeFreeFB2( drmModeFB2Ptr ptr );
 extern void drmModeFreeCrtc( drmModeCrtcPtr ptr );
 extern void drmModeFreeConnector( drmModeConnectorPtr ptr );
 extern void drmModeFreeEncoder( drmModeEncoderPtr ptr );
@@ -360,6 +374,7 @@ extern drmModeResPtr drmModeGetResources(int fd);
  * Retrive information about framebuffer bufferId
  */
 extern drmModeFBPtr drmModeGetFB(int fd, uint32_t bufferId);
+extern drmModeFB2Ptr drmModeGetFB2(int fd, uint32_t bufferId);
 
 /**
  * Creates a new framebuffer with an buffer object as its scanout buffer.
-- 
2.16.2

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

[PATCH i-g-t 1/3] tests/kms_getfb: Split property-ID get into helper

[Daniel Stone]

We'll want to reuse this, so split it out into a (smaller!) helper.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 tests/kms_getfb.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
index c5968e75..a9852626 100644
--- a/tests/kms_getfb.c
+++ b/tests/kms_getfb.c
@@ -72,6 +72,23 @@ static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret)
 		gem_close(fd, add.handles[0]);
 }
 
+/**
+ * Find and return an arbitrary valid property ID.
+ */
+static uint32_t get_prop_id(int fd)
+{
+	igt_display_t display;
+
+	igt_display_init(&display, fd);
+	for (int i = 0; i < display.n_outputs; i++) {
+		igt_output_t *output = &display.outputs[i];
+		if (output->props[IGT_CONNECTOR_DPMS] != 0)
+			return output->props[IGT_CONNECTOR_DPMS];
+	}
+
+	return 0;
+}
+
 static void test_handle_input(int fd)
 {
 	struct drm_mode_fb_cmd2 add = {};
@@ -111,23 +128,8 @@ static void test_handle_input(int fd)
 	}
 
 	igt_subtest("getfb-handle-not-fb") {
-		struct drm_mode_fb_cmd get = { };
-		uint32_t prop_id = 0;
-		igt_display_t display;
-
-		/* Find a valid property ID to use. */
-		igt_display_init(&display, fd);
-		for (int i = 0; i < display.n_outputs; i++) {
-			igt_output_t *output = &display.outputs[i];
-
-			if (output->props[IGT_CONNECTOR_DPMS] != 0) {
-				prop_id = output->props[IGT_CONNECTOR_DPMS];
-				break;
-			}
-		}
-		igt_require(prop_id > 0);
-
-		get.fb_id = prop_id;
+		struct drm_mode_fb_cmd get = { .fb_id = get_prop_id(fd) };
+		igt_require(get.fb_id > 0);
 		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB, &get, ENOENT);
 	}
 }
-- 
2.16.2

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

[igt-dev] [PATCH i-g-t 1/3] tests/kms_getfb: Split property-ID get into helper

[Daniel Stone]

We'll want to reuse this, so split it out into a (smaller!) helper.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 tests/kms_getfb.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
index c5968e75..a9852626 100644
--- a/tests/kms_getfb.c
+++ b/tests/kms_getfb.c
@@ -72,6 +72,23 @@ static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret)
 		gem_close(fd, add.handles[0]);
 }
 
+/**
+ * Find and return an arbitrary valid property ID.
+ */
+static uint32_t get_prop_id(int fd)
+{
+	igt_display_t display;
+
+	igt_display_init(&display, fd);
+	for (int i = 0; i < display.n_outputs; i++) {
+		igt_output_t *output = &display.outputs[i];
+		if (output->props[IGT_CONNECTOR_DPMS] != 0)
+			return output->props[IGT_CONNECTOR_DPMS];
+	}
+
+	return 0;
+}
+
 static void test_handle_input(int fd)
 {
 	struct drm_mode_fb_cmd2 add = {};
@@ -111,23 +128,8 @@ static void test_handle_input(int fd)
 	}
 
 	igt_subtest("getfb-handle-not-fb") {
-		struct drm_mode_fb_cmd get = { };
-		uint32_t prop_id = 0;
-		igt_display_t display;
-
-		/* Find a valid property ID to use. */
-		igt_display_init(&display, fd);
-		for (int i = 0; i < display.n_outputs; i++) {
-			igt_output_t *output = &display.outputs[i];
-
-			if (output->props[IGT_CONNECTOR_DPMS] != 0) {
-				prop_id = output->props[IGT_CONNECTOR_DPMS];
-				break;
-			}
-		}
-		igt_require(prop_id > 0);
-
-		get.fb_id = prop_id;
+		struct drm_mode_fb_cmd get = { .fb_id = get_prop_id(fd) };
+		igt_require(get.fb_id > 0);
 		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB, &get, ENOENT);
 	}
 }
-- 
2.16.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[PATCH i-g-t 2/3] NOMERGE: Update DRM UAPI to latest kernel version

[Daniel Stone]

This depends on unmerged kernel code, so.
---
 include/drm-uapi/amdgpu_drm.h  | 1 +
 include/drm-uapi/drm.h         | 1 +
 include/drm-uapi/drm_mode.h    | 9 ++++++---
 include/drm-uapi/etnaviv_drm.h | 1 +
 include/drm-uapi/msm_drm.h     | 9 ++++++++-
 include/drm-uapi/virtgpu_drm.h | 1 +
 6 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/drm-uapi/amdgpu_drm.h b/include/drm-uapi/amdgpu_drm.h
index 1816bd82..528f6d04 100644
--- a/include/drm-uapi/amdgpu_drm.h
+++ b/include/drm-uapi/amdgpu_drm.h
@@ -806,6 +806,7 @@ struct drm_amdgpu_info_firmware {
 #define AMDGPU_VRAM_TYPE_GDDR5 5
 #define AMDGPU_VRAM_TYPE_HBM   6
 #define AMDGPU_VRAM_TYPE_DDR3  7
+#define AMDGPU_VRAM_TYPE_DDR4  8
 
 struct drm_amdgpu_info_device {
 	/** PCI Device ID */
diff --git a/include/drm-uapi/drm.h b/include/drm-uapi/drm.h
index f0bd91de..c0a35878 100644
--- a/include/drm-uapi/drm.h
+++ b/include/drm-uapi/drm.h
@@ -886,6 +886,7 @@ extern "C" {
 #define DRM_IOCTL_MODE_LIST_LESSEES	DRM_IOWR(0xC7, struct drm_mode_list_lessees)
 #define DRM_IOCTL_MODE_GET_LEASE	DRM_IOWR(0xC8, struct drm_mode_get_lease)
 #define DRM_IOCTL_MODE_REVOKE_LEASE	DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
+#define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCA, struct drm_mode_fb_cmd2)
 
 /**
  * Device specific ioctls should only be in their respective headers
diff --git a/include/drm-uapi/drm_mode.h b/include/drm-uapi/drm_mode.h
index 2c575794..50bcf421 100644
--- a/include/drm-uapi/drm_mode.h
+++ b/include/drm-uapi/drm_mode.h
@@ -363,7 +363,7 @@ struct drm_mode_get_connector {
 	__u32 pad;
 };
 
-#define DRM_MODE_PROP_PENDING	(1<<0)
+#define DRM_MODE_PROP_PENDING	(1<<0) /* deprecated, do not use */
 #define DRM_MODE_PROP_RANGE	(1<<1)
 #define DRM_MODE_PROP_IMMUTABLE	(1<<2)
 #define DRM_MODE_PROP_ENUM	(1<<3) /* enumerated type with text strings */
@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {
 };
 
 struct drm_color_ctm {
-	/* Conversion matrix in S31.32 format. */
-	__s64 matrix[9];
+	/*
+	 * Conversion matrix in S31.32 sign-magnitude
+	 * (not two's complement!) format.
+	 */
+	__u64 matrix[9];
 };
 
 struct drm_color_lut {
diff --git a/include/drm-uapi/etnaviv_drm.h b/include/drm-uapi/etnaviv_drm.h
index e9b997a0..71958c80 100644
--- a/include/drm-uapi/etnaviv_drm.h
+++ b/include/drm-uapi/etnaviv_drm.h
@@ -145,6 +145,7 @@ struct drm_etnaviv_gem_submit_reloc {
  */
 #define ETNA_SUBMIT_BO_READ             0x0001
 #define ETNA_SUBMIT_BO_WRITE            0x0002
+#define ETNA_SUBMIT_BO_NO_IMPLICIT      0x0004
 struct drm_etnaviv_gem_submit_bo {
 	__u32 flags;          /* in, mask of ETNA_SUBMIT_BO_x */
 	__u32 handle;         /* in, GEM handle */
diff --git a/include/drm-uapi/msm_drm.h b/include/drm-uapi/msm_drm.h
index bbbaffad..57a67fb3 100644
--- a/include/drm-uapi/msm_drm.h
+++ b/include/drm-uapi/msm_drm.h
@@ -188,8 +188,13 @@ struct drm_msm_gem_submit_cmd {
  */
 #define MSM_SUBMIT_BO_READ             0x0001
 #define MSM_SUBMIT_BO_WRITE            0x0002
+#define MSM_SUBMIT_BO_NO_IMPLICIT      0x0004 /* disable implicit sync */
 
-#define MSM_SUBMIT_BO_FLAGS            (MSM_SUBMIT_BO_READ | MSM_SUBMIT_BO_WRITE)
+#define MSM_SUBMIT_BO_FLAGS			( \
+		MSM_SUBMIT_BO_READ		| \
+		MSM_SUBMIT_BO_WRITE		| \
+		MSM_SUBMIT_BO_NO_IMPLICIT	| \
+		0)
 
 struct drm_msm_gem_submit_bo {
 	__u32 flags;          /* in, mask of MSM_SUBMIT_BO_x */
@@ -201,10 +206,12 @@ struct drm_msm_gem_submit_bo {
 #define MSM_SUBMIT_NO_IMPLICIT   0x80000000 /* disable implicit sync */
 #define MSM_SUBMIT_FENCE_FD_IN   0x40000000 /* enable input fence_fd */
 #define MSM_SUBMIT_FENCE_FD_OUT  0x20000000 /* enable output fence_fd */
+#define MSM_SUBMIT_SUDO          0x10000000 /* run submitted cmds from RB */
 #define MSM_SUBMIT_FLAGS                ( \
 		MSM_SUBMIT_NO_IMPLICIT   | \
 		MSM_SUBMIT_FENCE_FD_IN   | \
 		MSM_SUBMIT_FENCE_FD_OUT  | \
+		MSM_SUBMIT_SUDO          | \
 		0)
 
 /* Each cmdstream submit consists of a table of buffers involved, and
diff --git a/include/drm-uapi/virtgpu_drm.h b/include/drm-uapi/virtgpu_drm.h
index 91a31ffe..9a781f06 100644
--- a/include/drm-uapi/virtgpu_drm.h
+++ b/include/drm-uapi/virtgpu_drm.h
@@ -63,6 +63,7 @@ struct drm_virtgpu_execbuffer {
 };
 
 #define VIRTGPU_PARAM_3D_FEATURES 1 /* do we have 3D features in the hw */
+#define VIRTGPU_PARAM_CAPSET_QUERY_FIX 2 /* do we have the capset fix */
 
 struct drm_virtgpu_getparam {
 	__u64 param;
-- 
2.16.2

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

[PATCH i-g-t 3/3] tests/kms_getfb: Add getfb2 tests

[Daniel Stone]

Mirroring addfb2, add tests for the new ioctl which will return us
information about framebuffers containing multiple buffers, as well as
modifiers.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 tests/kms_getfb.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
index a9852626..b8583694 100644
--- a/tests/kms_getfb.c
+++ b/tests/kms_getfb.c
@@ -186,7 +186,96 @@ static void test_duplicate_handles(int fd)
 		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &add.fb_id);
 		gem_close(fd, add.handles[0]);
 	}
+}
+
+static void test_getfb2(int fd)
+{
+	struct drm_mode_fb_cmd2 add_basic = {};
+
+	igt_fixture {
+		struct drm_mode_fb_cmd2 get = {};
+
+		add_basic.width = 1024;
+		add_basic.height = 1024;
+		add_basic.pixel_format = DRM_FORMAT_XRGB8888;
+		add_basic.pitches[0] = 1024*4;
+		add_basic.handles[0] = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
+		igt_assert(add_basic.handles[0]);
+		do_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &add_basic);
+
+		get.fb_id = add_basic.fb_id;
+		do_ioctl(fd, DRM_IOCTL_MODE_GETFB2, &get);
+		igt_assert_neq_u32(get.handles[0], 0);
+		gem_close(fd, get.handles[0]);
+	}
+
+	igt_subtest("getfb2-handle-zero") {
+		struct drm_mode_fb_cmd2 get = {};
+		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB2, &get, ENOENT);
+	}
+
+	igt_subtest("getfb2-handle-closed") {
+		struct drm_mode_fb_cmd2 add = add_basic;
+		struct drm_mode_fb_cmd2 get = { };
+
+		add.handles[0] = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
+		igt_assert(add.handles[0]);
+		do_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &add);
+		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &add.fb_id);
 
+		get.fb_id = add.fb_id;
+		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB2, &get, ENOENT);
+		gem_close(fd, add.handles[0]);
+	}
+
+	igt_subtest("getfb2-handle-not-fb") {
+		struct drm_mode_fb_cmd get = { .fb_id = get_prop_id(fd) };
+		igt_require(get.fb_id > 0);
+		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB, &get, ENOENT);
+	}
+
+	igt_subtest("getfb2-accept-ccs") {
+		struct drm_mode_fb_cmd2 add_ccs = { };
+		struct drm_mode_fb_cmd2 get = { };
+		int i;
+
+		get_ccs_fb(fd, &add_ccs);
+		igt_require(add_ccs.fb_id != 0);
+		get.fb_id = add_ccs.fb_id;
+		do_ioctl(fd, DRM_IOCTL_MODE_GETFB2, &get);
+
+		igt_assert_eq_u32(get.width, add_ccs.width);
+		igt_assert_eq_u32(get.height, add_ccs.height);
+		igt_assert(get.flags & DRM_MODE_FB_MODIFIERS);
+
+		for (i = 0; i < ARRAY_SIZE(get.handles); i++) {
+			igt_assert_eq_u32(get.pitches[i], add_ccs.pitches[i]);
+			igt_assert_eq_u32(get.offsets[i], add_ccs.offsets[i]);
+			if (add_ccs.handles[i] != 0) {
+				igt_assert_neq_u32(get.handles[i], 0);
+				igt_assert_neq_u32(get.handles[i],
+						   add_ccs.handles[i]);
+				igt_assert_eq_u64(get.modifier[i],
+						  add_ccs.modifier[i]);
+			} else {
+				igt_assert_eq_u32(get.handles[i], 0);
+				igt_assert_eq_u64(get.modifier[i],
+						  DRM_FORMAT_MOD_INVALID);
+			}
+		}
+		igt_assert_eq_u32(get.handles[0], get.handles[1]);
+
+		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &get.fb_id);
+		gem_close(fd, add_ccs.handles[0]);
+		gem_close(fd, get.handles[0]);
+	}
+
+	igt_fixture {
+		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &add_basic.fb_id);
+		gem_close(fd, add_basic.handles[0]);
+	}
 }
 
 igt_main
@@ -200,6 +289,8 @@ igt_main
 
 	test_duplicate_handles(fd);
 
+	test_getfb2(fd);
+
 	igt_fixture
 		close(fd);
 }
-- 
2.16.2

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

[igt-dev] [PATCH i-g-t 3/3] tests/kms_getfb: Add getfb2 tests

[Daniel Stone]

Mirroring addfb2, add tests for the new ioctl which will return us
information about framebuffers containing multiple buffers, as well as
modifiers.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 tests/kms_getfb.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
index a9852626..b8583694 100644
--- a/tests/kms_getfb.c
+++ b/tests/kms_getfb.c
@@ -186,7 +186,96 @@ static void test_duplicate_handles(int fd)
 		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &add.fb_id);
 		gem_close(fd, add.handles[0]);
 	}
+}
+
+static void test_getfb2(int fd)
+{
+	struct drm_mode_fb_cmd2 add_basic = {};
+
+	igt_fixture {
+		struct drm_mode_fb_cmd2 get = {};
+
+		add_basic.width = 1024;
+		add_basic.height = 1024;
+		add_basic.pixel_format = DRM_FORMAT_XRGB8888;
+		add_basic.pitches[0] = 1024*4;
+		add_basic.handles[0] = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
+		igt_assert(add_basic.handles[0]);
+		do_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &add_basic);
+
+		get.fb_id = add_basic.fb_id;
+		do_ioctl(fd, DRM_IOCTL_MODE_GETFB2, &get);
+		igt_assert_neq_u32(get.handles[0], 0);
+		gem_close(fd, get.handles[0]);
+	}
+
+	igt_subtest("getfb2-handle-zero") {
+		struct drm_mode_fb_cmd2 get = {};
+		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB2, &get, ENOENT);
+	}
+
+	igt_subtest("getfb2-handle-closed") {
+		struct drm_mode_fb_cmd2 add = add_basic;
+		struct drm_mode_fb_cmd2 get = { };
+
+		add.handles[0] = igt_create_bo_with_dimensions(fd, 1024, 1024,
+			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
+		igt_assert(add.handles[0]);
+		do_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &add);
+		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &add.fb_id);
 
+		get.fb_id = add.fb_id;
+		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB2, &get, ENOENT);
+		gem_close(fd, add.handles[0]);
+	}
+
+	igt_subtest("getfb2-handle-not-fb") {
+		struct drm_mode_fb_cmd get = { .fb_id = get_prop_id(fd) };
+		igt_require(get.fb_id > 0);
+		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB, &get, ENOENT);
+	}
+
+	igt_subtest("getfb2-accept-ccs") {
+		struct drm_mode_fb_cmd2 add_ccs = { };
+		struct drm_mode_fb_cmd2 get = { };
+		int i;
+
+		get_ccs_fb(fd, &add_ccs);
+		igt_require(add_ccs.fb_id != 0);
+		get.fb_id = add_ccs.fb_id;
+		do_ioctl(fd, DRM_IOCTL_MODE_GETFB2, &get);
+
+		igt_assert_eq_u32(get.width, add_ccs.width);
+		igt_assert_eq_u32(get.height, add_ccs.height);
+		igt_assert(get.flags & DRM_MODE_FB_MODIFIERS);
+
+		for (i = 0; i < ARRAY_SIZE(get.handles); i++) {
+			igt_assert_eq_u32(get.pitches[i], add_ccs.pitches[i]);
+			igt_assert_eq_u32(get.offsets[i], add_ccs.offsets[i]);
+			if (add_ccs.handles[i] != 0) {
+				igt_assert_neq_u32(get.handles[i], 0);
+				igt_assert_neq_u32(get.handles[i],
+						   add_ccs.handles[i]);
+				igt_assert_eq_u64(get.modifier[i],
+						  add_ccs.modifier[i]);
+			} else {
+				igt_assert_eq_u32(get.handles[i], 0);
+				igt_assert_eq_u64(get.modifier[i],
+						  DRM_FORMAT_MOD_INVALID);
+			}
+		}
+		igt_assert_eq_u32(get.handles[0], get.handles[1]);
+
+		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &get.fb_id);
+		gem_close(fd, add_ccs.handles[0]);
+		gem_close(fd, get.handles[0]);
+	}
+
+	igt_fixture {
+		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &add_basic.fb_id);
+		gem_close(fd, add_basic.handles[0]);
+	}
 }
 
 igt_main
@@ -200,6 +289,8 @@ igt_main
 
 	test_duplicate_handles(fd);
 
+	test_getfb2(fd);
+
 	igt_fixture
 		close(fd);
 }
-- 
2.16.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [PATCH 0/8] Add GetFB2 ioctl

[Daniel Stone]

On 23 March 2018 at 13:42, Daniel Stone <daniel@fooishbar.org> wrote:
> This submission rights that historical wrong, which allows Xorg
> -background none to continue to work in the face of exotic buffers.
> I've written patches to Xorg to use this as UABI verification, and
> I'll post a link to that very shortly.

Et voila:
https://lists.x.org/archives/xorg-devel/2018-March/056363.html

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

Re: [igt-dev] [PATCH 0/8] Add GetFB2 ioctl

[Daniel Stone]

On 23 March 2018 at 13:42, Daniel Stone <daniel@fooishbar.org> wrote:
> This submission rights that historical wrong, which allows Xorg
> -background none to continue to work in the face of exotic buffers.
> I've written patches to Xorg to use this as UABI verification, and
> I'll post a link to that very shortly.

Et voila:
https://lists.x.org/archives/xorg-devel/2018-March/056363.html

Cheers,
Daniel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer

[Ville Syrjälä]

On Fri, Mar 23, 2018 at 01:45:50PM +0000, Daniel Stone wrote:
> Since drm_framebuffer can now store GEM objects directly, place them
> there rather than in our own subclass.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +--
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 49b0772e9abc..e8100a4fc877 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13916,7 +13916,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>  	drm_framebuffer_cleanup(fb);
>  
>  	i915_gem_object_lock(obj);
> -	WARN_ON(!obj->framebuffer_references--);
> +	WARN_ON(obj->framebuffer_references < fb->format->num_planes);
> +	obj->framebuffer_references -= fb->format->num_planes;

Hmm. I'm thinking we can stick to the single reference per fb.
IIRC this counter is there just to prevent changes of the obj
tiling mode as long as any fb exists that uses the object. So
doesn't actually matter how many planes the fb has.

Naturally the story would be slightly difference if we supported
fbs using multiple different BOs, as each BO would need to get its
framebuffer_references adjusted.

>  	i915_gem_object_unlock(obj);
>  
>  	i915_gem_object_put(obj);
> @@ -14176,9 +14177,9 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  				      i, fb->pitches[i], stride_alignment);
>  			goto err;
>  		}
> -	}
>  
> -	intel_fb->obj = obj;
> +		fb->obj[i] = &obj->base;
> +	}
>  
>  	ret = intel_fill_fb_info(dev_priv, fb);
>  	if (ret)
> @@ -14190,6 +14191,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  		goto err;
>  	}
>  
> +	/* We already hold one reference to the fb, but in case it's
> +	 * multi-planar and we've placed multiple pointers in
> +	 * drm_framebuffer, hold extra refs.
> +	 */
> +	i915_gem_object_lock(obj);
> +	obj->framebuffer_references += fb->format->num_planes - 1;
> +	i915_gem_object_unlock(obj);
> +
>  	return 0;
>  
>  err:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 570f89b31544..d93ed9e59867 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -186,7 +186,6 @@ enum intel_output_type {
>  
>  struct intel_framebuffer {
>  	struct drm_framebuffer base;
> -	struct drm_i915_gem_object *obj;
>  	struct intel_rotation_info rot_info;
>  
>  	/* for each plane in the normal GTT view */
> @@ -985,7 +984,7 @@ struct cxsr_latency {
>  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
>  #define to_intel_plane(x) container_of(x, struct intel_plane, base)
>  #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
> -#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
> +#define intel_fb_obj(x) (((x) && (x)->obj[0]) ? to_intel_bo((x)->obj[0]) : NULL)

Ah, I've had that "(x)" part coded up so many times, but never sent it
out :)

>  
>  struct intel_hdmi {
>  	i915_reg_t hdmi_reg;
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: Reshuffle getfb error returns

[Ville Syrjälä]

On Fri, Mar 23, 2018 at 01:45:51PM +0000, Daniel Stone wrote:
> Make it a little more clear what's going on inside of getfb, and also
> make it easier to add alternate paths to get a handle in future.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>

Much better.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_framebuffer.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index ad67203de715..6d5ff541225a 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -468,29 +468,30 @@ int drm_mode_getfb(struct drm_device *dev,
>  		goto out;
>  	}
>  
> +	if (!fb->funcs->create_handle) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
>  	r->height = fb->height;
>  	r->width = fb->width;
>  	r->depth = fb->format->depth;
>  	r->bpp = fb->format->cpp[0] * 8;
>  	r->pitch = fb->pitches[0];
> -	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,
> -						       &r->handle);
> -		} 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->handle = 0;
> -			ret = 0;
> -		}
> -	} else {
> -		ret = -ENODEV;
> +
> +	/* 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. */
> +	if (!drm_is_current_master(file_priv) && !capable(CAP_SYS_ADMIN) &&
> +	    !drm_is_control_client(file_priv)) {
> +		r->handle = 0;
> +		ret = 0;
> +		goto out;
>  	}
>  
> +	ret = fb->funcs->create_handle(fb, file_priv, &r->handle);
> +
>  out:
>  	drm_framebuffer_put(fb);
>  
> -- 
> 2.16.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: Add getfb2 ioctl

[Ville Syrjälä]

On Fri, Mar 23, 2018 at 01:45:52PM +0000, Daniel Stone wrote:
> getfb2 allows us to pass multiple planes and modifiers, just like addfb2
> over addfb.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  drivers/gpu/drm/drm_crtc_internal.h |   2 +
>  drivers/gpu/drm/drm_framebuffer.c   | 109 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_ioctl.c         |   2 +
>  include/uapi/drm/drm.h              |   1 +
>  4 files changed, 114 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 3c2b82865ad2..0cd02f3d203d 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -173,6 +173,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_ioctl(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_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 6d5ff541225a..f1cfb6ddc776 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/drm_atomic.h>
>  #include <drm/drm_print.h>
>  
> @@ -494,7 +495,115 @@ int drm_mode_getfb(struct drm_device *dev,
>  
>  out:
>  	drm_framebuffer_put(fb);
> +	return ret;
> +}
> +
> +/**
> + * drm_mode_getfb2 - get extended 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_ioctl(struct drm_device *dev,
> +			  void *data, struct drm_file *file_priv)
> +{
> +	struct drm_mode_fb_cmd2 *r = data;
> +	struct drm_framebuffer *fb;
> +	unsigned int i;
> +	int ret;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +		return -EINVAL;
>  
> +	fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
> +	if (!fb)
> +		return -ENOENT;
> +
> +	/* For multi-plane framebuffers, we require the driver to place the
> +	 * GEM objects directly in the drm_framebuffer. For single-plane
> +	 * framebuffers, we can fall back to create_handle.
> +	 */
> +	if (!fb->obj[0] &&
> +	    (fb->format->num_planes > 1 || !fb->funcs->create_handle)) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	r->height = fb->height;
> +	r->width = fb->width;
> +	r->pixel_format = fb->format->format;
> +
> +	r->flags = 0;
> +	if (dev->mode_config.allow_fb_modifiers)
> +		r->flags |= DRM_MODE_FB_MODIFIERS;
> +
> +	for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> +		r->handles[i] = 0;
> +		r->pitches[i] = 0;
> +		r->offsets[i] = 0;
> +		r->modifier[i] = DRM_FORMAT_MOD_INVALID;

Don't we want to leave this zeroed too? For addfb2 we require any unused
modifier to be 0, so if someone does 'getfb2(&cmd); addfb2(&cmd);' they
would get an error from the addfb2().

> +	}
> +
> +	for (i = 0; i < fb->format->num_planes; i++) {
> +		int j;
> +
> +		r->pitches[i] = fb->pitches[i];
> +		r->offsets[i] = fb->offsets[i];
> +		if (dev->mode_config.allow_fb_modifiers)
> +			r->modifier[i] = fb->modifier;
> +
> +		/* If we reuse the same object for multiple planes, also
> +		 * return the same handle.
> +		 */
> +		for (j = 0; j < i; j++) {
> +			if (fb->obj[i] == fb->obj[j]) {
> +				r->handles[i] = r->handles[j];
> +				break;
> +			}
> +		}
> +
> +		if (r->handles[i])
> +			continue;
> +
> +		if (fb->obj[i]) {
> +			ret = drm_gem_handle_create(file_priv, fb->obj[i],
> +						    &r->handles[i]);
> +		} else {
> +			WARN_ON(i > 0);
> +			ret = fb->funcs->create_handle(fb, file_priv,
> +						       &r->handles[i]);
> +		}
> +
> +		if (ret != 0)
> +			goto out;
> +	}
> +
> +out:
> +	if (ret != 0) {
> +		/* Delete any previously-created handles on failure. */
> +		for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> +			int j;
> +
> +			if (r->handles[i])
> +				drm_gem_handle_delete(file_priv, r->handles[i]);
> +
> +			/* Zero out any handles identical to the one we just
> +			 * deleted. */
> +			for (j = i + 1; j < ARRAY_SIZE(r->handles); j++) {
> +				if (r->handles[j] == r->handles[i])
> +					r->handles[j] = 0;
> +			}
> +		}
> +	}
> +
> +	drm_framebuffer_put(fb);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index af782911c505..b5896e3615e5 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -669,6 +669,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB2, drm_mode_getfb2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  };
>  
>  #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 6fdff5945c8a..9a33613394a9 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -892,6 +892,7 @@ extern "C" {
>  #define DRM_IOCTL_MODE_LIST_LESSEES	DRM_IOWR(0xC7, struct drm_mode_list_lessees)
>  #define DRM_IOCTL_MODE_GET_LEASE	DRM_IOWR(0xC8, struct drm_mode_get_lease)
>  #define DRM_IOCTL_MODE_REVOKE_LEASE	DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
> +#define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCA, struct drm_mode_fb_cmd2)
>  
>  /**
>   * Device specific ioctls should only be in their respective headers
> -- 
> 2.16.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer

[Daniel Stone]

Hi Ville,

On 23 March 2018 at 14:42, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Mar 23, 2018 at 01:45:50PM +0000, Daniel Stone wrote:
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13916,7 +13916,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>>       drm_framebuffer_cleanup(fb);
>>
>>       i915_gem_object_lock(obj);
>> -     WARN_ON(!obj->framebuffer_references--);
>> +     WARN_ON(obj->framebuffer_references < fb->format->num_planes);
>> +     obj->framebuffer_references -= fb->format->num_planes;
>
> Hmm. I'm thinking we can stick to the single reference per fb.
> IIRC this counter is there just to prevent changes of the obj
> tiling mode as long as any fb exists that uses the object. So
> doesn't actually matter how many planes the fb has.
>
> Naturally the story would be slightly difference if we supported
> fbs using multiple different BOs, as each BO would need to get its
> framebuffer_references adjusted.

Yeah, fair enough. It looks a little bit weird (perhaps deserving of a
comment) there. The reason to do that was just the general principle
of having one reference per object pointer, especially when other
drivers (ones which can have separate BOs in a single logical image)
will and do refcount them separately. Having different refcounting
semantics in shared structures depending on which driver is in use
makes me itchy.

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

Re: [PATCH i-g-t 1/3] tests/kms_getfb: Split property-ID get into helper

[Ville Syrjälä]

On Fri, Mar 23, 2018 at 01:46:14PM +0000, Daniel Stone wrote:
> We'll want to reuse this, so split it out into a (smaller!) helper.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  tests/kms_getfb.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
> index c5968e75..a9852626 100644
> --- a/tests/kms_getfb.c
> +++ b/tests/kms_getfb.c
> @@ -72,6 +72,23 @@ static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret)
>  		gem_close(fd, add.handles[0]);
>  }
>  
> +/**
> + * Find and return an arbitrary valid property ID.
> + */
> +static uint32_t get_prop_id(int fd)

get_any_prop_id() or something like that maybe?

Either way
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +{
> +	igt_display_t display;
> +
> +	igt_display_init(&display, fd);
> +	for (int i = 0; i < display.n_outputs; i++) {
> +		igt_output_t *output = &display.outputs[i];
> +		if (output->props[IGT_CONNECTOR_DPMS] != 0)
> +			return output->props[IGT_CONNECTOR_DPMS];
> +	}
> +
> +	return 0;
> +}
> +
>  static void test_handle_input(int fd)
>  {
>  	struct drm_mode_fb_cmd2 add = {};
> @@ -111,23 +128,8 @@ static void test_handle_input(int fd)
>  	}
>  
>  	igt_subtest("getfb-handle-not-fb") {
> -		struct drm_mode_fb_cmd get = { };
> -		uint32_t prop_id = 0;
> -		igt_display_t display;
> -
> -		/* Find a valid property ID to use. */
> -		igt_display_init(&display, fd);
> -		for (int i = 0; i < display.n_outputs; i++) {
> -			igt_output_t *output = &display.outputs[i];
> -
> -			if (output->props[IGT_CONNECTOR_DPMS] != 0) {
> -				prop_id = output->props[IGT_CONNECTOR_DPMS];
> -				break;
> -			}
> -		}
> -		igt_require(prop_id > 0);
> -
> -		get.fb_id = prop_id;
> +		struct drm_mode_fb_cmd get = { .fb_id = get_prop_id(fd) };
> +		igt_require(get.fb_id > 0);
>  		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB, &get, ENOENT);
>  	}
>  }
> -- 
> 2.16.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [igt-dev] [PATCH i-g-t 1/3] tests/kms_getfb: Split property-ID get into helper

[Ville Syrjälä]

On Fri, Mar 23, 2018 at 01:46:14PM +0000, Daniel Stone wrote:
> We'll want to reuse this, so split it out into a (smaller!) helper.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  tests/kms_getfb.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
> index c5968e75..a9852626 100644
> --- a/tests/kms_getfb.c
> +++ b/tests/kms_getfb.c
> @@ -72,6 +72,23 @@ static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret)
>  		gem_close(fd, add.handles[0]);
>  }
>  
> +/**
> + * Find and return an arbitrary valid property ID.
> + */
> +static uint32_t get_prop_id(int fd)

get_any_prop_id() or something like that maybe?

Either way
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +{
> +	igt_display_t display;
> +
> +	igt_display_init(&display, fd);
> +	for (int i = 0; i < display.n_outputs; i++) {
> +		igt_output_t *output = &display.outputs[i];
> +		if (output->props[IGT_CONNECTOR_DPMS] != 0)
> +			return output->props[IGT_CONNECTOR_DPMS];
> +	}
> +
> +	return 0;
> +}
> +
>  static void test_handle_input(int fd)
>  {
>  	struct drm_mode_fb_cmd2 add = {};
> @@ -111,23 +128,8 @@ static void test_handle_input(int fd)
>  	}
>  
>  	igt_subtest("getfb-handle-not-fb") {
> -		struct drm_mode_fb_cmd get = { };
> -		uint32_t prop_id = 0;
> -		igt_display_t display;
> -
> -		/* Find a valid property ID to use. */
> -		igt_display_init(&display, fd);
> -		for (int i = 0; i < display.n_outputs; i++) {
> -			igt_output_t *output = &display.outputs[i];
> -
> -			if (output->props[IGT_CONNECTOR_DPMS] != 0) {
> -				prop_id = output->props[IGT_CONNECTOR_DPMS];
> -				break;
> -			}
> -		}
> -		igt_require(prop_id > 0);
> -
> -		get.fb_id = prop_id;
> +		struct drm_mode_fb_cmd get = { .fb_id = get_prop_id(fd) };
> +		igt_require(get.fb_id > 0);
>  		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB, &get, ENOENT);
>  	}
>  }
> -- 
> 2.16.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/3] tests/kms_getfb: Split property-ID get into helper

[Daniel Stone]

On 23 March 2018 at 14:53, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Mar 23, 2018 at 01:46:14PM +0000, Daniel Stone wrote:
>> +/**
>> + * Find and return an arbitrary valid property ID.
>> + */
>> +static uint32_t get_prop_id(int fd)
>
> get_any_prop_id() or something like that maybe?

Yeah, a good choice of colour. :)

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

Re: [igt-dev] [PATCH i-g-t 1/3] tests/kms_getfb: Split property-ID get into helper

[Daniel Stone]

On 23 March 2018 at 14:53, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Mar 23, 2018 at 01:46:14PM +0000, Daniel Stone wrote:
>> +/**
>> + * Find and return an arbitrary valid property ID.
>> + */
>> +static uint32_t get_prop_id(int fd)
>
> get_any_prop_id() or something like that maybe?

Yeah, a good choice of colour. :)

Cheers,
Daniel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_getfb: Add getfb2 tests

[Ville Syrjälä]

On Fri, Mar 23, 2018 at 01:46:16PM +0000, Daniel Stone wrote:
> Mirroring addfb2, add tests for the new ioctl which will return us
> information about framebuffers containing multiple buffers, as well as
> modifiers.

lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Maybe we also want to encode my earlier 'getfb2(&cmd); addfb2(&cmd);'
idea into a test?

> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  tests/kms_getfb.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
> index a9852626..b8583694 100644
> --- a/tests/kms_getfb.c
> +++ b/tests/kms_getfb.c
> @@ -186,7 +186,96 @@ static void test_duplicate_handles(int fd)
>  		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &add.fb_id);
>  		gem_close(fd, add.handles[0]);
>  	}
> +}
> +
> +static void test_getfb2(int fd)
> +{
> +	struct drm_mode_fb_cmd2 add_basic = {};
> +
> +	igt_fixture {
> +		struct drm_mode_fb_cmd2 get = {};
> +
> +		add_basic.width = 1024;
> +		add_basic.height = 1024;
> +		add_basic.pixel_format = DRM_FORMAT_XRGB8888;
> +		add_basic.pitches[0] = 1024*4;
> +		add_basic.handles[0] = igt_create_bo_with_dimensions(fd, 1024, 1024,
> +			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
> +		igt_assert(add_basic.handles[0]);
> +		do_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &add_basic);
> +
> +		get.fb_id = add_basic.fb_id;
> +		do_ioctl(fd, DRM_IOCTL_MODE_GETFB2, &get);
> +		igt_assert_neq_u32(get.handles[0], 0);
> +		gem_close(fd, get.handles[0]);
> +	}
> +
> +	igt_subtest("getfb2-handle-zero") {
> +		struct drm_mode_fb_cmd2 get = {};
> +		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB2, &get, ENOENT);
> +	}
> +
> +	igt_subtest("getfb2-handle-closed") {
> +		struct drm_mode_fb_cmd2 add = add_basic;
> +		struct drm_mode_fb_cmd2 get = { };
> +
> +		add.handles[0] = igt_create_bo_with_dimensions(fd, 1024, 1024,
> +			DRM_FORMAT_XRGB8888, 0, 0, NULL, NULL, NULL);
> +		igt_assert(add.handles[0]);
> +		do_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &add);
> +		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &add.fb_id);
>  
> +		get.fb_id = add.fb_id;
> +		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB2, &get, ENOENT);
> +		gem_close(fd, add.handles[0]);
> +	}
> +
> +	igt_subtest("getfb2-handle-not-fb") {
> +		struct drm_mode_fb_cmd get = { .fb_id = get_prop_id(fd) };
> +		igt_require(get.fb_id > 0);
> +		do_ioctl_err(fd, DRM_IOCTL_MODE_GETFB, &get, ENOENT);
> +	}
> +
> +	igt_subtest("getfb2-accept-ccs") {
> +		struct drm_mode_fb_cmd2 add_ccs = { };
> +		struct drm_mode_fb_cmd2 get = { };
> +		int i;
> +
> +		get_ccs_fb(fd, &add_ccs);
> +		igt_require(add_ccs.fb_id != 0);
> +		get.fb_id = add_ccs.fb_id;
> +		do_ioctl(fd, DRM_IOCTL_MODE_GETFB2, &get);
> +
> +		igt_assert_eq_u32(get.width, add_ccs.width);
> +		igt_assert_eq_u32(get.height, add_ccs.height);
> +		igt_assert(get.flags & DRM_MODE_FB_MODIFIERS);
> +
> +		for (i = 0; i < ARRAY_SIZE(get.handles); i++) {
> +			igt_assert_eq_u32(get.pitches[i], add_ccs.pitches[i]);
> +			igt_assert_eq_u32(get.offsets[i], add_ccs.offsets[i]);
> +			if (add_ccs.handles[i] != 0) {
> +				igt_assert_neq_u32(get.handles[i], 0);
> +				igt_assert_neq_u32(get.handles[i],
> +						   add_ccs.handles[i]);
> +				igt_assert_eq_u64(get.modifier[i],
> +						  add_ccs.modifier[i]);
> +			} else {
> +				igt_assert_eq_u32(get.handles[i], 0);
> +				igt_assert_eq_u64(get.modifier[i],
> +						  DRM_FORMAT_MOD_INVALID);
> +			}
> +		}
> +		igt_assert_eq_u32(get.handles[0], get.handles[1]);
> +
> +		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &get.fb_id);
> +		gem_close(fd, add_ccs.handles[0]);
> +		gem_close(fd, get.handles[0]);
> +	}
> +
> +	igt_fixture {
> +		do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &add_basic.fb_id);
> +		gem_close(fd, add_basic.handles[0]);
> +	}
>  }
>  
>  igt_main
> @@ -200,6 +289,8 @@ igt_main
>  
>  	test_duplicate_handles(fd);
>  
> +	test_getfb2(fd);
> +
>  	igt_fixture
>  		close(fd);
>  }
> -- 
> 2.16.2
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: Add getfb2 ioctl

[Daniel Stone]

Hi,

On 23 March 2018 at 14:49, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Mar 23, 2018 at 01:45:52PM +0000, Daniel Stone wrote:
>> +     for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
>> +             r->handles[i] = 0;
>> +             r->pitches[i] = 0;
>> +             r->offsets[i] = 0;
>> +             r->modifier[i] = DRM_FORMAT_MOD_INVALID;
>
> Don't we want to leave this zeroed too? For addfb2 we require any unused
> modifier to be 0, so if someone does 'getfb2(&cmd); addfb2(&cmd);' they
> would get an error from the addfb2().

My thinking is that since the primary userspace for this doesn't have
symmetry with add (args for add, struct for get), that it was better
to feed in INVALID directly. This is going to change, e.g., X server
to:
  modifier = (fb->flags ? DRM_MODE_FB_MODIFIERS) ? fb->modifier :
DRM_FORMAT_MOD_INVALID;

It's a good point about the symmetry though. Do you know of direct
non-libdrm users? Apart from igt of course ;)

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

Re: [PATCH 4/4] drm: Add getfb2 ioctl

[Ville Syrjälä]

On Fri, Mar 23, 2018 at 05:00:11PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 23 March 2018 at 14:49, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Mar 23, 2018 at 01:45:52PM +0000, Daniel Stone wrote:
> >> +     for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> >> +             r->handles[i] = 0;
> >> +             r->pitches[i] = 0;
> >> +             r->offsets[i] = 0;
> >> +             r->modifier[i] = DRM_FORMAT_MOD_INVALID;
> >
> > Don't we want to leave this zeroed too? For addfb2 we require any unused
> > modifier to be 0, so if someone does 'getfb2(&cmd); addfb2(&cmd);' they
> > would get an error from the addfb2().
> 
> My thinking is that since the primary userspace for this doesn't have
> symmetry with add (args for add, struct for get), that it was better
> to feed in INVALID directly. This is going to change, e.g., X server
> to:
>   modifier = (fb->flags ? DRM_MODE_FB_MODIFIERS) ? fb->modifier :
> DRM_FORMAT_MOD_INVALID;
> 
> It's a good point about the symmetry though. Do you know of direct
> non-libdrm users? Apart from igt of course ;)

Nope. Just thought that since both take the same struct it'd make some
sense. And figured it could serve as a quick sanity check to make sure
getfb outputs sane data. Or rather, if the driver accepts it back in
it can't be all bad at least.

But if you think it's not a particularly useful thing to do then I'm
certainly willing to accept that.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: Add getfb2 ioctl

[Daniel Stone]

On 23 March 2018 at 17:31, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Mar 23, 2018 at 05:00:11PM +0000, Daniel Stone wrote:
>> On 23 March 2018 at 14:49, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Fri, Mar 23, 2018 at 01:45:52PM +0000, Daniel Stone wrote:
>> >> +     for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
>> >> +             r->handles[i] = 0;
>> >> +             r->pitches[i] = 0;
>> >> +             r->offsets[i] = 0;
>> >> +             r->modifier[i] = DRM_FORMAT_MOD_INVALID;
>> >
>> > Don't we want to leave this zeroed too? For addfb2 we require any unused
>> > modifier to be 0, so if someone does 'getfb2(&cmd); addfb2(&cmd);' they
>> > would get an error from the addfb2().
>>
>> My thinking is that since the primary userspace for this doesn't have
>> symmetry with add (args for add, struct for get), that it was better
>> to feed in INVALID directly. This is going to change, e.g., X server
>> to:
>>   modifier = (fb->flags ? DRM_MODE_FB_MODIFIERS) ? fb->modifier :
>> DRM_FORMAT_MOD_INVALID;
>>
>> It's a good point about the symmetry though. Do you know of direct
>> non-libdrm users? Apart from igt of course ;)
>
> Nope. Just thought that since both take the same struct it'd make some
> sense. And figured it could serve as a quick sanity check to make sure
> getfb outputs sane data. Or rather, if the driver accepts it back in
> it can't be all bad at least.
>
> But if you think it's not a particularly useful thing to do then I'm
> certainly willing to accept that.

Makes sense. I'm on the fence; let's see if anyone else has any suggestions.

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

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer

[Daniel Stone]

Hi Ville,

On 23 March 2018 at 14:49, Daniel Stone <daniel@fooishbar.org> wrote:
> On 23 March 2018 at 14:42, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> Hmm. I'm thinking we can stick to the single reference per fb.
>> IIRC this counter is there just to prevent changes of the obj
>> tiling mode as long as any fb exists that uses the object. So
>> doesn't actually matter how many planes the fb has.
>>
>> Naturally the story would be slightly difference if we supported
>> fbs using multiple different BOs, as each BO would need to get its
>> framebuffer_references adjusted.
>
> Yeah, fair enough. It looks a little bit weird (perhaps deserving of a
> comment) there. The reason to do that was just the general principle
> of having one reference per object pointer, especially when other
> drivers (ones which can have separate BOs in a single logical image)
> will and do refcount them separately. Having different refcounting
> semantics in shared structures depending on which driver is in use
> makes me itchy.

Absent any other comment, I've dropped this change and will keep a
single framebuffer_reference[s] for v2.

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