Add GetFB2 ioctl

PATCH In review Daniel Stone 3 participants 21 messages Mar 23, 2018 – May 17, 2018

Subsystems (3)

DRM drivers DRM misc Intel i915

lore

Share & tools

lore

Daniel Stone

author

2018-03-23 13:42 UTC #0 lore raw

cover Add GetFB2 ioctl

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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Daniel Stone

2018-03-23 13:45 UTC #1 lore raw

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

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 <[email protected]>

Cc Jani Nikula <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc [email protected]

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

drivers/gpu/drm/i915/i915_debugfs.c +2 -2

index 7816cd53100a..68541df6f601 100644
@@ -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);

drivers/gpu/drm/i915/intel_display.c +10 -9

index 1b16b534871a..49b0772e9abc 100644
@@ -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");

drivers/gpu/drm/i915/intel_fbdev.c +5 -4

index 65a3313723c9..fdef18488fb1 100644
@@ -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);

Signature · 6 lines

2.16.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Stone

2018-03-23 13:45 UTC #2 lore raw

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

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

Signed-off-by Daniel Stone <[email protected]>

Cc Jani Nikula <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  3 +--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/gpu/drm/i915/intel_display.c +12 -3

index 49b0772e9abc..e8100a4fc877 100644
@@ -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:

drivers/gpu/drm/i915/intel_drv.h +1 -2

index 570f89b31544..d93ed9e59867 100644
@@ -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;

Signature · 6 lines

2.16.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä

2018-03-23 14:42 UTC #3 lore raw

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

Quoted 18 lines · 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 <[email protected]>
Cc Jani Nikula <[email protected]>
Cc Joonas Lahtinen <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Cc [email protected]
---
 drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  3 +--
 2 files changed, 13 insertions(+), 5 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -0
index 49b0772e9abc..e8100a4fc877 100644

drivers/gpu/drm/i915/intel_display.c static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb) · line 13920

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c +2 -1

@@ -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.

Quoted 43 lines

drivers/gpu/drm/i915/intel_display.c +10 -2

 	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:

drivers/gpu/drm/i915/intel_drv.h +0 -1

index 570f89b31544..d93ed9e59867 100644
@@ -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 */

drivers/gpu/drm/i915/intel_drv.h struct cxsr_latency { · line 987

Quoted 5 lines

drivers/gpu/drm/i915/intel_drv.h +1 -1

@@ -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 :)

Quoted 10 lines

drivers/gpu/drm/i915/intel_drv.h +0 -0

 struct intel_hdmi {
 	i915_reg_t hdmi_reg;

Signature · 6 lines

2.16.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines

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

Daniel Stone

author

2018-03-23 14:49 UTC #4 lore raw

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

Hi Ville,

Quoted 16 lines · Ville Syrjälä

drivers/gpu/drm/i915/intel_display.c static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb) · line 13920
Quoted 9 lines · Daniel Stone
drivers/gpu/drm/i915/intel_display.c +2 -1
@@ -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.

Quoted 3 lines

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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Stone

author

2018-05-17 13:18 UTC #5 lore raw

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

Hi Ville,

Quoted 10 lines · Daniel Stone

Quoted 8 lines · Ville Syrjälä
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.

Quoted 7 lines

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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Stone

2018-03-23 13:45 UTC #6 lore raw

PATCH 3/4 drm: Reshuffle getfb error returns

In reply to 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 <[email protected]>

---
 drivers/gpu/drm/drm_framebuffer.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

drivers/gpu/drm/drm_framebuffer.c +17 ~~-16~~

index ad67203de715..6d5ff541225a 100644
@@ -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);

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

Reviewed-by on v1 3/4

2018-03-23 14:43 UTC #7 lore raw

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

Quoted 4 lines · 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 <[email protected]>

Much better.

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 62 lines

---
 drivers/gpu/drm/drm_framebuffer.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

drivers/gpu/drm/drm_framebuffer.c +17 ~~-16~~

index ad67203de715..6d5ff541225a 100644
@@ -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);

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 6 lines

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

Daniel Stone

2018-03-23 13:45 UTC #8 lore raw

PATCH 4/4 drm: Add getfb2 ioctl

In reply to Daniel Stone

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

Signed-off-by Daniel Stone <[email protected]>

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

drivers/gpu/drm/drm_crtc_internal.h +2 -0

index 3c2b82865ad2..0cd02f3d203d 100644
@@ -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);

drivers/gpu/drm/drm_framebuffer.c +109 -0

122 lines not on this page · raw message

drivers/gpu/drm/drm_ioctl.c +2 -0

index af782911c505..b5896e3615e5 100644
@@ -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 )

include/uapi/drm/drm.h +1 -0

index 6fdff5945c8a..9a33613394a9 100644
@@ -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

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

2018-03-23 14:49 UTC #9 lore raw

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

Quoted 84 lines · Daniel Stone

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

Signed-off-by Daniel Stone <[email protected]>

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

drivers/gpu/drm/drm_crtc_internal.h +2 -0

index 3c2b82865ad2..0cd02f3d203d 100644
@@ -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);

drivers/gpu/drm/drm_framebuffer.c +44 -0

index 6d5ff541225a..f1cfb6ddc776 100644
@@ -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;

drivers/gpu/drm/drm_framebuffer.c int drm_mode_getfb(struct drm_device *dev, · line 551

Quoted 10 lines

drivers/gpu/drm/drm_framebuffer.c +10 -0

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

Quoted 90 lines

drivers/gpu/drm/drm_framebuffer.c +55 -0

+	}
+
+	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;
 }

drivers/gpu/drm/drm_ioctl.c +2 -0

index af782911c505..b5896e3615e5 100644
@@ -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 )

include/uapi/drm/drm.h +1 -0

index 6fdff5945c8a..9a33613394a9 100644
@@ -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

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 6 lines

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

Daniel Stone

author

2018-03-23 17:00 UTC #10 lore raw

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

Hi,

Quoted 10 lines · Ville Syrjälä

drivers/gpu/drm/drm_framebuffer.c int drm_mode_getfb(struct drm_device *dev, · line 551
Quoted 5 lines · Daniel Stone
drivers/gpu/drm/drm_framebuffer.c +5 -0
+     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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

2018-03-23 17:31 UTC #11 lore raw

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

Quoted 21 lines · Daniel Stone

Hi,
Quoted 10 lines · Ville Syrjälä
drivers/gpu/drm/drm_framebuffer.c int drm_mode_getfb(struct drm_device *dev, · line 551
Quoted 5 lines · Daniel Stone
drivers/gpu/drm/drm_framebuffer.c +5 -0
+     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;

Quoted 2 lines

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.

Signature · 6 lines

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

Daniel Stone

author

2018-03-24 10:12 UTC #12 lore raw

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

Quoted 23 lines · Ville Syrjälä

Quoted 19 lines · Daniel Stone
Quoted 10 lines · Ville Syrjälä
drivers/gpu/drm/drm_framebuffer.c int drm_mode_getfb(struct drm_device *dev, · line 551
Quoted 5 lines · Daniel Stone
drivers/gpu/drm/drm_framebuffer.c +5 -0
+     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;
Quoted 2 lines
It's a good point about the symmetry though. Do you know of direct non-libdrm users? Apart from igt of course ;)

Quoted 7 lines

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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Stone

2018-03-23 13:45 UTC #13 lore raw

PATCH 1/1 NOMERGE: Add drmModeGetFB2

In reply to 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(+)

include/drm/drm.h +1 -0

index f0bd91de..c0a35878 100644
@@ -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

xf86drmMode.c +40 -0

index cf8e1eac..d8cf6a0c 100644
@@ -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);
+}

xf86drmMode.h +15 -0

index 3cd27aee..70741c77 100644
@@ -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.

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Stone

2018-03-23 13:46 UTC #14 lore raw

PATCH 1/3 tests/kms_getfb: Split property-ID get into helper

In reply to Daniel Stone

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

Signed-off-by Daniel Stone <[email protected]>

---
 tests/kms_getfb.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

tests/kms_getfb.c +19 ~~-17~~

index c5968e75..a9852626 100644
@@ -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);
 	}
 }

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Stone

2018-03-23 13:46 UTC #15 lore raw

PATCH 2/3 NOMERGE: Update DRM UAPI to latest kernel version

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

include/drm-uapi/amdgpu_drm.h +1 -0

index 1816bd82..528f6d04 100644
@@ -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 */

include/drm-uapi/drm.h +1 -0

index f0bd91de..c0a35878 100644
@@ -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

include/drm-uapi/drm_mode.h +6 -3

index 2c575794..50bcf421 100644
@@ -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 {

include/drm-uapi/etnaviv_drm.h +1 -0

index e9b997a0..71958c80 100644
@@ -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 */

include/drm-uapi/msm_drm.h +8 -1

index bbbaffad..57a67fb3 100644
@@ -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

include/drm-uapi/virtgpu_drm.h +1 -0

index 91a31ffe..9a781f06 100644
@@ -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;

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Stone

2018-03-23 13:46 UTC #16 lore raw

PATCH 3/3 tests/kms_getfb: Add getfb2 tests

In reply to 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 <[email protected]>

---
 tests/kms_getfb.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

tests/kms_getfb.c +91 -0

index a9852626..b8583694 100644
@@ -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);
 }

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

Reviewed-by on v1 3/3

2018-03-23 15:01 UTC #17 lore raw

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

Quoted 3 lines · Daniel Stone

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ä <[email protected]>

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

Quoted 123 lines

Signed-off-by Daniel Stone <[email protected]>

---
 tests/kms_getfb.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

tests/kms_getfb.c +91 -0

index a9852626..b8583694 100644
@@ -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);
 }

Signature · 6 lines

2.16.2

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Signature · 6 lines

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

Ville Syrjälä

Reviewed-by on v1 1/3

2018-03-23 14:53 UTC #18 lore raw

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

In reply to Daniel Stone

Quoted 12 lines · Daniel Stone

We'll want to reuse this, so split it out into a (smaller!) helper.
Signed-off-by Daniel Stone <[email protected]>
---
 tests/kms_getfb.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)
tests/kms_getfb.c +0 -0
index c5968e75..a9852626 100644

tests/kms_getfb.c static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret) · line 78

Quoted 7 lines

tests/kms_getfb.c +4 -0

@@ -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ä <[email protected]>

Quoted 49 lines

tests/kms_getfb.c +15 ~~-17~~

+{
+	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);
 	}
 }

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 6 lines

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

Daniel Stone

author

2018-03-23 14:55 UTC #19 lore raw

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

Quoted 7 lines · Ville Syrjälä

tests/kms_getfb.c static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret) · line 78
Quoted 4 lines · Daniel Stone
tests/kms_getfb.c +4 -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?

Yeah, a good choice of colour. :)

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Stone

author

2018-03-23 13:51 UTC #20 lore raw

In reply to Daniel Stone

Quoted 4 lines · Daniel Stone

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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel