On Mon, Oct 13, 2014 at 08:15:01PM +0800, Yao Cheng wrote:
> Setup following resources during i915_driver_load:
> 1. create a child platform and resource
> 2. allocate a new IRQ line and irq chip
> 3. set up IRQ mask/unmask callbacks
> vxd392 driver (if installed) will bind itself to the
> platform device and create new drm device
>
> Signed-off-by: Yao Cheng <yao.cheng@xxxxxxxxx>
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.
Cheers, Daniel
> ---
> drivers/gpu/drm/i915/i915_dma.c | 98 +++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_drv.h | 6 +++
> drivers/gpu/drm/i915/i915_irq.c | 70 ++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h | 4 ++
> drivers/gpu/drm/i915/i915_trace.h | 15 ++++++
> 5 files changed, 193 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 85d14e1..73c78d1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -85,6 +85,96 @@ intel_read_legacy_status_page(struct drm_i915_private *dev_priv, int reg)
> #define READ_BREADCRUMB(dev_priv) READ_HWSP(dev_priv, I915_BREADCRUMB_INDEX)
> #define I915_BREADCRUMB_INDEX 0x21
>
> +static int valleyview_ved_init(struct drm_device *dev)
> +{
> + int ret;
> + int irq = -1;
> + struct resource *rsc = NULL;
> + struct drm_i915_private *dev_priv = dev->dev_private;;
> +
> + dev_priv->ved_platdev = platform_device_alloc("ipvr-ved", -1);
I think we should put vlv into the platform name, in case this driver will
ever be reused on other platforms. So maybe "ipvr-ved-vxd"?
> + if (unlikely(!dev_priv->ved_platdev)) {
> + DRM_ERROR("Failed to allocate VED platform device\n");
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + rsc = kzalloc(sizeof(*rsc) * 3, GFP_KERNEL);
> + if (unlikely(!rsc)) {
> + DRM_ERROR("Failed to allocate resource for VED platform device\n");
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* init IRQ number and chip/callbacks */
> + irq = irq_alloc_descs(-1, 0, 1, 0);
> + if (unlikely(irq < 0)) {
> + DRM_ERROR("Failed to allocate IRQ number: %d\n", irq);
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + ret = valleyview_initialize_ved_irq(dev, irq);
> + if (unlikely(ret)) {
> + DRM_ERROR("Failed to initialize VED IRQ: %d\n", ret);
> + goto err;
> + }
> +
> + dev_priv->ved_irq = irq;
> + rsc[0].start = rsc[0].end = irq;
> + rsc[0].flags = IORESOURCE_IRQ;
> + rsc[0].name = "ipvr-ved-irq";
> +
> + /* MMIO/REG for child's use */
> + rsc[1].start = pci_resource_start(dev->pdev, 0);
> + rsc[1].end = pci_resource_start(dev->pdev, 0) + 2*1024*1024; /* gen7 */
> + rsc[1].flags = IORESOURCE_MEM;
> + rsc[1].name = "ipvr-ved-mmio";
> +
> + rsc[2].start = VLV_VED_BASE;
> + rsc[2].end = VLV_VED_BASE + VLV_VED_SIZE;
> + rsc[2].flags = IORESOURCE_REG;
> + rsc[2].name = "ipvr-ved-reg";
> +
> + ret = platform_device_add_resources(dev_priv->ved_platdev, rsc, 3);
> + if (unlikely(ret)) {
> + DRM_ERROR("Failed to add resource for VED platform device: %d\n", ret);
> + goto err;
> + }
> +
> + /* Runtime-PM hook */
> + dev_priv->ved_platdev->dev.parent = dev->dev;
> + ret = platform_device_add(dev_priv->ved_platdev);
> + if (unlikely(ret)) {
> + DRM_ERROR("Failed to add VED platform device: %d\n", ret);
> + goto err;
> + }
> +
> + kfree(rsc);
> + DRM_INFO("Successfully initialized Valleyview-VED\n");
> + return 0;
> +err:
> + if (rsc)
> + kfree(rsc);
> + if (dev_priv->ved_platdev)
> + platform_device_unregister(dev_priv->ved_platdev);
> + if (irq >= 0)
> + irq_free_desc(irq);
> + return ret;
> +}
> +
> +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.
> +}
> +
> 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.
> +
> 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.
> + 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.
> + }
> /* 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
