[PATCH v13 0/7] drm/i915/gvt: Dma-buf support for GVT-g

[PATCH v13 0/7] drm/i915/gvt: Dma-buf support for GVT-g

[Tina Zhang]

v12->v13:
1) add comments to GEM proxy. (Chris)
2) don't ban GEM proxy in i915_gem_sw_finish_ioctl. (Chris)
3) check GEM proxy bar after finishing i915_gem_object_wait. (Chris)
4) remove GEM proxy bar in i915_gem_madvise_ioctl.

v11->v12:
1) add drm_format_mod back. (Gerd and Zhenyu)
2) add region_index. (Gerd)
3) refine the lifecycle of dmabuf.
4) send to dri-devel@lists.freedesktop.org. (Ville) 

v10->v11:
1) rename plane_type to drm_plane_type. (Gerd)
2) move fields of vfio_device_query_gfx_plane to
   vfio_device_gfx_plane_info. (Gerd)
3) remove drm_format_mod, start fields. (Daniel)
4) remove plane_id.

v9->v10:
1) remove dma-buf management
2) refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE
3) track the dma-buf create and release in kernel mode

v8->v9:
1) refine the dma-buf ioctl definition
2) add a lock to protect the dmabuf list
3) move drm format change to a separate patch
4) codes cleanup

v7->v8:
1) refine framebuffer decoder code
2) fix a bug in decoding primary plane

v6->v7:
1) release dma-buf related allocations in dma-buf's associated release
   function.
2) refine ioctl interface for querying plane info or create dma-buf
3) refine framebuffer decoder code
4) the patch series is based on 4.12.0-rc1

v5->v6:
1) align the dma-buf life cycle with the vfio device.
2) add the dma-buf releated operations in a separate patch.
3) i915 releated changes.

v4->v5:
1) fix bug while checking whether the gem obj is gvt's dma-buf when user
   change caching mode or domains. Add a helper function to do it.
2) add definition for the query plane and create dma-buf.

v3->v4:
1) fix bug while checking whether the gem obj is gvt's dma-buf when set
   caching mode or doamins.

v2->v3:
1) add a field gvt_plane_info in the drm_i915_gem_obj structure to save
   the decoded plane information to avoid look up while need the plane info.
2) declare a new flag I915_GEM_OBJECT_IS_GVT_DMABUF in drm_i915_gem_object
   to represent the gem obj for gvt's dma-buf. The tiling mode, caching mode
   and domains can not be changed for this kind of gem object.
3) change dma-buf related information to be more generic. So other vendor
   can use the same interface.

v1->v2:
1) create a management fd for dma-buf operations.
2) alloc gem object's backing storage in gem obj's get_pages() callback.

This patch set adds the dma-buf support for intel GVT-g.
dma-buf is a uniform mechanism to share DMA buffers across different
devices and sub-systems.
dma-buf for intel GVT-g is mainly used to share the vgpu's framebuffer
to other users or sub-systems so they can use the dma-buf to show the
desktop of a vm which uses intel vgpu.

The main idea is we create a gem object and set vgpu's framebuffer as
the backing storage of this gem object. And associate this gem obj
to a dma-buf object then export this dma-buf at the meantime
generate a file descriptor for this dma-buf. Finally deliver this file
descriptor to user space. And user can use this dma-buf fd to do render
or other operations.


Tina Zhang (7):
  drm/i915/gvt: Add framebuffer decoder support
  drm: Introduce RGB 64-bit 16:16:16:16 float format
  drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support
  drm/i915/gvt: Add opregion support
  vfio: ABI for mdev display dma-buf operation
  drm/i915: Introduce GEM proxy
  drm/i915/gvt: Dmabuf support for GVT-g

 drivers/gpu/drm/i915/gvt/Makefile      |   3 +-
 drivers/gpu/drm/i915/gvt/display.c     |   2 +-
 drivers/gpu/drm/i915/gvt/display.h     |   2 +
 drivers/gpu/drm/i915/gvt/dmabuf.c      | 380 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/dmabuf.h      |  58 +++++
 drivers/gpu/drm/i915/gvt/fb_decoder.c  | 440 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/fb_decoder.h  | 175 +++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.c         |   1 +
 drivers/gpu/drm/i915/gvt/gvt.h         |  10 +
 drivers/gpu/drm/i915/gvt/hypercall.h   |   3 +
 drivers/gpu/drm/i915/gvt/kvmgt.c       | 151 ++++++++++-
 drivers/gpu/drm/i915/gvt/mpt.h         |  45 ++++
 drivers/gpu/drm/i915/gvt/opregion.c    |  26 +-
 drivers/gpu/drm/i915/gvt/vgpu.c        |   6 +-
 drivers/gpu/drm/i915/i915_gem.c        |  24 +-
 drivers/gpu/drm/i915/i915_gem_object.h |   9 +
 drivers/gpu/drm/i915/i915_gem_tiling.c |   8 +
 include/uapi/drm/drm_fourcc.h          |   4 +
 include/uapi/linux/vfio.h              |  28 +++
 19 files changed, 1362 insertions(+), 13 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.c
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.h
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h

-- 
2.7.4

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

[PATCH v13 1/7] drm/i915/gvt: Add framebuffer decoder support

[Tina Zhang]

Framebuffer decoder returns guest framebuffer information.
Guest framebuffer includes primary, cursor and sprite plane.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/Makefile     |   3 +-
 drivers/gpu/drm/i915/gvt/display.c    |   2 +-
 drivers/gpu/drm/i915/gvt/display.h    |   2 +
 drivers/gpu/drm/i915/gvt/fb_decoder.c | 429 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/fb_decoder.h | 175 ++++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.h        |   1 +
 6 files changed, 610 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h

diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
index f5486cb9..019d596 100644
--- a/drivers/gpu/drm/i915/gvt/Makefile
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -1,7 +1,8 @@
 GVT_DIR := gvt
 GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \
 	interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
-	execlist.o scheduler.o sched_policy.o render.o cmd_parser.o
+	execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \
+	fb_decoder.o
 
 ccflags-y				+= -I$(src) -I$(src)/$(GVT_DIR)
 i915-y					+= $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
index 2deb05f..58d90cf 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -67,7 +67,7 @@ static int edp_pipe_is_enabled(struct intel_vgpu *vgpu)
 	return 1;
 }
 
-static int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
+int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
 {
 	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
 
diff --git a/drivers/gpu/drm/i915/gvt/display.h b/drivers/gpu/drm/i915/gvt/display.h
index d73de22..b46b868 100644
--- a/drivers/gpu/drm/i915/gvt/display.h
+++ b/drivers/gpu/drm/i915/gvt/display.h
@@ -179,4 +179,6 @@ int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 resolution);
 void intel_vgpu_reset_display(struct intel_vgpu *vgpu);
 void intel_vgpu_clean_display(struct intel_vgpu *vgpu);
 
+int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe);
+
 #endif
diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c
new file mode 100644
index 0000000..2bd5b3c
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
@@ -0,0 +1,429 @@
+/*
+ * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * Authors:
+ *    Kevin Tian <kevin.tian@intel.com>
+ *
+ * Contributors:
+ *    Bing Niu <bing.niu@intel.com>
+ *    Xu Han <xu.han@intel.com>
+ *    Ping Gao <ping.a.gao@intel.com>
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ *    Yang Liu <yang2.liu@intel.com>
+ *    Tina Zhang <tina.zhang@intel.com>
+ *
+ */
+
+#include <uapi/drm/drm_fourcc.h>
+#include "i915_drv.h"
+#include "gvt.h"
+
+#define PRIMARY_FORMAT_NUM	16
+struct pixel_format {
+	int	drm_format;	/* Pixel format in DRM definition */
+	int	bpp;		/* Bits per pixel, 0 indicates invalid */
+	char *desc;		/* The description */
+};
+
+/* non-supported format has bpp default to 0 */
+static struct pixel_format bdw_pixel_formats[PRIMARY_FORMAT_NUM] = {
+	[0x2] = {DRM_FORMAT_C8, 8, "8-bit Indexed"},
+	[0x5] = {DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"},
+	[0x6] = {DRM_FORMAT_XRGB8888, 32,
+		"32-bit BGRX (8:8:8:8 MSB-X:R:G:B)"},
+	[0x8] = {DRM_FORMAT_XBGR2101010, 32,
+		"32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"},
+	[0xa] = {DRM_FORMAT_XRGB2101010, 32,
+		"32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"},
+	[0xe] = {DRM_FORMAT_XBGR8888, 32,
+		"32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"},
+};
+
+/* non-supported format has bpp default to 0 */
+static struct pixel_format skl_pixel_formats[] = {
+	{DRM_FORMAT_YUYV, 16, "16-bit packed YUYV (8:8:8:8 MSB-V:Y2:U:Y1)"},
+	{DRM_FORMAT_UYVY, 16, "16-bit packed UYVY (8:8:8:8 MSB-Y2:V:Y1:U)"},
+	{DRM_FORMAT_YVYU, 16, "16-bit packed YVYU (8:8:8:8 MSB-U:Y2:V:Y1)"},
+	{DRM_FORMAT_VYUY, 16, "16-bit packed VYUY (8:8:8:8 MSB-Y2:U:Y1:V)"},
+
+	{DRM_FORMAT_C8, 8, "8-bit Indexed"},
+	{DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"},
+	{DRM_FORMAT_ABGR8888, 32, "32-bit RGBA (8:8:8:8 MSB-A:B:G:R)"},
+	{DRM_FORMAT_XBGR8888, 32, "32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"},
+
+	{DRM_FORMAT_ARGB8888, 32, "32-bit BGRA (8:8:8:8 MSB-A:R:G:B)"},
+	{DRM_FORMAT_XRGB8888, 32, "32-bit BGRX (8:8:8:8 MSB-X:R:G:B)"},
+	{DRM_FORMAT_XBGR2101010, 32, "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"},
+	{DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"},
+
+
+	/* non-supported format has bpp default to 0 */
+	{0, 0, NULL},
+};
+
+static int skl_format_to_drm(int format, bool rgb_order, bool alpha,
+	int yuv_order)
+{
+	int skl_pixel_formats_index = 14;
+
+	switch (format) {
+	case PLANE_CTL_FORMAT_INDEXED:
+		skl_pixel_formats_index = 4;
+		break;
+	case PLANE_CTL_FORMAT_RGB_565:
+		skl_pixel_formats_index = 5;
+		break;
+	case PLANE_CTL_FORMAT_XRGB_8888:
+		if (rgb_order)
+			skl_pixel_formats_index = alpha ? 6 : 7;
+		else
+			skl_pixel_formats_index = alpha ? 8 : 9;
+		break;
+	case PLANE_CTL_FORMAT_XRGB_2101010:
+		skl_pixel_formats_index = rgb_order ? 10 : 11;
+		break;
+	case PLANE_CTL_FORMAT_YUV422:
+		skl_pixel_formats_index = yuv_order >> 16;
+		if (skl_pixel_formats_index > 3)
+			return -EINVAL;
+		break;
+
+	default:
+		break;
+	}
+
+	return skl_pixel_formats_index;
+}
+
+static u32 intel_vgpu_get_stride(struct intel_vgpu *vgpu, int pipe,
+	u32 tiled, int stride_mask, int bpp)
+{
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+
+	u32 stride_reg = vgpu_vreg(vgpu, DSPSTRIDE(pipe)) & stride_mask;
+	u32 stride = stride_reg;
+
+	if (IS_SKYLAKE(dev_priv)) {
+		switch (tiled) {
+		case PLANE_CTL_TILED_LINEAR:
+			stride = stride_reg * 64;
+			break;
+		case PLANE_CTL_TILED_X:
+			stride = stride_reg * 512;
+			break;
+		case PLANE_CTL_TILED_Y:
+			stride = stride_reg * 128;
+			break;
+		case PLANE_CTL_TILED_YF:
+			if (bpp == 8)
+				stride = stride_reg * 64;
+			else if (bpp == 16 || bpp == 32 || bpp == 64)
+				stride = stride_reg * 128;
+			else
+				gvt_dbg_core("skl: unsupported bpp:%d\n", bpp);
+			break;
+		default:
+			gvt_dbg_core("skl: unsupported tile format:%x\n",
+				tiled);
+		}
+	}
+
+	return stride;
+}
+
+static int get_active_pipe(struct intel_vgpu *vgpu)
+{
+	int i;
+
+	for (i = 0; i < I915_MAX_PIPES; i++)
+		if (pipe_is_enabled(vgpu, i))
+			break;
+
+	return i;
+}
+
+/**
+ * intel_vgpu_decode_primary_plane - Decode primary plane
+ * @vgpu: input vgpu
+ * @plane: primary plane to save decoded info
+ * This function is called for decoding plane
+ *
+ * Returns:
+ * 0 on success, non-zero if failed.
+ */
+int intel_vgpu_decode_primary_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_primary_plane_format *plane)
+{
+	u32 val, fmt;
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+	int pipe;
+
+	pipe = get_active_pipe(vgpu);
+	if (pipe >= I915_MAX_PIPES)
+		return -ENODEV;
+
+	val = vgpu_vreg(vgpu, DSPCNTR(pipe));
+	plane->enabled = !!(val & DISPLAY_PLANE_ENABLE);
+	if (!plane->enabled)
+		return -ENODEV;
+
+	if (IS_SKYLAKE(dev_priv)) {
+		plane->tiled = (val & PLANE_CTL_TILED_MASK) >>
+		_PLANE_CTL_TILED_SHIFT;
+		fmt = skl_format_to_drm(
+			val & PLANE_CTL_FORMAT_MASK,
+			val & PLANE_CTL_ORDER_RGBX,
+			val & PLANE_CTL_ALPHA_MASK,
+			val & PLANE_CTL_YUV422_ORDER_MASK);
+		plane->bpp = skl_pixel_formats[fmt].bpp;
+		plane->drm_format = skl_pixel_formats[fmt].drm_format;
+	} else {
+		plane->tiled = !!(val & DISPPLANE_TILED);
+		fmt = (val & DISPPLANE_PIXFORMAT_MASK) >> _PRI_PLANE_FMT_SHIFT;
+		plane->bpp = bdw_pixel_formats[fmt].bpp;
+		plane->drm_format = bdw_pixel_formats[fmt].drm_format;
+	}
+
+	if (!plane->bpp) {
+		gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
+		return -EINVAL;
+	}
+
+	plane->hw_format = fmt;
+
+	plane->base = vgpu_vreg(vgpu, DSPSURF(pipe)) & GTT_PAGE_MASK;
+	plane->base_gpa = intel_vgpu_gma_to_gpa(vgpu->gtt.ggtt_mm, plane->base);
+	if (plane->base_gpa == INTEL_GVT_INVALID_ADDR) {
+		gvt_vgpu_err("invalid gma address: %lx\n",
+				(unsigned long)plane->base);
+	}
+
+	plane->stride = intel_vgpu_get_stride(vgpu, pipe, (plane->tiled << 10),
+		(IS_SKYLAKE(dev_priv)) ? (_PRI_PLANE_STRIDE_MASK >> 6) :
+		_PRI_PLANE_STRIDE_MASK, plane->bpp);
+
+	plane->width = (vgpu_vreg(vgpu, PIPESRC(pipe)) & _PIPE_H_SRCSZ_MASK) >>
+		_PIPE_H_SRCSZ_SHIFT;
+	plane->width += 1;
+	plane->height = (vgpu_vreg(vgpu, PIPESRC(pipe)) &
+			_PIPE_V_SRCSZ_MASK) >> _PIPE_V_SRCSZ_SHIFT;
+	plane->height += 1;	/* raw height is one minus the real value */
+
+	val = vgpu_vreg(vgpu, DSPTILEOFF(pipe));
+	plane->x_offset = (val & _PRI_PLANE_X_OFF_MASK) >>
+		_PRI_PLANE_X_OFF_SHIFT;
+	plane->y_offset = (val & _PRI_PLANE_Y_OFF_MASK) >>
+		_PRI_PLANE_Y_OFF_SHIFT;
+
+	return 0;
+}
+
+#define CURSOR_FORMAT_NUM	(1 << 6)
+struct cursor_mode_format {
+	int	drm_format;	/* Pixel format in DRM definition */
+	u8	bpp;		/* Bits per pixel; 0 indicates invalid */
+	u32	width;		/* In pixel */
+	u32	height;		/* In lines */
+	char	*desc;		/* The description */
+};
+
+/* non-supported format has bpp default to 0 */
+static struct cursor_mode_format cursor_pixel_formats[CURSOR_FORMAT_NUM] = {
+	[0x22]  = {DRM_FORMAT_ARGB8888, 32, 128, 128, "128x128 32bpp ARGB"},
+	[0x23]  = {DRM_FORMAT_ARGB8888, 32, 256, 256, "256x256 32bpp ARGB"},
+	[0x27]  = {DRM_FORMAT_ARGB8888, 32, 64, 64, "64x64 32bpp ARGB"},
+	[0x7]  = {DRM_FORMAT_ARGB8888, 32, 64, 64, "64x64 32bpp ARGB"},
+};
+
+/**
+ * intel_vgpu_decode_cursor_plane - Decode sprite plane
+ * @vgpu: input vgpu
+ * @plane: cursor plane to save decoded info
+ * This function is called for decoding plane
+ *
+ * Returns:
+ * 0 on success, non-zero if failed.
+ */
+int intel_vgpu_decode_cursor_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_cursor_plane_format *plane)
+{
+	u32 val, mode;
+	u32 alpha_plane, alpha_force;
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+	int pipe;
+
+	pipe = get_active_pipe(vgpu);
+	if (pipe >= I915_MAX_PIPES)
+		return -ENODEV;
+
+	val = vgpu_vreg(vgpu, CURCNTR(pipe));
+	mode = val & CURSOR_MODE;
+	plane->enabled = (mode != CURSOR_MODE_DISABLE);
+	if (!plane->enabled)
+		return -ENODEV;
+
+	if (!cursor_pixel_formats[mode].bpp) {
+		gvt_vgpu_err("Non-supported cursor mode (0x%x)\n", mode);
+		return -EINVAL;
+	}
+	plane->mode = mode;
+	plane->bpp = cursor_pixel_formats[mode].bpp;
+	plane->drm_format = cursor_pixel_formats[mode].drm_format;
+	plane->width = cursor_pixel_formats[mode].width;
+	plane->height = cursor_pixel_formats[mode].height;
+
+	alpha_plane = (val & _CURSOR_ALPHA_PLANE_MASK) >>
+				_CURSOR_ALPHA_PLANE_SHIFT;
+	alpha_force = (val & _CURSOR_ALPHA_FORCE_MASK) >>
+				_CURSOR_ALPHA_FORCE_SHIFT;
+	if (alpha_plane || alpha_force)
+		gvt_dbg_core("alpha_plane=0x%x, alpha_force=0x%x\n",
+			alpha_plane, alpha_force);
+
+	plane->base = vgpu_vreg(vgpu, CURBASE(pipe)) & GTT_PAGE_MASK;
+	plane->base_gpa = intel_vgpu_gma_to_gpa(vgpu->gtt.ggtt_mm, plane->base);
+	if (plane->base_gpa == INTEL_GVT_INVALID_ADDR) {
+		gvt_vgpu_err("invalid gma address: %lx\n",
+				(unsigned long)plane->base);
+	}
+
+	val = vgpu_vreg(vgpu, CURPOS(pipe));
+	plane->x_pos = (val & _CURSOR_POS_X_MASK) >> _CURSOR_POS_X_SHIFT;
+	plane->x_sign = (val & _CURSOR_SIGN_X_MASK) >> _CURSOR_SIGN_X_SHIFT;
+	plane->y_pos = (val & _CURSOR_POS_Y_MASK) >> _CURSOR_POS_Y_SHIFT;
+	plane->y_sign = (val & _CURSOR_SIGN_Y_MASK) >> _CURSOR_SIGN_Y_SHIFT;
+
+	return 0;
+}
+
+#define SPRITE_FORMAT_NUM	(1 << 3)
+
+static struct pixel_format sprite_pixel_formats[SPRITE_FORMAT_NUM] = {
+	[0x0]  = {DRM_FORMAT_YUV422, 16, "YUV 16-bit 4:2:2 packed"},
+	[0x1]  = {DRM_FORMAT_XRGB2101010, 32, "RGB 32-bit 2:10:10:10"},
+	[0x2]  = {DRM_FORMAT_XRGB8888, 32, "RGB 32-bit 8:8:8:8"},
+	[0x4] = {DRM_FORMAT_AYUV, 32,
+		"YUV 32-bit 4:4:4 packed (8:8:8:8 MSB-X:Y:U:V)"},
+};
+
+/**
+ * intel_vgpu_decode_sprite_plane - Decode sprite plane
+ * @vgpu: input vgpu
+ * @plane: sprite plane to save decoded info
+ * This function is called for decoding plane
+ *
+ * Returns:
+ * 0 on success, non-zero if failed.
+ */
+int intel_vgpu_decode_sprite_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_sprite_plane_format *plane)
+{
+	u32 val, fmt;
+	u32 color_order, yuv_order;
+	int drm_format;
+	int pipe;
+
+	pipe = get_active_pipe(vgpu);
+	if (pipe >= I915_MAX_PIPES)
+		return -ENODEV;
+
+	val = vgpu_vreg(vgpu, SPRCTL(pipe));
+	plane->enabled = !!(val & SPRITE_ENABLE);
+	if (!plane->enabled)
+		return -ENODEV;
+
+	plane->tiled = !!(val & SPRITE_TILED);
+	color_order = !!(val & SPRITE_RGB_ORDER_RGBX);
+	yuv_order = (val & SPRITE_YUV_BYTE_ORDER_MASK) >>
+				_SPRITE_YUV_ORDER_SHIFT;
+
+	fmt = (val & SPRITE_PIXFORMAT_MASK) >> _SPRITE_FMT_SHIFT;
+	if (!sprite_pixel_formats[fmt].bpp) {
+		gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
+		return -EINVAL;
+	}
+	plane->hw_format = fmt;
+	plane->bpp = sprite_pixel_formats[fmt].bpp;
+	drm_format = sprite_pixel_formats[fmt].drm_format;
+
+	/* Order of RGB values in an RGBxxx buffer may be ordered RGB or
+	 * BGR depending on the state of the color_order field
+	 */
+	if (!color_order) {
+		if (drm_format == DRM_FORMAT_XRGB2101010)
+			drm_format = DRM_FORMAT_XBGR2101010;
+		else if (drm_format == DRM_FORMAT_XRGB8888)
+			drm_format = DRM_FORMAT_XBGR8888;
+	}
+
+	if (drm_format == DRM_FORMAT_YUV422) {
+		switch (yuv_order) {
+		case 0:
+			drm_format = DRM_FORMAT_YUYV;
+			break;
+		case 1:
+			drm_format = DRM_FORMAT_UYVY;
+			break;
+		case 2:
+			drm_format = DRM_FORMAT_YVYU;
+			break;
+		case 3:
+			drm_format = DRM_FORMAT_VYUY;
+			break;
+		default:
+			/* yuv_order has only 2 bits */
+			break;
+		}
+	}
+
+	plane->drm_format = drm_format;
+
+	plane->base = vgpu_vreg(vgpu, SPRSURF(pipe)) & GTT_PAGE_MASK;
+	plane->base_gpa = intel_vgpu_gma_to_gpa(vgpu->gtt.ggtt_mm, plane->base);
+	if (plane->base_gpa == INTEL_GVT_INVALID_ADDR) {
+		gvt_vgpu_err("invalid gma address: %lx\n",
+				(unsigned long)plane->base);
+	}
+
+	plane->stride = vgpu_vreg(vgpu, SPRSTRIDE(pipe)) &
+				_SPRITE_STRIDE_MASK;
+
+	val = vgpu_vreg(vgpu, SPRSIZE(pipe));
+	plane->height = (val & _SPRITE_SIZE_HEIGHT_MASK) >>
+		_SPRITE_SIZE_HEIGHT_SHIFT;
+	plane->width = (val & _SPRITE_SIZE_WIDTH_MASK) >>
+		_SPRITE_SIZE_WIDTH_SHIFT;
+	plane->height += 1;	/* raw height is one minus the real value */
+	plane->width += 1;	/* raw width is one minus the real value */
+
+	val = vgpu_vreg(vgpu, SPRPOS(pipe));
+	plane->x_pos = (val & _SPRITE_POS_X_MASK) >> _SPRITE_POS_X_SHIFT;
+	plane->y_pos = (val & _SPRITE_POS_Y_MASK) >> _SPRITE_POS_Y_SHIFT;
+
+	val = vgpu_vreg(vgpu, SPROFFSET(pipe));
+	plane->x_offset = (val & _SPRITE_OFFSET_START_X_MASK) >>
+			   _SPRITE_OFFSET_START_X_SHIFT;
+	plane->y_offset = (val & _SPRITE_OFFSET_START_Y_MASK) >>
+			   _SPRITE_OFFSET_START_Y_SHIFT;
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.h b/drivers/gpu/drm/i915/gvt/fb_decoder.h
new file mode 100644
index 0000000..51a7e97
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/fb_decoder.h
@@ -0,0 +1,175 @@
+/*
+ * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * Authors:
+ *    Kevin Tian <kevin.tian@intel.com>
+ *
+ * Contributors:
+ *    Bing Niu <bing.niu@intel.com>
+ *    Xu Han <xu.han@intel.com>
+ *    Ping Gao <ping.a.gao@intel.com>
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ *    Yang Liu <yang2.liu@intel.com>
+ *    Tina Zhang <tina.zhang@intel.com>
+ *
+ */
+
+#ifndef _GVT_FB_DECODER_H_
+#define _GVT_FB_DECODER_H_
+
+#define _PLANE_CTL_FORMAT_SHIFT		24
+#define _PLANE_CTL_TILED_SHIFT		10
+#define _PIPE_V_SRCSZ_SHIFT		0
+#define _PIPE_V_SRCSZ_MASK		(0xfff << _PIPE_V_SRCSZ_SHIFT)
+#define _PIPE_H_SRCSZ_SHIFT		16
+#define _PIPE_H_SRCSZ_MASK		(0x1fff << _PIPE_H_SRCSZ_SHIFT)
+
+#define _PRI_PLANE_FMT_SHIFT		26
+#define _PRI_PLANE_STRIDE_MASK		(0x3ff << 6)
+#define _PRI_PLANE_X_OFF_SHIFT		0
+#define _PRI_PLANE_X_OFF_MASK		(0x1fff << _PRI_PLANE_X_OFF_SHIFT)
+#define _PRI_PLANE_Y_OFF_SHIFT		16
+#define _PRI_PLANE_Y_OFF_MASK		(0xfff << _PRI_PLANE_Y_OFF_SHIFT)
+
+#define _CURSOR_MODE			0x3f
+#define _CURSOR_ALPHA_FORCE_SHIFT	8
+#define _CURSOR_ALPHA_FORCE_MASK	(0x3 << _CURSOR_ALPHA_FORCE_SHIFT)
+#define _CURSOR_ALPHA_PLANE_SHIFT	10
+#define _CURSOR_ALPHA_PLANE_MASK	(0x3 << _CURSOR_ALPHA_PLANE_SHIFT)
+#define _CURSOR_POS_X_SHIFT		0
+#define _CURSOR_POS_X_MASK		(0x1fff << _CURSOR_POS_X_SHIFT)
+#define _CURSOR_SIGN_X_SHIFT		15
+#define _CURSOR_SIGN_X_MASK		(1 << _CURSOR_SIGN_X_SHIFT)
+#define _CURSOR_POS_Y_SHIFT		16
+#define _CURSOR_POS_Y_MASK		(0xfff << _CURSOR_POS_Y_SHIFT)
+#define _CURSOR_SIGN_Y_SHIFT		31
+#define _CURSOR_SIGN_Y_MASK		(1 << _CURSOR_SIGN_Y_SHIFT)
+
+#define _SPRITE_FMT_SHIFT		25
+#define _SPRITE_COLOR_ORDER_SHIFT	20
+#define _SPRITE_YUV_ORDER_SHIFT		16
+#define _SPRITE_STRIDE_SHIFT		6
+#define _SPRITE_STRIDE_MASK		(0x1ff << _SPRITE_STRIDE_SHIFT)
+#define _SPRITE_SIZE_WIDTH_SHIFT	0
+#define _SPRITE_SIZE_HEIGHT_SHIFT	16
+#define _SPRITE_SIZE_WIDTH_MASK		(0x1fff << _SPRITE_SIZE_WIDTH_SHIFT)
+#define _SPRITE_SIZE_HEIGHT_MASK	(0xfff << _SPRITE_SIZE_HEIGHT_SHIFT)
+#define _SPRITE_POS_X_SHIFT		0
+#define _SPRITE_POS_Y_SHIFT		16
+#define _SPRITE_POS_X_MASK		(0x1fff << _SPRITE_POS_X_SHIFT)
+#define _SPRITE_POS_Y_MASK		(0xfff << _SPRITE_POS_Y_SHIFT)
+#define _SPRITE_OFFSET_START_X_SHIFT	0
+#define _SPRITE_OFFSET_START_Y_SHIFT	16
+#define _SPRITE_OFFSET_START_X_MASK	(0x1fff << _SPRITE_OFFSET_START_X_SHIFT)
+#define _SPRITE_OFFSET_START_Y_MASK	(0xfff << _SPRITE_OFFSET_START_Y_SHIFT)
+
+enum GVT_FB_EVENT {
+	FB_MODE_SET_START = 1,
+	FB_MODE_SET_END,
+	FB_DISPLAY_FLIP,
+};
+
+enum DDI_PORT {
+	DDI_PORT_NONE	= 0,
+	DDI_PORT_B	= 1,
+	DDI_PORT_C	= 2,
+	DDI_PORT_D	= 3,
+	DDI_PORT_E	= 4
+};
+
+struct intel_gvt;
+
+struct gvt_fb_notify_msg {
+	unsigned int vm_id;
+	unsigned int pipe_id; /* id starting from 0 */
+	unsigned int plane_id; /* primary, cursor, or sprite */
+};
+
+/* color space conversion and gamma correction are not included */
+struct intel_vgpu_primary_plane_format {
+	u8	enabled;	/* plane is enabled */
+	u8	tiled;		/* X-tiled */
+	u8	bpp;		/* bits per pixel */
+	u32	hw_format;	/* format field in the PRI_CTL register */
+	u32	drm_format;	/* format in DRM definition */
+	u32	base;		/* framebuffer base in graphics memory */
+	u64     base_gpa;
+	u32	x_offset;	/* in pixels */
+	u32	y_offset;	/* in lines */
+	u32	width;		/* in pixels */
+	u32	height;		/* in lines */
+	u32	stride;		/* in bytes */
+};
+
+struct intel_vgpu_sprite_plane_format {
+	u8	enabled;	/* plane is enabled */
+	u8	tiled;		/* X-tiled */
+	u8	bpp;		/* bits per pixel */
+	u32	hw_format;	/* format field in the SPR_CTL register */
+	u32	drm_format;	/* format in DRM definition */
+	u32	base;		/* sprite base in graphics memory */
+	u64     base_gpa;
+	u32	x_pos;		/* in pixels */
+	u32	y_pos;		/* in lines */
+	u32	x_offset;	/* in pixels */
+	u32	y_offset;	/* in lines */
+	u32	width;		/* in pixels */
+	u32	height;		/* in lines */
+	u32	stride;		/* in bytes */
+};
+
+struct intel_vgpu_cursor_plane_format {
+	u8	enabled;
+	u8	mode;		/* cursor mode select */
+	u8	bpp;		/* bits per pixel */
+	u32	drm_format;	/* format in DRM definition */
+	u32	base;		/* cursor base in graphics memory */
+	u64     base_gpa;
+	u32	x_pos;		/* in pixels */
+	u32	y_pos;		/* in lines */
+	u8	x_sign;		/* X Position Sign */
+	u8	y_sign;		/* Y Position Sign */
+	u32	width;		/* in pixels */
+	u32	height;		/* in lines */
+	u32	x_hot;		/* in pixels */
+	u32	y_hot;		/* in pixels */
+};
+
+struct intel_vgpu_pipe_format {
+	struct intel_vgpu_primary_plane_format	primary;
+	struct intel_vgpu_sprite_plane_format	sprite;
+	struct intel_vgpu_cursor_plane_format	cursor;
+	enum DDI_PORT ddi_port;  /* the DDI port that pipe is connected to */
+};
+
+struct intel_vgpu_fb_format {
+	struct intel_vgpu_pipe_format	pipes[I915_MAX_PIPES];
+};
+
+int intel_vgpu_decode_primary_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_primary_plane_format *plane);
+int intel_vgpu_decode_cursor_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_cursor_plane_format *plane);
+int intel_vgpu_decode_sprite_plane(struct intel_vgpu *vgpu,
+	struct intel_vgpu_sprite_plane_format *plane);
+
+#endif
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index ba53ad1..d0fe4d0 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -46,6 +46,7 @@
 #include "sched_policy.h"
 #include "render.h"
 #include "cmd_parser.h"
+#include "fb_decoder.h"
 
 #define GVT_MAX_VGPU 8
 
-- 
2.7.4

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

[PATCH v13 2/7] drm: Introduce RGB 64-bit 16:16:16:16 float format

[Tina Zhang]

The RGB 64-bit 16:16:16:16 float pixel format is needed by windows
guest VM. This patch is to introduce the format to drm.

v1:
Suggested by Ville to submit this patch to dri-devel.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 include/uapi/drm/drm_fourcc.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 7586c46..3e002e3 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -113,6 +113,10 @@ extern "C" {
 
 #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */
 
+/* 64 bpp RGB */
+#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /* [63:0] x:R:G:B 16:16:16:16 little endian */
+#define DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0] x:B:G:R 16:16:16:16 little endian */
+
 /*
  * 2 plane RGB + A
  * index 0 = RGB plane, same format as the corresponding non _A8 format has
-- 
2.7.4

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

[PATCH v13 3/7] drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support

[Tina Zhang]

The RGB 64-bit 16:16:16:16 float pixel format is needed by windows 10
guest VM. This patch is to add this pixel format support to gvt device
model. Without this patch, some Apps, e.g. "DXGIGammaVM.exe", will crash
and make guest screen black.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/fb_decoder.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c
index 2bd5b3c..739ca81 100644
--- a/drivers/gpu/drm/i915/gvt/fb_decoder.c
+++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
@@ -54,6 +54,8 @@ static struct pixel_format bdw_pixel_formats[PRIMARY_FORMAT_NUM] = {
 		"32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"},
 	[0xa] = {DRM_FORMAT_XRGB2101010, 32,
 		"32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"},
+	[0xc] = {DRM_FORMAT_XRGB161616, 64,
+		"64-bit RGBX Floating Point(16:16:16:16 MSB-X:B:G:R)"},
 	[0xe] = {DRM_FORMAT_XBGR8888, 32,
 		"32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"},
 };
@@ -75,6 +77,10 @@ static struct pixel_format skl_pixel_formats[] = {
 	{DRM_FORMAT_XBGR2101010, 32, "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"},
 	{DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"},
 
+	{DRM_FORMAT_XRGB161616, 64,
+		"64-bit XRGB (16:16:16:16 MSB-X:R:G:B)"},
+	{DRM_FORMAT_XBGR161616, 64,
+		"64-bit XBGR (16:16:16:16 MSB-X:B:G:R)"},
 
 	/* non-supported format has bpp default to 0 */
 	{0, 0, NULL},
@@ -101,6 +107,9 @@ static int skl_format_to_drm(int format, bool rgb_order, bool alpha,
 	case PLANE_CTL_FORMAT_XRGB_2101010:
 		skl_pixel_formats_index = rgb_order ? 10 : 11;
 		break;
+	case PLANE_CTL_FORMAT_XRGB_16161616F:
+		skl_pixel_formats_index = rgb_order ? 12 : 13;
+		break;
 	case PLANE_CTL_FORMAT_YUV422:
 		skl_pixel_formats_index = yuv_order >> 16;
 		if (skl_pixel_formats_index > 3)
@@ -321,6 +330,8 @@ static struct pixel_format sprite_pixel_formats[SPRITE_FORMAT_NUM] = {
 	[0x0]  = {DRM_FORMAT_YUV422, 16, "YUV 16-bit 4:2:2 packed"},
 	[0x1]  = {DRM_FORMAT_XRGB2101010, 32, "RGB 32-bit 2:10:10:10"},
 	[0x2]  = {DRM_FORMAT_XRGB8888, 32, "RGB 32-bit 8:8:8:8"},
+	[0x3]  = {DRM_FORMAT_XRGB161616, 64,
+		    "RGB 64-bit 16:16:16:16 Floating Point"},
 	[0x4] = {DRM_FORMAT_AYUV, 32,
 		"YUV 32-bit 4:4:4 packed (8:8:8:8 MSB-X:Y:U:V)"},
 };
-- 
2.7.4

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

[PATCH v13 4/7] drm/i915/gvt: Add opregion support

[Tina Zhang]

Windows guest UPT driver can use operegion to configure the setting
for display. Without the opregion support, the display registers won't
be set and this blocks display model to get the correct information
of the guest display plane.

Signed-off-by: Bing Niu <bing.niu@intel.com>
Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/hypercall.h |   1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 109 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/gvt/mpt.h       |  15 +++++
 drivers/gpu/drm/i915/gvt/opregion.c  |  26 +++++++--
 drivers/gpu/drm/i915/gvt/vgpu.c      |   4 ++
 5 files changed, 146 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
index df7f33a..32c345c 100644
--- a/drivers/gpu/drm/i915/gvt/hypercall.h
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -55,6 +55,7 @@ struct intel_gvt_mpt {
 			      unsigned long mfn, unsigned int nr, bool map);
 	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
 			     bool map);
+	int (*set_opregion)(void *vgpu);
 };
 
 extern struct intel_gvt_mpt xengt_mpt;
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index fd0c85f..6b0a330 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -53,11 +53,23 @@ static const struct intel_gvt_ops *intel_gvt_ops;
 #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << VFIO_PCI_OFFSET_SHIFT)
 #define VFIO_PCI_OFFSET_MASK    (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)
 
+#define OPREGION_SIGNATURE "IntelGraphicsMem"
+
+struct vfio_region;
+struct intel_vgpu_regops {
+	size_t (*rw)(struct intel_vgpu *vgpu, char *buf,
+			size_t count, loff_t *ppos, bool iswrite);
+	void (*release)(struct intel_vgpu *vgpu,
+			struct vfio_region *region);
+};
+
 struct vfio_region {
 	u32				type;
 	u32				subtype;
 	size_t				size;
 	u32				flags;
+	const struct intel_vgpu_regops	*ops;
+	void				*data;
 };
 
 struct kvmgt_pgfn {
@@ -430,6 +442,91 @@ static void kvmgt_protect_table_del(struct kvmgt_guest_info *info,
 	}
 }
 
+static size_t intel_vgpu_reg_rw_opregion(struct intel_vgpu *vgpu, char *buf,
+		size_t count, loff_t *ppos, bool iswrite)
+{
+	unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
+			VFIO_PCI_NUM_REGIONS;
+	void *base = vgpu->vdev.region[i].data;
+	loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
+
+	if (pos >= vgpu->vdev.region[i].size || iswrite) {
+		gvt_vgpu_err("invalid op or offset for Intel vgpu OpRegion\n");
+		return -EINVAL;
+	}
+	count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
+	memcpy(buf, base + pos, count);
+
+	return count;
+}
+
+static void intel_vgpu_reg_release_opregion(struct intel_vgpu *vgpu,
+		struct vfio_region *region)
+{
+	memunmap(region->data);
+}
+
+static const struct intel_vgpu_regops intel_vgpu_regops_opregion = {
+	.rw = intel_vgpu_reg_rw_opregion,
+	.release = intel_vgpu_reg_release_opregion,
+};
+
+static int intel_vgpu_register_reg(struct intel_vgpu *vgpu,
+		unsigned int type, unsigned int subtype,
+		const struct intel_vgpu_regops *ops,
+		size_t size, u32 flags, void *data)
+{
+	struct vfio_region *region;
+
+	region = krealloc(vgpu->vdev.region,
+			(vgpu->vdev.num_regions + 1) * sizeof(*region),
+			GFP_KERNEL);
+	if (!region)
+		return -ENOMEM;
+
+	vgpu->vdev.region = region;
+	vgpu->vdev.region[vgpu->vdev.num_regions].type = type;
+	vgpu->vdev.region[vgpu->vdev.num_regions].subtype = subtype;
+	vgpu->vdev.region[vgpu->vdev.num_regions].ops = ops;
+	vgpu->vdev.region[vgpu->vdev.num_regions].size = size;
+	vgpu->vdev.region[vgpu->vdev.num_regions].flags = flags;
+	vgpu->vdev.region[vgpu->vdev.num_regions].data = data;
+	vgpu->vdev.num_regions++;
+
+	return 0;
+}
+
+static int kvmgt_set_opregion(void *p_vgpu)
+{
+	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
+	unsigned int addr;
+	void *base;
+	int ret;
+
+	addr = vgpu->gvt->opregion.opregion_pa;
+	if (!addr || !(~addr))
+		return -ENODEV;
+
+	base = memremap(addr, OPREGION_SIZE, MEMREMAP_WB);
+	if (!base)
+		return -ENOMEM;
+
+	if (memcmp(base, OPREGION_SIGNATURE, 16)) {
+		memunmap(base);
+		return -EINVAL;
+	}
+
+	ret = intel_vgpu_register_reg(vgpu,
+			PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
+			VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION,
+			&intel_vgpu_regops_opregion, OPREGION_SIZE,
+			VFIO_REGION_INFO_FLAG_READ, base);
+	if (ret)
+		memunmap(base);
+
+	return ret;
+}
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct intel_vgpu *vgpu = NULL;
@@ -646,7 +743,7 @@ static ssize_t intel_vgpu_rw(struct mdev_device *mdev, char *buf,
 	int ret = -EINVAL;
 
 
-	if (index >= VFIO_PCI_NUM_REGIONS) {
+	if (index >= VFIO_PCI_NUM_REGIONS + vgpu->vdev.num_regions) {
 		gvt_vgpu_err("invalid index: %u\n", index);
 		return -EINVAL;
 	}
@@ -680,8 +777,11 @@ static ssize_t intel_vgpu_rw(struct mdev_device *mdev, char *buf,
 	case VFIO_PCI_BAR5_REGION_INDEX:
 	case VFIO_PCI_VGA_REGION_INDEX:
 	case VFIO_PCI_ROM_REGION_INDEX:
+		break;
 	default:
-		gvt_vgpu_err("unsupported region: %u\n", index);
+		index -= VFIO_PCI_NUM_REGIONS;
+		return vgpu->vdev.region[index].ops->rw(vgpu, buf, count,
+				ppos, is_write);
 	}
 
 	return ret == 0 ? count : ret;
@@ -944,7 +1044,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 
 		info.flags = VFIO_DEVICE_FLAGS_PCI;
 		info.flags |= VFIO_DEVICE_FLAGS_RESET;
-		info.num_regions = VFIO_PCI_NUM_REGIONS;
+		info.num_regions = VFIO_PCI_NUM_REGIONS +
+				vgpu->vdev.num_regions;
 		info.num_irqs = VFIO_PCI_NUM_IRQS;
 
 		return copy_to_user((void __user *)arg, &info, minsz) ?
@@ -1065,6 +1166,7 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 		}
 
 		if (caps.size) {
+			info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
 			if (info.argsz < sizeof(info) + caps.size) {
 				info.argsz = sizeof(info) + caps.size;
 				info.cap_offset = 0;
@@ -1513,6 +1615,7 @@ struct intel_gvt_mpt kvmgt_mpt = {
 	.read_gpa = kvmgt_read_gpa,
 	.write_gpa = kvmgt_write_gpa,
 	.gfn_to_mfn = kvmgt_gfn_to_pfn,
+	.set_opregion = kvmgt_set_opregion,
 };
 EXPORT_SYMBOL_GPL(kvmgt_mpt);
 
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
index f0e5487..9e73f2e 100644
--- a/drivers/gpu/drm/i915/gvt/mpt.h
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -292,4 +292,19 @@ static inline int intel_gvt_hypervisor_set_trap_area(
 	return intel_gvt_host.mpt->set_trap_area(vgpu->handle, start, end, map);
 }
 
+/**
+ * intel_gvt_hypervisor_set_opregion - Set opregion for guest
+ * @vgpu: a vGPU
+ *
+ * Returns:
+ * Zero on success, negative error code if failed.
+ */
+static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->set_opregion)
+		return 0;
+
+	return intel_gvt_host.mpt->set_opregion(vgpu);
+}
+
 #endif /* _GVT_MPT_H_ */
diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index 3117991..04c0452 100644
--- a/drivers/gpu/drm/i915/gvt/opregion.c
+++ b/drivers/gpu/drm/i915/gvt/opregion.c
@@ -113,23 +113,37 @@ void intel_vgpu_clean_opregion(struct intel_vgpu *vgpu)
  */
 int intel_vgpu_init_opregion(struct intel_vgpu *vgpu, u32 gpa)
 {
-	int ret;
+	int ret = 0;
+	unsigned long pfn;
 
 	gvt_dbg_core("vgpu%d: init vgpu opregion\n", vgpu->id);
 
-	if (intel_gvt_host.hypervisor_type == INTEL_GVT_HYPERVISOR_XEN) {
+	switch (intel_gvt_host.hypervisor_type) {
+	case INTEL_GVT_HYPERVISOR_KVM:
+		pfn = intel_gvt_hypervisor_gfn_to_mfn(vgpu, gpa >> PAGE_SHIFT);
+		vgpu_opregion(vgpu)->va = memremap(pfn << PAGE_SHIFT,
+						INTEL_GVT_OPREGION_SIZE,
+						MEMREMAP_WB);
+		if (!vgpu_opregion(vgpu)->va) {
+			gvt_vgpu_err("failed to map guest opregion\n");
+			ret = -EFAULT;
+		}
+		break;
+	case INTEL_GVT_HYPERVISOR_XEN:
 		gvt_dbg_core("emulate opregion from kernel\n");
 
 		ret = init_vgpu_opregion(vgpu, gpa);
 		if (ret)
-			return ret;
+			break;
 
 		ret = map_vgpu_opregion(vgpu, true);
-		if (ret)
-			return ret;
+		break;
+	default:
+		ret = -EINVAL;
+		gvt_vgpu_err("not supported hypervisor\n");
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index 90c14e6..03f9539 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -383,6 +383,10 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
 	if (ret)
 		goto out_clean_shadow_ctx;
 
+	ret = intel_gvt_hypervisor_set_opregion(vgpu);
+	if (ret)
+		goto out_clean_shadow_ctx;
+
 	mutex_unlock(&gvt->lock);
 
 	return vgpu;
-- 
2.7.4

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

[PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Tina Zhang]

Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query and
get the plan and its related information.

The dma-buf's life cycle is handled by user mode and tracked by kernel.
The returned fd in struct vfio_device_query_gfx_plane can be a new
fd or an old fd of a re-exported dma-buf. Host User mode can check the
value of fd and to see if it needs to create new resource according to
the new fd or just use the existed resource related to the old fd.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index ae46105..827a230 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
 
 #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
 
+/**
+ * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14, struct vfio_device_query_gfx_plane)
+ *
+ * Set the drm_plane_type and retrieve information about the gfx plane.
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+struct vfio_device_gfx_plane_info {
+	__u32 argsz;
+	__u32 flags;
+	/* in */
+	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
+	/* out */
+	__u32 drm_format;	/* drm format of plane */
+	__u64 drm_format_mod;   /* tiled mode */
+	__u32 width;	/* width of plane */
+	__u32 height;	/* height of plane */
+	__u32 stride;	/* stride of plane */
+	__u32 size;	/* size of plane in bytes, align on page*/
+	__u32 x_pos;	/* horizontal position of cursor plane, upper left corner in pixels */
+	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/
+	__u32 region_index;
+	__s32 fd;	/* dma-buf fd */
+};
+
+#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
+
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
-- 
2.7.4

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

[PATCH v13 6/7] drm/i915: Introduce GEM proxy

[Tina Zhang]

GEM proxy is a kind of GEM, whose backing physical memory is pinned
and produced by guest VM and is used by host as read only. With GEM
proxy, host is able to access guest physical memory through GEM object
interface. As GEM proxy is such a special kind of GEM, a new flag
I915_GEM_OBJECT_IS_PROXY is introduced to ban host from changing the
backing storage of GEM proxy.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c        | 24 +++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_object.h |  7 +++++++
 drivers/gpu/drm/i915/i915_gem_tiling.c |  8 ++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1b2dfa8..75530fb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1624,6 +1624,16 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 	if (err)
 		goto out;
 
+	/* Proxy objects do not control access to the backing storage, ergo
+	 * they cannot be used as a means to manipulate the cache domain
+	 * tracking for that backing storage. The proxy object is always
+	 * considered to be outside of any cache domain.
+	 */
+	if (i915_gem_object_is_proxy(obj)) {
+		err = -EPERM;
+		goto out;
+	}
+
 	/* Flush and acquire obj->pages so that we are coherent through
 	 * direct access in memory with previous cached writes through
 	 * shmemfs and that our cache domain tracking remains valid.
@@ -1680,6 +1690,10 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOENT;
 
+	/* Proxy objects are barred from CPU access, so there is no
+	 * need to ban sw_finish as it is a nop.
+	 */
+
 	/* Pinned buffers may be scanout, so flush the cache */
 	i915_gem_object_flush_if_display(obj);
 	i915_gem_object_put(obj);
@@ -1730,7 +1744,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 	 */
 	if (!obj->base.filp) {
 		i915_gem_object_put(obj);
-		return -EINVAL;
+		return -EPERM;
 	}
 
 	addr = vm_mmap(obj->base.filp, 0, args->size,
@@ -3764,6 +3778,14 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOENT;
 
+	/* The caching mode of proxy object is handled by its generator, and not
+	 * expected to be changed by user mode.
+	 */
+	if (i915_gem_object_is_proxy(obj)) {
+		ret = -EPERM;
+		goto out;
+	}
+
 	if (obj->cache_level == level)
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index 5b19a49..f3b382a 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops {
 	unsigned int flags;
 #define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0)
 #define I915_GEM_OBJECT_IS_SHRINKABLE   BIT(1)
+#define I915_GEM_OBJECT_IS_PROXY	BIT(2)
 
 	/* Interface between the GEM object and its backing storage.
 	 * get_pages() is called once prior to the use of the associated set
@@ -300,6 +301,12 @@ i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
 }
 
 static inline bool
+i915_gem_object_is_proxy(const struct drm_i915_gem_object *obj)
+{
+	return obj->ops->flags & I915_GEM_OBJECT_IS_PROXY;
+}
+
+static inline bool
 i915_gem_object_is_active(const struct drm_i915_gem_object *obj)
 {
 	return obj->active_count;
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index fb5231f..d12859d 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -345,6 +345,14 @@ i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOENT;
 
+	/* The tiling mode of proxy objects is handled by its generator, and not
+	 * expected to be changed by user mode.
+	 */
+	if (i915_gem_object_is_proxy(obj)) {
+		err = -EPERM;
+		goto err;
+	}
+
 	if (!i915_tiling_ok(obj, args->tiling_mode, args->stride)) {
 		err = -EINVAL;
 		goto err;
-- 
2.7.4

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

[PATCH v13 7/7] drm/i915/gvt: Dmabuf support for GVT-g

[Tina Zhang]

This patch introduces a guest's framebuffer sharing mechanism based on
dma-buf subsystem. With this sharing mechanism, guest's framebuffer can
be shared between guest VM and host.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/Makefile      |   2 +-
 drivers/gpu/drm/i915/gvt/dmabuf.c      | 380 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/dmabuf.h      |  58 +++++
 drivers/gpu/drm/i915/gvt/gvt.c         |   1 +
 drivers/gpu/drm/i915/gvt/gvt.h         |   9 +
 drivers/gpu/drm/i915/gvt/hypercall.h   |   2 +
 drivers/gpu/drm/i915/gvt/kvmgt.c       |  42 ++++
 drivers/gpu/drm/i915/gvt/mpt.h         |  30 +++
 drivers/gpu/drm/i915/gvt/vgpu.c        |   2 +-
 drivers/gpu/drm/i915/i915_gem_object.h |   2 +
 10 files changed, 526 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.c
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.h

diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
index 019d596..18f43cb 100644
--- a/drivers/gpu/drm/i915/gvt/Makefile
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -2,7 +2,7 @@ GVT_DIR := gvt
 GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \
 	interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
 	execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \
-	fb_decoder.o
+	fb_decoder.o dmabuf.o
 
 ccflags-y				+= -I$(src) -I$(src)/$(GVT_DIR)
 i915-y					+= $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
new file mode 100644
index 0000000..b6ccd61f
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -0,0 +1,380 @@
+/*
+ * Copyright 2017 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Zhiyuan Lv <zhiyuan.lv@intel.com>
+ *
+ * Contributors:
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ *    Tina Zhang <tina.zhang@intel.com>
+ */
+
+#include <linux/dma-buf.h>
+#include <drm/drmP.h>
+#include <linux/vfio.h>
+
+#include "i915_drv.h"
+#include "gvt.h"
+
+#define GEN8_DECODE_PTE(pte) (pte & GENMASK_ULL(63, 12))
+
+#define VBLNAK_TIMER_PERIOD 16000000
+
+static struct sg_table *intel_vgpu_gem_get_pages(
+		struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
+	struct sg_table *st;
+	struct scatterlist *sg;
+	int i, ret;
+	gen8_pte_t __iomem *gtt_entries;
+	struct intel_vgpu_fb_info *fb_info;
+
+	fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info;
+	if (WARN_ON(!fb_info))
+		return ERR_PTR(-ENODEV);
+
+	st = kmalloc(sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return ERR_PTR(-ENOMEM);
+
+	ret = sg_alloc_table(st, fb_info->size, GFP_KERNEL);
+	if (ret) {
+		kfree(st);
+		return ERR_PTR(ret);
+	}
+	gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
+		(fb_info->start >> PAGE_SHIFT);
+	for_each_sg(st->sgl, sg, fb_info->size, i) {
+		sg->offset = 0;
+		sg->length = PAGE_SIZE;
+		sg_dma_address(sg) =
+			GEN8_DECODE_PTE(readq(&gtt_entries[i]));
+		sg_dma_len(sg) = PAGE_SIZE;
+	}
+
+	return st;
+}
+
+static void intel_vgpu_gem_put_pages(struct drm_i915_gem_object *obj,
+		struct sg_table *pages)
+{
+	sg_free_table(pages);
+	kfree(pages);
+}
+
+static void intel_vgpu_gem_release(struct drm_i915_gem_object *obj)
+{
+	struct intel_vgpu_dmabuf_obj *dmabuf_obj;
+	struct intel_vgpu_fb_info *fb_info;
+	struct intel_vgpu *vgpu;
+	struct list_head *pos;
+
+	fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info;
+	if (WARN_ON(!fb_info || !fb_info->vgpu)) {
+		gvt_err("gvt info is invalid\n");
+		goto out;
+	}
+
+	vgpu = fb_info->vgpu;
+	mutex_lock(&vgpu->dmabuf_list_lock);
+	list_for_each(pos, &vgpu->dmabuf_obj_list_head) {
+		dmabuf_obj = container_of(pos, struct intel_vgpu_dmabuf_obj,
+						list);
+		if ((dmabuf_obj != NULL) && (dmabuf_obj->obj == obj)) {
+			kfree(dmabuf_obj);
+			list_del(pos);
+			break;
+		}
+	}
+	mutex_unlock(&vgpu->dmabuf_list_lock);
+	intel_gvt_hypervisor_put_vfio_device(vgpu);
+out:
+	kfree(obj->gvt_info);
+}
+
+static const struct drm_i915_gem_object_ops intel_vgpu_gem_ops = {
+	.flags = I915_GEM_OBJECT_IS_PROXY,
+	.get_pages = intel_vgpu_gem_get_pages,
+	.put_pages = intel_vgpu_gem_put_pages,
+	.release = intel_vgpu_gem_release,
+};
+
+static struct drm_i915_gem_object *intel_vgpu_create_gem(struct drm_device *dev,
+		struct intel_vgpu_fb_info *info)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_i915_gem_object *obj;
+
+	obj = i915_gem_object_alloc(dev_priv);
+	if (obj == NULL)
+		return NULL;
+
+	drm_gem_private_object_init(dev, &obj->base,
+		info->size << PAGE_SHIFT);
+	i915_gem_object_init(obj, &intel_vgpu_gem_ops);
+
+	obj->base.read_domains = I915_GEM_DOMAIN_GTT;
+	obj->base.write_domain = 0;
+	if (IS_SKYLAKE(dev_priv)) {
+		unsigned int tiling_mode = 0;
+		unsigned int stride = 0;
+
+		switch (info->drm_format_mod << 10) {
+		case PLANE_CTL_TILED_LINEAR:
+			tiling_mode = I915_TILING_NONE;
+			break;
+		case PLANE_CTL_TILED_X:
+			tiling_mode = I915_TILING_X;
+			stride = info->stride;
+			break;
+		case PLANE_CTL_TILED_Y:
+			tiling_mode = I915_TILING_Y;
+			stride = info->stride;
+			break;
+		default:
+			gvt_dbg_core("not supported tiling mode\n");
+		}
+		obj->tiling_and_stride = tiling_mode | stride;
+	} else {
+		obj->tiling_and_stride = info->drm_format_mod ?
+					I915_TILING_X : 0;
+	}
+
+	return obj;
+}
+
+static int intel_vgpu_get_plane_info(struct drm_device *dev,
+		struct intel_vgpu *vgpu,
+		struct intel_vgpu_fb_info *info,
+		int plane_id)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_vgpu_primary_plane_format p;
+	struct intel_vgpu_cursor_plane_format c;
+	int ret;
+
+	if (plane_id == DRM_PLANE_TYPE_PRIMARY) {
+		ret = intel_vgpu_decode_primary_plane(vgpu, &p);
+		if (ret)
+			return ret;
+		info->start = p.base;
+		info->start_gpa = p.base_gpa;
+		info->width = p.width;
+		info->height = p.height;
+		info->stride = p.stride;
+		info->drm_format = p.drm_format;
+		info->drm_format_mod = p.tiled;
+		info->size = (((p.stride * p.height * p.bpp) / 8) +
+				(PAGE_SIZE - 1)) >> PAGE_SHIFT;
+	} else if (plane_id == DRM_PLANE_TYPE_CURSOR) {
+		ret = intel_vgpu_decode_cursor_plane(vgpu, &c);
+		if (ret)
+			return ret;
+		info->start = c.base;
+		info->start_gpa = c.base_gpa;
+		info->width = c.width;
+		info->height = c.height;
+		info->stride = c.width * (c.bpp / 8);
+		info->drm_format = c.drm_format;
+		info->drm_format_mod = 0;
+		info->x_pos = c.x_pos;
+		info->y_pos = c.y_pos;
+		info->size = (((info->stride * c.height * c.bpp) / 8)
+				+ (PAGE_SIZE - 1)) >> PAGE_SHIFT;
+	} else {
+		gvt_vgpu_err("invalid plane id:%d\n", plane_id);
+		return -EINVAL;
+	}
+
+	if (info->size == 0) {
+		gvt_vgpu_err("fb size is zero\n");
+		return -EINVAL;
+	}
+
+	if (info->start & (PAGE_SIZE - 1)) {
+		gvt_vgpu_err("Not aligned fb address:0x%llx\n", info->start);
+		return -EFAULT;
+	}
+	if (((info->start >> PAGE_SHIFT) + info->size) >
+		ggtt_total_entries(&dev_priv->ggtt)) {
+		gvt_vgpu_err("Invalid GTT offset or size\n");
+		return -EFAULT;
+	}
+
+	if (!intel_gvt_ggtt_validate_range(vgpu, info->start, info->size)) {
+		gvt_vgpu_err("invalid gma addr\n");
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
+static int intel_vgpu_pick_exposed_dmabuf(struct intel_vgpu *vgpu,
+		struct intel_vgpu_fb_info *latest_info)
+{
+	struct list_head *pos;
+	struct intel_vgpu_fb_info *fb_info;
+	struct intel_vgpu_dmabuf_obj *dmabuf_obj;
+	struct dma_buf *dmabuf;
+	int ret = -1;
+
+	mutex_lock(&vgpu->dmabuf_list_lock);
+	list_for_each(pos, &vgpu->dmabuf_obj_list_head) {
+		dmabuf_obj = container_of(pos, struct intel_vgpu_dmabuf_obj,
+						list);
+		if ((dmabuf_obj == NULL) ||
+		    (dmabuf_obj->obj == NULL) ||
+		    (dmabuf_obj->obj->gvt_info == NULL))
+			continue;
+
+		fb_info = dmabuf_obj->obj->gvt_info;
+
+		if ((fb_info->start == latest_info->start) &&
+		    (fb_info->start_gpa == latest_info->start_gpa) &&
+		    (fb_info->size == latest_info->size) &&
+		    (fb_info->drm_format_mod == latest_info->drm_format_mod) &&
+		    (fb_info->drm_format == latest_info->drm_format) &&
+		    (fb_info->width == latest_info->width) &&
+		    (fb_info->height == latest_info->height) &&
+		    (fb_info->stride == latest_info->stride)) {
+			dmabuf = dma_buf_get(dmabuf_obj->fd);
+			if (IS_ERR(dmabuf)) {
+				continue;
+			}
+			dma_buf_put(dmabuf);
+			ret = dmabuf_obj->fd;
+			break;
+		}
+	}
+	mutex_unlock(&vgpu->dmabuf_list_lock);
+
+	return ret;
+}
+
+static void update_fb_info(struct vfio_device_gfx_plane_info *gvt_dmabuf,
+		      struct intel_vgpu_fb_info *fb_info)
+{
+	gvt_dmabuf->drm_format = fb_info->drm_format;
+	gvt_dmabuf->width = fb_info->width;
+	gvt_dmabuf->height = fb_info->height;
+	gvt_dmabuf->stride = fb_info->stride;
+	gvt_dmabuf->size = fb_info->size;
+	gvt_dmabuf->x_pos = fb_info->x_pos;
+	gvt_dmabuf->y_pos = fb_info->y_pos;
+}
+
+int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args)
+{
+	struct drm_device *dev = &vgpu->gvt->dev_priv->drm;
+	struct vfio_device_gfx_plane_info *gvt_dmabuf = args;
+	struct intel_vgpu_dmabuf_obj *dmabuf_obj;
+	struct drm_i915_gem_object *obj;
+	struct intel_vgpu_fb_info fb_info;
+	struct dma_buf *dmabuf;
+	int ret;
+
+	ret = intel_vgpu_get_plane_info(dev, vgpu, &fb_info,
+					gvt_dmabuf->drm_plane_type);
+	if (ret != 0)
+		goto out;
+
+	/* If exists, pick up the exposed dmabuf fd */
+	ret = intel_vgpu_pick_exposed_dmabuf(vgpu, &fb_info);
+	if (ret > 0) {
+		update_fb_info(gvt_dmabuf, &fb_info);
+		gvt_dmabuf->fd = ret;
+		ret = 0;
+		goto out;
+	}
+
+	/* Need to expose a new one*/
+	dmabuf_obj = kmalloc(sizeof(struct intel_vgpu_dmabuf_obj), GFP_KERNEL);
+	if (!dmabuf_obj) {
+		gvt_vgpu_err("alloc dmabuf_obj failed\n");
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	obj = intel_vgpu_create_gem(dev, &fb_info);
+	if (obj == NULL) {
+		gvt_vgpu_err("create gvt gem obj failed:%d\n", vgpu->id);
+		ret = -ENOMEM;
+		goto out_free_dmabuf;
+	}
+
+	dmabuf_obj->obj = obj;
+
+	obj->gvt_info = kmalloc(sizeof(struct intel_vgpu_fb_info), GFP_KERNEL);
+	if (!obj->gvt_info) {
+		gvt_vgpu_err("allocate intel vgpu fb info failed\n");
+		ret = -ENOMEM;
+		goto out_free_gem;
+	}
+
+	fb_info.vgpu = vgpu;
+	dmabuf_obj->vgpu = vgpu;
+	memcpy(obj->gvt_info, &fb_info, sizeof(struct intel_vgpu_fb_info));
+
+	dmabuf = i915_gem_prime_export(dev, &obj->base, DRM_CLOEXEC | DRM_RDWR);
+	if (IS_ERR(dmabuf)) {
+		gvt_vgpu_err("export dma-buf failed\n");
+		ret = PTR_ERR(dmabuf);
+		goto out_free_info;
+	}
+	i915_gem_object_put(obj);
+	obj->base.dma_buf = dmabuf;
+
+	ret = dma_buf_fd(dmabuf, DRM_CLOEXEC | DRM_RDWR);
+	if (ret < 0) {
+		gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret);
+		goto out_free;
+	}
+
+	if (intel_gvt_hypervisor_get_vfio_device(vgpu)) {
+		gvt_vgpu_err("get vfio device failed\n");
+		put_unused_fd(ret);
+		goto out_free;
+	}
+
+	update_fb_info(gvt_dmabuf, &fb_info);
+	gvt_dmabuf->fd = ret;
+	dmabuf_obj->fd = ret;
+
+	INIT_LIST_HEAD(&dmabuf_obj->list);
+	mutex_lock(&vgpu->dmabuf_list_lock);
+	list_add_tail(&dmabuf_obj->list, &vgpu->dmabuf_obj_list_head);
+	mutex_unlock(&vgpu->dmabuf_list_lock);
+
+	return 0;
+
+out_free:
+	dma_buf_put(dmabuf);
+out_free_info:
+	kfree(obj->gvt_info);
+out_free_gem:
+	i915_gem_object_put(obj);
+out_free_dmabuf:
+	kfree(dmabuf_obj);
+out:
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.h b/drivers/gpu/drm/i915/gvt/dmabuf.h
new file mode 100644
index 0000000..f591f4f
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright(c) 2017 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * Authors:
+ *    Zhiyuan Lv <zhiyuan.lv@intel.com>
+ *
+ * Contributors:
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ *    Tina Zhang <tina.zhang@intel.com>
+ */
+
+#ifndef _GVT_DMABUF_H_
+#define _GVT_DMABUF_H_
+#include <linux/vfio.h>
+
+struct intel_vgpu_fb_info {
+	__u64 start;
+	__u64 start_gpa;
+	__u64 drm_format_mod;
+	__u32 drm_format;	/* drm format of plane */
+	__u32 width;	/* width of plane */
+	__u32 height;	/* height of plane */
+	__u32 stride;	/* stride of plane */
+	__u32 size;	/* size of plane in bytes, align on page*/
+	__u32 x_pos;	/* horizontal position of cursor plane, upper left corner in pixels */
+	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/
+	struct intel_vgpu *vgpu;
+};
+
+struct intel_vgpu_dmabuf_obj {
+	struct intel_vgpu *vgpu;
+	struct drm_i915_gem_object *obj;
+	int fd;
+	struct list_head list;
+};
+
+int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args);
+
+#endif
diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
index c27c683..f77770b 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.c
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -54,6 +54,7 @@ static const struct intel_gvt_ops intel_gvt_ops = {
 	.vgpu_reset = intel_gvt_reset_vgpu,
 	.vgpu_activate = intel_gvt_activate_vgpu,
 	.vgpu_deactivate = intel_gvt_deactivate_vgpu,
+	.vgpu_query_plane = intel_vgpu_query_plane,
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index d0fe4d0..da3ca64 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -47,6 +47,7 @@
 #include "render.h"
 #include "cmd_parser.h"
 #include "fb_decoder.h"
+#include "dmabuf.h"
 
 #define GVT_MAX_VGPU 8
 
@@ -183,8 +184,15 @@ struct intel_vgpu {
 		struct kvm *kvm;
 		struct work_struct release_work;
 		atomic_t released;
+		struct vfio_device *vfio_device;
 	} vdev;
 #endif
+
+	struct list_head dmabuf_obj_list_head;
+	struct mutex dmabuf_list_lock;
+
+	struct completion vblank_done;
+
 };
 
 struct intel_gvt_gm {
@@ -488,6 +496,7 @@ struct intel_gvt_ops {
 	void (*vgpu_reset)(struct intel_vgpu *);
 	void (*vgpu_activate)(struct intel_vgpu *);
 	void (*vgpu_deactivate)(struct intel_vgpu *);
+	int (*vgpu_query_plane)(struct intel_vgpu *vgpu, void *);
 };
 
 
diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
index 32c345c..a1bd82f 100644
--- a/drivers/gpu/drm/i915/gvt/hypercall.h
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -56,6 +56,8 @@ struct intel_gvt_mpt {
 	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
 			     bool map);
 	int (*set_opregion)(void *vgpu);
+	int (*get_vfio_device)(void *vgpu);
+	void (*put_vfio_device)(void *vgpu);
 };
 
 extern struct intel_gvt_mpt xengt_mpt;
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 6b0a330..76ac093 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -492,10 +492,23 @@ static int intel_vgpu_register_reg(struct intel_vgpu *vgpu,
 	vgpu->vdev.region[vgpu->vdev.num_regions].flags = flags;
 	vgpu->vdev.region[vgpu->vdev.num_regions].data = data;
 	vgpu->vdev.num_regions++;
+	return 0;
+}
+
+static int kvmgt_get_vfio_device(void *p_vgpu)
+{
+	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
 
+	vgpu->vdev.vfio_device = vfio_device_get_from_dev(
+		mdev_dev(vgpu->vdev.mdev));
+	if (!vgpu->vdev.vfio_device) {
+		gvt_vgpu_err("failed to get vfio device\n");
+		return -ENODEV;
+	}
 	return 0;
 }
 
+
 static int kvmgt_set_opregion(void *p_vgpu)
 {
 	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
@@ -527,6 +540,14 @@ static int kvmgt_set_opregion(void *p_vgpu)
 	return ret;
 }
 
+static void kvmgt_put_vfio_device(void *vgpu)
+{
+	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
+		return;
+
+	vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device);
+}
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct intel_vgpu *vgpu = NULL;
@@ -1253,6 +1274,22 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 	} else if (cmd == VFIO_DEVICE_RESET) {
 		intel_gvt_ops->vgpu_reset(vgpu);
 		return 0;
+	} else if (cmd == VFIO_DEVICE_QUERY_GFX_PLANE) {
+		struct vfio_device_gfx_plane_info dmabuf;
+		int ret = 0;
+
+		minsz = offsetofend(struct vfio_device_gfx_plane_info, fd);
+		if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
+			return -EFAULT;
+		if (dmabuf.argsz < minsz || dmabuf.flags != 0)
+			return -EINVAL;
+
+		ret = intel_gvt_ops->vgpu_query_plane(vgpu, &dmabuf);
+		if (ret != 0)
+			return ret;
+
+		return copy_to_user((void __user *)arg, &dmabuf, minsz) ?
+								-EFAULT : 0;
 	}
 
 	return 0;
@@ -1475,6 +1512,9 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
 	kvmgt_protect_table_init(info);
 	gvt_cache_init(vgpu);
 
+	mutex_init(&vgpu->dmabuf_list_lock);
+	init_completion(&vgpu->vblank_done);
+
 	info->track_node.track_write = kvmgt_page_track_write;
 	info->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
 	kvm_page_track_register_notifier(kvm, &info->track_node);
@@ -1616,6 +1656,8 @@ struct intel_gvt_mpt kvmgt_mpt = {
 	.write_gpa = kvmgt_write_gpa,
 	.gfn_to_mfn = kvmgt_gfn_to_pfn,
 	.set_opregion = kvmgt_set_opregion,
+	.get_vfio_device = kvmgt_get_vfio_device,
+	.put_vfio_device = kvmgt_put_vfio_device,
 };
 EXPORT_SYMBOL_GPL(kvmgt_mpt);
 
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
index 9e73f2e..86443ff 100644
--- a/drivers/gpu/drm/i915/gvt/mpt.h
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -307,4 +307,34 @@ static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)
 	return intel_gvt_host.mpt->set_opregion(vgpu);
 }
 
+/**
+ * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
+ * @vgpu: a vGPU
+ *
+ * Returns:
+ * Zero on success, negative error code if failed.
+ */
+static inline int intel_gvt_hypervisor_get_vfio_device(struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->get_vfio_device)
+		return 0;
+
+	return intel_gvt_host.mpt->get_vfio_device(vgpu);
+}
+
+/**
+ * intel_gvt_hypervisor_put_vfio_device - decrease vfio device ref count
+ * @vgpu: a vGPU
+ *
+ * Returns:
+ * Zero on success, negative error code if failed.
+ */
+static inline void intel_gvt_hypervisor_put_vfio_device(struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->put_vfio_device)
+		return;
+
+	intel_gvt_host.mpt->put_vfio_device(vgpu);
+}
+
 #endif /* _GVT_MPT_H_ */
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index 03f9539..f208ad0 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -346,7 +346,7 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
 	vgpu->gvt = gvt;
 	vgpu->sched_ctl.weight = param->weight;
 	bitmap_zero(vgpu->tlb_handle_pending, I915_NUM_ENGINES);
-
+	INIT_LIST_HEAD(&vgpu->dmabuf_obj_list_head);
 	intel_vgpu_init_cfg_space(vgpu, param->primary);
 
 	ret = intel_vgpu_init_mmio(vgpu);
diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index f3b382a..c91e336 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -199,6 +199,8 @@ struct drm_i915_gem_object {
 		} userptr;
 
 		unsigned long scratch;
+
+		void *gvt_info;
 	};
 
 	/** for phys allocated objects */
-- 
2.7.4

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

✗ Fi.CI.BAT: warning for drm/i915/gvt: Dma-buf support for GVT-g

[Patchwork]

== Series Details ==

Series: drm/i915/gvt: Dma-buf support for GVT-g
URL   : https://patchwork.freedesktop.org/series/27844/
State : warning

== Summary ==

Series 27844v1 drm/i915/gvt: Dma-buf support for GVT-g
https://patchwork.freedesktop.org/api/1.0/series/27844/revisions/1/mbox/

Test gem_ringfill:
        Subgroup basic-default:
                pass       -> SKIP       (fi-bsw-n3050)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-n2820) fdo#101705

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:280  pass:269  dwarn:0   dfail:0   fail:0   skip:11  time:444s
fi-bdw-gvtdvm    total:280  pass:265  dwarn:1   dfail:0   fail:0   skip:14  time:430s
fi-blb-e6850     total:280  pass:225  dwarn:1   dfail:0   fail:0   skip:54  time:352s
fi-bsw-n3050     total:280  pass:243  dwarn:0   dfail:0   fail:0   skip:37  time:529s
fi-bxt-j4205     total:280  pass:261  dwarn:0   dfail:0   fail:0   skip:19  time:512s
fi-byt-j1900     total:280  pass:256  dwarn:0   dfail:0   fail:0   skip:24  time:493s
fi-byt-n2820     total:280  pass:252  dwarn:0   dfail:0   fail:0   skip:28  time:489s
fi-glk-2a        total:280  pass:261  dwarn:0   dfail:0   fail:0   skip:19  time:600s
fi-hsw-4770      total:280  pass:264  dwarn:0   dfail:0   fail:0   skip:16  time:440s
fi-hsw-4770r     total:280  pass:264  dwarn:0   dfail:0   fail:0   skip:16  time:415s
fi-ilk-650       total:280  pass:230  dwarn:0   dfail:0   fail:0   skip:50  time:417s
fi-ivb-3520m     total:280  pass:262  dwarn:0   dfail:0   fail:0   skip:18  time:504s
fi-ivb-3770      total:280  pass:262  dwarn:0   dfail:0   fail:0   skip:18  time:478s
fi-kbl-7500u     total:280  pass:262  dwarn:0   dfail:0   fail:0   skip:18  time:469s
fi-kbl-7560u     total:280  pass:270  dwarn:0   dfail:0   fail:0   skip:10  time:575s
fi-kbl-r         total:280  pass:262  dwarn:0   dfail:0   fail:0   skip:18  time:587s
fi-pnv-d510      total:280  pass:223  dwarn:2   dfail:0   fail:0   skip:55  time:566s
fi-skl-6260u     total:280  pass:270  dwarn:0   dfail:0   fail:0   skip:10  time:456s
fi-skl-6700hq    total:280  pass:263  dwarn:0   dfail:0   fail:0   skip:17  time:585s
fi-skl-6700k     total:280  pass:262  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-skl-6770hq    total:280  pass:270  dwarn:0   dfail:0   fail:0   skip:10  time:482s
fi-skl-gvtdvm    total:280  pass:266  dwarn:1   dfail:0   fail:0   skip:13  time:440s
fi-skl-x1585l    total:280  pass:269  dwarn:0   dfail:0   fail:0   skip:11  time:472s
fi-snb-2520m     total:280  pass:252  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-snb-2600      total:280  pass:251  dwarn:0   dfail:0   fail:0   skip:29  time:399s

9e377e5fc2f8360c01787ebfe16e1bea778b4d9d drm-tip: 2017y-07m-25d-07h-06m-30s UTC integration manifest
35d5cd644bed drm/i915/gvt: Dmabuf support for GVT-g
9b4dfe796c3c drm/i915: Introduce GEM proxy
1399f4fa4e78 vfio: ABI for mdev display dma-buf operation
bcf25e9eb909 drm/i915/gvt: Add opregion support
1297435bc1a6 drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support
aea01e3fd71a drm: Introduce RGB 64-bit 16:16:16:16 float format
833445cae249 drm/i915/gvt: Add framebuffer decoder support

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5271/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Gerd Hoffmann]

  Hi,

> +/**
> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> struct vfio_device_query_gfx_plane)
> + *
> + * Set the drm_plane_type and retrieve information about the gfx
> plane.
+ *
> + * Return: 0 on success, -errno on failure.

I think this should be more verbose, especially documenting that the
"guest driver didn't initialize the display yet" case isn't and error
and fields should be set to zero then (as discussed on the list).

> + */
> +struct vfio_device_gfx_plane_info {
> +	__u32 argsz;
> +	__u32 flags;
> +	/* in */
> +	__u32 drm_plane_type;	/* type of plane:
> DRM_PLANE_TYPE_* */
> +	/* out */
> +	__u32 drm_format;	/* drm format of plane */
> +	__u64 drm_format_mod;   /* tiled mode */
> +	__u32 width;	/* width of plane */
> +	__u32 height;	/* height of plane */
> +	__u32 stride;	/* stride of plane */
> +	__u32 size;	/* size of plane in bytes, align on
> page*/
> +	__u32 x_pos;	/* horizontal position of cursor plane,
> upper left corner in pixels */
> +	__u32 y_pos;	/* vertical position of cursor plane,
> upper left corner in lines*/
> +	__u32 region_index;
> +	__s32 fd;	/* dma-buf fd */
> +};

Looks good to me.

Unfortunately I havn't been able to test the whole series yet due to
being busy with other stuff, and I'm about to leave for my summer
vacation.  Will be back online on Aug 21st.

cheers,
  Gerd


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

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Friday, July 28, 2017 4:27 PM
> To: Zhang, Tina <tina.zhang@intel.com>; intel-gfx@lists.freedesktop.org; intel-
> gvt-dev@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> ville.syrjala@linux.intel.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>;
> alex.williamson@redhat.com; chris@chris-wilson.co.uk; daniel@ffwll.ch;
> kwankhede@nvidia.com; Tian, Kevin <kevin.tian@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
>   Hi,
> 
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > struct vfio_device_query_gfx_plane)
> > + *
> > + * Set the drm_plane_type and retrieve information about the gfx
> > plane.
> + *
> > + * Return: 0 on success, -errno on failure.
> 
> I think this should be more verbose, especially documenting that the "guest
> driver didn't initialize the display yet" case isn't and error and fields should be set
> to zero then (as discussed on the list).
I can add this in the next version. 
Thanks.

Tina
> 
> > + */
> > +struct vfio_device_gfx_plane_info {
> > +	__u32 argsz;
> > +	__u32 flags;
> > +	/* in */
> > +	__u32 drm_plane_type;	/* type of plane:
> > DRM_PLANE_TYPE_* */
> > +	/* out */
> > +	__u32 drm_format;	/* drm format of plane */
> > +	__u64 drm_format_mod;   /* tiled mode */
> > +	__u32 width;	/* width of plane */
> > +	__u32 height;	/* height of plane */
> > +	__u32 stride;	/* stride of plane */
> > +	__u32 size;	/* size of plane in bytes, align on
> > page*/
> > +	__u32 x_pos;	/* horizontal position of cursor plane,
> > upper left corner in pixels */
> > +	__u32 y_pos;	/* vertical position of cursor plane,
> > upper left corner in lines*/
> > +	__u32 region_index;
> > +	__s32 fd;	/* dma-buf fd */
> > +};
> 
> Looks good to me.
> 
> Unfortunately I havn't been able to test the whole series yet due to being busy
> with other stuff, and I'm about to leave for my summer vacation.  Will be back
> online on Aug 21st.
Fine to me. I will also update our qemu sample code and some wiki according to the current interface in the next version, which
may give you some help for your test.
Thanks.

Tina

> 
> cheers,
>   Gerd
> 
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Alex Williamson]

On Tue, 25 Jul 2017 17:28:18 +0800
Tina Zhang <tina.zhang@intel.com> wrote:

> Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query and
> get the plan and its related information.
> 
> The dma-buf's life cycle is handled by user mode and tracked by kernel.
> The returned fd in struct vfio_device_query_gfx_plane can be a new
> fd or an old fd of a re-exported dma-buf. Host User mode can check the
> value of fd and to see if it needs to create new resource according to
> the new fd or just use the existed resource related to the old fd.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..827a230 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
>  
>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
>  
> +/**
> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14, struct vfio_device_query_gfx_plane)
> + *
> + * Set the drm_plane_type and retrieve information about the gfx plane.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct vfio_device_gfx_plane_info {
> +	__u32 argsz;
> +	__u32 flags;
> +	/* in */
> +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> +	/* out */
> +	__u32 drm_format;	/* drm format of plane */
> +	__u64 drm_format_mod;   /* tiled mode */
> +	__u32 width;	/* width of plane */
> +	__u32 height;	/* height of plane */
> +	__u32 stride;	/* stride of plane */
> +	__u32 size;	/* size of plane in bytes, align on page*/
> +	__u32 x_pos;	/* horizontal position of cursor plane, upper left corner in pixels */
> +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/
> +	__u32 region_index;
> +	__s32 fd;	/* dma-buf fd */

How do I know which of these is valid, region_index or fd?  Can I ask
for one vs the other?  What are the errno values to differentiate
unsupported vs not initialized?  Is there a "probe" flag that I can use
to determine what the device supports without allocating those
resources yet?

Kirti, does this otherwise meet your needs?

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

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Kirti Wankhede]

On 8/2/2017 2:48 AM, Alex Williamson wrote:
> On Tue, 25 Jul 2017 17:28:18 +0800
> Tina Zhang <tina.zhang@intel.com> wrote:
> 
>> Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query and
>> get the plan and its related information.
>>
>> The dma-buf's life cycle is handled by user mode and tracked by kernel.
>> The returned fd in struct vfio_device_query_gfx_plane can be a new
>> fd or an old fd of a re-exported dma-buf. Host User mode can check the
>> value of fd and to see if it needs to create new resource according to
>> the new fd or just use the existed resource related to the old fd.
>>
>> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
>> ---
>>  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index ae46105..827a230 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
>>  
>>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
>>  
>> +/**
>> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14, struct vfio_device_query_gfx_plane)
>> + *
>> + * Set the drm_plane_type and retrieve information about the gfx plane.
>> + *
>> + * Return: 0 on success, -errno on failure.
>> + */
>> +struct vfio_device_gfx_plane_info {
>> +	__u32 argsz;
>> +	__u32 flags;
>> +	/* in */
>> +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
>> +	/* out */
>> +	__u32 drm_format;	/* drm format of plane */
>> +	__u64 drm_format_mod;   /* tiled mode */
>> +	__u32 width;	/* width of plane */
>> +	__u32 height;	/* height of plane */
>> +	__u32 stride;	/* stride of plane */
>> +	__u32 size;	/* size of plane in bytes, align on page*/
>> +	__u32 x_pos;	/* horizontal position of cursor plane, upper left corner in pixels */
>> +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in lines*/
>> +	__u32 region_index;
>> +	__s32 fd;	/* dma-buf fd */
> 
> How do I know which of these is valid, region_index or fd?  Can I ask
> for one vs the other?  What are the errno values to differentiate
> unsupported vs not initialized?  Is there a "probe" flag that I can use
> to determine what the device supports without allocating those
> resources yet?
> 
> Kirti, does this otherwise meet your needs?
> 

I wanted to test my change with this interface, but I couldn't, was busy
with other stuff.
This structure looks good to me for region type support.

> What are the errno values to differentiate
> unsupported vs not initialized?

In previous version discussion, we decided that vendor driver should
set all members to 0 in such cases. Mainly, if size is 0 then there is
nothing to copy.

Thanks,
Kirti

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

RE: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -----Original Message-----
> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
> Alex Williamson
> Sent: Wednesday, August 2, 2017 5:18 AM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; kraxel@redhat.com; intel-
> gfx@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>;
> kwankhede@nvidia.com; dri-devel@lists.freedesktop.org; intel-gvt-
> dev@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Tue, 25 Jul 2017 17:28:18 +0800
> Tina Zhang <tina.zhang@intel.com> wrote:
> 
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> > and get the plan and its related information.
> >
> > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > or an old fd of a re-exported dma-buf. Host User mode can check the
> > value of fd and to see if it needs to create new resource according to
> > the new fd or just use the existed resource related to the old fd.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..827a230 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> >
> >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE +
> 13)
> >
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > +struct vfio_device_query_gfx_plane)
> > + *
> > + * Set the drm_plane_type and retrieve information about the gfx plane.
> > + *
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +struct vfio_device_gfx_plane_info {
> > +	__u32 argsz;
> > +	__u32 flags;
> > +	/* in */
> > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> > +	/* out */
> > +	__u32 drm_format;	/* drm format of plane */
> > +	__u64 drm_format_mod;   /* tiled mode */
> > +	__u32 width;	/* width of plane */
> > +	__u32 height;	/* height of plane */
> > +	__u32 stride;	/* stride of plane */
> > +	__u32 size;	/* size of plane in bytes, align on page*/
> > +	__u32 x_pos;	/* horizontal position of cursor plane, upper left corner
> in pixels */
> > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in
> lines*/
> > +	__u32 region_index;
> > +	__s32 fd;	/* dma-buf fd */
> 
> How do I know which of these is valid, region_index or fd?  Can I ask for one vs
> the other?  What are the errno values to differentiate unsupported vs not
> initialized?  Is there a "probe" flag that I can use to determine what the device
> supports without allocating those resources yet?
Dma-buf won't use region_index, which means region_index is zero all the time for dma-buf usage. 
As we discussed, there won't be errno if not initialized, just keep all fields zero.
I will add the comments about these in the next version. Thanks

Tina

> 
> Kirti, does this otherwise meet your needs?
> 
> Thanks,
> Alex
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Alex Williamson]

On Thu, 3 Aug 2017 03:17:09 +0000
"Zhang, Tina" <tina.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
> > Alex Williamson
> > Sent: Wednesday, August 2, 2017 5:18 AM
> > To: Zhang, Tina <tina.zhang@intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>; kraxel@redhat.com; intel-
> > gfx@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>;
> > kwankhede@nvidia.com; dri-devel@lists.freedesktop.org; intel-gvt-
> > dev@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>
> > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> > 
> > On Tue, 25 Jul 2017 17:28:18 +0800
> > Tina Zhang <tina.zhang@intel.com> wrote:
> >   
> > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query
> > > and get the plan and its related information.
> > >
> > > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > > The returned fd in struct vfio_device_query_gfx_plane can be a new fd
> > > or an old fd of a re-exported dma-buf. Host User mode can check the
> > > value of fd and to see if it needs to create new resource according to
> > > the new fd or just use the existed resource related to the old fd.
> > >
> > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > ---
> > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > index ae46105..827a230 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > >
> > >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE +  
> > 13)  
> > >
> > > +/**
> > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > > +struct vfio_device_query_gfx_plane)
> > > + *
> > > + * Set the drm_plane_type and retrieve information about the gfx plane.
> > > + *
> > > + * Return: 0 on success, -errno on failure.
> > > + */
> > > +struct vfio_device_gfx_plane_info {
> > > +	__u32 argsz;
> > > +	__u32 flags;
> > > +	/* in */
> > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> > > +	/* out */
> > > +	__u32 drm_format;	/* drm format of plane */
> > > +	__u64 drm_format_mod;   /* tiled mode */
> > > +	__u32 width;	/* width of plane */
> > > +	__u32 height;	/* height of plane */
> > > +	__u32 stride;	/* stride of plane */
> > > +	__u32 size;	/* size of plane in bytes, align on page*/
> > > +	__u32 x_pos;	/* horizontal position of cursor plane, upper left corner  
> > in pixels */  
> > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in  
> > lines*/  
> > > +	__u32 region_index;
> > > +	__s32 fd;	/* dma-buf fd */  
> > 
> > How do I know which of these is valid, region_index or fd?  Can I ask for one vs
> > the other?  What are the errno values to differentiate unsupported vs not
> > initialized?  Is there a "probe" flag that I can use to determine what the device
> > supports without allocating those resources yet?  
> Dma-buf won't use region_index, which means region_index is zero all the time for dma-buf usage. 
> As we discussed, there won't be errno if not initialized, just keep all fields zero.
> I will add the comments about these in the next version. Thanks

Zero is a valid region index.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Thursday, August 3, 2017 11:38 AM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org; kwankhede@nvidia.com; kraxel@redhat.com;
> intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>; Lv,
> Zhiyuan <zhiyuan.lv@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Thu, 3 Aug 2017 03:17:09 +0000
> "Zhang, Tina" <tina.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
> > > Behalf Of Alex Williamson
> > > Sent: Wednesday, August 2, 2017 5:18 AM
> > > To: Zhang, Tina <tina.zhang@intel.com>
> > > Cc: Tian, Kevin <kevin.tian@intel.com>; kraxel@redhat.com; intel-
> > > gfx@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>;
> > > kwankhede@nvidia.com; dri-devel@lists.freedesktop.org; intel-gvt-
> > > dev@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>
> > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > operation
> > >
> > > On Tue, 25 Jul 2017 17:28:18 +0800
> > > Tina Zhang <tina.zhang@intel.com> wrote:
> > >
> > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode
> > > > query and get the plan and its related information.
> > > >
> > > > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > > > The returned fd in struct vfio_device_query_gfx_plane can be a new
> > > > fd or an old fd of a re-exported dma-buf. Host User mode can check
> > > > the value of fd and to see if it needs to create new resource
> > > > according to the new fd or just use the existed resource related to the old
> fd.
> > > >
> > > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > > ---
> > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > index ae46105..827a230 100644
> > > > --- a/include/uapi/linux/vfio.h
> > > > +++ b/include/uapi/linux/vfio.h
> > > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > > >
> > > >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE +
> > > 13)
> > > >
> > > > +/**
> > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE +
> 14,
> > > > +struct vfio_device_query_gfx_plane)
> > > > + *
> > > > + * Set the drm_plane_type and retrieve information about the gfx plane.
> > > > + *
> > > > + * Return: 0 on success, -errno on failure.
> > > > + */
> > > > +struct vfio_device_gfx_plane_info {
> > > > +	__u32 argsz;
> > > > +	__u32 flags;
> > > > +	/* in */
> > > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> > > > +	/* out */
> > > > +	__u32 drm_format;	/* drm format of plane */
> > > > +	__u64 drm_format_mod;   /* tiled mode */
> > > > +	__u32 width;	/* width of plane */
> > > > +	__u32 height;	/* height of plane */
> > > > +	__u32 stride;	/* stride of plane */
> > > > +	__u32 size;	/* size of plane in bytes, align on page*/
> > > > +	__u32 x_pos;	/* horizontal position of cursor plane, upper left corner
> > > in pixels */
> > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in
> > > lines*/
> > > > +	__u32 region_index;
> > > > +	__s32 fd;	/* dma-buf fd */
> > >
> > > How do I know which of these is valid, region_index or fd?  Can I
> > > ask for one vs the other?  What are the errno values to
> > > differentiate unsupported vs not initialized?  Is there a "probe"
> > > flag that I can use to determine what the device supports without allocating
> those resources yet?
> > Dma-buf won't use region_index, which means region_index is zero all the
> time for dma-buf usage.
> > As we discussed, there won't be errno if not initialized, just keep all fields zero.
> > I will add the comments about these in the next version. Thanks
> 
> Zero is a valid region index.
If zero is valid, can we find out other invalid value? How about 0xffffffff?

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

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Alex Williamson]

On Thu, 3 Aug 2017 07:08:15 +0000
"Zhang, Tina" <tina.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Thursday, August 3, 2017 11:38 AM
> > To: Zhang, Tina <tina.zhang@intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org; kwankhede@nvidia.com; kraxel@redhat.com;
> > intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>; Lv,
> > Zhiyuan <zhiyuan.lv@intel.com>
> > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> > 
> > On Thu, 3 Aug 2017 03:17:09 +0000
> > "Zhang, Tina" <tina.zhang@intel.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
> > > > Behalf Of Alex Williamson
> > > > Sent: Wednesday, August 2, 2017 5:18 AM
> > > > To: Zhang, Tina <tina.zhang@intel.com>
> > > > Cc: Tian, Kevin <kevin.tian@intel.com>; kraxel@redhat.com; intel-
> > > > gfx@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>;
> > > > kwankhede@nvidia.com; dri-devel@lists.freedesktop.org; intel-gvt-
> > > > dev@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>
> > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > > operation
> > > >
> > > > On Tue, 25 Jul 2017 17:28:18 +0800
> > > > Tina Zhang <tina.zhang@intel.com> wrote:
> > > >  
> > > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode
> > > > > query and get the plan and its related information.
> > > > >
> > > > > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > > > > The returned fd in struct vfio_device_query_gfx_plane can be a new
> > > > > fd or an old fd of a re-exported dma-buf. Host User mode can check
> > > > > the value of fd and to see if it needs to create new resource
> > > > > according to the new fd or just use the existed resource related to the old  
> > fd.  
> > > > >
> > > > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > > > ---
> > > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 28 insertions(+)
> > > > >
> > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > > index ae46105..827a230 100644
> > > > > --- a/include/uapi/linux/vfio.h
> > > > > +++ b/include/uapi/linux/vfio.h
> > > > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > > > >
> > > > >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE +  
> > > > 13)  
> > > > >
> > > > > +/**
> > > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE +  
> > 14,  
> > > > > +struct vfio_device_query_gfx_plane)
> > > > > + *
> > > > > + * Set the drm_plane_type and retrieve information about the gfx plane.
> > > > > + *
> > > > > + * Return: 0 on success, -errno on failure.
> > > > > + */
> > > > > +struct vfio_device_gfx_plane_info {
> > > > > +	__u32 argsz;
> > > > > +	__u32 flags;
> > > > > +	/* in */
> > > > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> > > > > +	/* out */
> > > > > +	__u32 drm_format;	/* drm format of plane */
> > > > > +	__u64 drm_format_mod;   /* tiled mode */
> > > > > +	__u32 width;	/* width of plane */
> > > > > +	__u32 height;	/* height of plane */
> > > > > +	__u32 stride;	/* stride of plane */
> > > > > +	__u32 size;	/* size of plane in bytes, align on page*/
> > > > > +	__u32 x_pos;	/* horizontal position of cursor plane, upper left corner  
> > > > in pixels */  
> > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left corner in  
> > > > lines*/  
> > > > > +	__u32 region_index;
> > > > > +	__s32 fd;	/* dma-buf fd */  
> > > >
> > > > How do I know which of these is valid, region_index or fd?  Can I
> > > > ask for one vs the other?  What are the errno values to
> > > > differentiate unsupported vs not initialized?  Is there a "probe"
> > > > flag that I can use to determine what the device supports without allocating  
> > those resources yet?  
> > > Dma-buf won't use region_index, which means region_index is zero all the  
> > time for dma-buf usage.  
> > > As we discussed, there won't be errno if not initialized, just keep all fields zero.
> > > I will add the comments about these in the next version. Thanks  
> > 
> > Zero is a valid region index.  
> If zero is valid, can we find out other invalid value? How about 0xffffffff?

Unlikely, but also valid.  Isn't this why we have a flags field in the
structure, so we don't need to rely on implicit values as invalid.
Also, all of the previously discussed usage models needs to be
documented, either here in the header or in a Documentation/ file.
Thanks,

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

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -----Original Message-----
> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
> Alex Williamson
> Sent: Thursday, August 3, 2017 10:43 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org; kwankhede@nvidia.com; kraxel@redhat.com;
> intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>; Lv,
> Zhiyuan <zhiyuan.lv@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Thu, 3 Aug 2017 07:08:15 +0000
> "Zhang, Tina" <tina.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Thursday, August 3, 2017 11:38 AM
> > > To: Zhang, Tina <tina.zhang@intel.com>
> > > Cc: Tian, Kevin <kevin.tian@intel.com>;
> > > intel-gfx@lists.freedesktop.org; dri- devel@lists.freedesktop.org;
> > > kwankhede@nvidia.com; kraxel@redhat.com;
> > > intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
> > > <zhi.a.wang@intel.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>
> > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > operation
> > >
> > > On Thu, 3 Aug 2017 03:17:09 +0000
> > > "Zhang, Tina" <tina.zhang@intel.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org]
> > > > > On Behalf Of Alex Williamson
> > > > > Sent: Wednesday, August 2, 2017 5:18 AM
> > > > > To: Zhang, Tina <tina.zhang@intel.com>
> > > > > Cc: Tian, Kevin <kevin.tian@intel.com>; kraxel@redhat.com;
> > > > > intel- gfx@lists.freedesktop.org; Wang, Zhi A
> > > > > <zhi.a.wang@intel.com>; kwankhede@nvidia.com;
> > > > > dri-devel@lists.freedesktop.org; intel-gvt-
> > > > > dev@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>
> > > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > > > operation
> > > > >
> > > > > On Tue, 25 Jul 2017 17:28:18 +0800 Tina Zhang
> > > > > <tina.zhang@intel.com> wrote:
> > > > >
> > > > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user
> mode
> > > > > > query and get the plan and its related information.
> > > > > >
> > > > > > The dma-buf's life cycle is handled by user mode and tracked by kernel.
> > > > > > The returned fd in struct vfio_device_query_gfx_plane can be a
> > > > > > new fd or an old fd of a re-exported dma-buf. Host User mode
> > > > > > can check the value of fd and to see if it needs to create new
> > > > > > resource according to the new fd or just use the existed
> > > > > > resource related to the old
> > > fd.
> > > > > >
> > > > > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > > > > ---
> > > > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > > > > >  1 file changed, 28 insertions(+)
> > > > > >
> > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230 100644
> > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > > > > >
> > > > > >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE +
> > > > > 13)
> > > > > >
> > > > > > +/**
> > > > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE
> +
> > > 14,
> > > > > > +struct vfio_device_query_gfx_plane)
> > > > > > + *
> > > > > > + * Set the drm_plane_type and retrieve information about the gfx
> plane.
> > > > > > + *
> > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > + */
> > > > > > +struct vfio_device_gfx_plane_info {
> > > > > > +	__u32 argsz;
> > > > > > +	__u32 flags;
> > > > > > +	/* in */
> > > > > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_*
> */
> > > > > > +	/* out */
> > > > > > +	__u32 drm_format;	/* drm format of plane */
> > > > > > +	__u64 drm_format_mod;   /* tiled mode */
> > > > > > +	__u32 width;	/* width of plane */
> > > > > > +	__u32 height;	/* height of plane */
> > > > > > +	__u32 stride;	/* stride of plane */
> > > > > > +	__u32 size;	/* size of plane in bytes, align on page*/
> > > > > > +	__u32 x_pos;	/* horizontal position of cursor plane, upper
> left corner
> > > > > in pixels */
> > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left
> corner in
> > > > > lines*/
> > > > > > +	__u32 region_index;
> > > > > > +	__s32 fd;	/* dma-buf fd */
> > > > >
> > > > > How do I know which of these is valid, region_index or fd?  Can
> > > > > I ask for one vs the other?  What are the errno values to
> > > > > differentiate unsupported vs not initialized?  Is there a "probe"
> > > > > flag that I can use to determine what the device supports
> > > > > without allocating
> > > those resources yet?
> > > > Dma-buf won't use region_index, which means region_index is zero
> > > > all the
> > > time for dma-buf usage.
> > > > As we discussed, there won't be errno if not initialized, just keep all fields
> zero.
> > > > I will add the comments about these in the next version. Thanks
> > >
> > > Zero is a valid region index.
> > If zero is valid, can we find out other invalid value? How about 0xffffffff?
> 
> Unlikely, but also valid.  Isn't this why we have a flags field in the structure, so
> we don't need to rely on implicit values as invalid.
> Also, all of the previously discussed usage models needs to be documented,
> either here in the header or in a Documentation/ file.
According to the previously discussion, we also have the following propose:
enum vfio_device_gfx_type {
        VFIO_DEVICE_GFX_NONE,
        VFIO_DEVICE_GFX_DMABUF,
        VFIO_DEVICE_GFX_REGION,
};

struct vfio_device_gfx_query_caps {
        __u32 argsz;
        __u32 flags;
        enum vfio_device_gfx_type;
};
So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
User mode can query this before querying the plan info, and to see which usage
(dma-buf or region) is supported.
Does it still make sense?
Thanks.

Tina


> Thanks,
> 
> Alex
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

RE: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

After going through the previous discussions, here are some summaries may be related to the current discussion:
1. How does user mode figure the device capabilities between region and dma-buf?
VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
Otherwise, the mdev supports dma-buf.

2. For dma-buf, how to differentiate unsupported vs not initialized?
For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be returned. And -errno will return when meeting other failures, like -ENOMEM.
If the mdev is not initialized, there won't be any returned err. Just zero all the fields in structure vfio_device_gfx_plane_info.

3. The id field in structure vfio_device_gfx_plane_info
So far we haven't figured out the usage of this field for dma-buf usage. So, this field is changed to "region_index" and only used for region usage.
In previous discussions, we thought this "id" field might be used for both dma-buf and region usages.
So, we proposed some ways, like adding flags field to the structure. Another way to do it was to add this:
enum vfio_device_gfx_type {
         VFIO_DEVICE_GFX_NONE,
         VFIO_DEVICE_GFX_DMABUF,
         VFIO_DEVICE_GFX_REGION,
 };
 
 struct vfio_device_gfx_query_caps {
         __u32 argsz;
         __u32 flags;
         enum vfio_device_gfx_type;
 };
Obviously, we don't need to consider this again, unless we find the "region_index" means something for dma-buf usage.

Thanks.

BR,
Tina

> -----Original Message-----
> From: Zhang, Tina
> Sent: Monday, August 7, 2017 11:23 AM
> To: 'Alex Williamson' <alex.williamson@redhat.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org; kwankhede@nvidia.com; kraxel@redhat.com;
> intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>; Lv,
> Zhiyuan <zhiyuan.lv@intel.com>
> Subject: RE: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> 
> 
> > -----Original Message-----
> > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On
> > Behalf Of Alex Williamson
> > Sent: Thursday, August 3, 2017 10:43 PM
> > To: Zhang, Tina <tina.zhang@intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>;
> > intel-gfx@lists.freedesktop.org; dri- devel@lists.freedesktop.org;
> > kwankhede@nvidia.com; kraxel@redhat.com;
> > intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
> > <zhi.a.wang@intel.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>
> > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > operation
> >
> > On Thu, 3 Aug 2017 07:08:15 +0000
> > "Zhang, Tina" <tina.zhang@intel.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > Sent: Thursday, August 3, 2017 11:38 AM
> > > > To: Zhang, Tina <tina.zhang@intel.com>
> > > > Cc: Tian, Kevin <kevin.tian@intel.com>;
> > > > intel-gfx@lists.freedesktop.org; dri- devel@lists.freedesktop.org;
> > > > kwankhede@nvidia.com; kraxel@redhat.com;
> > > > intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
> > > > <zhi.a.wang@intel.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>
> > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf
> > > > operation
> > > >
> > > > On Thu, 3 Aug 2017 03:17:09 +0000
> > > > "Zhang, Tina" <tina.zhang@intel.com> wrote:
> > > >
> > > > > > -----Original Message-----
> > > > > > From: dri-devel
> > > > > > [mailto:dri-devel-bounces@lists.freedesktop.org]
> > > > > > On Behalf Of Alex Williamson
> > > > > > Sent: Wednesday, August 2, 2017 5:18 AM
> > > > > > To: Zhang, Tina <tina.zhang@intel.com>
> > > > > > Cc: Tian, Kevin <kevin.tian@intel.com>; kraxel@redhat.com;
> > > > > > intel- gfx@lists.freedesktop.org; Wang, Zhi A
> > > > > > <zhi.a.wang@intel.com>; kwankhede@nvidia.com;
> > > > > > dri-devel@lists.freedesktop.org; intel-gvt-
> > > > > > dev@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>
> > > > > > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display
> > > > > > dma-buf operation
> > > > > >
> > > > > > On Tue, 25 Jul 2017 17:28:18 +0800 Tina Zhang
> > > > > > <tina.zhang@intel.com> wrote:
> > > > > >
> > > > > > > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user
> > mode
> > > > > > > query and get the plan and its related information.
> > > > > > >
> > > > > > > The dma-buf's life cycle is handled by user mode and tracked by
> kernel.
> > > > > > > The returned fd in struct vfio_device_query_gfx_plane can be
> > > > > > > a new fd or an old fd of a re-exported dma-buf. Host User
> > > > > > > mode can check the value of fd and to see if it needs to
> > > > > > > create new resource according to the new fd or just use the
> > > > > > > existed resource related to the old
> > > > fd.
> > > > > > >
> > > > > > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > > > > > ---
> > > > > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 28 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230 100644
> > > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > > > > > >
> > > > > > >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE,
> VFIO_BASE +
> > > > > > 13)
> > > > > > >
> > > > > > > +/**
> > > > > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE,
> VFIO_BASE
> > +
> > > > 14,
> > > > > > > +struct vfio_device_query_gfx_plane)
> > > > > > > + *
> > > > > > > + * Set the drm_plane_type and retrieve information about
> > > > > > > +the gfx
> > plane.
> > > > > > > + *
> > > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > > + */
> > > > > > > +struct vfio_device_gfx_plane_info {
> > > > > > > +	__u32 argsz;
> > > > > > > +	__u32 flags;
> > > > > > > +	/* in */
> > > > > > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_*
> > */
> > > > > > > +	/* out */
> > > > > > > +	__u32 drm_format;	/* drm format of plane */
> > > > > > > +	__u64 drm_format_mod;   /* tiled mode */
> > > > > > > +	__u32 width;	/* width of plane */
> > > > > > > +	__u32 height;	/* height of plane */
> > > > > > > +	__u32 stride;	/* stride of plane */
> > > > > > > +	__u32 size;	/* size of plane in bytes, align on page*/
> > > > > > > +	__u32 x_pos;	/* horizontal position of cursor plane, upper
> > left corner
> > > > > > in pixels */
> > > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left
> > corner in
> > > > > > lines*/
> > > > > > > +	__u32 region_index;
> > > > > > > +	__s32 fd;	/* dma-buf fd */
> > > > > >
> > > > > > How do I know which of these is valid, region_index or fd?
> > > > > > Can I ask for one vs the other?  What are the errno values to
> > > > > > differentiate unsupported vs not initialized?  Is there a "probe"
> > > > > > flag that I can use to determine what the device supports
> > > > > > without allocating
> > > > those resources yet?
> > > > > Dma-buf won't use region_index, which means region_index is zero
> > > > > all the
> > > > time for dma-buf usage.
> > > > > As we discussed, there won't be errno if not initialized, just
> > > > > keep all fields
> > zero.
> > > > > I will add the comments about these in the next version. Thanks
> > > >
> > > > Zero is a valid region index.
> > > If zero is valid, can we find out other invalid value? How about 0xffffffff?
> >
> > Unlikely, but also valid.  Isn't this why we have a flags field in the
> > structure, so we don't need to rely on implicit values as invalid.
> > Also, all of the previously discussed usage models needs to be
> > documented, either here in the header or in a Documentation/ file.
> According to the previously discussion, we also have the following propose:
> enum vfio_device_gfx_type {
>         VFIO_DEVICE_GFX_NONE,
>         VFIO_DEVICE_GFX_DMABUF,
>         VFIO_DEVICE_GFX_REGION,
> };
> 
> struct vfio_device_gfx_query_caps {
>         __u32 argsz;
>         __u32 flags;
>         enum vfio_device_gfx_type;
> };
> So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
> User mode can query this before querying the plan info, and to see which usage
> (dma-buf or region) is supported.
> Does it still make sense?
> Thanks.
So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode whether the region case is using, instead of introducing a new ioctl.
Thanks

Tina
> 
> Tina
> 
> 
> > Thanks,
> >
> > Alex
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v13 6/7] drm/i915: Introduce GEM proxy

[Joonas Lahtinen]

On ti, 2017-07-25 at 17:28 +0800, Tina Zhang wrote:
> GEM proxy is a kind of GEM, whose backing physical memory is pinned
> and produced by guest VM and is used by host as read only. With GEM
> proxy, host is able to access guest physical memory through GEM object
> interface. As GEM proxy is such a special kind of GEM, a new flag
> I915_GEM_OBJECT_IS_PROXY is introduced to ban host from changing the
> backing storage of GEM proxy.
> 

It is a good idea to add a changelog here to indicate what was changed
since the last patch revision. It'll be easier for the reviewer to spot
the differences.

It's also a good idea to add Cc: line for somebody who provided
previous review, this will, too, allow spotting the patch quicker.

> Signed-off-by: Tina Zhang <tina.zhang@intel.com>

<SNIP>

> @@ -1730,7 +1744,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>  	 */
>  	if (!obj->base.filp) {
>  		i915_gem_object_put(obj);
> -		return -EINVAL;
> +		return -EPERM;
>  	}

This hunk should be separate bugfix patch, but other than that the
patch looks OK to me.

I'll let Chris comment if he feels previous comments were adequately
addressed.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Alex Williamson]

On Mon, 7 Aug 2017 08:11:43 +0000
"Zhang, Tina" <tina.zhang@intel.com> wrote:

> After going through the previous discussions, here are some summaries may be related to the current discussion:
> 1. How does user mode figure the device capabilities between region and dma-buf?
> VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
> Otherwise, the mdev supports dma-buf.

Why do we need to make this assumption?  What happens when dma-buf is
superseded?  What happens if a device supports both dma-buf and
regions?  We have a flags field in vfio_device_gfx_plane_info, doesn't
it make sense to use it to identify which field, between region_index
and fd, is valid?  We could even put region_index and fd into a union
with the flag bits indicating how to interpret the union, but I'm not
sure everyone was onboard with this idea.  Seems like a waste of 4
bytes not to do that though.

Thinking further, is the user ever in a situation where they query the
graphics plane info and can handle either a dma-buf or a region?  It
seems more likely that the user needs to know early on which is
supported and would then require that they continue to see compatible
plane information...  Should the user then be able to specify whether
they want a dma-buf or a region?  Perhaps these flag bits are actually
input and the return should be -errno if the driver cannot produce
something compatible.

Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In
this initial implementation, DMABUF or REGION would always be set by
the user to request that type of interface.  Additionally, the QUERY
bit could be set to probe compatibility, thus if PROBE and REGION are
set, the vendor driver would return success only if it supports the
region based interface.  If PROBE and DMABUF are set, the vendor driver
returns success only if the dma-buf based interface is supported.  The
value of the remainder of the structure is undefined for PROBE.
Additionally setting both DMABUF and REGION is invalid.  Undefined
flags bits must be validated as zero by the drivers for future use
(thus if we later define DMABUFv2, an older driver should
automatically return -errno when probed or requested).

It seems like this handles all the cases, the user can ask what's
supported and specifies the interface they want on every call.  The
user therefore can also choose between region_index and fd and we can
make that a union.

> 2. For dma-buf, how to differentiate unsupported vs not initialized?
> For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be returned. And -errno will return when meeting other failures, like -ENOMEM.
> If the mdev is not initialized, there won't be any returned err. Just zero all the fields in structure vfio_device_gfx_plane_info.

So we're relying on special values again :-\  For which fields is zero
not a valid value?  I prefer the probe interface above unless there are
better ideas.
 
> 3. The id field in structure vfio_device_gfx_plane_info
> So far we haven't figured out the usage of this field for dma-buf usage. So, this field is changed to "region_index" and only used for region usage.
> In previous discussions, we thought this "id" field might be used for both dma-buf and region usages.
> So, we proposed some ways, like adding flags field to the structure. Another way to do it was to add this:
> enum vfio_device_gfx_type {
>          VFIO_DEVICE_GFX_NONE,
>          VFIO_DEVICE_GFX_DMABUF,
>          VFIO_DEVICE_GFX_REGION,
>  };
>  
>  struct vfio_device_gfx_query_caps {
>          __u32 argsz;
>          __u32 flags;
>          enum vfio_device_gfx_type;
>  };
> Obviously, we don't need to consider this again, unless we find the "region_index" means something for dma-buf usage.

The usefulness of this ioctl seems really limited and once again we get
into a situation where having two ioctls leaves doubt whether this is
describing the current plane state.  Thanks,

Alex

> > > > > > > >  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230 100644
> > > > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > > > > > > >
> > > > > > > >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE,  
> > VFIO_BASE +  
> > > > > > > 13)  
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE,  
> > VFIO_BASE  
> > > +  
> > > > > 14,  
> > > > > > > > +struct vfio_device_query_gfx_plane)
> > > > > > > > + *
> > > > > > > > + * Set the drm_plane_type and retrieve information about
> > > > > > > > +the gfx  
> > > plane.  
> > > > > > > > + *
> > > > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > > > + */
> > > > > > > > +struct vfio_device_gfx_plane_info {
> > > > > > > > +	__u32 argsz;
> > > > > > > > +	__u32 flags;
> > > > > > > > +	/* in */
> > > > > > > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_*  
> > > */  
> > > > > > > > +	/* out */
> > > > > > > > +	__u32 drm_format;	/* drm format of plane */
> > > > > > > > +	__u64 drm_format_mod;   /* tiled mode */
> > > > > > > > +	__u32 width;	/* width of plane */
> > > > > > > > +	__u32 height;	/* height of plane */
> > > > > > > > +	__u32 stride;	/* stride of plane */
> > > > > > > > +	__u32 size;	/* size of plane in bytes, align on page*/
> > > > > > > > +	__u32 x_pos;	/* horizontal position of cursor plane, upper  
> > > left corner  
> > > > > > > in pixels */  
> > > > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left  
> > > corner in  
> > > > > > > lines*/  
> > > > > > > > +	__u32 region_index;
> > > > > > > > +	__s32 fd;	/* dma-buf fd */  
> > > > > > >
> > > > > > > How do I know which of these is valid, region_index or fd?
> > > > > > > Can I ask for one vs the other?  What are the errno values to
> > > > > > > differentiate unsupported vs not initialized?  Is there a "probe"
> > > > > > > flag that I can use to determine what the device supports
> > > > > > > without allocating  
> > > > > those resources yet?  
> > > > > > Dma-buf won't use region_index, which means region_index is zero
> > > > > > all the  
> > > > > time for dma-buf usage.  
> > > > > > As we discussed, there won't be errno if not initialized, just
> > > > > > keep all fields  
> > > zero.  
> > > > > > I will add the comments about these in the next version. Thanks  
> > > > >
> > > > > Zero is a valid region index.  
> > > > If zero is valid, can we find out other invalid value? How about 0xffffffff?  
> > >
> > > Unlikely, but also valid.  Isn't this why we have a flags field in the
> > > structure, so we don't need to rely on implicit values as invalid.
> > > Also, all of the previously discussed usage models needs to be
> > > documented, either here in the header or in a Documentation/ file.  
> > According to the previously discussion, we also have the following propose:
> > enum vfio_device_gfx_type {
> >         VFIO_DEVICE_GFX_NONE,
> >         VFIO_DEVICE_GFX_DMABUF,
> >         VFIO_DEVICE_GFX_REGION,
> > };
> > 
> > struct vfio_device_gfx_query_caps {
> >         __u32 argsz;
> >         __u32 flags;
> >         enum vfio_device_gfx_type;
> > };
> > So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
> > User mode can query this before querying the plan info, and to see which usage
> > (dma-buf or region) is supported.
> > Does it still make sense?
> > Thanks.  
> So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode whether the region case is using, instead of introducing a new ioctl.
> Thanks
> 
> Tina
> > 
> > Tina
> > 
> >   
> > > Thanks,
> > >
> > > Alex
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel  

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

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Kirti Wankhede]

On 8/7/2017 11:13 PM, Alex Williamson wrote:
> On Mon, 7 Aug 2017 08:11:43 +0000
> "Zhang, Tina" <tina.zhang@intel.com> wrote:
> 
>> After going through the previous discussions, here are some summaries may be related to the current discussion:
>> 1. How does user mode figure the device capabilities between region and dma-buf?
>> VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
>> Otherwise, the mdev supports dma-buf.
> 
> Why do we need to make this assumption?

Right, we should not make such assumption. Vendor driver might not
support both or disable console vnc ( for example, for performance
testing console VNC need to be disabled)

>  What happens when dma-buf is
> superseded?  What happens if a device supports both dma-buf and
> regions?  We have a flags field in vfio_device_gfx_plane_info, doesn't
> it make sense to use it to identify which field, between region_index
> and fd, is valid?  We could even put region_index and fd into a union
> with the flag bits indicating how to interpret the union, but I'm not
> sure everyone was onboard with this idea.  Seems like a waste of 4
> bytes not to do that though.
> 

Agree.

> Thinking further, is the user ever in a situation where they query the
> graphics plane info and can handle either a dma-buf or a region?  It
> seems more likely that the user needs to know early on which is
> supported and would then require that they continue to see compatible
> plane information...  Should the user then be able to specify whether
> they want a dma-buf or a region?  Perhaps these flag bits are actually
> input and the return should be -errno if the driver cannot produce
> something compatible.
> 
> Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In
> this initial implementation, DMABUF or REGION would always be set by
> the user to request that type of interface.  Additionally, the QUERY
> bit could be set to probe compatibility, thus if PROBE and REGION are
> set, the vendor driver would return success only if it supports the
> region based interface.  If PROBE and DMABUF are set, the vendor driver
> returns success only if the dma-buf based interface is supported.  The
> value of the remainder of the structure is undefined for PROBE.
> Additionally setting both DMABUF and REGION is invalid.  Undefined
> flags bits must be validated as zero by the drivers for future use
> (thus if we later define DMABUFv2, an older driver should
> automatically return -errno when probed or requested).
> 

Are you saying all of this to be part of VFIO_DEVICE_QUERY_GFX_PLANE ioctl?

Let me summarize what we need, taking QEMU as reference:
1. From vfio_initfn(), for REGION case, get region info:
vfio_get_dev_region_info(.., VFIO_REGION_SUBTYPE_CONSOLE_REGION,
&vdev->console_opregion)

If above return success, setup console REGION and mmap.
I don't know what is required for DMABUF at this moment.

If console VNC is supported either DMABUF or REGION, initialize console
and register its callback operations:

static const GraphicHwOps vfio_console_ops = {
    .gfx_update  = vfio_console_update_display,
};

vdev->console = graphic_console_init(DEVICE(vdev), 0, &vfio_console_ops,
vdev);

2. On above console registration, vfio_console_update_display() gets
called for each refresh cycle of console. In that:
- call ioctl(VFIO_DEVICE_QUERY_GFX_PLANE)
- if (queried size > 0), update QEMU console surface (for REGION case
read mmaped region, for DMABUF read surface using fd)


Alex, in your proposal above, my understanding is
ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with PROBE flag should be called in
step #1.
In step #2, ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) will be without PROBE
flag, but either DMABUF or REGION flag based on what is returned as
supported by vendor driver in step #1. Is that correct?


> It seems like this handles all the cases, the user can ask what's
> supported and specifies the interface they want on every call.  The
> user therefore can also choose between region_index and fd and we can
> make that a union.
>
>> 2. For dma-buf, how to differentiate unsupported vs not initialized?
>> For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be returned. And -errno will return when meeting other failures, like -ENOMEM.
>> If the mdev is not initialized, there won't be any returned err. Just zero all the fields in structure vfio_device_gfx_plane_info.
> 
> So we're relying on special values again :-\  For which fields is zero
> not a valid value?  I prefer the probe interface above unless there are
> better ideas.
> 

PROBE will be called during vdev initialization and after that
vfio_console_update_display() gets called at every console refresh
cycle. But until driver in guest is loaded, mmaped buffer/ DMABUF will
not be populated with correct surface data. In that case for QUERY,
vendor driver should return (atleast) size=0 which means there is
nothing to copy. Once guest driver is loaded and surface is created by
guest driver, QUERY interface should return valid size.

Thanks,
Kirti

>> 3. The id field in structure vfio_device_gfx_plane_info
>> So far we haven't figured out the usage of this field for dma-buf usage. So, this field is changed to "region_index" and only used for region usage.
>> In previous discussions, we thought this "id" field might be used for both dma-buf and region usages.
>> So, we proposed some ways, like adding flags field to the structure. Another way to do it was to add this:
>> enum vfio_device_gfx_type {
>>          VFIO_DEVICE_GFX_NONE,
>>          VFIO_DEVICE_GFX_DMABUF,
>>          VFIO_DEVICE_GFX_REGION,
>>  };
>>  
>>  struct vfio_device_gfx_query_caps {
>>          __u32 argsz;
>>          __u32 flags;
>>          enum vfio_device_gfx_type;
>>  };
>> Obviously, we don't need to consider this again, unless we find the "region_index" means something for dma-buf usage.
> 
> The usefulness of this ioctl seems really limited and once again we get
> into a situation where having two ioctls leaves doubt whether this is
> describing the current plane state.  Thanks,
> 
> Alex
> 
>>>>>>>>>  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
>>>>>>>>>  1 file changed, 28 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/include/uapi/linux/vfio.h
>>>>>>>>> b/include/uapi/linux/vfio.h index ae46105..827a230 100644
>>>>>>>>> --- a/include/uapi/linux/vfio.h
>>>>>>>>> +++ b/include/uapi/linux/vfio.h
>>>>>>>>> @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
>>>>>>>>>
>>>>>>>>>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE,  
>>> VFIO_BASE +  
>>>>>>>> 13)  
>>>>>>>>>
>>>>>>>>> +/**
>>>>>>>>> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE,  
>>> VFIO_BASE  
>>>> +  
>>>>>> 14,  
>>>>>>>>> +struct vfio_device_query_gfx_plane)
>>>>>>>>> + *
>>>>>>>>> + * Set the drm_plane_type and retrieve information about
>>>>>>>>> +the gfx  
>>>> plane.  
>>>>>>>>> + *
>>>>>>>>> + * Return: 0 on success, -errno on failure.
>>>>>>>>> + */
>>>>>>>>> +struct vfio_device_gfx_plane_info {
>>>>>>>>> +	__u32 argsz;
>>>>>>>>> +	__u32 flags;
>>>>>>>>> +	/* in */
>>>>>>>>> +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_*  
>>>> */  
>>>>>>>>> +	/* out */
>>>>>>>>> +	__u32 drm_format;	/* drm format of plane */
>>>>>>>>> +	__u64 drm_format_mod;   /* tiled mode */
>>>>>>>>> +	__u32 width;	/* width of plane */
>>>>>>>>> +	__u32 height;	/* height of plane */
>>>>>>>>> +	__u32 stride;	/* stride of plane */
>>>>>>>>> +	__u32 size;	/* size of plane in bytes, align on page*/
>>>>>>>>> +	__u32 x_pos;	/* horizontal position of cursor plane, upper  
>>>> left corner  
>>>>>>>> in pixels */  
>>>>>>>>> +	__u32 y_pos;	/* vertical position of cursor plane, upper left  
>>>> corner in  
>>>>>>>> lines*/  
>>>>>>>>> +	__u32 region_index;
>>>>>>>>> +	__s32 fd;	/* dma-buf fd */  
>>>>>>>>
>>>>>>>> How do I know which of these is valid, region_index or fd?
>>>>>>>> Can I ask for one vs the other?  What are the errno values to
>>>>>>>> differentiate unsupported vs not initialized?  Is there a "probe"
>>>>>>>> flag that I can use to determine what the device supports
>>>>>>>> without allocating  
>>>>>> those resources yet?  
>>>>>>> Dma-buf won't use region_index, which means region_index is zero
>>>>>>> all the  
>>>>>> time for dma-buf usage.  
>>>>>>> As we discussed, there won't be errno if not initialized, just
>>>>>>> keep all fields  
>>>> zero.  
>>>>>>> I will add the comments about these in the next version. Thanks  
>>>>>>
>>>>>> Zero is a valid region index.  
>>>>> If zero is valid, can we find out other invalid value? How about 0xffffffff?  
>>>>
>>>> Unlikely, but also valid.  Isn't this why we have a flags field in the
>>>> structure, so we don't need to rely on implicit values as invalid.
>>>> Also, all of the previously discussed usage models needs to be
>>>> documented, either here in the header or in a Documentation/ file.  
>>> According to the previously discussion, we also have the following propose:
>>> enum vfio_device_gfx_type {
>>>         VFIO_DEVICE_GFX_NONE,
>>>         VFIO_DEVICE_GFX_DMABUF,
>>>         VFIO_DEVICE_GFX_REGION,
>>> };
>>>
>>> struct vfio_device_gfx_query_caps {
>>>         __u32 argsz;
>>>         __u32 flags;
>>>         enum vfio_device_gfx_type;
>>> };
>>> So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
>>> User mode can query this before querying the plan info, and to see which usage
>>> (dma-buf or region) is supported.
>>> Does it still make sense?
>>> Thanks.  
>> So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode whether the region case is using, instead of introducing a new ioctl.
>> Thanks
>>
>> Tina
>>>
>>> Tina
>>>
>>>   
>>>> Thanks,
>>>>
>>>> Alex
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel  
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Alex Williamson]

On Tue, 8 Aug 2017 14:18:07 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 8/7/2017 11:13 PM, Alex Williamson wrote:
> > On Mon, 7 Aug 2017 08:11:43 +0000
> > "Zhang, Tina" <tina.zhang@intel.com> wrote:
> >   
> >> After going through the previous discussions, here are some summaries may be related to the current discussion:
> >> 1. How does user mode figure the device capabilities between region and dma-buf?
> >> VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
> >> Otherwise, the mdev supports dma-buf.  
> > 
> > Why do we need to make this assumption?  
> 
> Right, we should not make such assumption. Vendor driver might not
> support both or disable console vnc ( for example, for performance
> testing console VNC need to be disabled)
> 
> >  What happens when dma-buf is
> > superseded?  What happens if a device supports both dma-buf and
> > regions?  We have a flags field in vfio_device_gfx_plane_info, doesn't
> > it make sense to use it to identify which field, between region_index
> > and fd, is valid?  We could even put region_index and fd into a union
> > with the flag bits indicating how to interpret the union, but I'm not
> > sure everyone was onboard with this idea.  Seems like a waste of 4
> > bytes not to do that though.
> >   
> 
> Agree.
> 
> > Thinking further, is the user ever in a situation where they query the
> > graphics plane info and can handle either a dma-buf or a region?  It
> > seems more likely that the user needs to know early on which is
> > supported and would then require that they continue to see compatible
> > plane information...  Should the user then be able to specify whether
> > they want a dma-buf or a region?  Perhaps these flag bits are actually
> > input and the return should be -errno if the driver cannot produce
> > something compatible.
> > 
> > Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In
> > this initial implementation, DMABUF or REGION would always be set by
> > the user to request that type of interface.  Additionally, the QUERY
> > bit could be set to probe compatibility, thus if PROBE and REGION are
> > set, the vendor driver would return success only if it supports the
> > region based interface.  If PROBE and DMABUF are set, the vendor driver
> > returns success only if the dma-buf based interface is supported.  The
> > value of the remainder of the structure is undefined for PROBE.
> > Additionally setting both DMABUF and REGION is invalid.  Undefined
> > flags bits must be validated as zero by the drivers for future use
> > (thus if we later define DMABUFv2, an older driver should
> > automatically return -errno when probed or requested).
> >   
> 
> Are you saying all of this to be part of VFIO_DEVICE_QUERY_GFX_PLANE ioctl?
> 
> Let me summarize what we need, taking QEMU as reference:
> 1. From vfio_initfn(), for REGION case, get region info:
> vfio_get_dev_region_info(.., VFIO_REGION_SUBTYPE_CONSOLE_REGION,
> &vdev->console_opregion)
> 
> If above return success, setup console REGION and mmap.
> I don't know what is required for DMABUF at this moment.
> 
> If console VNC is supported either DMABUF or REGION, initialize console
> and register its callback operations:
> 
> static const GraphicHwOps vfio_console_ops = {
>     .gfx_update  = vfio_console_update_display,
> };
> 
> vdev->console = graphic_console_init(DEVICE(vdev), 0, &vfio_console_ops,
> vdev);

I might structure it that vfio_initfn() would call
ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with the PROBE bit set with either
DMABUF or REGION set as the interface type in the flags field.  If
REGION is the probed interface and success is returned, then QEMU might
go look for regions of the appropriate type, however the
vfio_device_gfx_plane_info structure is canonical source for the region
index, so QEMU would probably be wise to use that and only use the
region type for consistency testing.

> 2. On above console registration, vfio_console_update_display() gets
> called for each refresh cycle of console. In that:
> - call ioctl(VFIO_DEVICE_QUERY_GFX_PLANE)
> - if (queried size > 0), update QEMU console surface (for REGION case
> read mmaped region, for DMABUF read surface using fd)

The ioctl would be called with REGION or DMABUF based on the initial
probe call, ex. we probed that REGION is supported and now we continue
to ask for region based updates.  QEMU would need to verify the region
index matches that already mapped, remapping a different region if
necessary, and interpret the graphics parameters to provide the screen
update.
 
> Alex, in your proposal above, my understanding is
> ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with PROBE flag should be called in
> step #1.
> In step #2, ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) will be without PROBE
> flag, but either DMABUF or REGION flag based on what is returned as
> supported by vendor driver in step #1. Is that correct?

Yes, that's the idea.  Does it make sense/provide value?

> > It seems like this handles all the cases, the user can ask what's
> > supported and specifies the interface they want on every call.  The
> > user therefore can also choose between region_index and fd and we can
> > make that a union.
> >  
> >> 2. For dma-buf, how to differentiate unsupported vs not initialized?
> >> For dma-buf, when the mdev doesn't support some arguments, -EINVAL will be returned. And -errno will return when meeting other failures, like -ENOMEM.
> >> If the mdev is not initialized, there won't be any returned err. Just zero all the fields in structure vfio_device_gfx_plane_info.  
> > 
> > So we're relying on special values again :-\  For which fields is zero
> > not a valid value?  I prefer the probe interface above unless there are
> > better ideas.
> >   
> 
> PROBE will be called during vdev initialization and after that
> vfio_console_update_display() gets called at every console refresh
> cycle. But until driver in guest is loaded, mmaped buffer/ DMABUF will
> not be populated with correct surface data. In that case for QUERY,
> vendor driver should return (atleast) size=0 which means there is
> nothing to copy. Once guest driver is loaded and surface is created by
> guest driver, QUERY interface should return valid size.

That seems reasonable and well specified.  Thanks,

Alex

> >> 3. The id field in structure vfio_device_gfx_plane_info
> >> So far we haven't figured out the usage of this field for dma-buf usage. So, this field is changed to "region_index" and only used for region usage.
> >> In previous discussions, we thought this "id" field might be used for both dma-buf and region usages.
> >> So, we proposed some ways, like adding flags field to the structure. Another way to do it was to add this:
> >> enum vfio_device_gfx_type {
> >>          VFIO_DEVICE_GFX_NONE,
> >>          VFIO_DEVICE_GFX_DMABUF,
> >>          VFIO_DEVICE_GFX_REGION,
> >>  };
> >>  
> >>  struct vfio_device_gfx_query_caps {
> >>          __u32 argsz;
> >>          __u32 flags;
> >>          enum vfio_device_gfx_type;
> >>  };
> >> Obviously, we don't need to consider this again, unless we find the "region_index" means something for dma-buf usage.  
> > 
> > The usefulness of this ioctl seems really limited and once again we get
> > into a situation where having two ioctls leaves doubt whether this is
> > describing the current plane state.  Thanks,
> > 
> > Alex
> >   
> >>>>>>>>>  include/uapi/linux/vfio.h | 28 ++++++++++++++++++++++++++++
> >>>>>>>>>  1 file changed, 28 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/include/uapi/linux/vfio.h
> >>>>>>>>> b/include/uapi/linux/vfio.h index ae46105..827a230 100644
> >>>>>>>>> --- a/include/uapi/linux/vfio.h
> >>>>>>>>> +++ b/include/uapi/linux/vfio.h
> >>>>>>>>> @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> >>>>>>>>>
> >>>>>>>>>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE,    
> >>> VFIO_BASE +    
> >>>>>>>> 13)    
> >>>>>>>>>
> >>>>>>>>> +/**
> >>>>>>>>> + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE,    
> >>> VFIO_BASE    
> >>>> +    
> >>>>>> 14,    
> >>>>>>>>> +struct vfio_device_query_gfx_plane)
> >>>>>>>>> + *
> >>>>>>>>> + * Set the drm_plane_type and retrieve information about
> >>>>>>>>> +the gfx    
> >>>> plane.    
> >>>>>>>>> + *
> >>>>>>>>> + * Return: 0 on success, -errno on failure.
> >>>>>>>>> + */
> >>>>>>>>> +struct vfio_device_gfx_plane_info {
> >>>>>>>>> +	__u32 argsz;
> >>>>>>>>> +	__u32 flags;
> >>>>>>>>> +	/* in */
> >>>>>>>>> +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_*    
> >>>> */    
> >>>>>>>>> +	/* out */
> >>>>>>>>> +	__u32 drm_format;	/* drm format of plane */
> >>>>>>>>> +	__u64 drm_format_mod;   /* tiled mode */
> >>>>>>>>> +	__u32 width;	/* width of plane */
> >>>>>>>>> +	__u32 height;	/* height of plane */
> >>>>>>>>> +	__u32 stride;	/* stride of plane */
> >>>>>>>>> +	__u32 size;	/* size of plane in bytes, align on page*/
> >>>>>>>>> +	__u32 x_pos;	/* horizontal position of cursor plane, upper    
> >>>> left corner    
> >>>>>>>> in pixels */    
> >>>>>>>>> +	__u32 y_pos;	/* vertical position of cursor plane, upper left    
> >>>> corner in    
> >>>>>>>> lines*/    
> >>>>>>>>> +	__u32 region_index;
> >>>>>>>>> +	__s32 fd;	/* dma-buf fd */    
> >>>>>>>>
> >>>>>>>> How do I know which of these is valid, region_index or fd?
> >>>>>>>> Can I ask for one vs the other?  What are the errno values to
> >>>>>>>> differentiate unsupported vs not initialized?  Is there a "probe"
> >>>>>>>> flag that I can use to determine what the device supports
> >>>>>>>> without allocating    
> >>>>>> those resources yet?    
> >>>>>>> Dma-buf won't use region_index, which means region_index is zero
> >>>>>>> all the    
> >>>>>> time for dma-buf usage.    
> >>>>>>> As we discussed, there won't be errno if not initialized, just
> >>>>>>> keep all fields    
> >>>> zero.    
> >>>>>>> I will add the comments about these in the next version. Thanks    
> >>>>>>
> >>>>>> Zero is a valid region index.    
> >>>>> If zero is valid, can we find out other invalid value? How about 0xffffffff?    
> >>>>
> >>>> Unlikely, but also valid.  Isn't this why we have a flags field in the
> >>>> structure, so we don't need to rely on implicit values as invalid.
> >>>> Also, all of the previously discussed usage models needs to be
> >>>> documented, either here in the header or in a Documentation/ file.    
> >>> According to the previously discussion, we also have the following propose:
> >>> enum vfio_device_gfx_type {
> >>>         VFIO_DEVICE_GFX_NONE,
> >>>         VFIO_DEVICE_GFX_DMABUF,
> >>>         VFIO_DEVICE_GFX_REGION,
> >>> };
> >>>
> >>> struct vfio_device_gfx_query_caps {
> >>>         __u32 argsz;
> >>>         __u32 flags;
> >>>         enum vfio_device_gfx_type;
> >>> };
> >>> So, we may need to add one more ioctl, e.g. VFIO_DEVICE_QUERY_GFX_CAPS.
> >>> User mode can query this before querying the plan info, and to see which usage
> >>> (dma-buf or region) is supported.
> >>> Does it still make sense?
> >>> Thanks.    
> >> So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode whether the region case is using, instead of introducing a new ioctl.
> >> Thanks
> >>
> >> Tina  
> >>>
> >>> Tina
> >>>
> >>>     
> >>>> Thanks,
> >>>>
> >>>> Alex
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel    
> >   

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

RE: [Intel-gfx] [PATCH v13 6/7] drm/i915: Introduce GEM proxy

[Zhang, Tina]

> -----Original Message-----
> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
> Joonas Lahtinen
> Sent: Monday, August 7, 2017 4:24 PM
> To: Zhang, Tina <tina.zhang@intel.com>; intel-gfx@lists.freedesktop.org; intel-
> gvt-dev@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> ville.syrjala@linux.intel.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>;
> alex.williamson@redhat.com; kraxel@redhat.com; chris@chris-wilson.co.uk;
> daniel@ffwll.ch; kwankhede@nvidia.com; Tian, Kevin <kevin.tian@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v13 6/7] drm/i915: Introduce GEM proxy
> 
> On ti, 2017-07-25 at 17:28 +0800, Tina Zhang wrote:
> > GEM proxy is a kind of GEM, whose backing physical memory is pinned
> > and produced by guest VM and is used by host as read only. With GEM
> > proxy, host is able to access guest physical memory through GEM object
> > interface. As GEM proxy is such a special kind of GEM, a new flag
> > I915_GEM_OBJECT_IS_PROXY is introduced to ban host from changing the
> > backing storage of GEM proxy.
> >
> 
> It is a good idea to add a changelog here to indicate what was changed since the
> last patch revision. It'll be easier for the reviewer to spot the differences.
> 
> It's also a good idea to add Cc: line for somebody who provided previous review,
> this will, too, allow spotting the patch quicker.
Indeed. Thanks.

Tina
> 
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> 
> <SNIP>
> 
> > @@ -1730,7 +1744,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev,
> void *data,
> >  	 */
> >  	if (!obj->base.filp) {
> >  		i915_gem_object_put(obj);
> > -		return -EINVAL;
> > +		return -EPERM;
> >  	}
> 
> This hunk should be separate bugfix patch, but other than that the patch looks
> OK to me.
Thanks. I will send this patch separately.

Tina
> 
> I'll let Chris comment if he feels previous comments were adequately addressed.
> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Alex Williamson
> Sent: Tuesday, August 8, 2017 1:43 AM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org; kwankhede@nvidia.com; kraxel@redhat.com;
> intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>; Lv,
> Zhiyuan <zhiyuan.lv@intel.com>
> Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> 
> On Mon, 7 Aug 2017 08:11:43 +0000
> "Zhang, Tina" <tina.zhang@intel.com> wrote:
> 
> > After going through the previous discussions, here are some summaries may
> be related to the current discussion:
> > 1. How does user mode figure the device capabilities between region and
> dma-buf?
> > VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case.
> > Otherwise, the mdev supports dma-buf.
> 
> Why do we need to make this assumption?  What happens when dma-buf is
> superseded?  What happens if a device supports both dma-buf and regions?
> We have a flags field in vfio_device_gfx_plane_info, doesn't it make sense to use
> it to identify which field, between region_index and fd, is valid?  We could even
> put region_index and fd into a union with the flag bits indicating how to
> interpret the union, but I'm not sure everyone was onboard with this idea.
> Seems like a waste of 4 bytes not to do that though.
It seems we discussed this idea before:
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001304.html
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001333.html
Thanks.

Tina
> 
> Thinking further, is the user ever in a situation where they query the graphics
> plane info and can handle either a dma-buf or a region?  It seems more likely
> that the user needs to know early on which is supported and would then require
> that they continue to see compatible plane information...  Should the user then
> be able to specify whether they want a dma-buf or a region?  Perhaps these flag
> bits are actually input and the return should be -errno if the driver cannot
> produce something compatible.
From the previously discussion, it seems user space workflow will look quite different
for these two cases. So once user space finds out which case is supported, it just uses
that case, and won't change it.

Meanwhile, I'm not sure whether there will be a mdev would like to support both region
and dma-buf cases. In my opinion, either region or dma-buf is supported by one mdev. (Yeah,
agree, there may be other cases in future)
It's like we want to propose a general interface used to share guest's buffer with host. And the
general interface, so far, has two choice: region and dma-buf. So each mdev likes this interface
can implement one kind of it and gets the benefit from the general interface.
So, if we think about this, the difference in user mode should be as little as possible.
Thanks.

Tina
> 
> Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In this
> initial implementation, DMABUF or REGION would always be set by the user to
> request that type of interface.  Additionally, the QUERY bit could be set to probe
> compatibility, thus if PROBE and REGION are set, the vendor driver would return
> success only if it supports the region based interface.  If PROBE and DMABUF are
> set, the vendor driver returns success only if the dma-buf based interface is
> supported.  The value of the remainder of the structure is undefined for PROBE.
> Additionally setting both DMABUF and REGION is invalid.  Undefined flags bits
> must be validated as zero by the drivers for future use (thus if we later define
> DMABUFv2, an older driver should automatically return -errno when probed or
> requested).
> 
> It seems like this handles all the cases, the user can ask what's supported and
> specifies the interface they want on every call.  The user therefore can also
> choose between region_index and fd and we can make that a union.
Agree, that's a good proposal, which can handle all the cases.
I'm just not sure about the usage case of "on every call". In previous discussion, it seems we think static is enough.
Thanks.

Tina

> 
> > 2. For dma-buf, how to differentiate unsupported vs not initialized?
> > For dma-buf, when the mdev doesn't support some arguments, -EINVAL will
> be returned. And -errno will return when meeting other failures, like -ENOMEM.
> > If the mdev is not initialized, there won't be any returned err. Just zero all the
> fields in structure vfio_device_gfx_plane_info.
> 
> So we're relying on special values again :-\  For which fields is zero not a valid
> value?  I prefer the probe interface above unless there are better ideas.
> 
> > 3. The id field in structure vfio_device_gfx_plane_info So far we
> > haven't figured out the usage of this field for dma-buf usage. So, this field is
> changed to "region_index" and only used for region usage.
> > In previous discussions, we thought this "id" field might be used for both dma-
> buf and region usages.
> > So, we proposed some ways, like adding flags field to the structure. Another
> way to do it was to add this:
> > enum vfio_device_gfx_type {
> >          VFIO_DEVICE_GFX_NONE,
> >          VFIO_DEVICE_GFX_DMABUF,
> >          VFIO_DEVICE_GFX_REGION,
> >  };
> >
> >  struct vfio_device_gfx_query_caps {
> >          __u32 argsz;
> >          __u32 flags;
> >          enum vfio_device_gfx_type;
> >  };
> > Obviously, we don't need to consider this again, unless we find the
> "region_index" means something for dma-buf usage.
> 
> The usefulness of this ioctl seems really limited and once again we get into a
> situation where having two ioctls leaves doubt whether this is describing the
> current plane state.  Thanks,
> 
> Alex
> 
> > > > > > > > >  include/uapi/linux/vfio.h | 28
> > > > > > > > > ++++++++++++++++++++++++++++
> > > > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230
> > > > > > > > > 100644
> > > > > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > > > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > > > > > > > >
> > > > > > > > >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE,
> > > VFIO_BASE +
> > > > > > > > 13)
> > > > > > > > >
> > > > > > > > > +/**
> > > > > > > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE,
> > > VFIO_BASE
> > > > +
> > > > > > 14,
> > > > > > > > > +struct vfio_device_query_gfx_plane)
> > > > > > > > > + *
> > > > > > > > > + * Set the drm_plane_type and retrieve information
> > > > > > > > > +about the gfx
> > > > plane.
> > > > > > > > > + *
> > > > > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > > > > + */
> > > > > > > > > +struct vfio_device_gfx_plane_info {
> > > > > > > > > +	__u32 argsz;
> > > > > > > > > +	__u32 flags;
> > > > > > > > > +	/* in */
> > > > > > > > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_*
> > > > */
> > > > > > > > > +	/* out */
> > > > > > > > > +	__u32 drm_format;	/* drm format of plane */
> > > > > > > > > +	__u64 drm_format_mod;   /* tiled mode */
> > > > > > > > > +	__u32 width;	/* width of plane */
> > > > > > > > > +	__u32 height;	/* height of plane */
> > > > > > > > > +	__u32 stride;	/* stride of plane */
> > > > > > > > > +	__u32 size;	/* size of plane in bytes, align on page*/
> > > > > > > > > +	__u32 x_pos;	/* horizontal position of cursor plane, upper
> > > > left corner
> > > > > > > > in pixels */
> > > > > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left
> > > > corner in
> > > > > > > > lines*/
> > > > > > > > > +	__u32 region_index;
> > > > > > > > > +	__s32 fd;	/* dma-buf fd */
> > > > > > > >
> > > > > > > > How do I know which of these is valid, region_index or fd?
> > > > > > > > Can I ask for one vs the other?  What are the errno values
> > > > > > > > to differentiate unsupported vs not initialized?  Is there a "probe"
> > > > > > > > flag that I can use to determine what the device supports
> > > > > > > > without allocating
> > > > > > those resources yet?
> > > > > > > Dma-buf won't use region_index, which means region_index is
> > > > > > > zero all the
> > > > > > time for dma-buf usage.
> > > > > > > As we discussed, there won't be errno if not initialized,
> > > > > > > just keep all fields
> > > > zero.
> > > > > > > I will add the comments about these in the next version.
> > > > > > > Thanks
> > > > > >
> > > > > > Zero is a valid region index.
> > > > > If zero is valid, can we find out other invalid value? How about 0xffffffff?
> > > >
> > > > Unlikely, but also valid.  Isn't this why we have a flags field in
> > > > the structure, so we don't need to rely on implicit values as invalid.
> > > > Also, all of the previously discussed usage models needs to be
> > > > documented, either here in the header or in a Documentation/ file.
> > > According to the previously discussion, we also have the following propose:
> > > enum vfio_device_gfx_type {
> > >         VFIO_DEVICE_GFX_NONE,
> > >         VFIO_DEVICE_GFX_DMABUF,
> > >         VFIO_DEVICE_GFX_REGION,
> > > };
> > >
> > > struct vfio_device_gfx_query_caps {
> > >         __u32 argsz;
> > >         __u32 flags;
> > >         enum vfio_device_gfx_type;
> > > };
> > > So, we may need to add one more ioctl, e.g.
> VFIO_DEVICE_QUERY_GFX_CAPS.
> > > User mode can query this before querying the plan info, and to see
> > > which usage (dma-buf or region) is supported.
> > > Does it still make sense?
> > > Thanks.
> > So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode
> whether the region case is using, instead of introducing a new ioctl.
> > Thanks
> >
> > Tina
> > >
> > > Tina
> > >
> > >
> > > > Thanks,
> > > >
> > > > Alex
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Kirti Wankhede]

On 8/8/2017 11:37 PM, Alex Williamson wrote:
> On Tue, 8 Aug 2017 14:18:07 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 8/7/2017 11:13 PM, Alex Williamson wrote:
>>> On Mon, 7 Aug 2017 08:11:43 +0000
>>> "Zhang, Tina" <tina.zhang@intel.com> wrote:
>>>   
>>>> After going through the previous discussions, here are some summaries may be related to the current discussion:
>>>> 1. How does user mode figure the device capabilities between region and dma-buf?
>>>> VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case. 
>>>> Otherwise, the mdev supports dma-buf.  
>>>
>>> Why do we need to make this assumption?  
>>
>> Right, we should not make such assumption. Vendor driver might not
>> support both or disable console vnc ( for example, for performance
>> testing console VNC need to be disabled)
>>
>>>  What happens when dma-buf is
>>> superseded?  What happens if a device supports both dma-buf and
>>> regions?  We have a flags field in vfio_device_gfx_plane_info, doesn't
>>> it make sense to use it to identify which field, between region_index
>>> and fd, is valid?  We could even put region_index and fd into a union
>>> with the flag bits indicating how to interpret the union, but I'm not
>>> sure everyone was onboard with this idea.  Seems like a waste of 4
>>> bytes not to do that though.
>>>   
>>
>> Agree.
>>
>>> Thinking further, is the user ever in a situation where they query the
>>> graphics plane info and can handle either a dma-buf or a region?  It
>>> seems more likely that the user needs to know early on which is
>>> supported and would then require that they continue to see compatible
>>> plane information...  Should the user then be able to specify whether
>>> they want a dma-buf or a region?  Perhaps these flag bits are actually
>>> input and the return should be -errno if the driver cannot produce
>>> something compatible.
>>>
>>> Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In
>>> this initial implementation, DMABUF or REGION would always be set by
>>> the user to request that type of interface.  Additionally, the QUERY
>>> bit could be set to probe compatibility, thus if PROBE and REGION are
>>> set, the vendor driver would return success only if it supports the
>>> region based interface.  If PROBE and DMABUF are set, the vendor driver
>>> returns success only if the dma-buf based interface is supported.  The
>>> value of the remainder of the structure is undefined for PROBE.
>>> Additionally setting both DMABUF and REGION is invalid.  Undefined
>>> flags bits must be validated as zero by the drivers for future use
>>> (thus if we later define DMABUFv2, an older driver should
>>> automatically return -errno when probed or requested).
>>>   
>>
>> Are you saying all of this to be part of VFIO_DEVICE_QUERY_GFX_PLANE ioctl?
>>
>> Let me summarize what we need, taking QEMU as reference:
>> 1. From vfio_initfn(), for REGION case, get region info:
>> vfio_get_dev_region_info(.., VFIO_REGION_SUBTYPE_CONSOLE_REGION,
>> &vdev->console_opregion)
>>
>> If above return success, setup console REGION and mmap.
>> I don't know what is required for DMABUF at this moment.
>>
>> If console VNC is supported either DMABUF or REGION, initialize console
>> and register its callback operations:
>>
>> static const GraphicHwOps vfio_console_ops = {
>>     .gfx_update  = vfio_console_update_display,
>> };
>>
>> vdev->console = graphic_console_init(DEVICE(vdev), 0, &vfio_console_ops,
>> vdev);
> 
> I might structure it that vfio_initfn() would call
> ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with the PROBE bit set with either
> DMABUF or REGION set as the interface type in the flags field.  If
> REGION is the probed interface and success is returned, then QEMU might
> go look for regions of the appropriate type, however the
> vfio_device_gfx_plane_info structure is canonical source for the region
> index, so QEMU would probably be wise to use that and only use the
> region type for consistency testing.
> 
>> 2. On above console registration, vfio_console_update_display() gets
>> called for each refresh cycle of console. In that:
>> - call ioctl(VFIO_DEVICE_QUERY_GFX_PLANE)
>> - if (queried size > 0), update QEMU console surface (for REGION case
>> read mmaped region, for DMABUF read surface using fd)
> 
> The ioctl would be called with REGION or DMABUF based on the initial
> probe call, ex. we probed that REGION is supported and now we continue
> to ask for region based updates.  QEMU would need to verify the region
> index matches that already mapped, remapping a different region if
> necessary, and interpret the graphics parameters to provide the screen
> update.
>  
>> Alex, in your proposal above, my understanding is
>> ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) with PROBE flag should be called in
>> step #1.
>> In step #2, ioctl(VFIO_DEVICE_QUERY_GFX_PLANE) will be without PROBE
>> flag, but either DMABUF or REGION flag based on what is returned as
>> supported by vendor driver in step #1. Is that correct?
> 
> Yes, that's the idea.  Does it make sense/provide value?
> 

Yes, sounds good to me.

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

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Alex Williamson]

On Wed, 9 Aug 2017 08:31:00 +0000
"Zhang, Tina" <tina.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> > Behalf Of Alex Williamson
> > Sent: Tuesday, August 8, 2017 1:43 AM
> > To: Zhang, Tina <tina.zhang@intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org; kwankhede@nvidia.com; kraxel@redhat.com;
> > intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>; Lv,
> > Zhiyuan <zhiyuan.lv@intel.com>
> > Subject: Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation
> > 
> > On Mon, 7 Aug 2017 08:11:43 +0000
> > "Zhang, Tina" <tina.zhang@intel.com> wrote:
> >   
> > > After going through the previous discussions, here are some summaries may  
> > be related to the current discussion:  
> > > 1. How does user mode figure the device capabilities between region and  
> > dma-buf?  
> > > VFIO_DEVICE_GET_REGION_INFO could tell if the mdev supports region case.
> > > Otherwise, the mdev supports dma-buf.  
> > 
> > Why do we need to make this assumption?  What happens when dma-buf is
> > superseded?  What happens if a device supports both dma-buf and regions?
> > We have a flags field in vfio_device_gfx_plane_info, doesn't it make sense to use
> > it to identify which field, between region_index and fd, is valid?  We could even
> > put region_index and fd into a union with the flag bits indicating how to
> > interpret the union, but I'm not sure everyone was onboard with this idea.
> > Seems like a waste of 4 bytes not to do that though.  
> It seems we discussed this idea before:
> https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001304.html
> https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001333.html

These are both from Gerd.  Gerd, do you have any objection to using a
union to provide either the dmabuf fd or region index?  It's
inefficient to provide separate fields when we can only provide one or
the other and I don't like the idea of using implicit values to
determine which is active.

> > Thinking further, is the user ever in a situation where they query the graphics
> > plane info and can handle either a dma-buf or a region?  It seems more likely
> > that the user needs to know early on which is supported and would then require
> > that they continue to see compatible plane information...  Should the user then
> > be able to specify whether they want a dma-buf or a region?  Perhaps these flag
> > bits are actually input and the return should be -errno if the driver cannot
> > produce something compatible.  
> From the previously discussion, it seems user space workflow will look quite different
> for these two cases. So once user space finds out which case is supported, it just uses
> that case, and won't change it.

And that's supported, the user tests whether a given interface type is
supported and continues to request updates for that interface type.
 
> Meanwhile, I'm not sure whether there will be a mdev would like to support both region
> and dma-buf cases. In my opinion, either region or dma-buf is supported by one mdev. (Yeah,
> agree, there may be other cases in future)

There's no requirement to support both.

> It's like we want to propose a general interface used to share guest's buffer with host. And the
> general interface, so far, has two choice: region and dma-buf. So each mdev likes this interface
> can implement one kind of it and gets the benefit from the general interface.
> So, if we think about this, the difference in user mode should be as little as possible.

The difference seems pretty minimal here, the user probes supported
interface types, and explicitly picks one by requesting updates using
that interface type.  The difference is only in the interpretation of
one dword field.  Furthermore, we're not limiting ourselves to these
two interface types, this same API could support dmabuf-v2 if we define
a flag bit for it and define the structure of the interface union.

> > Maybe we'd therefore define 3 flag bits: PROBE, DMABUF, REGION.  In this
> > initial implementation, DMABUF or REGION would always be set by the user to
> > request that type of interface.  Additionally, the QUERY bit could be set to probe
> > compatibility, thus if PROBE and REGION are set, the vendor driver would return
> > success only if it supports the region based interface.  If PROBE and DMABUF are
> > set, the vendor driver returns success only if the dma-buf based interface is
> > supported.  The value of the remainder of the structure is undefined for PROBE.
> > Additionally setting both DMABUF and REGION is invalid.  Undefined flags bits
> > must be validated as zero by the drivers for future use (thus if we later define
> > DMABUFv2, an older driver should automatically return -errno when probed or
> > requested).
> > 
> > It seems like this handles all the cases, the user can ask what's supported and
> > specifies the interface they want on every call.  The user therefore can also
> > choose between region_index and fd and we can make that a union.  
> Agree, that's a good proposal, which can handle all the cases.
> I'm just not sure about the usage case of "on every call". In previous discussion, it seems we think static is enough.

Is it somehow a problem for the user to set the type bit in the flag on
every call?  This makes it explicit that the user is asking for an
update for a specific interface type, if the vendor driver doesn't
support it, return -errno.  This makes it explicit whether the return
is a dmabuf fd or a region index and the user knows how to interpret
that union because they have asked for a specific type.  As you've
likely gathered from my previous replies, I don't like implicit
interfaces.  I don't want the interpretation of a field to depend on
something the user has done in the past and I see no reason to impose a
limitation on the vendor driver supporting only one of two pre-defined
interfaces types.  Thanks,

Alex

> > > 2. For dma-buf, how to differentiate unsupported vs not initialized?
> > > For dma-buf, when the mdev doesn't support some arguments, -EINVAL will  
> > be returned. And -errno will return when meeting other failures, like -ENOMEM.  
> > > If the mdev is not initialized, there won't be any returned err. Just zero all the  
> > fields in structure vfio_device_gfx_plane_info.
> > 
> > So we're relying on special values again :-\  For which fields is zero not a valid
> > value?  I prefer the probe interface above unless there are better ideas.
> >   
> > > 3. The id field in structure vfio_device_gfx_plane_info So far we
> > > haven't figured out the usage of this field for dma-buf usage. So, this field is  
> > changed to "region_index" and only used for region usage.  
> > > In previous discussions, we thought this "id" field might be used for both dma-  
> > buf and region usages.  
> > > So, we proposed some ways, like adding flags field to the structure. Another  
> > way to do it was to add this:  
> > > enum vfio_device_gfx_type {
> > >          VFIO_DEVICE_GFX_NONE,
> > >          VFIO_DEVICE_GFX_DMABUF,
> > >          VFIO_DEVICE_GFX_REGION,
> > >  };
> > >
> > >  struct vfio_device_gfx_query_caps {
> > >          __u32 argsz;
> > >          __u32 flags;
> > >          enum vfio_device_gfx_type;
> > >  };
> > > Obviously, we don't need to consider this again, unless we find the  
> > "region_index" means something for dma-buf usage.
> > 
> > The usefulness of this ioctl seems really limited and once again we get into a
> > situation where having two ioctls leaves doubt whether this is describing the
> > current plane state.  Thanks,
> > 
> > Alex
> >   
> > > > > > > > > >  include/uapi/linux/vfio.h | 28
> > > > > > > > > > ++++++++++++++++++++++++++++
> > > > > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > > > > > b/include/uapi/linux/vfio.h index ae46105..827a230
> > > > > > > > > > 100644
> > > > > > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > > > > > @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset {
> > > > > > > > > >
> > > > > > > > > >  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE,  
> > > > VFIO_BASE +  
> > > > > > > > > 13)  
> > > > > > > > > >
> > > > > > > > > > +/**
> > > > > > > > > > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE,  
> > > > VFIO_BASE  
> > > > > +  
> > > > > > > 14,  
> > > > > > > > > > +struct vfio_device_query_gfx_plane)
> > > > > > > > > > + *
> > > > > > > > > > + * Set the drm_plane_type and retrieve information
> > > > > > > > > > +about the gfx  
> > > > > plane.  
> > > > > > > > > > + *
> > > > > > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > > > > > + */
> > > > > > > > > > +struct vfio_device_gfx_plane_info {
> > > > > > > > > > +	__u32 argsz;
> > > > > > > > > > +	__u32 flags;
> > > > > > > > > > +	/* in */
> > > > > > > > > > +	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_*  
> > > > > */  
> > > > > > > > > > +	/* out */
> > > > > > > > > > +	__u32 drm_format;	/* drm format of plane */
> > > > > > > > > > +	__u64 drm_format_mod;   /* tiled mode */
> > > > > > > > > > +	__u32 width;	/* width of plane */
> > > > > > > > > > +	__u32 height;	/* height of plane */
> > > > > > > > > > +	__u32 stride;	/* stride of plane */
> > > > > > > > > > +	__u32 size;	/* size of plane in bytes, align on page*/
> > > > > > > > > > +	__u32 x_pos;	/* horizontal position of cursor plane, upper  
> > > > > left corner  
> > > > > > > > > in pixels */  
> > > > > > > > > > +	__u32 y_pos;	/* vertical position of cursor plane, upper left  
> > > > > corner in  
> > > > > > > > > lines*/  
> > > > > > > > > > +	__u32 region_index;
> > > > > > > > > > +	__s32 fd;	/* dma-buf fd */  
> > > > > > > > >
> > > > > > > > > How do I know which of these is valid, region_index or fd?
> > > > > > > > > Can I ask for one vs the other?  What are the errno values
> > > > > > > > > to differentiate unsupported vs not initialized?  Is there a "probe"
> > > > > > > > > flag that I can use to determine what the device supports
> > > > > > > > > without allocating  
> > > > > > > those resources yet?  
> > > > > > > > Dma-buf won't use region_index, which means region_index is
> > > > > > > > zero all the  
> > > > > > > time for dma-buf usage.  
> > > > > > > > As we discussed, there won't be errno if not initialized,
> > > > > > > > just keep all fields  
> > > > > zero.  
> > > > > > > > I will add the comments about these in the next version.
> > > > > > > > Thanks  
> > > > > > >
> > > > > > > Zero is a valid region index.  
> > > > > > If zero is valid, can we find out other invalid value? How about 0xffffffff?  
> > > > >
> > > > > Unlikely, but also valid.  Isn't this why we have a flags field in
> > > > > the structure, so we don't need to rely on implicit values as invalid.
> > > > > Also, all of the previously discussed usage models needs to be
> > > > > documented, either here in the header or in a Documentation/ file.  
> > > > According to the previously discussion, we also have the following propose:
> > > > enum vfio_device_gfx_type {
> > > >         VFIO_DEVICE_GFX_NONE,
> > > >         VFIO_DEVICE_GFX_DMABUF,
> > > >         VFIO_DEVICE_GFX_REGION,
> > > > };
> > > >
> > > > struct vfio_device_gfx_query_caps {
> > > >         __u32 argsz;
> > > >         __u32 flags;
> > > >         enum vfio_device_gfx_type;
> > > > };
> > > > So, we may need to add one more ioctl, e.g.  
> > VFIO_DEVICE_QUERY_GFX_CAPS.  
> > > > User mode can query this before querying the plan info, and to see
> > > > which usage (dma-buf or region) is supported.
> > > > Does it still make sense?
> > > > Thanks.  
> > > So, I think we can rely on VFIO_DEVICE_GET_REGION_INFO to tell user mode  
> > whether the region case is using, instead of introducing a new ioctl.  
> > > Thanks
> > >
> > > Tina  
> > > >
> > > > Tina
> > > >
> > > >  
> > > > > Thanks,
> > > > >
> > > > > Alex
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel  
> > 
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev  

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

Re: [PATCH v13 2/7] drm: Introduce RGB 64-bit 16:16:16:16 float format

[Daniel Vetter]

On Tue, Jul 25, 2017 at 05:28:15PM +0800, Tina Zhang wrote:
> The RGB 64-bit 16:16:16:16 float pixel format is needed by windows
> guest VM. This patch is to introduce the format to drm.
> 
> v1:
> Suggested by Ville to submit this patch to dri-devel.
> 
> Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 7586c46..3e002e3 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -113,6 +113,10 @@ extern "C" {
>  
>  #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */
>  
> +/* 64 bpp RGB */
> +#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /* [63:0] x:R:G:B 16:16:16:16 little endian */
> +#define DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0] x:B:G:R 16:16:16:16 little endian */

I think the comment should go a bit more into detail what the float
format is supposed to look like. And I think we should have a F or _FLOAT suffix to
the defined name, since the same layout could also work for integers (and
some hw somewhere probably has that. Maybe also put that F into the last
slot of the fourcc.
-Daniel
> +
>  /*
>   * 2 plane RGB + A
>   * index 0 = RGB plane, same format as the corresponding non _A8 format has
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v13 2/7] drm: Introduce RGB 64-bit 16:16:16:16 float format

[Zhang, Tina]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, August 15, 2017 10:50 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org; ville.syrjala@linux.intel.com;
> zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A
> <zhi.a.wang@intel.com>; alex.williamson@redhat.com; kraxel@redhat.com;
> chris@chris-wilson.co.uk; daniel@ffwll.ch; kwankhede@nvidia.com; Tian, Kevin
> <kevin.tian@intel.com>; Chen, Xiaoguang <xiaoguang.chen@intel.com>
> Subject: Re: [PATCH v13 2/7] drm: Introduce RGB 64-bit 16:16:16:16 float
> format
> 
> On Tue, Jul 25, 2017 at 05:28:15PM +0800, Tina Zhang wrote:
> > The RGB 64-bit 16:16:16:16 float pixel format is needed by windows
> > guest VM. This patch is to introduce the format to drm.
> >
> > v1:
> > Suggested by Ville to submit this patch to dri-devel.
> >
> > Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h
> > b/include/uapi/drm/drm_fourcc.h index 7586c46..3e002e3 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -113,6 +113,10 @@ extern "C" {
> >
> >  #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0]
> A:Y:Cb:Cr 8:8:8:8 little endian */
> >
> > +/* 64 bpp RGB */
> > +#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /*
> > +[63:0] x:R:G:B 16:16:16:16 little endian */ #define
> > +DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0]
> > +x:B:G:R 16:16:16:16 little endian */
> 
> I think the comment should go a bit more into detail what the float format is
> supposed to look like. And I think we should have a F or _FLOAT suffix to the
> defined name, since the same layout could also work for integers (and some hw
> somewhere probably has that. Maybe also put that F into the last slot of the
> fourcc.
Thanks. I prefer to add F and also put F into the last slot of the fourcc. 
I'm not sure about the code rule. Does it make sense?
#define	DRM_FORMAT_XRGB161616F fourcc_code('X', 'R', '3', 'F')
Where '3' stands for the three fields (R, G, B) without Alpha.

BR,
Tina
> -Daniel
> > +
> >  /*
> >   * 2 plane RGB + A
> >   * index 0 = RGB plane, same format as the corresponding non _A8
> > format has
> > --
> > 2.7.4
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v13 2/7] drm: Introduce RGB 64-bit 16:16:16:16 float format

[Daniel Vetter]

On Wed, Aug 16, 2017 at 03:16:46AM +0000, Zhang, Tina wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, August 15, 2017 10:50 PM
> > To: Zhang, Tina <tina.zhang@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org; ville.syrjala@linux.intel.com;
> > zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A
> > <zhi.a.wang@intel.com>; alex.williamson@redhat.com; kraxel@redhat.com;
> > chris@chris-wilson.co.uk; daniel@ffwll.ch; kwankhede@nvidia.com; Tian, Kevin
> > <kevin.tian@intel.com>; Chen, Xiaoguang <xiaoguang.chen@intel.com>
> > Subject: Re: [PATCH v13 2/7] drm: Introduce RGB 64-bit 16:16:16:16 float
> > format
> > 
> > On Tue, Jul 25, 2017 at 05:28:15PM +0800, Tina Zhang wrote:
> > > The RGB 64-bit 16:16:16:16 float pixel format is needed by windows
> > > guest VM. This patch is to introduce the format to drm.
> > >
> > > v1:
> > > Suggested by Ville to submit this patch to dri-devel.
> > >
> > > Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
> > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > ---
> > >  include/uapi/drm/drm_fourcc.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_fourcc.h
> > > b/include/uapi/drm/drm_fourcc.h index 7586c46..3e002e3 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -113,6 +113,10 @@ extern "C" {
> > >
> > >  #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0]
> > A:Y:Cb:Cr 8:8:8:8 little endian */
> > >
> > > +/* 64 bpp RGB */
> > > +#define DRM_FORMAT_XRGB161616  fourcc_code('X', 'R', '4', '8') /*
> > > +[63:0] x:R:G:B 16:16:16:16 little endian */ #define
> > > +DRM_FORMAT_XBGR161616  fourcc_code('X', 'B', '4', '8') /* [63:0]
> > > +x:B:G:R 16:16:16:16 little endian */
> > 
> > I think the comment should go a bit more into detail what the float format is
> > supposed to look like. And I think we should have a F or _FLOAT suffix to the
> > defined name, since the same layout could also work for integers (and some hw
> > somewhere probably has that. Maybe also put that F into the last slot of the
> > fourcc.
> Thanks. I prefer to add F and also put F into the last slot of the fourcc. 
> I'm not sure about the code rule. Does it make sense?
> #define	DRM_FORMAT_XRGB161616F fourcc_code('X', 'R', '3', 'F')
> Where '3' stands for the three fields (R, G, B) without Alpha.

Seems reasonable. There's not really any rules for fourcc codes, the only
semi-standard ones (and not even those are really standard) are only for
video formats. Best we can do is try to be somewhat consistent in
drm_fourcc.h.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v13 5/7] vfio: ABI for mdev display dma-buf operation

[Gerd Hoffmann]

  Hi,

> These are both from Gerd.  Gerd, do you have any objection to using a
> union to provide either the dmabuf fd or region index?

No.

> > It's like we want to propose a general interface used to share
> > guest's buffer with host. And the
> > general interface, so far, has two choice: region and dma-buf. So
> > each mdev likes this interface
> > can implement one kind of it and gets the benefit from the general
> > interface.
> > So, if we think about this, the difference in user mode should be
> > as little as possible.
> 
> The difference seems pretty minimal here, the user probes supported
> interface types, and explicitly picks one by requesting updates using
> that interface type.  The difference is only in the interpretation of
> one dword field.  Furthermore, we're not limiting ourselves to these
> two interface types, this same API could support dmabuf-v2 if we
> define
> a flag bit for it and define the structure of the interface union.

Yep, using the flags is more future-proof and continues to work in case
another interface type shows up (unlike looking for a gfx region being
present).

> > Agree, that's a good proposal, which can handle all the cases.
> > I'm just not sure about the usage case of "on every call". In
> > previous discussion, it seems we think static is enough.
> 
> Is it somehow a problem for the user to set the type bit in the flag
> on
> every call?

Not at all.

cheers,
  Gerd

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