RE: [Intel-gfx] [RFC PATCH 1/3] drm/i915: add vxd392 bridge in i915

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux