> 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 VED
> irq: %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.
>
> > + }
> > /* Call regardless, as some status bits might not be
> > * signalled in iir */
> > valleyview_pipestat_irq_handler(dev, iir); diff --git
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
