Ok, bunch of comments. First a high-level one: I think this qualifies as a new subsystem of i915, and so it would be good to extract this into a new file (i915_ved.c maybe), including adding kerneldoc for the setup function, a short DOC: overview section and pulling it all into the drm kerneldoc (probably a new subsection in the driver core section).
Aside from the lack of documentation just a few small comments below. Overall I really like how cleanly we can integrate vxd support into i915, so good work.
I915_ved.c sounds to be a good place for these code, thx for this suggestion!
For the kerneldoc, I'll add a subsection in the core section for your review.
+static void valleyview_ved_cleanup(struct drm_device *dev) {
- int irq;
- struct drm_i915_private *dev_priv = dev->dev_private;
- irq = platform_get_irq(dev_priv->ved_platdev, 0);
- if (irq >= 0)
irq_free_desc(irq);- platform_device_unregister(dev_priv->ved_platdev);
I think you should unregister the platform device _before_ you free the irq. Otherwise the driver cleanup might freak out. Aside: Does the module reload test for i915 in i-g-t still work with the vxd driver loaded on vlv? Iirc you need to manually unload the vxd driver first to avoid inherit races in the platform device support code, so if that's the case you need to supply a patch for igt, too.
Sorry, what is i-g-t? I'll follow your suggestion, test i-g-t, and patch it if needed.
+}
void i915_update_dri1_breadcrumb(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -1793,6 +1883,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
intel_runtime_pm_enable(dev_priv);
- if (IS_VALLEYVIEW(dev)) {
BUG_ON(valleyview_ved_init(dev));- }
As Jani said, now BUG_ON here please. Also, I think you should just print an error here but not even fail driver load for i915 (i.e. still return 0). We try to only fail i915 load if there's a hard issue with the modeset part of the driver, if render/GT/... parts fail to init then we'll continue. The idea is that if the user at least has a working display he can grab a lot more useful information about the init failure than when i915 is completely dead.
Sorry for this BUG_ON. Will replace with useful return value and message.
- return 0;
out_power_well: @@ -1833,6 +1927,10 @@ int i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int ret;
- if (IS_VALLEYVIEW(dev)) {
valleyview_ved_cleanup(dev);- }
- ret = i915_gem_suspend(dev); if (ret) { DRM_ERROR("failed to idle hardware: %d\n", ret); diff --git
a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 821ba26..aa8a183 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1709,6 +1709,10 @@ struct drm_i915_private {
uint32_t bios_vgacntr;
- /* used for setup sub device for valleyview */
- struct platform_device *ved_platdev;
- int ved_irq;
- /* Old dri1 support infrastructure, beware the dragons ya fools
entering
* here! */struct i915_dri1_state dri1; @@ -2921,6 +2925,8 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val); int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val); int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
+extern int valleyview_initialize_ved_irq(struct drm_device *dev, int +irq);
#define FORCEWAKE_RENDER (1 << 0) #define FORCEWAKE_MEDIA (1 << 1) #define FORCEWAKE_ALL (FORCEWAKE_RENDER |
FORCEWAKE_MEDIA)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 737b239..25c8cde 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2142,12 +2142,75 @@ static void i9xx_hpd_irq_handler(struct
drm_device *dev)
} }
+static void valleyview_enable_ved_irq(struct irq_data *d) {
- struct drm_device *dev = d->chip_data;
- struct drm_i915_private *dev_priv = (struct drm_i915_private *)
dev->dev_private;
- unsigned long irqflags;
- u32 imr, ier;
- spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
- ier = I915_READ(VLV_IER);
- ier |= VLV_VED_BLOCK_INTERRUPT;
- DRM_DEBUG_DRIVER("%s IER=>0x%08x\n", __func__, ier);
- I915_WRITE(VLV_IER, ier);
- POSTING_READ(VLV_IER);
- imr = I915_READ(VLV_IMR);
- imr &= ~VLV_VED_BLOCK_INTERRUPT;
- dev_priv->irq_mask = imr;
- DRM_DEBUG_DRIVER("%s IMR=>0x%08x\n", __func__, imr);
- I915_WRITE(VLV_IMR, imr);
- POSTING_READ(VLV_IMR);
- spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
+static void valleyview_disable_ved_irq(struct irq_data *d) {
- struct drm_device *dev = d->chip_data;
- struct drm_i915_private *dev_priv = (struct drm_i915_private *)
dev->dev_private;
- unsigned long irqflags;
- u32 imr, ier;
- spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
- ier = I915_READ(VLV_IER);
- ier &= ~VLV_VED_BLOCK_INTERRUPT;
- DRM_DEBUG_DRIVER("%s IER=>0x%08x\n", __func__, ier);
- I915_WRITE(VLV_IER, ier);
- POSTING_READ(VLV_IER);
- imr = I915_READ(VLV_IMR);
- imr |= VLV_VED_BLOCK_INTERRUPT;
- dev_priv->irq_mask = imr;
- DRM_DEBUG_DRIVER("%s IMR=>0x%08x\n", __func__, imr);
- I915_WRITE(VLV_IMR, imr);
- POSTING_READ(VLV_IMR);
- spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
+int valleyview_initialize_ved_irq(struct drm_device *dev, int irq) {
- static struct irq_chip ved_irqchip = {
.name = "ipvr_ved_irqchip",.irq_mask = valleyview_disable_ved_irq,.irq_unmask = valleyview_enable_ved_irq,- };
Please move this struct out of the function, we generally keep vtables global. Also please add a WARN_ON(!intel_irqs_enabled()) here so that we make sure that the driver load ordering is correct. Same for the unregister side, interrupts should still be working when we remove the platform device.
Sure. Will move this to i915_ved.c as you suggested.
- irq_set_chip_and_handler_name(irq,
&ved_irqchip,handle_simple_irq,"ipvr_ved_irq_handler");- return irq_set_chip_data(irq, dev);
+}
static irqreturn_t valleyview_irq_handler(int irq, void *arg) { struct drm_device *dev = arg; struct drm_i915_private *dev_priv = dev->dev_private; u32 iir, gt_iir, pm_iir; irqreturn_t ret = IRQ_NONE;
int ved_ret;
while (true) { /* Find, clear, then process each source of interrupt */ @@ -
2177,6
+2240,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) snb_gt_irq_handler(dev, dev_priv, gt_iir); if (pm_iir) gen6_rps_irq_handler(dev_priv, pm_iir);
if (IS_VALLEYVIEW(dev) && dev_priv->ved_irq >= 0&& (iir & VLV_VED_BLOCK_INTERRUPT)) {ved_ret = generic_handle_irq(dev_priv->ved_irq);if (ved_ret)DRM_ERROR("Error forwarding VEDirq: %d\n", ved_ret);
trace_valleyview_ved_event(iir);I don't see a value in this tracepoint here - we don't have any other tracepoints in i915 for irq events. If you want this and the irq core doesn't provide any tracpoints (haven't checked but would be really suprising), then I think the right place to add this is in the vxd driver itself.
OK vxd driver has its own irq trace_points so will remove this one.
}valleyview_pipestat_irq_handler(dev, iir); diff --git
- signalled in iir */
a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 2ed02c3..89c8a06 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1281,9 +1281,13 @@ enum punit_power_well { #define GFX_PSMI_GRANULARITY (1<<10) #define GFX_PPGTT_ENABLE (1<<9)
+#define VLV_VED_BASE 0x170000 +#define VLV_VED_SIZE 0x010000 #define VLV_DISPLAY_BASE 0x180000 #define VLV_MIPI_BASE VLV_DISPLAY_BASE
+#define VLV_VED_BLOCK_INTERRUPT (1 << 23)
#define VLV_GU_CTL0 (VLV_DISPLAY_BASE + 0x2030) #define VLV_GU_CTL1 (VLV_DISPLAY_BASE + 0x2034) #define SCPD0 0x0209c /* 915+ only */ diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index f5aa006..522bd1d 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -587,6 +587,21 @@ TRACE_EVENT(intel_gpu_freq_change, TP_printk("new_freq=%u", __entry->freq) );
+TRACE_EVENT(valleyview_ved_event,
TP_PROTO(u32 iir),TP_ARGS(iir),TP_STRUCT__entry(__field(u32, iir)),TP_fast_assign(__entry->iir = iir;),TP_printk("forwarded with iir 0x%08x", __entry->iir) );#endif /* _I915_TRACE_H_ */
/* This part must be outside protection */
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch