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

[Tian, Kevin]

> From: Zhenyu Wang
> Sent: Monday, August 26, 2019 3:56 PM
> 
> On 2019.08.16 10:35:26 +0800, Tina Zhang wrote:
> > 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.

can you move optimization to another patch?

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

what is the normal case? double buffer? which events are skipped in
such scenario?

> >
> > For single framebuffer case, deliver the refresh events to userspace at
> > all vblanks. This heuristic at each vblank leverages pageflip_count

at all vblanks? later words said the other way i.e. delivering events only
after the threshold is exceeded. Please be consistent in what you try to
explain here.

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

why may the heuristic make incorrect decision? under what condition?

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

'actual' vs. what? a 'faked' single framebuffer case? 

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

footprint? I guess you meant reducing the frequency of delivered events...

> >
> > v2:
> > - Support vfio_irq_info_cap_display_plane_events. (Tina)
> >
> > Signed-off-by: Tina Zhang <tina.zhang@xxxxxxxxx>
> > Signed-off-by: Kechen Lu <kechen.lu@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/gvt/display.c |  22 ++++
> >  drivers/gpu/drm/i915/gvt/gvt.h     |   2 +
> >  drivers/gpu/drm/i915/gvt/kvmgt.c   | 159 +++++++++++++++++++++++++++-
> -
> >  3 files changed, 174 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/display.c
> b/drivers/gpu/drm/i915/gvt/display.c
> > index 1a0a4ae4826e..616285e4a014 100644
> > --- a/drivers/gpu/drm/i915/gvt/display.c
> > +++ b/drivers/gpu/drm/i915/gvt/display.c
> > @@ -34,6 +34,8 @@
> >
> >  #include "i915_drv.h"
> >  #include "gvt.h"
> > +#include <uapi/linux/vfio.h>
> > +#include <drm/drm_plane.h>
> >
> >  static int get_edp_pipe(struct intel_vgpu *vgpu)
> >  {
> > @@ -387,6 +389,8 @@ void intel_gvt_check_vblank_emulation(struct
> intel_gvt *gvt)
> >  	mutex_unlock(&gvt->lock);
> >  }
> >
> > +#define PAGEFLIP_DELAY_THR 10
> > +
> >  static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> > @@ -396,7 +400,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 no_pageflip_count;
> >
> >  	if (pipe < PIPE_A || pipe > PIPE_C)
> >  		return;
> > @@ -407,11 +414,26 @@ 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_VAL;
> > +
> >  		intel_vgpu_trigger_virtual_event(vgpu, event);
> >  	}
> >
> > +	if (eventfd_signal_val)
> > +		no_pageflip_count = 0;
> > +	else if (!eventfd_signal_val && no_pageflip_count >
> PAGEFLIP_DELAY_THR)
> 
> extra !eventfd_signal_val
> 
> > +		eventfd_signal_val |= DISPLAY_PRI_REFRESH_EVENT_VAL;

do you need reset the count to zero here?

> > +	else
> > +		no_pageflip_count++;
> 
> no_pageflip_count should be per-vgpu instead of static.
> 
> > +
> > +	if (vgpu->vdev.vblank_trigger && !vgpu->vdev.display_event_mask
> &&
> > +		eventfd_signal_val)

is this mask per vGPU or per plane? If the latter, you need check specific bit here.

> > +		eventfd_signal(vgpu->vdev.vblank_trigger,
> eventfd_signal_val);
> > +
> >  	if (pipe_is_enabled(vgpu, pipe)) {
> >  		vgpu_vreg_t(vgpu, PIPE_FRMCOUNT_G4X(pipe))++;
> > +
> 
> extra line
> 
> >  		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 cd29ea28d7ed..6c8ed030c30b 100644
> > --- a/drivers/gpu/drm/i915/gvt/gvt.h
> > +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> > @@ -205,6 +205,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 fd1633342e53..9ace1f4ff9eb 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -1250,6 +1250,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;
> >  }
> > @@ -1297,7 +1299,60 @@ 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,

vgu -> 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 |= 1;
> 
> see below..
> 
> > +
> > +	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 &= 0;
> 
> looks display_event_mask is used as flag for enable/disable, just write 1 or 0?

Do we plan to support per-plane mask in the future? If yes, then use bit
operation but let's define the bit meaning explicitly now.,

> 
> 
> > +
> > +	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)
> >  {
> > @@ -1330,6 +1385,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)
> > @@ -1361,7 +1445,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;
> > @@ -1521,32 +1605,88 @@ 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;
> > +
> > +			if (vgpu->vdev.irq[i].ops->add_capability) {
> > +				ret = vgpu->vdev.irq[i].ops-
> >add_capability(vgpu,
> > +
> &caps);
> > +				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;
> > @@ -1565,7 +1705,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
> >
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827