diff mbox series

[RFC,v2,1/3] vfio: Use capability chains to handle device specific irq

Message ID 20190604095534.10337-2-tina.zhang@intel.com (mailing list archive)
State New, archived
Headers show
Series Deliver vGPU page flip events to userspace | expand

Commit Message

Zhang, Tina June 4, 2019, 9:55 a.m. UTC
Caps the number of irqs with fixed indexes and uses capability chains
to chain device specific irqs.

VFIO vGPU leverages this mechanism to trigger primary plane and cursor
plane page flip event to the user space.

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

Comments

Zhenyu Wang June 5, 2019, 4:04 a.m. UTC | #1
On 2019.06.04 17:55:32 +0800, Tina Zhang wrote:
> Caps the number of irqs with fixed indexes and uses capability chains
> to chain device specific irqs.
> 
> VFIO vGPU leverages this mechanism to trigger primary plane and cursor
> plane page flip event to the user space.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  include/uapi/linux/vfio.h | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 02bb7ad6e986..9b5e25937c7d 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -444,11 +444,31 @@ 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	cap_offset;	/* Offset within info struct of first cap */
>  	__u32	count;		/* Number of IRQs within this index */

This would break ABI for get irq info. I think irq cap chain can just follow
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 */
> +};
> +
> +#define VFIO_IRQ_TYPE_GFX				(1)
> +#define VFIO_IRQ_SUBTYPE_GFX_PRI_PLANE_FLIP		(1)
> +#define VFIO_IRQ_SUBTYPE_GFX_CUR_PLANE_FLIP		(2)
> +

Really need to split for different planes? I'd like a VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
so user space can probe change for all.

>  /**
>   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
>   *
> @@ -550,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
>
Zhang, Tina June 5, 2019, 9:18 a.m. UTC | #2
> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Wednesday, June 5, 2019 12:05 PM
> To: Zhang, Tina <tina.zhang@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 v2 1/3] vfio: Use capability chains to handle device
> specific irq
> 
> On 2019.06.04 17:55:32 +0800, Tina Zhang wrote:
> > Caps the number of irqs with fixed indexes and uses capability chains
> > to chain device specific irqs.
> >
> > VFIO vGPU leverages this mechanism to trigger primary plane and cursor
> > plane page flip event to the user space.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >  include/uapi/linux/vfio.h | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 02bb7ad6e986..9b5e25937c7d 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -444,11 +444,31 @@ 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	cap_offset;	/* Offset within info struct of first cap */
> >  	__u32	count;		/* Number of IRQs within this index */
> 
> This would break ABI for get irq info. I think irq cap chain can just follow
> 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 */
> > +};
> > +
> > +#define VFIO_IRQ_TYPE_GFX				(1)
> > +#define VFIO_IRQ_SUBTYPE_GFX_PRI_PLANE_FLIP		(1)
> > +#define VFIO_IRQ_SUBTYPE_GFX_CUR_PLANE_FLIP		(2)
> > +
> 
> Really need to split for different planes? I'd like a
> VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> so user space can probe change for all.
User space can choose to user different handlers according to the specific event. For example, user space might not want to handle every cursor event due to performance consideration. Besides, it can reduce the probe times, as we don't need to probe twice to make sure if both cursor plane and primary plane have been updated.
Thanks.

BR,
Tina

> 
> >  /**
> >   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct
> vfio_irq_set)
> >   *
> > @@ -550,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
Gerd Hoffmann June 5, 2019, 10:09 a.m. UTC | #3
Hi,

> > Really need to split for different planes? I'd like a
> > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> > so user space can probe change for all.

> User space can choose to user different handlers according to the
> specific event. For example, user space might not want to handle every
> cursor event due to performance consideration. Besides, it can reduce
> the probe times, as we don't need to probe twice to make sure if both
> cursor plane and primary plane have been updated.

I'd suggest to use the value passed via eventfd for that, i.e. instead
of sending "1" unconditionally send a mask of changed planes.

cheers,
  Gerd
Tian, Kevin June 6, 2019, 2:57 a.m. UTC | #4
> From: kraxel@redhat.com
> Sent: Wednesday, June 5, 2019 6:10 PM
> 
>   Hi,
> 
> > > Really need to split for different planes? I'd like a
> > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> > > so user space can probe change for all.
> 
> > User space can choose to user different handlers according to the
> > specific event. For example, user space might not want to handle every
> > cursor event due to performance consideration. Besides, it can reduce
> > the probe times, as we don't need to probe twice to make sure if both
> > cursor plane and primary plane have been updated.
> 
> I'd suggest to use the value passed via eventfd for that, i.e. instead
> of sending "1" unconditionally send a mask of changed planes.
> 

sounds reasonable.
Zhang, Tina June 6, 2019, 10:17 a.m. UTC | #5
> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of kraxel@redhat.com
> Sent: Wednesday, June 5, 2019 6:10 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Zhenyu Wang <zhenyuw@linux.intel.com>; Yuan,
> Hang <hang.yuan@intel.com>; alex.williamson@redhat.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
> <zhi.a.wang@intel.com>
> Subject: Re: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device
> specific irq
> 
>   Hi,
> 
> > > Really need to split for different planes? I'd like a
> > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> > > so user space can probe change for all.
> 
> > User space can choose to user different handlers according to the
> > specific event. For example, user space might not want to handle every
> > cursor event due to performance consideration. Besides, it can reduce
> > the probe times, as we don't need to probe twice to make sure if both
> > cursor plane and primary plane have been updated.
> 
> I'd suggest to use the value passed via eventfd for that, i.e. instead of
> sending "1" unconditionally send a mask of changed planes.
If there is only one eventfd working for GFX_DISPLAY, should it be  VFIO_IRQ_INFO_EVENTFD and VFIO_IRQ_INFO_AUTOMASKED? i.e. after signaling, the interrupt is automatically masked and the user space needs to unmask the line to receive new irq event.

BR,
Tina


> 
> cheers,
>   Gerd
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
Alex Williamson June 6, 2019, 4:25 p.m. UTC | #6
On Thu, 6 Jun 2019 10:17:51 +0000
"Zhang, Tina" <tina.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> > Behalf Of kraxel@redhat.com
> > Sent: Wednesday, June 5, 2019 6:10 PM
> > To: Zhang, Tina <tina.zhang@intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Zhenyu Wang <zhenyuw@linux.intel.com>; Yuan,
> > Hang <hang.yuan@intel.com>; alex.williamson@redhat.com; Lv, Zhiyuan
> > <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
> > <zhi.a.wang@intel.com>
> > Subject: Re: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device
> > specific irq
> > 
> >   Hi,
> >   
> > > > Really need to split for different planes? I'd like a
> > > > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
> > > > so user space can probe change for all.  
> >   
> > > User space can choose to user different handlers according to the
> > > specific event. For example, user space might not want to handle every
> > > cursor event due to performance consideration. Besides, it can reduce
> > > the probe times, as we don't need to probe twice to make sure if both
> > > cursor plane and primary plane have been updated.  
> > 
> > I'd suggest to use the value passed via eventfd for that, i.e. instead of
> > sending "1" unconditionally send a mask of changed planes.  
> If there is only one eventfd working for GFX_DISPLAY, should it be
> VFIO_IRQ_INFO_EVENTFD and VFIO_IRQ_INFO_AUTOMASKED? i.e. after
> signaling, the interrupt is automatically masked and the user space
> needs to unmask the line to receive new irq event.

If there's any way at all the interrupt is rate limited already, I'd
suggest not to use automasked.  This flag is generally intended for
cases where we need to mask a host interrupt and don't have a generic
or efficient way to determine acknowledgement of the interrupt and
therefore require an explicit unmask.  If the events here are not at a
high frequency or you can tell by other interactions that they've been
acted upon, I'd suggest to handle these as an edge triggered interrupt
w/o automasked.  Thanks,

Alex
diff mbox series

Patch

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 02bb7ad6e986..9b5e25937c7d 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -444,11 +444,31 @@  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	cap_offset;	/* Offset within info struct of first cap */
 	__u32	count;		/* Number of IRQs within this index */
 };
 #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 */
+};
+
+#define VFIO_IRQ_TYPE_GFX				(1)
+#define VFIO_IRQ_SUBTYPE_GFX_PRI_PLANE_FLIP		(1)
+#define VFIO_IRQ_SUBTYPE_GFX_CUR_PLANE_FLIP		(2)
+
 /**
  * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
  *
@@ -550,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 */
 };
 
 /*