| Message ID | 1412071538-19059-8-git-send-email-jike.song@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 30, 2014 at 06:05:37PM +0800, Jike Song wrote: > vgt owns the hardware interrupt of the GPU, to satisfy the > interrupt requirement from both host side and guest side > (e.g. host may have MI_USER_INTERRUPT disabled, while a VM > may have it enabled). Sometimes vgt may also need to emulate > a virtual interrupt to the host, w/o a hardware interrupt > actually triggered. So we need to split the handling > between physical interrupts to vgt and virtual interrupts > to host i915. > > Regarding to above requirements, this patch registers a > vgt interrupt handler when vgt is enabled, while letting > original i915 interrupt handler instead carried in a > tasklet. Whenever a virtual interrupt needs to be injected > to host i915, tasklet_schedule gets called. So the tasklet requirement comes from being able to inject interrupts on the host side from vgt to i915? Why would you ever need to do that instead of just either calling the i915 irq handler as the tail handler from hardirq context or directly calling whatever function you want to call anywhere else? I guess we need even more information here about the design ;-) Cheers, Daniel > > Signed-off-by: Jike Song <jike.song@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 6 ++++++ > drivers/gpu/drm/i915/i915_irq.c | 35 +++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_vgt.h | 20 ++++++++++++++++++++ > drivers/gpu/drm/i915/vgt/vgt.c | 22 ++++++++++++++++++++++ > 4 files changed, 83 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 742fe8a..e33455a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1668,6 +1668,12 @@ struct drm_i915_private { > struct vlv_s0ix_state vlv_s0ix_state; > > struct { > + struct tasklet_struct host_irq_task; > + irqreturn_t (*host_isr)(int, void *); > + void (*host_irq_uninstall)(struct drm_device *); > + } igvt; > + > + struct { > /* > * Raw watermark latency values: > * in 0.1us units for WM0, > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 080981b..df7c868 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -36,6 +36,7 @@ > #include "i915_drv.h" > #include "i915_trace.h" > #include "intel_drv.h" > +#include "i915_vgt.h" > > static const u32 hpd_ibx[] = { > [HPD_CRT] = SDE_CRT_HOTPLUG, > @@ -4650,6 +4651,30 @@ static void intel_hpd_irq_reenable_work(struct work_struct *work) > intel_runtime_pm_put(dev_priv); > } > > +static void vgt_host_isr_wrapper(unsigned long data) > +{ > + struct drm_device *dev = (struct drm_device *)data; > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + dev_priv->igvt.host_isr(dev->pdev->irq, dev); > +} > + > +void vgt_schedule_host_isr(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + tasklet_schedule(&dev_priv->igvt.host_irq_task); > +} > + > +static void vgt_irq_uninstall(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + tasklet_kill(&dev_priv->igvt.host_irq_task); > + dev_priv->igvt.host_irq_uninstall(dev); > + vgt_fini_irq(dev->pdev); > +} > + > void intel_irq_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -4756,6 +4781,16 @@ void intel_irq_init(struct drm_device *dev) > dev->driver->enable_vblank = i915_enable_vblank; > dev->driver->disable_vblank = i915_disable_vblank; > } > + > + if (i915.enable_vgt) { > + vgt_init_irq(dev->pdev, dev); > + dev_priv->igvt.host_isr = dev->driver->irq_handler; > + dev->driver->irq_handler = vgt_interrupt; > + dev_priv->igvt.host_irq_uninstall = dev->driver->irq_uninstall; > + dev->driver->irq_uninstall = vgt_irq_uninstall; > + tasklet_init(&dev_priv->igvt.host_irq_task, > + vgt_host_isr_wrapper, (unsigned long)dev); > + } > } > > void intel_hpd_init(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/i915_vgt.h b/drivers/gpu/drm/i915/i915_vgt.h > index 03e7f00..553f920 100644 > --- a/drivers/gpu/drm/i915/i915_vgt.h > +++ b/drivers/gpu/drm/i915/i915_vgt.h > @@ -1,6 +1,9 @@ > #ifndef _I915_VGT_H_ > #define _I915_VGT_H_ > > +#include <linux/interrupt.h> > + > +struct drm_device; > struct drm_i915_private; > > #ifdef CONFIG_I915_IGVT > @@ -9,6 +12,10 @@ bool i915_start_vgt(struct pci_dev *); > void i915_vgt_record_priv(struct drm_i915_private *); > bool vgt_emulate_host_read(u32, void *, int, bool, bool); > bool vgt_emulate_host_write(u32, void *, int, bool, bool); > +void vgt_schedule_host_isr(struct drm_device *); > +void vgt_init_irq(struct pci_dev *, struct drm_device *); > +void vgt_fini_irq(struct pci_dev *); > +irqreturn_t vgt_interrupt(int, void *); > > #else /* !CONFIG_I915_IGVT */ > > @@ -33,6 +40,19 @@ static inline bool vgt_emulate_host_write(u32 reg, void *val, int len, > return false; > } > > +static inline void vgt_init_irq(struct pci_dev *pdev, struct drm_device *dev) > +{ > +} > + > +static inline void vgt_fini_irq(struct pci_dev *pdev) > +{ > +} > + > +static inline irqreturn_t vgt_interrupt(int irq, void *data) > +{ > + return IRQ_NONE; > +} > + > #endif /* CONFIG_I915_IGVT */ > > #endif > diff --git a/drivers/gpu/drm/i915/vgt/vgt.c b/drivers/gpu/drm/i915/vgt/vgt.c > index f33baf3..dab4bfc 100644 > --- a/drivers/gpu/drm/i915/vgt/vgt.c > +++ b/drivers/gpu/drm/i915/vgt/vgt.c > @@ -1,6 +1,7 @@ > #include <linux/module.h> > #include <linux/kthread.h> > #include <linux/pci.h> > +#include <linux/interrupt.h> > > #include "../i915_drv.h" > #include "vgt.h" > @@ -121,3 +122,24 @@ void i915_vgt_record_priv(struct drm_i915_private *priv) > { > dev_priv = priv; > } > + > +static void vgt_host_irq(struct drm_device *dev) > +{ > + vgt_schedule_host_isr(dev); > +} > + > +void vgt_init_irq(struct pci_dev *pdev, struct drm_device *dev) > +{ > + /* TODO: initialize vgt-specific irq handlings after vgt integration */ > +} > + > +void vgt_fini_irq(struct pci_dev *pdev) > +{ > + /* TODO: cleanup vgt-specific irq handlings after vgt integration */ > +} > + > +irqreturn_t vgt_interrupt(int irq, void *data) > +{ > + vgt_host_irq(data); > + return IRQ_HANDLED; > +} > -- > 1.9.1 >
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Tuesday, September 30, 2014 3:30 AM > > On Tue, Sep 30, 2014 at 06:05:37PM +0800, Jike Song wrote: > > vgt owns the hardware interrupt of the GPU, to satisfy the > > interrupt requirement from both host side and guest side > > (e.g. host may have MI_USER_INTERRUPT disabled, while a VM > > may have it enabled). Sometimes vgt may also need to emulate > > a virtual interrupt to the host, w/o a hardware interrupt > > actually triggered. So we need to split the handling > > between physical interrupts to vgt and virtual interrupts > > to host i915. > > > > Regarding to above requirements, this patch registers a > > vgt interrupt handler when vgt is enabled, while letting > > original i915 interrupt handler instead carried in a > > tasklet. Whenever a virtual interrupt needs to be injected > > to host i915, tasklet_schedule gets called. > > So the tasklet requirement comes from being able to inject interrupts on > the host side from vgt to i915? yes. > > Why would you ever need to do that instead of just either calling the i915 > irq handler as the tail handler from hardirq context or directly calling > whatever function you want to call anywhere else? > > I guess we need even more information here about the design ;-) > We want to separate pirq and virq handling in a consistent way for both host and guest side. In pirq handler, it simply checks physical pending events, and loop all VM's virtual imr/ier to decide whether to pend virtual iir. At the end of pirq handling, kick off an asynchronous event to either host i915 or guest i915, if there's pending virtual iir, in the same manner as how a physical interrupt is triggered. For guest virq injection, the kick-off goes through APIC emulation to deliver to guest i915. For host virq injection, we choose tasklet now to implement the asynchronous manner. But definitely tasklet is not hardirq context, so there's some limitation. Similarly asynchronous manner is required when emulating a register access, e.g. to execlist submission port, which may trigger a virtual interrupt. Invoking i915 interrupt handler in the emulation path may cause tricky lock problem, since i915 handler also accesses registers which further causes nested emulations. From virtualization p.o.v, the ideal case is to run host i915 irq handler in the interrupt context, which meets all the assumption from original code. Using tasklet or other manner still has some restriction. This is a major open we'd like to hear more from you guys. Is it possible to have i915 driver to request two irq numbers: irq1 for real device and irq2 is purely faked one. vgt handler registers on irq1 and i915 hanlder registers on irq2, and then we can use self IPI to trigger irq2 when injection is required. But I'm not sure whether this is an existing feature in kernel, or need some core enhancement in irq sub-system... Thanks Kevin
On 10/01/2014 12:26 AM, Tian, Kevin wrote: > From virtualization p.o.v, the ideal case is to run host i915 irq handler in the > interrupt context, which meets all the assumption from original code. Using > tasklet or other manner still has some restriction. This is a major open we'd > like to hear more from you guys. Is it possible to have i915 driver to request > two irq numbers: irq1 for real device and irq2 is purely faked one. vgt handler > registers on irq1 and i915 hanlder registers on irq2, and then we can use self > IPI to trigger irq2 when injection is required. But I'm not sure whether this is > an existing feature in kernel, or need some core enhancement in irq sub-system... Hi Kevin, Daniel, I'm so excited to know that, there is an existing feature in kernel: irq_work. Basicly it allows us to run the host i915 ISR in hardirq context prefectly, what's needed is to select CONFIG_IRQ_WORK in Kconfig :) I'll use that in the v2 patches. > > Thanks > Kevin > -- Thanks, Jike
> From: Song, Jike > Sent: Wednesday, October 22, 2014 3:35 PM > > On 10/01/2014 12:26 AM, Tian, Kevin wrote: > > From virtualization p.o.v, the ideal case is to run host i915 irq handler in > the > > interrupt context, which meets all the assumption from original code. Using > > tasklet or other manner still has some restriction. This is a major open we'd > > like to hear more from you guys. Is it possible to have i915 driver to request > > two irq numbers: irq1 for real device and irq2 is purely faked one. vgt > handler > > registers on irq1 and i915 hanlder registers on irq2, and then we can use self > > IPI to trigger irq2 when injection is required. But I'm not sure whether this is > > an existing feature in kernel, or need some core enhancement in irq > sub-system... > > Hi Kevin, Daniel, > > I'm so excited to know that, there is an existing feature in kernel: irq_work. > Basicly it allows us to run the host i915 ISR in hardirq context prefectly, what's > needed is to select CONFIG_IRQ_WORK in Kconfig :) > > I'll use that in the v2 patches. > Sounds like a better option. :-) Thanks Kevin
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 742fe8a..e33455a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1668,6 +1668,12 @@ struct drm_i915_private { struct vlv_s0ix_state vlv_s0ix_state; struct { + struct tasklet_struct host_irq_task; + irqreturn_t (*host_isr)(int, void *); + void (*host_irq_uninstall)(struct drm_device *); + } igvt; + + struct { /* * Raw watermark latency values: * in 0.1us units for WM0, diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 080981b..df7c868 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -36,6 +36,7 @@ #include "i915_drv.h" #include "i915_trace.h" #include "intel_drv.h" +#include "i915_vgt.h" static const u32 hpd_ibx[] = { [HPD_CRT] = SDE_CRT_HOTPLUG, @@ -4650,6 +4651,30 @@ static void intel_hpd_irq_reenable_work(struct work_struct *work) intel_runtime_pm_put(dev_priv); } +static void vgt_host_isr_wrapper(unsigned long data) +{ + struct drm_device *dev = (struct drm_device *)data; + struct drm_i915_private *dev_priv = dev->dev_private; + + dev_priv->igvt.host_isr(dev->pdev->irq, dev); +} + +void vgt_schedule_host_isr(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + tasklet_schedule(&dev_priv->igvt.host_irq_task); +} + +static void vgt_irq_uninstall(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + tasklet_kill(&dev_priv->igvt.host_irq_task); + dev_priv->igvt.host_irq_uninstall(dev); + vgt_fini_irq(dev->pdev); +} + void intel_irq_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -4756,6 +4781,16 @@ void intel_irq_init(struct drm_device *dev) dev->driver->enable_vblank = i915_enable_vblank; dev->driver->disable_vblank = i915_disable_vblank; } + + if (i915.enable_vgt) { + vgt_init_irq(dev->pdev, dev); + dev_priv->igvt.host_isr = dev->driver->irq_handler; + dev->driver->irq_handler = vgt_interrupt; + dev_priv->igvt.host_irq_uninstall = dev->driver->irq_uninstall; + dev->driver->irq_uninstall = vgt_irq_uninstall; + tasklet_init(&dev_priv->igvt.host_irq_task, + vgt_host_isr_wrapper, (unsigned long)dev); + } } void intel_hpd_init(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/i915_vgt.h b/drivers/gpu/drm/i915/i915_vgt.h index 03e7f00..553f920 100644 --- a/drivers/gpu/drm/i915/i915_vgt.h +++ b/drivers/gpu/drm/i915/i915_vgt.h @@ -1,6 +1,9 @@ #ifndef _I915_VGT_H_ #define _I915_VGT_H_ +#include <linux/interrupt.h> + +struct drm_device; struct drm_i915_private; #ifdef CONFIG_I915_IGVT @@ -9,6 +12,10 @@ bool i915_start_vgt(struct pci_dev *); void i915_vgt_record_priv(struct drm_i915_private *); bool vgt_emulate_host_read(u32, void *, int, bool, bool); bool vgt_emulate_host_write(u32, void *, int, bool, bool); +void vgt_schedule_host_isr(struct drm_device *); +void vgt_init_irq(struct pci_dev *, struct drm_device *); +void vgt_fini_irq(struct pci_dev *); +irqreturn_t vgt_interrupt(int, void *); #else /* !CONFIG_I915_IGVT */ @@ -33,6 +40,19 @@ static inline bool vgt_emulate_host_write(u32 reg, void *val, int len, return false; } +static inline void vgt_init_irq(struct pci_dev *pdev, struct drm_device *dev) +{ +} + +static inline void vgt_fini_irq(struct pci_dev *pdev) +{ +} + +static inline irqreturn_t vgt_interrupt(int irq, void *data) +{ + return IRQ_NONE; +} + #endif /* CONFIG_I915_IGVT */ #endif diff --git a/drivers/gpu/drm/i915/vgt/vgt.c b/drivers/gpu/drm/i915/vgt/vgt.c index f33baf3..dab4bfc 100644 --- a/drivers/gpu/drm/i915/vgt/vgt.c +++ b/drivers/gpu/drm/i915/vgt/vgt.c @@ -1,6 +1,7 @@ #include <linux/module.h> #include <linux/kthread.h> #include <linux/pci.h> +#include <linux/interrupt.h> #include "../i915_drv.h" #include "vgt.h" @@ -121,3 +122,24 @@ void i915_vgt_record_priv(struct drm_i915_private *priv) { dev_priv = priv; } + +static void vgt_host_irq(struct drm_device *dev) +{ + vgt_schedule_host_isr(dev); +} + +void vgt_init_irq(struct pci_dev *pdev, struct drm_device *dev) +{ + /* TODO: initialize vgt-specific irq handlings after vgt integration */ +} + +void vgt_fini_irq(struct pci_dev *pdev) +{ + /* TODO: cleanup vgt-specific irq handlings after vgt integration */ +} + +irqreturn_t vgt_interrupt(int irq, void *data) +{ + vgt_host_irq(data); + return IRQ_HANDLED; +}
vgt owns the hardware interrupt of the GPU, to satisfy the interrupt requirement from both host side and guest side (e.g. host may have MI_USER_INTERRUPT disabled, while a VM may have it enabled). Sometimes vgt may also need to emulate a virtual interrupt to the host, w/o a hardware interrupt actually triggered. So we need to split the handling between physical interrupts to vgt and virtual interrupts to host i915. Regarding to above requirements, this patch registers a vgt interrupt handler when vgt is enabled, while letting original i915 interrupt handler instead carried in a tasklet. Whenever a virtual interrupt needs to be injected to host i915, tasklet_schedule gets called. Signed-off-by: Jike Song <jike.song@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 6 ++++++ drivers/gpu/drm/i915/i915_irq.c | 35 +++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_vgt.h | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/vgt/vgt.c | 22 ++++++++++++++++++++++ 4 files changed, 83 insertions(+)