[RFC PATCH v4 0/6] Deliver vGPU display refresh event to userspace

[RFC PATCH v4 0/6] Deliver vGPU display refresh event to userspace

[Kechen Lu]

This series tries to send the vGPU display refresh event to user land.

Instead of delivering page flip events only or vblank events only, we 
choose to combine two of them, i.e. post display refresh event at vblanks 
and skip some of them when no page flip happens. Vblanks as upper bound 
are safe and skipping no-page-flip vblanks guarantees both trivial performance 
impacts and good user experience without screen tearing. Plus, we have the 
mask/unmask mechansim providing user space flexibility to switch between 
event-notified refresh and classic timer-based refresh.

In addition, there are some cases that guest app only uses one framebuffer 
for both drawing and display. In such case, guest OS won't do the plane page 
flip when the framebuffer is updated, thus the user land won't be notified 
about the updated framebuffer. Hence, in single framebuffer case, we apply
a heuristic to determine whether it is the case or not. If it is, notify user
land when each vblank event triggers.

v4:
- Deliver page flip event and single framebuffer refresh event bounded 
by display vblanks. (Kechen)
v3:
- Deliver display vblank event instead of page flip event. (Zhenyu)
v2:
- Use VFIO irq chain to get eventfds from userspace instead of adding
a new ABI. (Alex)
v1:
- https://patchwork.kernel.org/cover/10962341/

Kechen Lu (2):
  drm/i915/gvt: Deliver async primary plane page flip events at vblank
  drm/i915/gvt: Add cursor plane reg update trap emulation handler

Tina Zhang (4):
  vfio: Define device specific irq type capability
  vfio: Introduce vGPU display irq type
  drm/i915/gvt: Register vGPU display event irq
  drm/i915/gvt: Deliver vGPU refresh event to userspace

 drivers/gpu/drm/i915/gvt/cmd_parser.c |   5 +-
 drivers/gpu/drm/i915/gvt/display.c    |  54 ++++++-
 drivers/gpu/drm/i915/gvt/gvt.h        |  11 ++
 drivers/gpu/drm/i915/gvt/handlers.c   |  32 ++++-
 drivers/gpu/drm/i915/gvt/hypercall.h  |   1 +
 drivers/gpu/drm/i915/gvt/interrupt.c  |   7 +
 drivers/gpu/drm/i915/gvt/interrupt.h  |   3 +
 drivers/gpu/drm/i915/gvt/kvmgt.c      | 197 ++++++++++++++++++++++++--
 drivers/gpu/drm/i915/gvt/mpt.h        |  17 +++
 include/uapi/linux/vfio.h             |  22 ++-
 10 files changed, 330 insertions(+), 19 deletions(-)

-- 
2.17.1

[RFC PATCH v4 1/6] vfio: Define device specific irq type capability

[Kechen Lu]

From: Tina Zhang <tina.zhang@intel.com>

Cap the number of irqs with fixed indexes and use capability chains
to chain device specific irqs.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/uapi/linux/vfio.h | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 8f10748dac79..be6adab4f759 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -448,11 +448,27 @@ struct vfio_irq_info {
 #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
 #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
 #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
+#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info supports caps */
 	__u32	index;		/* IRQ index */
 	__u32	count;		/* Number of IRQs within this index */
+	__u32	cap_offset;	/* Offset within info struct of first cap */
 };
 #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE + 9)
 
+/*
+ * The irq type capability allows irqs unique to a specific device or
+ * class of devices to be exposed.
+ *
+ * The structures below define version 1 of this capability.
+ */
+#define VFIO_IRQ_INFO_CAP_TYPE      3
+
+struct vfio_irq_info_cap_type {
+	struct vfio_info_cap_header header;
+	__u32 type;     /* global per bus driver */
+	__u32 subtype;  /* type specific */
+};
+
 /**
  * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
  *
@@ -554,7 +570,8 @@ enum {
 	VFIO_PCI_MSIX_IRQ_INDEX,
 	VFIO_PCI_ERR_IRQ_INDEX,
 	VFIO_PCI_REQ_IRQ_INDEX,
-	VFIO_PCI_NUM_IRQS
+	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5 use   */
+				/* device specific cap to define content */
 };
 
 /*
-- 
2.17.1

Re: [RFC PATCH v4 1/6] vfio: Define device specific irq type capability

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

On 2019.07.18 23:56:35 +0800, Kechen Lu wrote:
> From: Tina Zhang <tina.zhang@intel.com>
> 
> Cap the number of irqs with fixed indexes and use capability chains
> to chain device specific irqs.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  include/uapi/linux/vfio.h | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 8f10748dac79..be6adab4f759 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -448,11 +448,27 @@ struct vfio_irq_info {
>  #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
>  #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
>  #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
> +#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info supports caps */
>  	__u32	index;		/* IRQ index */
>  	__u32	count;		/* Number of IRQs within this index */
> +	__u32	cap_offset;	/* Offset within info struct of first cap */

This still breaks ABI as argsz would be updated with this new field, so it would
cause compat issue. I think my last suggestion was to assume cap list starts after
vfio_irq_info.

>  };
>  #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE + 9)
>  
> +/*
> + * The irq type capability allows irqs unique to a specific device or
> + * class of devices to be exposed.
> + *
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IRQ_INFO_CAP_TYPE      3
> +
> +struct vfio_irq_info_cap_type {
> +	struct vfio_info_cap_header header;
> +	__u32 type;     /* global per bus driver */
> +	__u32 subtype;  /* type specific */
> +};
> +
>  /**
>   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
>   *
> @@ -554,7 +570,8 @@ enum {
>  	VFIO_PCI_MSIX_IRQ_INDEX,
>  	VFIO_PCI_ERR_IRQ_INDEX,
>  	VFIO_PCI_REQ_IRQ_INDEX,
> -	VFIO_PCI_NUM_IRQS
> +	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5 use   */
> +				/* device specific cap to define content */
>  };
>  
>  /*
> -- 
> 2.17.1
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [RFC PATCH v4 1/6] vfio: Define device specific irq type capability

[Lu, Kechen]

Hi,

> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Friday, July 19, 2019 2:06 PM
> To: Lu, Kechen <kechen.lu@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux- 
> kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>; 
> kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan 
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, 
> Kevin <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>; 
> alex.williamson@redhat.com; Eric Auger <eric.auger@redhat.com>
> Subject: Re: [RFC PATCH v4 1/6] vfio: Define device specific irq type 
> capability
> 
> On 2019.07.18 23:56:35 +0800, Kechen Lu wrote:
> > From: Tina Zhang <tina.zhang@intel.com>
> >
> > Cap the number of irqs with fixed indexes and use capability chains 
> > to chain device specific irqs.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > ---
> >  include/uapi/linux/vfio.h | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h 
> > index 8f10748dac79..be6adab4f759 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -448,11 +448,27 @@ struct vfio_irq_info {
> >  #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
> >  #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
> >  #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
> > +#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info supports caps
> */
> >  	__u32	index;		/* IRQ index */
> >  	__u32	count;		/* Number of IRQs within this index */
> > +	__u32	cap_offset;	/* Offset within info struct of first cap */
> 
> This still breaks ABI as argsz would be updated with this new field, 
> so it would cause compat issue. I think my last suggestion was to 
> assume cap list starts after vfio_irq_info.
>
 
In the common practice, the general logic is first use the "count" as the "minsz" boundary to perform copy from user, and then perform following logic, so that the incompatibility issue would not happen. BTW, this patch has been double checked by Eric Auger before included in his patch-set. 

Best Regards,
Kechen

> >  };
> >  #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE + 9)
> >
> > +/*
> > + * The irq type capability allows irqs unique to a specific device 
> > +or
> > + * class of devices to be exposed.
> > + *
> > + * The structures below define version 1 of this capability.
> > + */
> > +#define VFIO_IRQ_INFO_CAP_TYPE      3
> > +
> > +struct vfio_irq_info_cap_type {
> > +	struct vfio_info_cap_header header;
> > +	__u32 type;     /* global per bus driver */
> > +	__u32 subtype;  /* type specific */ };
> > +
> >  /**
> >   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct
> vfio_irq_set)
> >   *
> > @@ -554,7 +570,8 @@ enum {
> >  	VFIO_PCI_MSIX_IRQ_INDEX,
> >  	VFIO_PCI_ERR_IRQ_INDEX,
> >  	VFIO_PCI_REQ_IRQ_INDEX,
> > -	VFIO_PCI_NUM_IRQS
> > +	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5 use
> */
> > +				/* device specific cap to define content */
> >  };
> >
> >  /*
> > --
> > 2.17.1
> >
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

Re: [RFC PATCH v4 1/6] vfio: Define device specific irq type capability

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 3821 bytes --]

On 2019.07.19 09:02:33 +0000, Lu, Kechen wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> > Sent: Friday, July 19, 2019 2:06 PM
> > To: Lu, Kechen <kechen.lu@intel.com>
> > Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux- 
> > kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>; 
> > kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan 
> > <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, 
> > Kevin <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>; 
> > alex.williamson@redhat.com; Eric Auger <eric.auger@redhat.com>
> > Subject: Re: [RFC PATCH v4 1/6] vfio: Define device specific irq type 
> > capability
> > 
> > On 2019.07.18 23:56:35 +0800, Kechen Lu wrote:
> > > From: Tina Zhang <tina.zhang@intel.com>
> > >
> > > Cap the number of irqs with fixed indexes and use capability chains 
> > > to chain device specific irqs.
> > >
> > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > > ---
> > >  include/uapi/linux/vfio.h | 19 ++++++++++++++++++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h 
> > > index 8f10748dac79..be6adab4f759 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -448,11 +448,27 @@ struct vfio_irq_info {
> > >  #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
> > >  #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
> > >  #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
> > > +#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info supports caps
> > */
> > >  	__u32	index;		/* IRQ index */
> > >  	__u32	count;		/* Number of IRQs within this index */
> > > +	__u32	cap_offset;	/* Offset within info struct of first cap */
> > 
> > This still breaks ABI as argsz would be updated with this new field, 
> > so it would cause compat issue. I think my last suggestion was to 
> > assume cap list starts after vfio_irq_info.
> >
>  
> In the common practice, the general logic is first use the "count" as the "minsz" boundary to perform copy from user, and then perform following logic, so that the incompatibility issue would not happen. BTW, this patch has been double checked by Eric Auger before included in his patch-set. 
> 

yeah, sorry I was thinking vfio might fail in that case but it seems
current code assume argsz should be larger than minsz for count here,
so that's fine.

> 
> > >  };
> > >  #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE + 9)
> > >
> > > +/*
> > > + * The irq type capability allows irqs unique to a specific device 
> > > +or
> > > + * class of devices to be exposed.
> > > + *
> > > + * The structures below define version 1 of this capability.
> > > + */
> > > +#define VFIO_IRQ_INFO_CAP_TYPE      3
> > > +
> > > +struct vfio_irq_info_cap_type {
> > > +	struct vfio_info_cap_header header;
> > > +	__u32 type;     /* global per bus driver */
> > > +	__u32 subtype;  /* type specific */ };
> > > +
> > >  /**
> > >   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct
> > vfio_irq_set)
> > >   *
> > > @@ -554,7 +570,8 @@ enum {
> > >  	VFIO_PCI_MSIX_IRQ_INDEX,
> > >  	VFIO_PCI_ERR_IRQ_INDEX,
> > >  	VFIO_PCI_REQ_IRQ_INDEX,
> > > -	VFIO_PCI_NUM_IRQS
> > > +	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5 use
> > */
> > > +				/* device specific cap to define content */
> > >  };
> > >
> > >  /*
> > > --
> > > 2.17.1
> > >
> > 
> > --
> > Open Source Technology Center, Intel ltd.
> > 
> > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type

[Kechen Lu]

From: Tina Zhang <tina.zhang@intel.com>

Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU display

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

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index be6adab4f759..df28b17a6e2e 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -469,6 +469,9 @@ struct vfio_irq_info_cap_type {
 	__u32 subtype;  /* type specific */
 };
 
+#define VFIO_IRQ_TYPE_GFX				(1)
+#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)
+
 /**
  * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
  *
-- 
2.17.1

Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type

[Alex Williamson]

On Thu, 18 Jul 2019 23:56:36 +0800
Kechen Lu <kechen.lu@intel.com> wrote:

> From: Tina Zhang <tina.zhang@intel.com>
> 
> Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
> VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU display
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  include/uapi/linux/vfio.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index be6adab4f759..df28b17a6e2e 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -469,6 +469,9 @@ struct vfio_irq_info_cap_type {
>  	__u32 subtype;  /* type specific */
>  };
>  
> +#define VFIO_IRQ_TYPE_GFX				(1)
> +#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)
> +

Please include a description defining exactly what this IRQ is intended
to signal.  For instance, if another vGPU vendor wanted to implement
this in their driver and didn't have the QEMU code for reference to
what it does with the IRQ, what would they need to know?  Thanks,

Alex 

>  /**
>   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
>   *

RE: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type

[Lu, Kechen]

Hi, 

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Saturday, July 20, 2019 12:25 AM
> To: Lu, Kechen <kechen.lu@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, Kevin
> <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>
> Subject: Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type
> 
> On Thu, 18 Jul 2019 23:56:36 +0800
> Kechen Lu <kechen.lu@intel.com> wrote:
> 
> > From: Tina Zhang <tina.zhang@intel.com>
> >
> > Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
> > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU display
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >  include/uapi/linux/vfio.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index be6adab4f759..df28b17a6e2e 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -469,6 +469,9 @@ struct vfio_irq_info_cap_type {
> >  	__u32 subtype;  /* type specific */
> >  };
> >
> > +#define VFIO_IRQ_TYPE_GFX				(1)
> > +#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)
> > +
> 
> Please include a description defining exactly what this IRQ is intended to signal.
> For instance, if another vGPU vendor wanted to implement this in their driver
> and didn't have the QEMU code for reference to what it does with the IRQ, what
> would they need to know?  Thanks,
> 
> Alex
> 

Yes, that makes more sense. I'll add the description for it at next version patch.

BTW, may I have one more question? In the current design ideas, we partitioned 
the vGPU display eventfd counted 8-byte value into at most 8 events to deliver 
multiple display events, so we need different increasement counter value to 
differentiate the events. As this is the exposed thing the QEMU has to know, we
plan adds a macro here VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENTFD_BASE_SHIFT to
make sure the partitions shift in 1 byte, does it make sense putting here? Looking  
forward to your and Gerd's comments. Thanks!


Best Regards,
Kechen

> >  /**
> >   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct
> vfio_irq_set)
> >   *

Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type

[Alex Williamson]

On Mon, 22 Jul 2019 05:28:35 +0000
"Lu, Kechen" <kechen.lu@intel.com> wrote:

> Hi, 
> 
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Saturday, July 20, 2019 12:25 AM
> > To: Lu, Kechen <kechen.lu@intel.com>
> > Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> > kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> > <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, Kevin
> > <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>
> > Subject: Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type
> > 
> > On Thu, 18 Jul 2019 23:56:36 +0800
> > Kechen Lu <kechen.lu@intel.com> wrote:
> >   
> > > From: Tina Zhang <tina.zhang@intel.com>
> > >
> > > Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
> > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU display
> > >
> > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > ---
> > >  include/uapi/linux/vfio.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > index be6adab4f759..df28b17a6e2e 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -469,6 +469,9 @@ struct vfio_irq_info_cap_type {
> > >  	__u32 subtype;  /* type specific */
> > >  };
> > >
> > > +#define VFIO_IRQ_TYPE_GFX				(1)
> > > +#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)
> > > +  
> > 
> > Please include a description defining exactly what this IRQ is intended to signal.
> > For instance, if another vGPU vendor wanted to implement this in their driver
> > and didn't have the QEMU code for reference to what it does with the IRQ, what
> > would they need to know?  Thanks,
> > 
> > Alex
> >   
> 
> Yes, that makes more sense. I'll add the description for it at next version patch.
> 
> BTW, may I have one more question? In the current design ideas, we partitioned 
> the vGPU display eventfd counted 8-byte value into at most 8 events to deliver 
> multiple display events, so we need different increasement counter value to 
> differentiate the events. As this is the exposed thing the QEMU has to know, we
> plan adds a macro here VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENTFD_BASE_SHIFT to
> make sure the partitions shift in 1 byte, does it make sense putting here? Looking  
> forward to your and Gerd's comments. Thanks!

Couldn't you expose this as another capability within the IRQ_INFO
return data?  If you were to define it as a macro, I assume that means
it would be hard coded, in which case this probably becomes an Intel
specific IRQ, rather than what appears to be framed as a generic
graphics IRQ extension.  A new capability could instead allow the
vendor to specify their own value, where we could define how userspace
should interpret and make use of this value.  Thanks,

Alex

RE: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type

[Zhang, Tina]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, July 23, 2019 3:41 AM
> To: Lu, Kechen <kechen.lu@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, Kevin
> <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>
> Subject: Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type
> 
> On Mon, 22 Jul 2019 05:28:35 +0000
> "Lu, Kechen" <kechen.lu@intel.com> wrote:
> 
> > Hi,
> >
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Saturday, July 20, 2019 12:25 AM
> > > To: Lu, Kechen <kechen.lu@intel.com>
> > > Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> > > kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> > > <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian,
> > > Kevin <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>
> > > Subject: Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq
> > > type
> > >
> > > On Thu, 18 Jul 2019 23:56:36 +0800
> > > Kechen Lu <kechen.lu@intel.com> wrote:
> > >
> > > > From: Tina Zhang <tina.zhang@intel.com>
> > > >
> > > > Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
> > > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU display
> > > >
> > > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > > ---
> > > >  include/uapi/linux/vfio.h | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > index be6adab4f759..df28b17a6e2e 100644
> > > > --- a/include/uapi/linux/vfio.h
> > > > +++ b/include/uapi/linux/vfio.h
> > > > @@ -469,6 +469,9 @@ struct vfio_irq_info_cap_type {
> > > >  	__u32 subtype;  /* type specific */  };
> > > >
> > > > +#define VFIO_IRQ_TYPE_GFX				(1)
> > > > +#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)
> > > > +
> > >
> > > Please include a description defining exactly what this IRQ is intended to
> signal.
> > > For instance, if another vGPU vendor wanted to implement this in
> > > their driver and didn't have the QEMU code for reference to what it
> > > does with the IRQ, what would they need to know?  Thanks,
> > >
> > > Alex
> > >
> >
> > Yes, that makes more sense. I'll add the description for it at next version
> patch.
> >
> > BTW, may I have one more question? In the current design ideas, we
> > partitioned the vGPU display eventfd counted 8-byte value into at most
> > 8 events to deliver multiple display events, so we need different
> > increasement counter value to differentiate the events. As this is the
> > exposed thing the QEMU has to know, we plan adds a macro here
> > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENTFD_BASE_SHIFT to make sure
> the
> > partitions shift in 1 byte, does it make sense putting here? Looking forward
> to your and Gerd's comments. Thanks!
> 
> Couldn't you expose this as another capability within the IRQ_INFO return
> data?  If you were to define it as a macro, I assume that means it would be
> hard coded, in which case this probably becomes an Intel specific IRQ, rather
> than what appears to be framed as a generic graphics IRQ extension.  A new
> capability could instead allow the vendor to specify their own value, where
> we could define how userspace should interpret and make use of this value.
> Thanks,
Good suggestion. Currently, vfio_irq_info is used to save one irq info. What we need here is to use it to save several events info. Maybe we could figure out a general layout of this capability so that it can be leveraged by others, not only for display irq/events.
Thanks.

BR,
Tina

> 
> Alex

Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type

[Alex Williamson]

On Tue, 23 Jul 2019 01:08:19 +0000
"Zhang, Tina" <tina.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Tuesday, July 23, 2019 3:41 AM
> > To: Lu, Kechen <kechen.lu@intel.com>
> > Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> > kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> > <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, Kevin
> > <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>
> > Subject: Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type
> > 
> > On Mon, 22 Jul 2019 05:28:35 +0000
> > "Lu, Kechen" <kechen.lu@intel.com> wrote:
> >   
> > > Hi,
> > >  
> > > > -----Original Message-----
> > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > Sent: Saturday, July 20, 2019 12:25 AM
> > > > To: Lu, Kechen <kechen.lu@intel.com>
> > > > Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> > > > kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> > > > <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian,
> > > > Kevin <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>
> > > > Subject: Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq
> > > > type
> > > >
> > > > On Thu, 18 Jul 2019 23:56:36 +0800
> > > > Kechen Lu <kechen.lu@intel.com> wrote:
> > > >  
> > > > > From: Tina Zhang <tina.zhang@intel.com>
> > > > >
> > > > > Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
> > > > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU display
> > > > >
> > > > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > > > ---
> > > > >  include/uapi/linux/vfio.h | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > > > index be6adab4f759..df28b17a6e2e 100644
> > > > > --- a/include/uapi/linux/vfio.h
> > > > > +++ b/include/uapi/linux/vfio.h
> > > > > @@ -469,6 +469,9 @@ struct vfio_irq_info_cap_type {
> > > > >  	__u32 subtype;  /* type specific */  };
> > > > >
> > > > > +#define VFIO_IRQ_TYPE_GFX				(1)
> > > > > +#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)
> > > > > +  
> > > >
> > > > Please include a description defining exactly what this IRQ is intended to  
> > signal.  
> > > > For instance, if another vGPU vendor wanted to implement this in
> > > > their driver and didn't have the QEMU code for reference to what it
> > > > does with the IRQ, what would they need to know?  Thanks,
> > > >
> > > > Alex
> > > >  
> > >
> > > Yes, that makes more sense. I'll add the description for it at next version  
> > patch.  
> > >
> > > BTW, may I have one more question? In the current design ideas, we
> > > partitioned the vGPU display eventfd counted 8-byte value into at most
> > > 8 events to deliver multiple display events, so we need different
> > > increasement counter value to differentiate the events. As this is the
> > > exposed thing the QEMU has to know, we plan adds a macro here
> > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENTFD_BASE_SHIFT to make sure  
> > the  
> > > partitions shift in 1 byte, does it make sense putting here? Looking forward  
> > to your and Gerd's comments. Thanks!
> > 
> > Couldn't you expose this as another capability within the IRQ_INFO return
> > data?  If you were to define it as a macro, I assume that means it would be
> > hard coded, in which case this probably becomes an Intel specific IRQ, rather
> > than what appears to be framed as a generic graphics IRQ extension.  A new
> > capability could instead allow the vendor to specify their own value, where
> > we could define how userspace should interpret and make use of this value.
> > Thanks,  
> Good suggestion. Currently, vfio_irq_info is used to save one irq
> info. What we need here is to use it to save several events info.
> Maybe we could figure out a general layout of this capability so that
> it can be leveraged by others, not only for display irq/events.

You could also expose a device specific IRQ with count > 1 (ie. similar
to MSI/X) and avoid munging the eventfd value, which is not something
we do elsewhere, at least in vfio.  Thanks,

Alex

RE: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type

[Zhang, Tina]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, July 23, 2019 9:19 AM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Lu, Kechen <kechen.lu@intel.com>; intel-gvt-dev@lists.freedesktop.org;
> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; kraxel@redhat.com;
> zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A
> <zhi.a.wang@intel.com>; Tian, Kevin <kevin.tian@intel.com>; Yuan, Hang
> <hang.yuan@intel.com>
> Subject: Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type
> 
> On Tue, 23 Jul 2019 01:08:19 +0000
> "Zhang, Tina" <tina.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Tuesday, July 23, 2019 3:41 AM
> > > To: Lu, Kechen <kechen.lu@intel.com>
> > > Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> > > kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> > > <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian,
> > > Kevin <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>
> > > Subject: Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq
> > > type
> > >
> > > On Mon, 22 Jul 2019 05:28:35 +0000
> > > "Lu, Kechen" <kechen.lu@intel.com> wrote:
> > >
> > > > Hi,
> > > >
> > > > > -----Original Message-----
> > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > Sent: Saturday, July 20, 2019 12:25 AM
> > > > > To: Lu, Kechen <kechen.lu@intel.com>
> > > > > Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org;
> > > > > linux- kernel@vger.kernel.org; Zhang, Tina
> > > > > <tina.zhang@intel.com>; kraxel@redhat.com;
> > > > > zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>;
> > > > > Wang, Zhi A <zhi.a.wang@intel.com>; Tian, Kevin
> > > > > <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>
> > > > > Subject: Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq
> > > > > type
> > > > >
> > > > > On Thu, 18 Jul 2019 23:56:36 +0800 Kechen Lu
> > > > > <kechen.lu@intel.com> wrote:
> > > > >
> > > > > > From: Tina Zhang <tina.zhang@intel.com>
> > > > > >
> > > > > > Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
> > > > > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU
> > > > > > display
> > > > > >
> > > > > > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > > > > > ---
> > > > > >  include/uapi/linux/vfio.h | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/include/uapi/linux/vfio.h
> > > > > > b/include/uapi/linux/vfio.h index be6adab4f759..df28b17a6e2e
> > > > > > 100644
> > > > > > --- a/include/uapi/linux/vfio.h
> > > > > > +++ b/include/uapi/linux/vfio.h
> > > > > > @@ -469,6 +469,9 @@ struct vfio_irq_info_cap_type {
> > > > > >  	__u32 subtype;  /* type specific */  };
> > > > > >
> > > > > > +#define VFIO_IRQ_TYPE_GFX				(1)
> > > > > > +#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)
> > > > > > +
> > > > >
> > > > > Please include a description defining exactly what this IRQ is
> > > > > intended to
> > > signal.
> > > > > For instance, if another vGPU vendor wanted to implement this in
> > > > > their driver and didn't have the QEMU code for reference to what
> > > > > it does with the IRQ, what would they need to know?  Thanks,
> > > > >
> > > > > Alex
> > > > >
> > > >
> > > > Yes, that makes more sense. I'll add the description for it at
> > > > next version
> > > patch.
> > > >
> > > > BTW, may I have one more question? In the current design ideas, we
> > > > partitioned the vGPU display eventfd counted 8-byte value into at
> > > > most
> > > > 8 events to deliver multiple display events, so we need different
> > > > increasement counter value to differentiate the events. As this is
> > > > the exposed thing the QEMU has to know, we plan adds a macro here
> > > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENTFD_BASE_SHIFT to make sure
> > > the
> > > > partitions shift in 1 byte, does it make sense putting here?
> > > > Looking forward
> > > to your and Gerd's comments. Thanks!
> > >
> > > Couldn't you expose this as another capability within the IRQ_INFO
> > > return data?  If you were to define it as a macro, I assume that
> > > means it would be hard coded, in which case this probably becomes an
> > > Intel specific IRQ, rather than what appears to be framed as a
> > > generic graphics IRQ extension.  A new capability could instead
> > > allow the vendor to specify their own value, where we could define how
> userspace should interpret and make use of this value.
> > > Thanks,
> > Good suggestion. Currently, vfio_irq_info is used to save one irq
> > info. What we need here is to use it to save several events info.
> > Maybe we could figure out a general layout of this capability so that
> > it can be leveraged by others, not only for display irq/events.
> 
> You could also expose a device specific IRQ with count > 1 (ie. similar to
> MSI/X) and avoid munging the eventfd value, which is not something we do
> elsewhere, at least in vfio.  Thanks,
Actually, we had this implementation before. At that time, we got the suggestion that count > 1 means more than one eventfd which might be not necessary.
Anyway, we can consider the "count > 1" again if anyone is agree on this. Thanks

BR,
Tina

> 
> Alex

Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type

[kraxel]

  Hi,

> > > Couldn't you expose this as another capability within the IRQ_INFO return
> > > data?  If you were to define it as a macro, I assume that means it would be
> > > hard coded, in which case this probably becomes an Intel specific IRQ, rather
> > > than what appears to be framed as a generic graphics IRQ extension.  A new
> > > capability could instead allow the vendor to specify their own value, where
> > > we could define how userspace should interpret and make use of this value.
> > > Thanks,  
> > Good suggestion. Currently, vfio_irq_info is used to save one irq
> > info. What we need here is to use it to save several events info.
> > Maybe we could figure out a general layout of this capability so that
> > it can be leveraged by others, not only for display irq/events.
> 
> You could also expose a device specific IRQ with count > 1 (ie. similar
> to MSI/X) and avoid munging the eventfd value, which is not something
> we do elsewhere, at least in vfio.  Thanks,

Well, the basic idea is to use the eventfd value to signal the kind of
changes which did happen, simliar to IRQ status register bits.

So, when the guest changes the primary plane, the mdev driver notes
this.  Same with the cursor plane.  On vblank (when the guests update
is actually applied) the mdev driver wakes the eventfd and uses eventfd
value to signal whenever primary plane or cursor plane or both did
change.

Then userspace knows which planes need an update without an extra
VFIO_DEVICE_QUERY_GFX_PLANE roundtrip to the kernel.

Alternatively we could have one eventfd for each change type.  But given
that these changes are typically applied at the same time (vblank) we
would have multiple eventfds being signaled at the same time.  Which
doesn't look ideal to me ...

cheers,
  Gerd

Re: [RFC PATCH v4 2/6] vfio: Introduce vGPU display irq type

[Alex Williamson]

On Fri, 2 Aug 2019 15:35:31 +0200
"kraxel@redhat.com" <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > > Couldn't you expose this as another capability within the IRQ_INFO return
> > > > data?  If you were to define it as a macro, I assume that means it would be
> > > > hard coded, in which case this probably becomes an Intel specific IRQ, rather
> > > > than what appears to be framed as a generic graphics IRQ extension.  A new
> > > > capability could instead allow the vendor to specify their own value, where
> > > > we could define how userspace should interpret and make use of this value.
> > > > Thanks,    
> > > Good suggestion. Currently, vfio_irq_info is used to save one irq
> > > info. What we need here is to use it to save several events info.
> > > Maybe we could figure out a general layout of this capability so that
> > > it can be leveraged by others, not only for display irq/events.  
> > 
> > You could also expose a device specific IRQ with count > 1 (ie. similar
> > to MSI/X) and avoid munging the eventfd value, which is not something
> > we do elsewhere, at least in vfio.  Thanks,  
> 
> Well, the basic idea is to use the eventfd value to signal the kind of
> changes which did happen, simliar to IRQ status register bits.
> 
> So, when the guest changes the primary plane, the mdev driver notes
> this.  Same with the cursor plane.  On vblank (when the guests update
> is actually applied) the mdev driver wakes the eventfd and uses eventfd
> value to signal whenever primary plane or cursor plane or both did
> change.
> 
> Then userspace knows which planes need an update without an extra
> VFIO_DEVICE_QUERY_GFX_PLANE roundtrip to the kernel.
> 
> Alternatively we could have one eventfd for each change type.  But given
> that these changes are typically applied at the same time (vblank) we
> would have multiple eventfds being signaled at the same time.  Which
> doesn't look ideal to me ...

Good point, looking at the bits in the eventfd value seems better than
a flood of concurrent interrupts.  Thanks,

Alex

[RFC PATCH v4 3/6] drm/i915/gvt: Register vGPU display event irq

[Kechen Lu]

From: Tina Zhang <tina.zhang@intel.com>

Gvt-g emulates and injects the vGPU's display interrupts in kernel
space. However the dma-buf based framebuffer consumer in the user
land (e.g. Qemu vfio/display) may also need to be notified by this
event.

Register the display irq as VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ to
each vGPU, so that the display interrupt event can be delivered to
userspace through eventfd.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/display.c   | 10 +++++--
 drivers/gpu/drm/i915/gvt/gvt.h       |  2 ++
 drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 43 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/mpt.h       | 17 +++++++++++
 5 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
index e1c313da6c00..1a0a4ae4826e 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -506,16 +506,22 @@ void intel_vgpu_clean_display(struct intel_vgpu *vgpu)
 int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 resolution)
 {
 	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+	int ret;
 
 	intel_vgpu_init_i2c_edid(vgpu);
 
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv) ||
 	    IS_COFFEELAKE(dev_priv))
-		return setup_virtual_dp_monitor(vgpu, PORT_D, GVT_DP_D,
+		ret = setup_virtual_dp_monitor(vgpu, PORT_D, GVT_DP_D,
 						resolution);
 	else
-		return setup_virtual_dp_monitor(vgpu, PORT_B, GVT_DP_B,
+		ret = setup_virtual_dp_monitor(vgpu, PORT_B, GVT_DP_B,
 						resolution);
+
+	if (ret == 0)
+		intel_gvt_hypervisor_register_display_irq(vgpu);
+
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index b54f2bdc13a4..64d1c1aaa42a 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -201,6 +201,8 @@ struct intel_vgpu {
 		struct mdev_device *mdev;
 		struct vfio_region *region;
 		int num_regions;
+		struct vfio_irq *irq;
+		int num_irqs;
 		struct eventfd_ctx *intx_trigger;
 		struct eventfd_ctx *msi_trigger;
 
diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
index 4862fb12778e..be33f20f3bc1 100644
--- a/drivers/gpu/drm/i915/gvt/hypercall.h
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -68,6 +68,7 @@ struct intel_gvt_mpt {
 			     bool map);
 	int (*set_opregion)(void *vgpu);
 	int (*set_edid)(void *vgpu, int port_num);
+	int (*register_display_irq)(void *vgpu);
 	int (*get_vfio_device)(void *vgpu);
 	void (*put_vfio_device)(void *vgpu);
 	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 144301b778df..6fe825763d05 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -78,6 +78,13 @@ struct vfio_region {
 	void				*data;
 };
 
+struct vfio_irq {
+	u32	type;
+	u32	subtype;
+	u32	flags;
+	u32	count;
+};
+
 struct vfio_edid_region {
 	struct vfio_region_gfx_edid vfio_edid_regs;
 	void *edid_blob;
@@ -635,6 +642,37 @@ static int kvmgt_set_edid(void *p_vgpu, int port_num)
 	return ret;
 }
 
+static int intel_vgpu_register_irq(struct intel_vgpu *vgpu,
+		unsigned int type, unsigned int subtype, u32 count, u32 flags)
+{
+	struct vfio_irq *irq;
+
+	irq = krealloc(vgpu->vdev.irq,
+			(vgpu->vdev.num_irqs + 1) * sizeof(*irq),
+			GFP_KERNEL);
+	if (!irq)
+		return -ENOMEM;
+
+	vgpu->vdev.irq = irq;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].type = type;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].subtype = subtype;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].count = count;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].flags = flags;
+	vgpu->vdev.num_irqs++;
+	return 0;
+}
+
+static int kvmgt_register_display_irq(void *p_vgpu)
+{
+	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
+
+	intel_vgpu_register_irq(vgpu, VFIO_IRQ_TYPE_GFX,
+				VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ,
+				1,
+				VFIO_IRQ_INFO_MASKABLE | VFIO_IRQ_INFO_EVENTFD);
+	return 0;
+}
+
 static void kvmgt_put_vfio_device(void *vgpu)
 {
 	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
@@ -1838,6 +1876,10 @@ static void kvmgt_detach_vgpu(void *p_vgpu)
 	vgpu->vdev.num_regions = 0;
 	kfree(vgpu->vdev.region);
 	vgpu->vdev.region = NULL;
+
+	vgpu->vdev.num_irqs = 0;
+	kfree(vgpu->vdev.irq);
+	vgpu->vdev.irq = NULL;
 }
 
 static int kvmgt_inject_msi(unsigned long handle, u32 addr, u16 data)
@@ -2039,6 +2081,7 @@ static struct intel_gvt_mpt kvmgt_mpt = {
 	.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
 	.set_opregion = kvmgt_set_opregion,
 	.set_edid = kvmgt_set_edid,
+	.register_display_irq = kvmgt_register_display_irq,
 	.get_vfio_device = kvmgt_get_vfio_device,
 	.put_vfio_device = kvmgt_put_vfio_device,
 	.is_valid_gfn = kvmgt_is_valid_gfn,
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
index 0f9440128123..abf4a69920d3 100644
--- a/drivers/gpu/drm/i915/gvt/mpt.h
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -330,6 +330,23 @@ static inline int intel_gvt_hypervisor_set_edid(struct intel_vgpu *vgpu,
 	return intel_gvt_host.mpt->set_edid(vgpu, port_num);
 }
 
+/**
+ * intel_gvt_hypervisor_set_irq - register vgpu specific irq
+ * @vgpu: a vGPU
+ * @port_num: display port number
+ *
+ * Returns:
+ * Zero on success, negative error code if failed.
+ */
+static inline int intel_gvt_hypervisor_register_display_irq(
+						struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->register_display_irq)
+		return 0;
+
+	return intel_gvt_host.mpt->register_display_irq(vgpu);
+}
+
 /**
  * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
  * @vgpu: a vGPU
-- 
2.17.1

[RFC PATCH v4 4/6] drm/i915/gvt: Deliver vGPU refresh event to userspace

[Kechen Lu]

From: Tina Zhang <tina.zhang@intel.com>

Deliver the display refresh events to the user land. Userspace can use
the irq mask/unmask mechanism to disable or enable the event delivery.

As we know, delivering refresh event at each vblank safely avoids
tearing and unexpected event overwhelming, but there are still spaces
to optimize.

For handling the normal case, deliver the page flip refresh
event at each vblank, in other words, bounded by vblanks. Skipping some
events bring performance enhancement while not hurting user experience.

For single framebuffer case, deliver the refresh events to userspace at
all vblanks. This heuristic at each vblank leverages pageflip_count
incresements to determine if there is no page flip happens after a certain
period and so that the case is regarded as single framebuffer one.
Although this heuristic makes incorrect decision sometimes and it depends
on guest behavior, for example, when no cursor movements happen, the
user experience does not harm and front buffer is still correctly acquired.
Meanwhile, in actual single framebuffer case, the user experience is
enhanced compared with page flip events only.

Addtionally, to mitigate the events delivering footprints, one eventfd and
8 byte eventfd counter partition are leveraged.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
Signed-off-by: Kechen Lu <kechen.lu@intel.com>
---
 drivers/gpu/drm/i915/gvt/display.c |  21 ++++
 drivers/gpu/drm/i915/gvt/gvt.h     |   7 ++
 drivers/gpu/drm/i915/gvt/kvmgt.c   | 154 +++++++++++++++++++++++++++--
 3 files changed, 173 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
index 1a0a4ae4826e..036db8199983 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -387,6 +387,8 @@ void intel_gvt_check_vblank_emulation(struct intel_gvt *gvt)
 	mutex_unlock(&gvt->lock);
 }
 
+#define PAGEFLIP_INC_COUNT 5
+
 static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
 {
 	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
@@ -396,7 +398,10 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
 		[PIPE_B] = PIPE_B_VBLANK,
 		[PIPE_C] = PIPE_C_VBLANK,
 	};
+	int pri_flip_event = SKL_FLIP_EVENT(pipe, PLANE_PRIMARY);
 	int event;
+	u64 eventfd_signal_val = 0;
+	static int pageflip_count;
 
 	if (pipe < PIPE_A || pipe > PIPE_C)
 		return;
@@ -407,11 +412,27 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
 		if (!pipe_is_enabled(vgpu, pipe))
 			continue;
 
+		if (event == pri_flip_event) {
+			eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
+			pageflip_count += PAGEFLIP_INC_COUNT;
+		}
+
 		intel_vgpu_trigger_virtual_event(vgpu, event);
 	}
 
+	if (--pageflip_count < 0) {
+		eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
+		pageflip_count = 0;
+	}
+
+	if (vgpu->vdev.vblank_trigger && !(vgpu->vdev.display_event_mask
+		& (DISPLAY_PRI_REFRESH_EVENT | DISPLAY_CUR_REFRESH_EVENT)) &&
+		eventfd_signal_val)
+		eventfd_signal(vgpu->vdev.vblank_trigger, eventfd_signal_val);
+
 	if (pipe_is_enabled(vgpu, pipe)) {
 		vgpu_vreg_t(vgpu, PIPE_FRMCOUNT_G4X(pipe))++;
+
 		intel_vgpu_trigger_virtual_event(vgpu, vblank_event[pipe]);
 	}
 }
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 64d1c1aaa42a..b654b6fa0663 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -165,6 +165,11 @@ struct intel_vgpu_submission {
 	bool active;
 };
 
+#define DISPLAY_PRI_REFRESH_EVENT	(1 << 0)
+#define DISPLAY_PRI_REFRESH_EVENT_INC	(1UL << 56)
+#define DISPLAY_CUR_REFRESH_EVENT	(1 << 1)
+#define DISPLAY_CUR_REFRESH_EVENT_INC	(1UL << 48)
+
 struct intel_vgpu {
 	struct intel_gvt *gvt;
 	struct mutex vgpu_lock;
@@ -205,6 +210,8 @@ struct intel_vgpu {
 		int num_irqs;
 		struct eventfd_ctx *intx_trigger;
 		struct eventfd_ctx *msi_trigger;
+		struct eventfd_ctx *vblank_trigger;
+		u32 display_event_mask;
 
 		/*
 		 * Two caches are used to avoid mapping duplicated pages (eg.
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 6fe825763d05..61c634618217 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1222,6 +1222,8 @@ static int intel_vgpu_get_irq_count(struct intel_vgpu *vgpu, int type)
 {
 	if (type == VFIO_PCI_INTX_IRQ_INDEX || type == VFIO_PCI_MSI_IRQ_INDEX)
 		return 1;
+	else if (type < VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs)
+		return vgpu->vdev.irq[type - VFIO_PCI_NUM_IRQS].count;
 
 	return 0;
 }
@@ -1269,7 +1271,62 @@ static int intel_vgpu_set_msi_trigger(struct intel_vgpu *vgpu,
 	return 0;
 }
 
-static int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
+static int intel_vgu_set_display_irq_mask(struct intel_vgpu *vgpu,
+		unsigned int index, unsigned int start, unsigned int count,
+		u32 flags, void *data)
+{
+	if (start != 0 || count > 2)
+		return -EINVAL;
+
+	if (flags & VFIO_IRQ_SET_DATA_NONE)
+		vgpu->vdev.display_event_mask |= DISPLAY_PRI_REFRESH_EVENT |
+			DISPLAY_CUR_REFRESH_EVENT;
+
+	return 0;
+}
+
+static int intel_vgu_set_display_irq_unmask(struct intel_vgpu *vgpu,
+		unsigned int index, unsigned int start, unsigned int count,
+		u32 flags, void *data)
+{
+	if (start != 0 || count > 2)
+		return -EINVAL;
+
+	if (flags & VFIO_IRQ_SET_DATA_NONE)
+		vgpu->vdev.display_event_mask &= ~(DISPLAY_PRI_REFRESH_EVENT |
+			   DISPLAY_CUR_REFRESH_EVENT);
+
+	return 0;
+}
+
+static int intel_vgpu_set_display_event_trigger(struct intel_vgpu *vgpu,
+		unsigned int index, unsigned int start, unsigned int count,
+		u32 flags, void *data)
+{
+	struct eventfd_ctx *trigger;
+
+	if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
+		int fd = *(int *)data;
+
+		trigger = eventfd_ctx_fdget(fd);
+		if (IS_ERR(trigger)) {
+			gvt_vgpu_err("eventfd_ctx_fdget failed\n");
+			return PTR_ERR(trigger);
+		}
+		vgpu->vdev.vblank_trigger = trigger;
+		vgpu->vdev.display_event_mask = 0;
+	} else if ((flags & VFIO_IRQ_SET_DATA_NONE) && !count) {
+		trigger = vgpu->vdev.vblank_trigger;
+		if (trigger) {
+			eventfd_ctx_put(trigger);
+			vgpu->vdev.vblank_trigger = NULL;
+		}
+	}
+
+	return 0;
+}
+
+int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
 		unsigned int index, unsigned int start, unsigned int count,
 		void *data)
 {
@@ -1302,6 +1359,35 @@ static int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
 			break;
 		}
 		break;
+	default:
+	{
+		int i;
+
+		if (index >= VFIO_PCI_NUM_IRQS +
+					vgpu->vdev.num_irqs)
+			return -EINVAL;
+		index =
+			array_index_nospec(index,
+						VFIO_PCI_NUM_IRQS +
+						vgpu->vdev.num_irqs);
+
+		i = index - VFIO_PCI_NUM_IRQS;
+		if (vgpu->vdev.irq[i].type == VFIO_IRQ_TYPE_GFX &&
+		    vgpu->vdev.irq[i].subtype ==
+		    VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ) {
+			switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
+			case VFIO_IRQ_SET_ACTION_MASK:
+				func = intel_vgu_set_display_irq_mask;
+				break;
+			case VFIO_IRQ_SET_ACTION_UNMASK:
+				func = intel_vgu_set_display_irq_unmask;
+				break;
+			case VFIO_IRQ_SET_ACTION_TRIGGER:
+				func = intel_vgpu_set_display_event_trigger;
+				break;
+			}
+		}
+	}
 	}
 
 	if (!func)
@@ -1333,7 +1419,7 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 		info.flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.num_regions = VFIO_PCI_NUM_REGIONS +
 				vgpu->vdev.num_regions;
-		info.num_irqs = VFIO_PCI_NUM_IRQS;
+		info.num_irqs = VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs;
 
 		return copy_to_user((void __user *)arg, &info, minsz) ?
 			-EFAULT : 0;
@@ -1493,32 +1579,81 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 			-EFAULT : 0;
 	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
 		struct vfio_irq_info info;
+		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
+		unsigned int i;
+		int ret;
 
 		minsz = offsetofend(struct vfio_irq_info, count);
 
 		if (copy_from_user(&info, (void __user *)arg, minsz))
 			return -EFAULT;
 
-		if (info.argsz < minsz || info.index >= VFIO_PCI_NUM_IRQS)
+		if (info.argsz < minsz)
 			return -EINVAL;
 
 		switch (info.index) {
 		case VFIO_PCI_INTX_IRQ_INDEX:
 		case VFIO_PCI_MSI_IRQ_INDEX:
+			info.flags = VFIO_IRQ_INFO_EVENTFD;
 			break;
-		default:
+		case VFIO_PCI_MSIX_IRQ_INDEX:
+		case VFIO_PCI_ERR_IRQ_INDEX:
+		case VFIO_PCI_REQ_IRQ_INDEX:
 			return -EINVAL;
-		}
+		default:
+		{
+			struct vfio_irq_info_cap_type cap_type = {
+				.header.id = VFIO_IRQ_INFO_CAP_TYPE,
+				.header.version = 1 };
 
-		info.flags = VFIO_IRQ_INFO_EVENTFD;
+			if (info.index >= VFIO_PCI_NUM_IRQS +
+					vgpu->vdev.num_irqs)
+				return -EINVAL;
+			info.index =
+				array_index_nospec(info.index,
+						VFIO_PCI_NUM_IRQS +
+						vgpu->vdev.num_irqs);
+
+			i = info.index - VFIO_PCI_NUM_IRQS;
+
+			info.flags = vgpu->vdev.irq[i].flags;
+			cap_type.type = vgpu->vdev.irq[i].type;
+			cap_type.subtype = vgpu->vdev.irq[i].subtype;
+
+			ret = vfio_info_add_capability(&caps,
+						&cap_type.header,
+						sizeof(cap_type));
+			if (ret)
+				return ret;
+		}
+		}
 
 		info.count = intel_vgpu_get_irq_count(vgpu, info.index);
 
 		if (info.index == VFIO_PCI_INTX_IRQ_INDEX)
 			info.flags |= (VFIO_IRQ_INFO_MASKABLE |
 				       VFIO_IRQ_INFO_AUTOMASKED);
-		else
-			info.flags |= VFIO_IRQ_INFO_NORESIZE;
+
+		if (caps.size) {
+			info.flags |= VFIO_IRQ_INFO_FLAG_CAPS;
+			if (info.argsz < sizeof(info) + caps.size) {
+				info.argsz = sizeof(info) + caps.size;
+				info.cap_offset = 0;
+			} else {
+				vfio_info_cap_shift(&caps, sizeof(info));
+				if (copy_to_user((void __user *)arg +
+						  sizeof(info), caps.buf,
+						  caps.size)) {
+					kfree(caps.buf);
+					return -EFAULT;
+				}
+				info.cap_offset = sizeof(info);
+				if (offsetofend(struct vfio_irq_info, cap_offset) > minsz)
+					minsz = offsetofend(struct vfio_irq_info, cap_offset);
+			}
+
+			kfree(caps.buf);
+		}
 
 		return copy_to_user((void __user *)arg, &info, minsz) ?
 			-EFAULT : 0;
@@ -1537,7 +1672,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 			int max = intel_vgpu_get_irq_count(vgpu, hdr.index);
 
 			ret = vfio_set_irqs_validate_and_prepare(&hdr, max,
-						VFIO_PCI_NUM_IRQS, &data_size);
+					VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs,
+								 &data_size);
 			if (ret) {
 				gvt_vgpu_err("intel:vfio_set_irqs_validate_and_prepare failed\n");
 				return -EINVAL;
-- 
2.17.1

Re: [RFC PATCH v4 4/6] drm/i915/gvt: Deliver vGPU refresh event to userspace

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 12055 bytes --]

On 2019.07.18 23:56:38 +0800, Kechen Lu wrote:
> From: Tina Zhang <tina.zhang@intel.com>
> 
> Deliver the display refresh events to the user land. Userspace can use
> the irq mask/unmask mechanism to disable or enable the event delivery.
> 
> As we know, delivering refresh event at each vblank safely avoids
> tearing and unexpected event overwhelming, but there are still spaces
> to optimize.
> 
> For handling the normal case, deliver the page flip refresh
> event at each vblank, in other words, bounded by vblanks. Skipping some
> events bring performance enhancement while not hurting user experience.
> 
> For single framebuffer case, deliver the refresh events to userspace at
> all vblanks. This heuristic at each vblank leverages pageflip_count
> incresements to determine if there is no page flip happens after a certain
> period and so that the case is regarded as single framebuffer one.
> Although this heuristic makes incorrect decision sometimes and it depends
> on guest behavior, for example, when no cursor movements happen, the
> user experience does not harm and front buffer is still correctly acquired.
> Meanwhile, in actual single framebuffer case, the user experience is
> enhanced compared with page flip events only.
> 
> Addtionally, to mitigate the events delivering footprints, one eventfd and
> 8 byte eventfd counter partition are leveraged.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> Signed-off-by: Kechen Lu <kechen.lu@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/display.c |  21 ++++
>  drivers/gpu/drm/i915/gvt/gvt.h     |   7 ++
>  drivers/gpu/drm/i915/gvt/kvmgt.c   | 154 +++++++++++++++++++++++++++--
>  3 files changed, 173 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
> index 1a0a4ae4826e..036db8199983 100644
> --- a/drivers/gpu/drm/i915/gvt/display.c
> +++ b/drivers/gpu/drm/i915/gvt/display.c
> @@ -387,6 +387,8 @@ void intel_gvt_check_vblank_emulation(struct intel_gvt *gvt)
>  	mutex_unlock(&gvt->lock);
>  }
>  
> +#define PAGEFLIP_INC_COUNT 5
> +
>  static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> @@ -396,7 +398,10 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
>  		[PIPE_B] = PIPE_B_VBLANK,
>  		[PIPE_C] = PIPE_C_VBLANK,
>  	};
> +	int pri_flip_event = SKL_FLIP_EVENT(pipe, PLANE_PRIMARY);
>  	int event;
> +	u64 eventfd_signal_val = 0;
> +	static int pageflip_count;
>  
>  	if (pipe < PIPE_A || pipe > PIPE_C)
>  		return;
> @@ -407,11 +412,27 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
>  		if (!pipe_is_enabled(vgpu, pipe))
>  			continue;
>  
> +		if (event == pri_flip_event) {
> +			eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
> +			pageflip_count += PAGEFLIP_INC_COUNT;
> +		}
> +
>  		intel_vgpu_trigger_virtual_event(vgpu, event);
>  	}
>  
> +	if (--pageflip_count < 0) {
> +		eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
> +		pageflip_count = 0;
> +	}

If pageflip_count has been increased to a big number from page flip
event for some time, then if guest switch for single buffer render, it
would take 5x vblank time to send refresh then..

> +
> +	if (vgpu->vdev.vblank_trigger && !(vgpu->vdev.display_event_mask
> +		& (DISPLAY_PRI_REFRESH_EVENT | DISPLAY_CUR_REFRESH_EVENT)) &&
> +		eventfd_signal_val)
> +		eventfd_signal(vgpu->vdev.vblank_trigger, eventfd_signal_val);
> +
>  	if (pipe_is_enabled(vgpu, pipe)) {
>  		vgpu_vreg_t(vgpu, PIPE_FRMCOUNT_G4X(pipe))++;
> +
>  		intel_vgpu_trigger_virtual_event(vgpu, vblank_event[pipe]);
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index 64d1c1aaa42a..b654b6fa0663 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -165,6 +165,11 @@ struct intel_vgpu_submission {
>  	bool active;
>  };
>  
> +#define DISPLAY_PRI_REFRESH_EVENT	(1 << 0)
> +#define DISPLAY_PRI_REFRESH_EVENT_INC	(1UL << 56)
> +#define DISPLAY_CUR_REFRESH_EVENT	(1 << 1)
> +#define DISPLAY_CUR_REFRESH_EVENT_INC	(1UL << 48)
> +

As this is for eventfd interface definition, need to put in vfio header instead of gvt's,
as this is userspace API. And better reorder for different usage on irq masking and eventfd value.

For eventfd value, this looks like counter for each plane? Or do we just need a flag?

>  struct intel_vgpu {
>  	struct intel_gvt *gvt;
>  	struct mutex vgpu_lock;
> @@ -205,6 +210,8 @@ struct intel_vgpu {
>  		int num_irqs;
>  		struct eventfd_ctx *intx_trigger;
>  		struct eventfd_ctx *msi_trigger;
> +		struct eventfd_ctx *vblank_trigger;
> +		u32 display_event_mask;
>  
>  		/*
>  		 * Two caches are used to avoid mapping duplicated pages (eg.
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 6fe825763d05..61c634618217 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1222,6 +1222,8 @@ static int intel_vgpu_get_irq_count(struct intel_vgpu *vgpu, int type)
>  {
>  	if (type == VFIO_PCI_INTX_IRQ_INDEX || type == VFIO_PCI_MSI_IRQ_INDEX)
>  		return 1;
> +	else if (type < VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs)
> +		return vgpu->vdev.irq[type - VFIO_PCI_NUM_IRQS].count;
>  
>  	return 0;
>  }
> @@ -1269,7 +1271,62 @@ static int intel_vgpu_set_msi_trigger(struct intel_vgpu *vgpu,
>  	return 0;
>  }
>  
> -static int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
> +static int intel_vgu_set_display_irq_mask(struct intel_vgpu *vgpu,
> +		unsigned int index, unsigned int start, unsigned int count,
> +		u32 flags, void *data)
> +{
> +	if (start != 0 || count > 2)
> +		return -EINVAL;
> +
> +	if (flags & VFIO_IRQ_SET_DATA_NONE)
> +		vgpu->vdev.display_event_mask |= DISPLAY_PRI_REFRESH_EVENT |
> +			DISPLAY_CUR_REFRESH_EVENT;
> +
> +	return 0;
> +}
> +
> +static int intel_vgu_set_display_irq_unmask(struct intel_vgpu *vgpu,
> +		unsigned int index, unsigned int start, unsigned int count,
> +		u32 flags, void *data)
> +{
> +	if (start != 0 || count > 2)
> +		return -EINVAL;
> +
> +	if (flags & VFIO_IRQ_SET_DATA_NONE)
> +		vgpu->vdev.display_event_mask &= ~(DISPLAY_PRI_REFRESH_EVENT |
> +			   DISPLAY_CUR_REFRESH_EVENT);
> +
> +	return 0;
> +}
> +
> +static int intel_vgpu_set_display_event_trigger(struct intel_vgpu *vgpu,
> +		unsigned int index, unsigned int start, unsigned int count,
> +		u32 flags, void *data)
> +{
> +	struct eventfd_ctx *trigger;
> +
> +	if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> +		int fd = *(int *)data;
> +
> +		trigger = eventfd_ctx_fdget(fd);
> +		if (IS_ERR(trigger)) {
> +			gvt_vgpu_err("eventfd_ctx_fdget failed\n");
> +			return PTR_ERR(trigger);
> +		}
> +		vgpu->vdev.vblank_trigger = trigger;
> +		vgpu->vdev.display_event_mask = 0;
> +	} else if ((flags & VFIO_IRQ_SET_DATA_NONE) && !count) {
> +		trigger = vgpu->vdev.vblank_trigger;
> +		if (trigger) {
> +			eventfd_ctx_put(trigger);
> +			vgpu->vdev.vblank_trigger = NULL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
>  		unsigned int index, unsigned int start, unsigned int count,
>  		void *data)
>  {
> @@ -1302,6 +1359,35 @@ static int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
>  			break;
>  		}
>  		break;
> +	default:
> +	{
> +		int i;
> +
> +		if (index >= VFIO_PCI_NUM_IRQS +
> +					vgpu->vdev.num_irqs)
> +			return -EINVAL;
> +		index =
> +			array_index_nospec(index,
> +						VFIO_PCI_NUM_IRQS +
> +						vgpu->vdev.num_irqs);
> +
> +		i = index - VFIO_PCI_NUM_IRQS;
> +		if (vgpu->vdev.irq[i].type == VFIO_IRQ_TYPE_GFX &&
> +		    vgpu->vdev.irq[i].subtype ==
> +		    VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ) {
> +			switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> +			case VFIO_IRQ_SET_ACTION_MASK:
> +				func = intel_vgu_set_display_irq_mask;
> +				break;
> +			case VFIO_IRQ_SET_ACTION_UNMASK:
> +				func = intel_vgu_set_display_irq_unmask;
> +				break;
> +			case VFIO_IRQ_SET_ACTION_TRIGGER:
> +				func = intel_vgpu_set_display_event_trigger;
> +				break;
> +			}
> +		}
> +	}
>  	}
>  
>  	if (!func)
> @@ -1333,7 +1419,7 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
>  		info.flags |= VFIO_DEVICE_FLAGS_RESET;
>  		info.num_regions = VFIO_PCI_NUM_REGIONS +
>  				vgpu->vdev.num_regions;
> -		info.num_irqs = VFIO_PCI_NUM_IRQS;
> +		info.num_irqs = VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs;
>  
>  		return copy_to_user((void __user *)arg, &info, minsz) ?
>  			-EFAULT : 0;
> @@ -1493,32 +1579,81 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
>  			-EFAULT : 0;
>  	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
>  		struct vfio_irq_info info;
> +		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> +		unsigned int i;
> +		int ret;
>  
>  		minsz = offsetofend(struct vfio_irq_info, count);
>  
>  		if (copy_from_user(&info, (void __user *)arg, minsz))
>  			return -EFAULT;
>  
> -		if (info.argsz < minsz || info.index >= VFIO_PCI_NUM_IRQS)
> +		if (info.argsz < minsz)
>  			return -EINVAL;
>  
>  		switch (info.index) {
>  		case VFIO_PCI_INTX_IRQ_INDEX:
>  		case VFIO_PCI_MSI_IRQ_INDEX:
> +			info.flags = VFIO_IRQ_INFO_EVENTFD;
>  			break;
> -		default:
> +		case VFIO_PCI_MSIX_IRQ_INDEX:
> +		case VFIO_PCI_ERR_IRQ_INDEX:
> +		case VFIO_PCI_REQ_IRQ_INDEX:
>  			return -EINVAL;
> -		}
> +		default:
> +		{
> +			struct vfio_irq_info_cap_type cap_type = {
> +				.header.id = VFIO_IRQ_INFO_CAP_TYPE,
> +				.header.version = 1 };
>  
> -		info.flags = VFIO_IRQ_INFO_EVENTFD;
> +			if (info.index >= VFIO_PCI_NUM_IRQS +
> +					vgpu->vdev.num_irqs)
> +				return -EINVAL;
> +			info.index =
> +				array_index_nospec(info.index,
> +						VFIO_PCI_NUM_IRQS +
> +						vgpu->vdev.num_irqs);
> +
> +			i = info.index - VFIO_PCI_NUM_IRQS;
> +
> +			info.flags = vgpu->vdev.irq[i].flags;
> +			cap_type.type = vgpu->vdev.irq[i].type;
> +			cap_type.subtype = vgpu->vdev.irq[i].subtype;
> +
> +			ret = vfio_info_add_capability(&caps,
> +						&cap_type.header,
> +						sizeof(cap_type));
> +			if (ret)
> +				return ret;
> +		}
> +		}
>  
>  		info.count = intel_vgpu_get_irq_count(vgpu, info.index);
>  
>  		if (info.index == VFIO_PCI_INTX_IRQ_INDEX)
>  			info.flags |= (VFIO_IRQ_INFO_MASKABLE |
>  				       VFIO_IRQ_INFO_AUTOMASKED);
> -		else
> -			info.flags |= VFIO_IRQ_INFO_NORESIZE;
> +
> +		if (caps.size) {
> +			info.flags |= VFIO_IRQ_INFO_FLAG_CAPS;
> +			if (info.argsz < sizeof(info) + caps.size) {
> +				info.argsz = sizeof(info) + caps.size;
> +				info.cap_offset = 0;
> +			} else {
> +				vfio_info_cap_shift(&caps, sizeof(info));
> +				if (copy_to_user((void __user *)arg +
> +						  sizeof(info), caps.buf,
> +						  caps.size)) {
> +					kfree(caps.buf);
> +					return -EFAULT;
> +				}
> +				info.cap_offset = sizeof(info);
> +				if (offsetofend(struct vfio_irq_info, cap_offset) > minsz)
> +					minsz = offsetofend(struct vfio_irq_info, cap_offset);
> +			}
> +
> +			kfree(caps.buf);
> +		}
>  
>  		return copy_to_user((void __user *)arg, &info, minsz) ?
>  			-EFAULT : 0;
> @@ -1537,7 +1672,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
>  			int max = intel_vgpu_get_irq_count(vgpu, hdr.index);
>  
>  			ret = vfio_set_irqs_validate_and_prepare(&hdr, max,
> -						VFIO_PCI_NUM_IRQS, &data_size);
> +					VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs,
> +								 &data_size);
>  			if (ret) {
>  				gvt_vgpu_err("intel:vfio_set_irqs_validate_and_prepare failed\n");
>  				return -EINVAL;
> -- 
> 2.17.1
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [RFC PATCH v4 4/6] drm/i915/gvt: Deliver vGPU refresh event to userspace

[Lu, Kechen]

Hi, 

> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Friday, July 19, 2019 2:25 PM
> To: Lu, Kechen <kechen.lu@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> kraxel@redhat.com; zhenyuw@linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, Kevin
> <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>;
> alex.williamson@redhat.com
> Subject: Re: [RFC PATCH v4 4/6] drm/i915/gvt: Deliver vGPU refresh event to
> userspace
> 
> On 2019.07.18 23:56:38 +0800, Kechen Lu wrote:
> > From: Tina Zhang <tina.zhang@intel.com>
> >
> > Deliver the display refresh events to the user land. Userspace can use
> > the irq mask/unmask mechanism to disable or enable the event delivery.
> >
> > As we know, delivering refresh event at each vblank safely avoids
> > tearing and unexpected event overwhelming, but there are still spaces
> > to optimize.
> >
> > For handling the normal case, deliver the page flip refresh event at
> > each vblank, in other words, bounded by vblanks. Skipping some events
> > bring performance enhancement while not hurting user experience.
> >
> > For single framebuffer case, deliver the refresh events to userspace
> > at all vblanks. This heuristic at each vblank leverages pageflip_count
> > incresements to determine if there is no page flip happens after a
> > certain period and so that the case is regarded as single framebuffer one.
> > Although this heuristic makes incorrect decision sometimes and it
> > depends on guest behavior, for example, when no cursor movements
> > happen, the user experience does not harm and front buffer is still correctly
> acquired.
> > Meanwhile, in actual single framebuffer case, the user experience is
> > enhanced compared with page flip events only.
> >
> > Addtionally, to mitigate the events delivering footprints, one eventfd
> > and
> > 8 byte eventfd counter partition are leveraged.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > Signed-off-by: Kechen Lu <kechen.lu@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/display.c |  21 ++++
> >  drivers/gpu/drm/i915/gvt/gvt.h     |   7 ++
> >  drivers/gpu/drm/i915/gvt/kvmgt.c   | 154 +++++++++++++++++++++++++++--
> >  3 files changed, 173 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/display.c
> > b/drivers/gpu/drm/i915/gvt/display.c
> > index 1a0a4ae4826e..036db8199983 100644
> > --- a/drivers/gpu/drm/i915/gvt/display.c
> > +++ b/drivers/gpu/drm/i915/gvt/display.c
> > @@ -387,6 +387,8 @@ void intel_gvt_check_vblank_emulation(struct
> intel_gvt *gvt)
> >  	mutex_unlock(&gvt->lock);
> >  }
> >
> > +#define PAGEFLIP_INC_COUNT 5
> > +
> >  static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
> > {
> >  	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv; @@ -396,7
> > +398,10 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int
> pipe)
> >  		[PIPE_B] = PIPE_B_VBLANK,
> >  		[PIPE_C] = PIPE_C_VBLANK,
> >  	};
> > +	int pri_flip_event = SKL_FLIP_EVENT(pipe, PLANE_PRIMARY);
> >  	int event;
> > +	u64 eventfd_signal_val = 0;
> > +	static int pageflip_count;
> >
> >  	if (pipe < PIPE_A || pipe > PIPE_C)
> >  		return;
> > @@ -407,11 +412,27 @@ static void emulate_vblank_on_pipe(struct
> intel_vgpu *vgpu, int pipe)
> >  		if (!pipe_is_enabled(vgpu, pipe))
> >  			continue;
> >
> > +		if (event == pri_flip_event) {
> > +			eventfd_signal_val +=
> DISPLAY_PRI_REFRESH_EVENT_INC;
> > +			pageflip_count += PAGEFLIP_INC_COUNT;
> > +		}
> > +
> >  		intel_vgpu_trigger_virtual_event(vgpu, event);
> >  	}
> >
> > +	if (--pageflip_count < 0) {
> > +		eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
> > +		pageflip_count = 0;
> > +	}
> 
> If pageflip_count has been increased to a big number from page flip event for
> some time, then if guest switch for single buffer render, it would take 5x vblank
> time to send refresh then..
> 

Yep. You are right. That could be the corner case. After discussing with Tina, we can change the pageflip_count to nopageflip_count using a changed heuristic to avoid this issue and bound the delay to begin refresh. 

> > +
> > +	if (vgpu->vdev.vblank_trigger && !(vgpu->vdev.display_event_mask
> > +		& (DISPLAY_PRI_REFRESH_EVENT |
> DISPLAY_CUR_REFRESH_EVENT)) &&
> > +		eventfd_signal_val)
> > +		eventfd_signal(vgpu->vdev.vblank_trigger, eventfd_signal_val);
> > +
> >  	if (pipe_is_enabled(vgpu, pipe)) {
> >  		vgpu_vreg_t(vgpu, PIPE_FRMCOUNT_G4X(pipe))++;
> > +
> >  		intel_vgpu_trigger_virtual_event(vgpu, vblank_event[pipe]);
> >  	}
> >  }
> > diff --git a/drivers/gpu/drm/i915/gvt/gvt.h
> > b/drivers/gpu/drm/i915/gvt/gvt.h index 64d1c1aaa42a..b654b6fa0663
> > 100644
> > --- a/drivers/gpu/drm/i915/gvt/gvt.h
> > +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> > @@ -165,6 +165,11 @@ struct intel_vgpu_submission {
> >  	bool active;
> >  };
> >
> > +#define DISPLAY_PRI_REFRESH_EVENT	(1 << 0)
> > +#define DISPLAY_PRI_REFRESH_EVENT_INC	(1UL << 56)
> > +#define DISPLAY_CUR_REFRESH_EVENT	(1 << 1)
> > +#define DISPLAY_CUR_REFRESH_EVENT_INC	(1UL << 48)
> > +
> 
> As this is for eventfd interface definition, need to put in vfio header instead of
> gvt's, as this is userspace API. And better reorder for different usage on irq
> masking and eventfd value.
> 

I think I made redundant extra flags here. Currently if we are using single irq masking/unmasking, DISPLAY_XXX_REFRESH_EVENT defined is unnecessary. I'll change it in the next version patch.

> For eventfd value, this looks like counter for each plane? Or do we just need a
> flag?
> 

We need the count val for each plane here in userspace, and we need this to perform partitioned eventfd value checking logic. But it seems too specific to be defined in vfio header. After discussing with Tina, we think we can define a base mask like  VFIO_IRQ_EVENTFD_COUNT_BASE_MASK as 0xff, to read/write 1 byte of 8-byte counter of eventfd, and shifted by the drm plane specific flag, but it still needs the discussion with Gerd and Alex.
Thanks.

Best Regards,
Kechen

> >  struct intel_vgpu {
> >  	struct intel_gvt *gvt;
> >  	struct mutex vgpu_lock;
> > @@ -205,6 +210,8 @@ struct intel_vgpu {
> >  		int num_irqs;
> >  		struct eventfd_ctx *intx_trigger;
> >  		struct eventfd_ctx *msi_trigger;
> > +		struct eventfd_ctx *vblank_trigger;
> > +		u32 display_event_mask;
> >
> >  		/*
> >  		 * Two caches are used to avoid mapping duplicated pages (eg.
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index 6fe825763d05..61c634618217 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -1222,6 +1222,8 @@ static int intel_vgpu_get_irq_count(struct
> > intel_vgpu *vgpu, int type)  {
> >  	if (type == VFIO_PCI_INTX_IRQ_INDEX || type ==
> VFIO_PCI_MSI_IRQ_INDEX)
> >  		return 1;
> > +	else if (type < VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs)
> > +		return vgpu->vdev.irq[type - VFIO_PCI_NUM_IRQS].count;
> >
> >  	return 0;
> >  }
> > @@ -1269,7 +1271,62 @@ static int intel_vgpu_set_msi_trigger(struct
> intel_vgpu *vgpu,
> >  	return 0;
> >  }
> >
> > -static int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
> > +static int intel_vgu_set_display_irq_mask(struct intel_vgpu *vgpu,
> > +		unsigned int index, unsigned int start, unsigned int count,
> > +		u32 flags, void *data)
> > +{
> > +	if (start != 0 || count > 2)
> > +		return -EINVAL;
> > +
> > +	if (flags & VFIO_IRQ_SET_DATA_NONE)
> > +		vgpu->vdev.display_event_mask |=
> DISPLAY_PRI_REFRESH_EVENT |
> > +			DISPLAY_CUR_REFRESH_EVENT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int intel_vgu_set_display_irq_unmask(struct intel_vgpu *vgpu,
> > +		unsigned int index, unsigned int start, unsigned int count,
> > +		u32 flags, void *data)
> > +{
> > +	if (start != 0 || count > 2)
> > +		return -EINVAL;
> > +
> > +	if (flags & VFIO_IRQ_SET_DATA_NONE)
> > +		vgpu->vdev.display_event_mask &=
> ~(DISPLAY_PRI_REFRESH_EVENT |
> > +			   DISPLAY_CUR_REFRESH_EVENT);
> > +
> > +	return 0;
> > +}
> > +
> > +static int intel_vgpu_set_display_event_trigger(struct intel_vgpu *vgpu,
> > +		unsigned int index, unsigned int start, unsigned int count,
> > +		u32 flags, void *data)
> > +{
> > +	struct eventfd_ctx *trigger;
> > +
> > +	if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> > +		int fd = *(int *)data;
> > +
> > +		trigger = eventfd_ctx_fdget(fd);
> > +		if (IS_ERR(trigger)) {
> > +			gvt_vgpu_err("eventfd_ctx_fdget failed\n");
> > +			return PTR_ERR(trigger);
> > +		}
> > +		vgpu->vdev.vblank_trigger = trigger;
> > +		vgpu->vdev.display_event_mask = 0;
> > +	} else if ((flags & VFIO_IRQ_SET_DATA_NONE) && !count) {
> > +		trigger = vgpu->vdev.vblank_trigger;
> > +		if (trigger) {
> > +			eventfd_ctx_put(trigger);
> > +			vgpu->vdev.vblank_trigger = NULL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
> >  		unsigned int index, unsigned int start, unsigned int count,
> >  		void *data)
> >  {
> > @@ -1302,6 +1359,35 @@ static int intel_vgpu_set_irqs(struct intel_vgpu
> *vgpu, u32 flags,
> >  			break;
> >  		}
> >  		break;
> > +	default:
> > +	{
> > +		int i;
> > +
> > +		if (index >= VFIO_PCI_NUM_IRQS +
> > +					vgpu->vdev.num_irqs)
> > +			return -EINVAL;
> > +		index =
> > +			array_index_nospec(index,
> > +						VFIO_PCI_NUM_IRQS +
> > +						vgpu->vdev.num_irqs);
> > +
> > +		i = index - VFIO_PCI_NUM_IRQS;
> > +		if (vgpu->vdev.irq[i].type == VFIO_IRQ_TYPE_GFX &&
> > +		    vgpu->vdev.irq[i].subtype ==
> > +		    VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ) {
> > +			switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> > +			case VFIO_IRQ_SET_ACTION_MASK:
> > +				func = intel_vgu_set_display_irq_mask;
> > +				break;
> > +			case VFIO_IRQ_SET_ACTION_UNMASK:
> > +				func = intel_vgu_set_display_irq_unmask;
> > +				break;
> > +			case VFIO_IRQ_SET_ACTION_TRIGGER:
> > +				func = intel_vgpu_set_display_event_trigger;
> > +				break;
> > +			}
> > +		}
> > +	}
> >  	}
> >
> >  	if (!func)
> > @@ -1333,7 +1419,7 @@ static long intel_vgpu_ioctl(struct mdev_device
> *mdev, unsigned int cmd,
> >  		info.flags |= VFIO_DEVICE_FLAGS_RESET;
> >  		info.num_regions = VFIO_PCI_NUM_REGIONS +
> >  				vgpu->vdev.num_regions;
> > -		info.num_irqs = VFIO_PCI_NUM_IRQS;
> > +		info.num_irqs = VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs;
> >
> >  		return copy_to_user((void __user *)arg, &info, minsz) ?
> >  			-EFAULT : 0;
> > @@ -1493,32 +1579,81 @@ static long intel_vgpu_ioctl(struct mdev_device
> *mdev, unsigned int cmd,
> >  			-EFAULT : 0;
> >  	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
> >  		struct vfio_irq_info info;
> > +		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> > +		unsigned int i;
> > +		int ret;
> >
> >  		minsz = offsetofend(struct vfio_irq_info, count);
> >
> >  		if (copy_from_user(&info, (void __user *)arg, minsz))
> >  			return -EFAULT;
> >
> > -		if (info.argsz < minsz || info.index >= VFIO_PCI_NUM_IRQS)
> > +		if (info.argsz < minsz)
> >  			return -EINVAL;
> >
> >  		switch (info.index) {
> >  		case VFIO_PCI_INTX_IRQ_INDEX:
> >  		case VFIO_PCI_MSI_IRQ_INDEX:
> > +			info.flags = VFIO_IRQ_INFO_EVENTFD;
> >  			break;
> > -		default:
> > +		case VFIO_PCI_MSIX_IRQ_INDEX:
> > +		case VFIO_PCI_ERR_IRQ_INDEX:
> > +		case VFIO_PCI_REQ_IRQ_INDEX:
> >  			return -EINVAL;
> > -		}
> > +		default:
> > +		{
> > +			struct vfio_irq_info_cap_type cap_type = {
> > +				.header.id = VFIO_IRQ_INFO_CAP_TYPE,
> > +				.header.version = 1 };
> >
> > -		info.flags = VFIO_IRQ_INFO_EVENTFD;
> > +			if (info.index >= VFIO_PCI_NUM_IRQS +
> > +					vgpu->vdev.num_irqs)
> > +				return -EINVAL;
> > +			info.index =
> > +				array_index_nospec(info.index,
> > +						VFIO_PCI_NUM_IRQS +
> > +						vgpu->vdev.num_irqs);
> > +
> > +			i = info.index - VFIO_PCI_NUM_IRQS;
> > +
> > +			info.flags = vgpu->vdev.irq[i].flags;
> > +			cap_type.type = vgpu->vdev.irq[i].type;
> > +			cap_type.subtype = vgpu->vdev.irq[i].subtype;
> > +
> > +			ret = vfio_info_add_capability(&caps,
> > +						&cap_type.header,
> > +						sizeof(cap_type));
> > +			if (ret)
> > +				return ret;
> > +		}
> > +		}
> >
> >  		info.count = intel_vgpu_get_irq_count(vgpu, info.index);
> >
> >  		if (info.index == VFIO_PCI_INTX_IRQ_INDEX)
> >  			info.flags |= (VFIO_IRQ_INFO_MASKABLE |
> >  				       VFIO_IRQ_INFO_AUTOMASKED);
> > -		else
> > -			info.flags |= VFIO_IRQ_INFO_NORESIZE;
> > +
> > +		if (caps.size) {
> > +			info.flags |= VFIO_IRQ_INFO_FLAG_CAPS;
> > +			if (info.argsz < sizeof(info) + caps.size) {
> > +				info.argsz = sizeof(info) + caps.size;
> > +				info.cap_offset = 0;
> > +			} else {
> > +				vfio_info_cap_shift(&caps, sizeof(info));
> > +				if (copy_to_user((void __user *)arg +
> > +						  sizeof(info), caps.buf,
> > +						  caps.size)) {
> > +					kfree(caps.buf);
> > +					return -EFAULT;
> > +				}
> > +				info.cap_offset = sizeof(info);
> > +				if (offsetofend(struct vfio_irq_info, cap_offset) >
> minsz)
> > +					minsz = offsetofend(struct
> vfio_irq_info, cap_offset);
> > +			}
> > +
> > +			kfree(caps.buf);
> > +		}
> >
> >  		return copy_to_user((void __user *)arg, &info, minsz) ?
> >  			-EFAULT : 0;
> > @@ -1537,7 +1672,8 @@ static long intel_vgpu_ioctl(struct mdev_device
> *mdev, unsigned int cmd,
> >  			int max = intel_vgpu_get_irq_count(vgpu, hdr.index);
> >
> >  			ret = vfio_set_irqs_validate_and_prepare(&hdr, max,
> > -						VFIO_PCI_NUM_IRQS,
> &data_size);
> > +					VFIO_PCI_NUM_IRQS + vgpu-
> >vdev.num_irqs,
> > +								 &data_size);
> >  			if (ret) {
> >
> 	gvt_vgpu_err("intel:vfio_set_irqs_validate_and_prepare failed\n");
> >  				return -EINVAL;
> > --
> > 2.17.1
> >
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[RFC PATCH v4 5/6] drm/i915/gvt: Deliver async primary plane page flip events at vblank

[Kechen Lu]

Only sync primary plane page flip events are checked and delivered
as the display refresh events before, this patch tries to deliver async
primary page flip events bounded by vblanks.

To deliver correct async page flip, the new async flip bitmap is
introduced and in vblank emulation handler to check bitset.

Signed-off-by: Kechen Lu <kechen.lu@intel.com>
---
 drivers/gpu/drm/i915/gvt/cmd_parser.c |  5 +++--
 drivers/gpu/drm/i915/gvt/display.c    | 12 ++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.h        |  2 ++
 drivers/gpu/drm/i915/gvt/handlers.c   |  5 +++--
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
index 5cb59c0b4bbe..e504cc7be559 100644
--- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
+++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
@@ -1334,9 +1334,10 @@ static int gen8_update_plane_mmio_from_mi_display_flip(
 	if (info->plane == PLANE_PRIMARY)
 		vgpu_vreg_t(vgpu, PIPE_FLIPCOUNT_G4X(info->pipe))++;
 
-	if (info->async_flip)
+	if (info->async_flip) {
 		intel_vgpu_trigger_virtual_event(vgpu, info->event);
-	else
+		set_bit(info->plane, vgpu->display.async_flip_event[info->pipe]);
+	} else
 		set_bit(info->event, vgpu->irq.flip_done_event[info->pipe]);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
index 036db8199983..df52e4b4c1b0 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -420,6 +420,18 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
 		intel_vgpu_trigger_virtual_event(vgpu, event);
 	}
 
+	for_each_set_bit(event, vgpu->display.async_flip_event[pipe],
+			I915_MAX_PLANES) {
+		clear_bit(event, vgpu->display.async_flip_event[pipe]);
+		if (!pipe_is_enabled(vgpu, pipe))
+			continue;
+
+		if (event == PLANE_PRIMARY) {
+			eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
+			pageflip_count += PAGEFLIP_INC_COUNT;
+		}
+	}
+
 	if (--pageflip_count < 0) {
 		eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
 		pageflip_count = 0;
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index b654b6fa0663..98a3dc309acb 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -128,6 +128,8 @@ struct intel_vgpu_display {
 	struct intel_vgpu_i2c_edid i2c_edid;
 	struct intel_vgpu_port ports[I915_MAX_PORTS];
 	struct intel_vgpu_sbi sbi;
+	DECLARE_BITMAP(async_flip_event[I915_MAX_PIPES],
+		       I915_MAX_PLANES);
 };
 
 struct vgpu_sched_ctl {
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
index e09bd6e0cc4d..6ad29c4f08e5 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -758,9 +758,10 @@ static int pri_surf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
 
 	vgpu_vreg_t(vgpu, PIPE_FLIPCOUNT_G4X(pipe))++;
 
-	if (vgpu_vreg_t(vgpu, DSPCNTR(pipe)) & PLANE_CTL_ASYNC_FLIP)
+	if (vgpu_vreg_t(vgpu, DSPCNTR(pipe)) & PLANE_CTL_ASYNC_FLIP) {
 		intel_vgpu_trigger_virtual_event(vgpu, event);
-	else
+		set_bit(PLANE_PRIMARY, vgpu->display.async_flip_event[pipe]);
+	} else
 		set_bit(event, vgpu->irq.flip_done_event[pipe]);
 
 	return 0;
-- 
2.17.1

[RFC PATCH v4 6/6] drm/i915/gvt: Add cursor plane reg update trap emulation handler

[Kechen Lu]

This patch adds the cursor plane CURBASE reg update trap handler
in order to :

- Deliver the cursor refresh event at each vblank emulation,
the flip_done_event bit check is supposed to do here. If cursor
plane updates happen, deliver the cursor refresh events.

- Support the sync and async cursor plane updates and
corresponding cursor plane flip interrupts reporting.

Signed-off-by: Kechen Lu <kechen.lu@intel.com>
---
 drivers/gpu/drm/i915/gvt/display.c   | 11 +++++++++++
 drivers/gpu/drm/i915/gvt/handlers.c  | 27 ++++++++++++++++++++++++---
 drivers/gpu/drm/i915/gvt/interrupt.c |  7 +++++++
 drivers/gpu/drm/i915/gvt/interrupt.h |  3 +++
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
index df52e4b4c1b0..a0accc51d44f 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -399,6 +399,7 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
 		[PIPE_C] = PIPE_C_VBLANK,
 	};
 	int pri_flip_event = SKL_FLIP_EVENT(pipe, PLANE_PRIMARY);
+	int cur_flip_event = CURSOR_A_FLIP_DONE + pipe;
 	int event;
 	u64 eventfd_signal_val = 0;
 	static int pageflip_count;
@@ -417,6 +418,11 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
 			pageflip_count += PAGEFLIP_INC_COUNT;
 		}
 
+		if (event == cur_flip_event) {
+			eventfd_signal_val += DISPLAY_CUR_REFRESH_EVENT_INC;
+			pageflip_count += PAGEFLIP_INC_COUNT;
+		}
+
 		intel_vgpu_trigger_virtual_event(vgpu, event);
 	}
 
@@ -430,6 +436,11 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
 			eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
 			pageflip_count += PAGEFLIP_INC_COUNT;
 		}
+
+		if (event == PLANE_CURSOR) {
+			eventfd_signal_val += DISPLAY_CUR_REFRESH_EVENT_INC;
+			pageflip_count += PAGEFLIP_INC_COUNT;
+		}
 	}
 
 	if (--pageflip_count < 0) {
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
index 6ad29c4f08e5..821ff88977d8 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -767,6 +767,27 @@ static int pri_surf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
 	return 0;
 }
 
+#define CURBASE_TO_PIPE(reg) \
+	calc_index(offset, _CURABASE, _CURBBASE, 0, CURBASE(PIPE_C))
+
+static int cur_surf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
+		void *p_data, unsigned int bytes)
+{
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+	u32 pipe = CURBASE_TO_PIPE(offset);
+	int event = CURSOR_A_FLIP_DONE + pipe;
+
+	write_vreg(vgpu, offset, p_data, bytes);
+
+	if (vgpu_vreg_t(vgpu, CURCNTR(pipe)) & PLANE_CTL_ASYNC_FLIP) {
+		intel_vgpu_trigger_virtual_event(vgpu, event);
+		set_bit(PLANE_CURSOR, vgpu->display.async_flip_event[pipe]);
+	} else
+		set_bit(event, vgpu->irq.flip_done_event[pipe]);
+
+	return 0;
+}
+
 #define SPRSURF_TO_PIPE(offset) \
 	calc_index(offset, _SPRA_SURF, _SPRB_SURF, 0, SPRSURF(PIPE_C))
 
@@ -1955,9 +1976,9 @@ static int init_generic_mmio_info(struct intel_gvt *gvt)
 	MMIO_D(CURPOS(PIPE_B), D_ALL);
 	MMIO_D(CURPOS(PIPE_C), D_ALL);
 
-	MMIO_D(CURBASE(PIPE_A), D_ALL);
-	MMIO_D(CURBASE(PIPE_B), D_ALL);
-	MMIO_D(CURBASE(PIPE_C), D_ALL);
+	MMIO_DH(CURBASE(PIPE_A), D_ALL, NULL, cur_surf_mmio_write);
+	MMIO_DH(CURBASE(PIPE_B), D_ALL, NULL, cur_surf_mmio_write);
+	MMIO_DH(CURBASE(PIPE_C), D_ALL, NULL, cur_surf_mmio_write);
 
 	MMIO_D(CUR_FBC_CTL(PIPE_A), D_ALL);
 	MMIO_D(CUR_FBC_CTL(PIPE_B), D_ALL);
diff --git a/drivers/gpu/drm/i915/gvt/interrupt.c b/drivers/gpu/drm/i915/gvt/interrupt.c
index 951681813230..9c2b9d2e1529 100644
--- a/drivers/gpu/drm/i915/gvt/interrupt.c
+++ b/drivers/gpu/drm/i915/gvt/interrupt.c
@@ -113,6 +113,9 @@ static const char * const irq_name[INTEL_GVT_EVENT_MAX] = {
 	[SPRITE_A_FLIP_DONE] = "Sprite Plane A flip done",
 	[SPRITE_B_FLIP_DONE] = "Sprite Plane B flip done",
 	[SPRITE_C_FLIP_DONE] = "Sprite Plane C flip done",
+	[CURSOR_A_FLIP_DONE] = "Cursor Plane A flip done",
+	[CURSOR_B_FLIP_DONE] = "Cursor Plane B flip done",
+	[CURSOR_C_FLIP_DONE] = "Cursor Plane C flip done",
 
 	[PCU_THERMAL] = "PCU Thermal Event",
 	[PCU_PCODE2DRIVER_MAILBOX] = "PCU pcode2driver mailbox event",
@@ -593,6 +596,10 @@ static void gen8_init_irq(
 		SET_BIT_INFO(irq, 4, SPRITE_A_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_A);
 		SET_BIT_INFO(irq, 4, SPRITE_B_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_B);
 		SET_BIT_INFO(irq, 4, SPRITE_C_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_C);
+
+		SET_BIT_INFO(irq, 6, CURSOR_A_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_A);
+		SET_BIT_INFO(irq, 6, CURSOR_B_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_B);
+		SET_BIT_INFO(irq, 6, CURSOR_C_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_C);
 	}
 
 	/* GEN8 interrupt PCU events */
diff --git a/drivers/gpu/drm/i915/gvt/interrupt.h b/drivers/gpu/drm/i915/gvt/interrupt.h
index 5313fb1b33e1..158f1c7a23f2 100644
--- a/drivers/gpu/drm/i915/gvt/interrupt.h
+++ b/drivers/gpu/drm/i915/gvt/interrupt.h
@@ -92,6 +92,9 @@ enum intel_gvt_event_type {
 	SPRITE_A_FLIP_DONE,
 	SPRITE_B_FLIP_DONE,
 	SPRITE_C_FLIP_DONE,
+	CURSOR_A_FLIP_DONE,
+	CURSOR_B_FLIP_DONE,
+	CURSOR_C_FLIP_DONE,
 
 	PCU_THERMAL,
 	PCU_PCODE2DRIVER_MAILBOX,
-- 
2.17.1

Re: [RFC PATCH v4 6/6] drm/i915/gvt: Add cursor plane reg update trap emulation handler

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 5863 bytes --]

On 2019.07.18 23:56:40 +0800, Kechen Lu wrote:
> This patch adds the cursor plane CURBASE reg update trap handler
> in order to :
> 
> - Deliver the cursor refresh event at each vblank emulation,
> the flip_done_event bit check is supposed to do here. If cursor
> plane updates happen, deliver the cursor refresh events.
> 
> - Support the sync and async cursor plane updates and
> corresponding cursor plane flip interrupts reporting.
> 
> Signed-off-by: Kechen Lu <kechen.lu@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/display.c   | 11 +++++++++++
>  drivers/gpu/drm/i915/gvt/handlers.c  | 27 ++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/gvt/interrupt.c |  7 +++++++
>  drivers/gpu/drm/i915/gvt/interrupt.h |  3 +++
>  4 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
> index df52e4b4c1b0..a0accc51d44f 100644
> --- a/drivers/gpu/drm/i915/gvt/display.c
> +++ b/drivers/gpu/drm/i915/gvt/display.c
> @@ -399,6 +399,7 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
>  		[PIPE_C] = PIPE_C_VBLANK,
>  	};
>  	int pri_flip_event = SKL_FLIP_EVENT(pipe, PLANE_PRIMARY);
> +	int cur_flip_event = CURSOR_A_FLIP_DONE + pipe;
>  	int event;
>  	u64 eventfd_signal_val = 0;
>  	static int pageflip_count;
> @@ -417,6 +418,11 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
>  			pageflip_count += PAGEFLIP_INC_COUNT;
>  		}
>  
> +		if (event == cur_flip_event) {
> +			eventfd_signal_val += DISPLAY_CUR_REFRESH_EVENT_INC;
> +			pageflip_count += PAGEFLIP_INC_COUNT;
> +		}
> +
>  		intel_vgpu_trigger_virtual_event(vgpu, event);
>  	}
>  
> @@ -430,6 +436,11 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
>  			eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
>  			pageflip_count += PAGEFLIP_INC_COUNT;
>  		}
> +
> +		if (event == PLANE_CURSOR) {
> +			eventfd_signal_val += DISPLAY_CUR_REFRESH_EVENT_INC;
> +			pageflip_count += PAGEFLIP_INC_COUNT;
> +		}
>  	}
>  
>  	if (--pageflip_count < 0) {
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> index 6ad29c4f08e5..821ff88977d8 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -767,6 +767,27 @@ static int pri_surf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
>  	return 0;
>  }
>  
> +#define CURBASE_TO_PIPE(reg) \
> +	calc_index(offset, _CURABASE, _CURBBASE, 0, CURBASE(PIPE_C))
> +
> +static int cur_surf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> +		void *p_data, unsigned int bytes)
> +{
> +	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> +	u32 pipe = CURBASE_TO_PIPE(offset);
> +	int event = CURSOR_A_FLIP_DONE + pipe;
> +
> +	write_vreg(vgpu, offset, p_data, bytes);
> +
> +	if (vgpu_vreg_t(vgpu, CURCNTR(pipe)) & PLANE_CTL_ASYNC_FLIP) {
> +		intel_vgpu_trigger_virtual_event(vgpu, event);
> +		set_bit(PLANE_CURSOR, vgpu->display.async_flip_event[pipe]);
> +	} else
> +		set_bit(event, vgpu->irq.flip_done_event[pipe]);
> +
> +	return 0;
> +}
> +
>  #define SPRSURF_TO_PIPE(offset) \
>  	calc_index(offset, _SPRA_SURF, _SPRB_SURF, 0, SPRSURF(PIPE_C))
>  
> @@ -1955,9 +1976,9 @@ static int init_generic_mmio_info(struct intel_gvt *gvt)
>  	MMIO_D(CURPOS(PIPE_B), D_ALL);
>  	MMIO_D(CURPOS(PIPE_C), D_ALL);
>  
> -	MMIO_D(CURBASE(PIPE_A), D_ALL);
> -	MMIO_D(CURBASE(PIPE_B), D_ALL);
> -	MMIO_D(CURBASE(PIPE_C), D_ALL);
> +	MMIO_DH(CURBASE(PIPE_A), D_ALL, NULL, cur_surf_mmio_write);
> +	MMIO_DH(CURBASE(PIPE_B), D_ALL, NULL, cur_surf_mmio_write);
> +	MMIO_DH(CURBASE(PIPE_C), D_ALL, NULL, cur_surf_mmio_write);

I think we should also track cursor pos change right?

>  
>  	MMIO_D(CUR_FBC_CTL(PIPE_A), D_ALL);
>  	MMIO_D(CUR_FBC_CTL(PIPE_B), D_ALL);
> diff --git a/drivers/gpu/drm/i915/gvt/interrupt.c b/drivers/gpu/drm/i915/gvt/interrupt.c
> index 951681813230..9c2b9d2e1529 100644
> --- a/drivers/gpu/drm/i915/gvt/interrupt.c
> +++ b/drivers/gpu/drm/i915/gvt/interrupt.c
> @@ -113,6 +113,9 @@ static const char * const irq_name[INTEL_GVT_EVENT_MAX] = {
>  	[SPRITE_A_FLIP_DONE] = "Sprite Plane A flip done",
>  	[SPRITE_B_FLIP_DONE] = "Sprite Plane B flip done",
>  	[SPRITE_C_FLIP_DONE] = "Sprite Plane C flip done",
> +	[CURSOR_A_FLIP_DONE] = "Cursor Plane A flip done",
> +	[CURSOR_B_FLIP_DONE] = "Cursor Plane B flip done",
> +	[CURSOR_C_FLIP_DONE] = "Cursor Plane C flip done",
>  
>  	[PCU_THERMAL] = "PCU Thermal Event",
>  	[PCU_PCODE2DRIVER_MAILBOX] = "PCU pcode2driver mailbox event",
> @@ -593,6 +596,10 @@ static void gen8_init_irq(
>  		SET_BIT_INFO(irq, 4, SPRITE_A_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_A);
>  		SET_BIT_INFO(irq, 4, SPRITE_B_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_B);
>  		SET_BIT_INFO(irq, 4, SPRITE_C_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_C);
> +
> +		SET_BIT_INFO(irq, 6, CURSOR_A_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_A);
> +		SET_BIT_INFO(irq, 6, CURSOR_B_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_B);
> +		SET_BIT_INFO(irq, 6, CURSOR_C_FLIP_DONE, INTEL_GVT_IRQ_INFO_DE_PIPE_C);
>  	}
>  
>  	/* GEN8 interrupt PCU events */
> diff --git a/drivers/gpu/drm/i915/gvt/interrupt.h b/drivers/gpu/drm/i915/gvt/interrupt.h
> index 5313fb1b33e1..158f1c7a23f2 100644
> --- a/drivers/gpu/drm/i915/gvt/interrupt.h
> +++ b/drivers/gpu/drm/i915/gvt/interrupt.h
> @@ -92,6 +92,9 @@ enum intel_gvt_event_type {
>  	SPRITE_A_FLIP_DONE,
>  	SPRITE_B_FLIP_DONE,
>  	SPRITE_C_FLIP_DONE,
> +	CURSOR_A_FLIP_DONE,
> +	CURSOR_B_FLIP_DONE,
> +	CURSOR_C_FLIP_DONE,
>  
>  	PCU_THERMAL,
>  	PCU_PCODE2DRIVER_MAILBOX,
> -- 
> 2.17.1
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [RFC PATCH v4 6/6] drm/i915/gvt: Add cursor plane reg update trap emulation handler

[Lu, Kechen]

Hi,

> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Friday, July 19, 2019 2:35 PM
> To: Lu, Kechen <kechen.lu@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux- 
> kernel@vger.kernel.org; kraxel@redhat.com; zhenyuw@linux.intel.com; 
> Lv, Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A 
> <zhi.a.wang@intel.com>; Tian, Kevin <kevin.tian@intel.com>; Yuan, Hang 
> <hang.yuan@intel.com>; alex.williamson@redhat.com
> Subject: Re: [RFC PATCH v4 6/6] drm/i915/gvt: Add cursor plane reg 
> update trap emulation handler
> 
> On 2019.07.18 23:56:40 +0800, Kechen Lu wrote:
> > This patch adds the cursor plane CURBASE reg update trap handler in 
> > order to :
> >
> > - Deliver the cursor refresh event at each vblank emulation, the 
> > flip_done_event bit check is supposed to do here. If cursor plane 
> > updates happen, deliver the cursor refresh events.
> >
> > - Support the sync and async cursor plane updates and corresponding 
> > cursor plane flip interrupts reporting.
> >
> > Signed-off-by: Kechen Lu <kechen.lu@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/display.c   | 11 +++++++++++
> >  drivers/gpu/drm/i915/gvt/handlers.c  | 27 
> > ++++++++++++++++++++++++--- drivers/gpu/drm/i915/gvt/interrupt.c |  
> > 7 +++++++ drivers/gpu/drm/i915/gvt/interrupt.h |  3 +++
> >  4 files changed, 45 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/display.c
> > b/drivers/gpu/drm/i915/gvt/display.c
> > index df52e4b4c1b0..a0accc51d44f 100644
> > --- a/drivers/gpu/drm/i915/gvt/display.c
> > +++ b/drivers/gpu/drm/i915/gvt/display.c
> > @@ -399,6 +399,7 @@ static void emulate_vblank_on_pipe(struct 
> > intel_vgpu
> *vgpu, int pipe)
> >  		[PIPE_C] = PIPE_C_VBLANK,
> >  	};
> >  	int pri_flip_event = SKL_FLIP_EVENT(pipe, PLANE_PRIMARY);
> > +	int cur_flip_event = CURSOR_A_FLIP_DONE + pipe;
> >  	int event;
> >  	u64 eventfd_signal_val = 0;
> >  	static int pageflip_count;
> > @@ -417,6 +418,11 @@ static void emulate_vblank_on_pipe(struct
> intel_vgpu *vgpu, int pipe)
> >  			pageflip_count += PAGEFLIP_INC_COUNT;
> >  		}
> >
> > +		if (event == cur_flip_event) {
> > +			eventfd_signal_val +=
> DISPLAY_CUR_REFRESH_EVENT_INC;
> > +			pageflip_count += PAGEFLIP_INC_COUNT;
> > +		}
> > +
> >  		intel_vgpu_trigger_virtual_event(vgpu, event);
> >  	}
> >
> > @@ -430,6 +436,11 @@ static void emulate_vblank_on_pipe(struct
> intel_vgpu *vgpu, int pipe)
> >  			eventfd_signal_val +=
> DISPLAY_PRI_REFRESH_EVENT_INC;
> >  			pageflip_count += PAGEFLIP_INC_COUNT;
> >  		}
> > +
> > +		if (event == PLANE_CURSOR) {
> > +			eventfd_signal_val +=
> DISPLAY_CUR_REFRESH_EVENT_INC;
> > +			pageflip_count += PAGEFLIP_INC_COUNT;
> > +		}
> >  	}
> >
> >  	if (--pageflip_count < 0) {
> > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> > b/drivers/gpu/drm/i915/gvt/handlers.c
> > index 6ad29c4f08e5..821ff88977d8 100644
> > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > @@ -767,6 +767,27 @@ static int pri_surf_mmio_write(struct 
> > intel_vgpu
> *vgpu, unsigned int offset,
> >  	return 0;
> >  }
> >
> > +#define CURBASE_TO_PIPE(reg) \
> > +	calc_index(offset, _CURABASE, _CURBBASE, 0, CURBASE(PIPE_C))
> > +
> > +static int cur_surf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> > +		void *p_data, unsigned int bytes) {
> > +	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> > +	u32 pipe = CURBASE_TO_PIPE(offset);
> > +	int event = CURSOR_A_FLIP_DONE + pipe;
> > +
> > +	write_vreg(vgpu, offset, p_data, bytes);
> > +
> > +	if (vgpu_vreg_t(vgpu, CURCNTR(pipe)) & PLANE_CTL_ASYNC_FLIP) {
> > +		intel_vgpu_trigger_virtual_event(vgpu, event);
> > +		set_bit(PLANE_CURSOR, vgpu->display.async_flip_event[pipe]);
> > +	} else
> > +		set_bit(event, vgpu->irq.flip_done_event[pipe]);
> > +
> > +	return 0;
> > +}
> > +
> >  #define SPRSURF_TO_PIPE(offset) \
> >  	calc_index(offset, _SPRA_SURF, _SPRB_SURF, 0, SPRSURF(PIPE_C))
> >
> > @@ -1955,9 +1976,9 @@ static int init_generic_mmio_info(struct 
> > intel_gvt
> *gvt)
> >  	MMIO_D(CURPOS(PIPE_B), D_ALL);
> >  	MMIO_D(CURPOS(PIPE_C), D_ALL);
> >
> > -	MMIO_D(CURBASE(PIPE_A), D_ALL);
> > -	MMIO_D(CURBASE(PIPE_B), D_ALL);
> > -	MMIO_D(CURBASE(PIPE_C), D_ALL);
> > +	MMIO_DH(CURBASE(PIPE_A), D_ALL, NULL, cur_surf_mmio_write);
> > +	MMIO_DH(CURBASE(PIPE_B), D_ALL, NULL, cur_surf_mmio_write);
> > +	MMIO_DH(CURBASE(PIPE_C), D_ALL, NULL, cur_surf_mmio_write);
> 
> I think we should also track cursor pos change right?
> 

Yes, this may be the case, according to the bspec, the CURPOS regs take into effects at vblanks. But in Linux i915 driver, the CURPOS regs programming companies with CURBASE regs. I'm not sure the Windows driver logic, but I can do some experiments. If that is the case, yes, we also need to trap the cursor pos change. 
Thanks.

Best Regards,
Kechen

> >
> >  	MMIO_D(CUR_FBC_CTL(PIPE_A), D_ALL);
> >  	MMIO_D(CUR_FBC_CTL(PIPE_B), D_ALL); diff --git 
> > a/drivers/gpu/drm/i915/gvt/interrupt.c
> > b/drivers/gpu/drm/i915/gvt/interrupt.c
> > index 951681813230..9c2b9d2e1529 100644
> > --- a/drivers/gpu/drm/i915/gvt/interrupt.c
> > +++ b/drivers/gpu/drm/i915/gvt/interrupt.c
> > @@ -113,6 +113,9 @@ static const char * const
> irq_name[INTEL_GVT_EVENT_MAX] = {
> >  	[SPRITE_A_FLIP_DONE] = "Sprite Plane A flip done",
> >  	[SPRITE_B_FLIP_DONE] = "Sprite Plane B flip done",
> >  	[SPRITE_C_FLIP_DONE] = "Sprite Plane C flip done",
> > +	[CURSOR_A_FLIP_DONE] = "Cursor Plane A flip done",
> > +	[CURSOR_B_FLIP_DONE] = "Cursor Plane B flip done",
> > +	[CURSOR_C_FLIP_DONE] = "Cursor Plane C flip done",
> >
> >  	[PCU_THERMAL] = "PCU Thermal Event",
> >  	[PCU_PCODE2DRIVER_MAILBOX] = "PCU pcode2driver mailbox event",
> @@
> > -593,6 +596,10 @@ static void gen8_init_irq(
> >  		SET_BIT_INFO(irq, 4, SPRITE_A_FLIP_DONE,
> INTEL_GVT_IRQ_INFO_DE_PIPE_A);
> >  		SET_BIT_INFO(irq, 4, SPRITE_B_FLIP_DONE,
> INTEL_GVT_IRQ_INFO_DE_PIPE_B);
> >  		SET_BIT_INFO(irq, 4, SPRITE_C_FLIP_DONE, 
> > INTEL_GVT_IRQ_INFO_DE_PIPE_C);
> > +
> > +		SET_BIT_INFO(irq, 6, CURSOR_A_FLIP_DONE,
> INTEL_GVT_IRQ_INFO_DE_PIPE_A);
> > +		SET_BIT_INFO(irq, 6, CURSOR_B_FLIP_DONE,
> INTEL_GVT_IRQ_INFO_DE_PIPE_B);
> > +		SET_BIT_INFO(irq, 6, CURSOR_C_FLIP_DONE, 
> > +INTEL_GVT_IRQ_INFO_DE_PIPE_C);
> >  	}
> >
> >  	/* GEN8 interrupt PCU events */
> > diff --git a/drivers/gpu/drm/i915/gvt/interrupt.h
> > b/drivers/gpu/drm/i915/gvt/interrupt.h
> > index 5313fb1b33e1..158f1c7a23f2 100644
> > --- a/drivers/gpu/drm/i915/gvt/interrupt.h
> > +++ b/drivers/gpu/drm/i915/gvt/interrupt.h
> > @@ -92,6 +92,9 @@ enum intel_gvt_event_type {
> >  	SPRITE_A_FLIP_DONE,
> >  	SPRITE_B_FLIP_DONE,
> >  	SPRITE_C_FLIP_DONE,
> > +	CURSOR_A_FLIP_DONE,
> > +	CURSOR_B_FLIP_DONE,
> > +	CURSOR_C_FLIP_DONE,
> >
> >  	PCU_THERMAL,
> >  	PCU_PCODE2DRIVER_MAILBOX,
> > --
> > 2.17.1
> >
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827