| Message ID | 20190816023528.30210-5-tina.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Deliver vGPU display refresh event to userspace | expand |
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. > > 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. > > v2: > - Support vfio_irq_info_cap_display_plane_events. (Tina) > > 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 | 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; > + 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) > + 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, > + 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? > + > + 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 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@intel.com> > > Signed-off-by: Kechen Lu <kechen.lu@intel.com> > > --- > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev > > -- > Open Source Technology Center, Intel ltd. > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> -----Original Message----- > From: Tian, Kevin > Sent: Wednesday, August 28, 2019 9:50 AM > To: Zhenyu Wang <zhenyuw@linux.intel.com>; Zhang, Tina > <tina.zhang@intel.com> > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Yuan, Hang > <hang.yuan@intel.com>; alex.williamson@redhat.com; kraxel@redhat.com; > Lu, Kechen <kechen.lu@intel.com>; intel-gvt-dev@lists.freedesktop.org; Lv, > Zhiyuan <zhiyuan.lv@intel.com> > Subject: RE: [PATCH v5 4/6] drm/i915/gvt: Deliver vGPU refresh event to > userspace > > > 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? OK. I'll try. > > > > > > > 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? Here normal case means >= 2 buffers. In this situation, we have to skip the redundant page flip events between two vblanks and notify user space with one display refresh event (i.e. turn those page flip operations between two vblanks into one display refresh event). > > > > > > > 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. Actually, there're two steps: 1) single framebuffer case recognition The heuristic needs several vblanks to see if vgpu is working in the single front buffer mode. 2) deliver the display refresh event at all vblanks. If vgpu is working in single front buffer mode, the display refresh event will be sent at all vblanks. > > > > 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? E.g. when guest window manager is waiting for user input and there're no window update requests from the apps in guest. In situation, although guest doesn't work in single front buffer mode, the heuristic will consider it does and send the display refresh event at all vblanks. Ideally, in this situation, as guest window manager is working in multi-buffer mode, gvt-g should only send the refresh event when page flip happens. However, it seems there's no simple way for gvt-g to tell this case and the real single front buffer case apart. > > > > 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? I think the 'actual' here means vgpu does work in the single front buffer mode. > > > > > > > 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... Exactly. Thanks. BR, Tina > > > > > > > v2: > > > - Support vfio_irq_info_cap_display_plane_events. (Tina) > > > > > > 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 | 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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev > > > > -- > > Open Source Technology Center, Intel ltd. > > > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> From: Zhang, Tina > Sent: Wednesday, August 28, 2019 2:59 PM > > > -----Original Message----- > > From: Tian, Kevin > > Sent: Wednesday, August 28, 2019 9:50 AM > > To: Zhenyu Wang <zhenyuw@linux.intel.com>; Zhang, Tina > > <tina.zhang@intel.com> > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Yuan, Hang > > <hang.yuan@intel.com>; alex.williamson@redhat.com; kraxel@redhat.com; > > Lu, Kechen <kechen.lu@intel.com>; intel-gvt-dev@lists.freedesktop.org; Lv, > > Zhiyuan <zhiyuan.lv@intel.com> > > Subject: RE: [PATCH v5 4/6] drm/i915/gvt: Deliver vGPU refresh event to > > userspace > > > > > 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? > OK. I'll try. > > > > > > > > > > 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? > Here normal case means >= 2 buffers. In this situation, we have to skip the > redundant page flip events between two vblanks and notify user space with > one display refresh event (i.e. turn those page flip operations between two > vblanks into one display refresh event). > "have to" refers to something mandatory, instead of performance enhancement. > > > > > > > > > > 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. > > Actually, there're two steps: > 1) single framebuffer case recognition > The heuristic needs several vblanks to see if vgpu is working in the single front > buffer mode. > what is the difference between "single framebuffer" and "single front buffer"? > 2) deliver the display refresh event at all vblanks. > If vgpu is working in single front buffer mode, the display refresh event will be > sent at all vblanks. I didn't get it. why is it OK to not deliver events before you identify the single framebuffer mode, while not OK to do so after the identification? If the definition of such mode can tolerate misses of vblank events, then you don't need deliver at every vblank. Otherwise, the detection itself already breaks the guest assumption. > > > > > > > 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? > > E.g. when guest window manager is waiting for user input and there're no > window update requests from the apps in guest. In situation, although guest > doesn't work in single front buffer mode, the heuristic will consider it does > and send the display refresh event at all vblanks. Ideally, in this situation, as > guest window manager is working in multi-buffer mode, gvt-g should only > send the refresh event when page flip happens. However, it seems there's no > simple way for gvt-g to tell this case and the real single front buffer case apart. I think you need some background to explain the vblank requirement in single vs. multi-buffer mode, before talking about the actual optimization. It's not clear to me so far. > > > > > > > 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? > > I think the 'actual' here means vgpu does work in the single front buffer mode. then just remove 'actual' > > > > > > > > > > > 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... > > Exactly. Thanks. > there are other comments embedded in the code. You may overlook them. > BR, > Tina > > > > > > > > > > v2: > > > > - Support vfio_irq_info_cap_display_plane_events. (Tina) > > > > > > > > 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 | 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@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev > > > > > > -- > > > Open Source Technology Center, Intel ltd. > > > > > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> -----Original Message----- > From: Tian, Kevin > Sent: Wednesday, August 28, 2019 3:10 PM > To: Zhang, Tina <tina.zhang@intel.com>; Zhenyu Wang > <zhenyuw@linux.intel.com> > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Yuan, Hang > <hang.yuan@intel.com>; alex.williamson@redhat.com; kraxel@redhat.com; > Lu, Kechen <kechen.lu@intel.com>; intel-gvt-dev@lists.freedesktop.org; Lv, > Zhiyuan <zhiyuan.lv@intel.com> > Subject: RE: [PATCH v5 4/6] drm/i915/gvt: Deliver vGPU refresh event to > userspace > > > From: Zhang, Tina > > Sent: Wednesday, August 28, 2019 2:59 PM > > > > > -----Original Message----- > > > From: Tian, Kevin > > > Sent: Wednesday, August 28, 2019 9:50 AM > > > To: Zhenyu Wang <zhenyuw@linux.intel.com>; Zhang, Tina > > > <tina.zhang@intel.com> > > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Yuan, Hang > > > <hang.yuan@intel.com>; alex.williamson@redhat.com; > > > kraxel@redhat.com; Lu, Kechen <kechen.lu@intel.com>; > > > intel-gvt-dev@lists.freedesktop.org; Lv, Zhiyuan > > > <zhiyuan.lv@intel.com> > > > Subject: RE: [PATCH v5 4/6] drm/i915/gvt: Deliver vGPU refresh event > > > to userspace > > > > > > > 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? > > OK. I'll try. > > > > > > > > > > > > > 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? > > Here normal case means >= 2 buffers. In this situation, we have to > > skip the redundant page flip events between two vblanks and notify > > user space with one display refresh event (i.e. turn those page flip > > operations between two vblanks into one display refresh event). > > > > "have to" refers to something mandatory, instead of performance > enhancement. I think the "performance enhancement" here means in multi-buffer cases, no need to deliver display refresh events at all vblanks unless the page flip operation does happen between two vblanks. > > > > > > > > > > > > > > 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. > > > > Actually, there're two steps: > > 1) single framebuffer case recognition The heuristic needs several > > vblanks to see if vgpu is working in the single front buffer mode. > > > > > what is the difference between "single framebuffer" and "single front buffer"? Same meaning. > > > 2) deliver the display refresh event at all vblanks. > > If vgpu is working in single front buffer mode, the display refresh event will > be > > sent at all vblanks. > > I didn't get it. why is it OK to not deliver events before you identify the > single framebuffer mode, while not OK to do so after the identification? > If the definition of such mode can tolerate misses of vblank events, then > you don't need deliver at every vblank. Otherwise, the detection itself > already breaks the guest assumption. There is a small window (about less than 10 vblank periods). During that time, gvt-g won't send any display refresh events unless there is a page flip operation happens. And it's considered that this window is working as the performance enhancement. Considering the case that guest has multi-framebuffers and the conducting page flip rate is lower than the vblank rate, gvt-g only sends the events when the page flip operation really happens. On the other hand, this window is needed by the heuristic to guess if the guest is working in the single front buffer mode. For the "performance enhancement" case, this window could be as large as possible. However, for single front buffer recognition, this window cannot be too large, as it will impact the user experience. So, we just let it be 10 vblank periods. > > > > > > > > > > > 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? > > > > E.g. when guest window manager is waiting for user input and there're no > > window update requests from the apps in guest. In situation, although > guest > > doesn't work in single front buffer mode, the heuristic will consider it does > > and send the display refresh event at all vblanks. Ideally, in this situation, > as > > guest window manager is working in multi-buffer mode, gvt-g should only > > send the refresh event when page flip happens. However, it seems there's > no > > simple way for gvt-g to tell this case and the real single front buffer case > apart. > > I think you need some background to explain the vblank requirement in > single vs. multi-buffer mode, before talking about the actual optimization. > It's not clear to me so far. > > > > > > > > > > > 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? > > > > I think the 'actual' here means vgpu does work in the single front buffer > mode. > > then just remove 'actual' > > > > > > > > > > > > > > > > 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... > > > > Exactly. Thanks. > > > > there are other comments embedded in the code. You may overlook them. Thanks. BR, Tina > > > BR, > > Tina > > > > > > > > > > > > > v2: > > > > > - Support vfio_irq_info_cap_display_plane_events. (Tina) > > > > > > > > > > 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 | 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@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev > > > > > > > > -- > > > > Open Source Technology Center, Intel ltd. > > > > > > > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
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) + eventfd_signal_val |= DISPLAY_PRI_REFRESH_EVENT_VAL; + else + no_pageflip_count++; + + if (vgpu->vdev.vblank_trigger && !vgpu->vdev.display_event_mask && + 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 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, + 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; + + 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; + + 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;