Cc Jani for notifying the patch update: Add missing v2 changelog: Take Jani's comment: remove BUG_ON in i915_driver_load Take Daniel's comment: extract the VED setup related functions to i915_ved.c Take Daniel's comment: add kerneldoc to describe i915_ved.c Take Daniel's comment: rename "ipvr-ved" to "ipvr-ved-vlv" Take Daniel's comment: unregister platform device before irq_free_desc() when teardown VED. Take Daniel's comment: update i-g-t to manually unload ipvr.ko before unloading i915.ko Take Daniels' comment: add WARN_ON(!intel_irqs_enabled()) in VED irqchip initialization function. Take Daniel's comment: remove the trace point for VED interrupt. > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf > Of Cheng, Yao > Sent: Wednesday, October 22, 2014 15:09 > To: Ville Syrjälä > Cc: Rao, Ram R; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- > devel@xxxxxxxxxxxxxxxxxxxxx; Jiang, Fei; Abel, Michael J; Vetter, Daniel > Subject: Re: [Intel-gfx] [RFC PATCH v2 1/4] drm/i915: add i915_ved.c to setup > bridge for VED > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > > Sent: Tuesday, October 21, 2014 6:30 PM > > To: Cheng, Yao > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > > Kelley, Sean V; Vetter, Daniel; Abel, Michael J; Jiang, Fei; Rao, Ram > > R > > Subject: Re: [Intel-gfx] [RFC PATCH v2 1/4] drm/i915: add i915_ved.c > > to setup bridge for VED > > > > On Tue, Oct 21, 2014 at 02:36:41PM +0800, Yao Cheng wrote: > > > Setup minimum required resources during i915_driver_load: > > > 1. Create a platform device to share MMIO/IRQ resources 2. Make the > > > platform device child of i915 device for runtime PM. > > > 3. Create IRQ chip to forward the VED irqs. > > > VED driver (a standalone drm driver) probes the VED device and > > > creates a new dri card on install. > > > > > > Currently only supports VED on valleyview. > > > Kerneldoc is updated for i915_ved.c. > > > > > > Signed-off-by: Yao Cheng <yao.cheng@xxxxxxxxx> > > > --- > > > Documentation/DocBook/drm.tmpl | 5 + > > > drivers/gpu/drm/i915/Makefile | 3 + > > > drivers/gpu/drm/i915/i915_dma.c | 11 ++ > > > drivers/gpu/drm/i915/i915_drv.h | 9 ++ > > > drivers/gpu/drm/i915/i915_irq.c | 2 + > > > drivers/gpu/drm/i915/i915_reg.h | 5 + > > > drivers/gpu/drm/i915/i915_ved.c | 264 > > > ++++++++++++++++++++++++++++++++++++++++ > > > 7 files changed, 299 insertions(+) > > > create mode 100644 drivers/gpu/drm/i915/i915_ved.c > > > > > > diff --git a/Documentation/DocBook/drm.tmpl > > > b/Documentation/DocBook/drm.tmpl index d7cfc98..f1787b4 100644 > > > --- a/Documentation/DocBook/drm.tmpl > > > +++ b/Documentation/DocBook/drm.tmpl > > > @@ -3806,6 +3806,11 @@ int num_ioctls;</synopsis> > > > !Fdrivers/gpu/drm/i915/i915_irq.c > > > intel_runtime_pm_disable_interrupts > > > !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts > > > </sect2> > > > + <sect2> > > > + <title>VED video core integration</title> > > > +!Pdrivers/gpu/drm/i915/i915_ved.c VED video core integration > > > +!Idrivers/gpu/drm/i915/i915_ved.c > > > + </sect2> > > > </sect1> > > > <sect1> > > > <title>Display Hardware Handling</title> diff --git > > > a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > > > index > > > 3a6bce0..a4b9252 100644 > > > --- a/drivers/gpu/drm/i915/Makefile > > > +++ b/drivers/gpu/drm/i915/Makefile > > > @@ -80,6 +80,9 @@ i915-y += dvo_ch7017.o \ i915-y += i915_dma.o \ > > > i915_ums.o > > > > > > +# VED for VLV > > > +i915-y += i915_ved.o > > > + > > > obj-$(CONFIG_DRM_I915) += i915.o > > > > > > CFLAGS_i915_trace_points.o := -I$(src) diff --git > > > a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > index 85d14e1..47714e1 100644 > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > @@ -1791,6 +1791,13 @@ int i915_driver_load(struct drm_device *dev, > > unsigned long flags) > > > if (IS_GEN5(dev)) > > > intel_gpu_ips_init(dev_priv); > > > > > > + if (IS_VALLEYVIEW(dev)) { > > > > These must be (IS_VLV && !IS_CHV), or maybe define some HAS_VED() > > macro to hide that. > > Accepted. Will add HAS_VED() to hide this. > > > > > > + ret = vlv_setup_ved(dev); > > > + if (ret < 0) { > > > + DRM_ERROR("failed to setup VED bridge: %d\n", ret); > > > + } > > > + } > > > + > > > intel_runtime_pm_enable(dev_priv); > > > > > > return 0; > > > @@ -1833,6 +1840,10 @@ int i915_driver_unload(struct drm_device > *dev) > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > int ret; > > > > > > + if (IS_VALLEYVIEW(dev)) { > > > + vlv_teardown_ved(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..952df34 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 VED bridge */ > > > + struct platform_device *ved_platdev; > > > + int ved_irq; > > > + > > > > Could be neater to wrap this in a struct: > > > > struct { > > struct platform_device *platdev; > > int irq; > > } ved; > > Ok, thanks for the suggestion. > > > > > > > > /* Old dri1 support infrastructure, beware the dragons ya fools > > entering > > > * here! */ > > > struct i915_dri1_state dri1; > > > @@ -2785,6 +2789,11 @@ void i915_restore_display_reg(struct > > drm_device > > > *dev); void i915_setup_sysfs(struct drm_device *dev_priv); void > > > i915_teardown_sysfs(struct drm_device *dev_priv); > > > > > > +/* i915_ved.c */ > > > +int vlv_setup_ved(struct drm_device *dev); void > > > +vlv_teardown_ved(struct drm_device *dev); void > > > +vlv_ved_irq_handler(struct drm_device *dev); > > > + > > > /* intel_i2c.c */ > > > extern int intel_setup_gmbus(struct drm_device *dev); extern void > > > intel_teardown_gmbus(struct drm_device *dev); diff --git > > > a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > index 737b239..68c2977 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -2177,6 +2177,8 @@ 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) && (iir & > > VLV_VED_BLOCK_INTERRUPT)) > > > + vlv_ved_irq_handler(dev); > > > > This is the vlv irq handler so no need to check the platform type. > > Thnks for pointing out this. Will remove the platform check. > > > > > > /* 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..95421ef 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -1284,6 +1284,11 @@ enum punit_power_well { #define > > > VLV_DISPLAY_BASE 0x180000 #define VLV_MIPI_BASE > > VLV_DISPLAY_BASE > > > > > > +/* forwarding VED irq and sharing MMIO to the VED driver */ > > > +#define VLV_VED_BLOCK_INTERRUPT (1 << 23) > > > > This define should be alongside the other IxR bits. > > Do you mean like this: > Rename it to VLV_VED_IRQ and put alongside VLV_IIR? > > Similar to the following: > #define GTIER 0x4401c > #define GEN8_PCU_IRQ (1<<30) > > > > > > > +#define VLV_VED_BASE 0x170000 > > > +#define VLV_VED_SIZE 0x010000 > > > + > > > #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_ved.c > > > b/drivers/gpu/drm/i915/i915_ved.c new file mode 100644 index > > > 0000000..5dfe4af > > > --- /dev/null > > > +++ b/drivers/gpu/drm/i915/i915_ved.c > > > @@ -0,0 +1,264 @@ > > > +/* > > > + * Copyright © 2014 Intel Corporation > > > + * > > > + * Permission is hereby granted, free of charge, to any person > > > +obtaining a > > > + * copy of this software and associated documentation files (the > > > +"Software"), > > > + * to deal in the Software without restriction, including without > > > +limitation > > > + * the rights to use, copy, modify, merge, publish, distribute, > > > +sublicense, > > > + * and/or sell copies of the Software, and to permit persons to > > > +whom the > > > + * Software is furnished to do so, subject to the following conditions: > > > + * > > > + * The above copyright notice and this permission notice (including > > > +the next > > > + * paragraph) shall be included in all copies or substantial > > > +portions of the > > > + * Software. > > > + * > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY > > KIND, > > > +EXPRESS OR > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > > +MERCHANTABILITY, > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN > > NO EVENT > > > +SHALL > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, > > DAMAGES > > > +OR OTHER > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR > OTHERWISE, > > > +ARISING > > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE > > OR > > > +OTHER DEALINGS > > > + * IN THE SOFTWARE. > > > + * > > > + * Authors: > > > + * Yao Cheng <yao.cheng@xxxxxxxxx> > > > + * > > > + */ > > > + > > > +#include "i915_drv.h" > > > + > > > +/** > > > + * DOC: VED video core integration > > > + * > > > + * Motivation: > > > + * Some platforms (e.g. valleyview) integrates a VED inside GPU to > > > +extend the > > > + * video decoding capability. > > > + * The VED is driven by the standalone drm driver "ipvr" which > > > +covers PowerVR > > > + * VPUs. Since the PowerVR VPUs are also integrated by non-i915 > > > +platforms such > > > + * as GMA500, we'd like to keep ipvr driver and i915 driver > > > +separated and > > > + * independent to each other. To achieve this we do the minimum > > > +work in i915 > > > + * to setup a bridge between ipvr and i915: > > > + * 1. Create a platform device to share MMIO/IRQ resources > > > + * 2. Make the platform device child of i915 device for runtime PM. > > > + * 3. Create IRQ chip to forward the VED irqs. > > > + * ipvr driver probes the VED device and creates a new dri card on install. > > > + * > > > + * Threats: > > > + * Due to the restriction in Linux platform device model, user need > > > +manually > > > + * uninstall ipvr driver before uninstalling i915 module, otherwise > > > +he might > > > + * run into use-after-free issues after i915 removes the platform device. > > > + * > > > + * Implementation: > > > + * The MMIO/REG platform resources are created according to the > > > +registers > > > + * specification. > > > + * When forwarding VED irqs, the flow control handler selection > > > +depends on the > > > + * platform, for example on valleyview handle_simple_irq is enough. > > > + * > > > + */ > > > + > > > +static struct platform_device* vlv_ved_platdev_create(struct > > > +drm_device *dev) { > > > + int ret; > > > + struct resource *rsc = NULL; > > > + struct platform_device *platdev; > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + > > > + platdev = platform_device_alloc("ipvr-ved-vlv", -1); > > > + if (!platdev) { > > > + DRM_ERROR("Failed to allocate VED platform device\n"); > > > + ret = -ENOMEM; > > > + goto err; > > > + } > > > + > > > + rsc = kzalloc(sizeof(*rsc) * 3, GFP_KERNEL); > > > > struct resource seems small enough that this could be on the stack. > > Accepted. Thanks for pointing out. > > > > > > + if (!rsc) { > > > + DRM_ERROR("Failed to allocate resource for VED platform > > device\n"); > > > + ret = -ENOMEM; > > > + goto err; > > > + } > > > + > > > + WARN_ON(dev_priv->ved_irq < 0); > > > + rsc[0].start = rsc[0].end = dev_priv->ved_irq; > > > + rsc[0].flags = IORESOURCE_IRQ; > > > + rsc[0].name = "ipvr-ved-vlv-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-vlv-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-vlv-reg"; > > > > I don't see why you need both MEM and REG resources. Just the MEM > > itself should suffice: > > > > rsc[1].start = pci_resource_start(dev->pdev, 0) + VLV_VED_BASE; > > rsc[1].end = pci_resource_start(dev->pdev, 0) + VLV_VED_BASE + > > VLV_VED_SIZE; > > rsc[1].flags = IORESOURCE_MEM; > > rsc[1].name = "ipvr-ved-vlv-mmio"; > > > > When I write the original code on k3.10 I always got ioremap conflict by > binding only one MMIO resource. > I just tested this on k3.17 and no conflict. :) thanks for pointing out this and I > will update the code. > BTW, it's interesting, is there any update on the ioremap code from 3.10 to > 3.17? > > > Also in the ved driver you're mapping it with ioremap_wc() which isn't > > generally safe to do with registers. I'm not sure the kernel would > > even allow it since i915 has already mapped it as UC. > > Thanks, I'll change it to UC. > > > > > > + > > > + ret = platform_device_add_resources(platdev, rsc, 3); > > > + if (ret) { > > > + DRM_ERROR("Failed to add resource for VED platform > > device: %d\n", ret); > > > + goto err; > > > + } > > > + > > > + /* Runtime-PM hook */ > > > + platdev->dev.parent = dev->dev; > > > + ret = platform_device_add(platdev); > > > + if (ret) { > > > + DRM_ERROR("Failed to add VED platform device: %d\n", ret); > > > + goto err; > > > + } > > > + > > > + kfree(rsc); > > > + return platdev; > > > +err: > > > + if (rsc) > > > + kfree(rsc); > > > + if (platdev) > > > + platform_device_unregister(platdev); > > > + return ERR_PTR(ret); > > > +} > > > + > > > +static void vlv_ved_platdev_destroy(struct drm_device *dev) { > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + if (dev_priv->ved_platdev) > > > + platform_device_unregister(dev_priv->ved_platdev); > > > +} > > > + > > > +static void vlv_ved_irq_unmask(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; > > > + I915_WRITE(VLV_IER, ier); > > > + POSTING_READ(VLV_IER); > > > + > > > + imr = I915_READ(VLV_IMR); > > > + imr &= ~VLV_VED_BLOCK_INTERRUPT; > > > + dev_priv->irq_mask = imr; > > > + I915_WRITE(VLV_IMR, imr); > > > + POSTING_READ(VLV_IMR); > > > > I think you could just follow the same procedure that > > valleyview_display_irqs_install() uses: > > > > dev_priv->irq_mask &= ~VLV_VED_BLOCK_INTERRUPT; > I915_WRITE(VLV_IIR, > > VLV_VED_BLOCK_INTERRUPT); I915_WRITE(VLV_IIR, > > VLV_VED_BLOCK_INTERRUPT); I915_WRITE(VLV_IMR, dev_priv- > >irq_mask); > > I915_WRITE(VLV_IER, ~dev_priv->irq_mask); POSTING_READ(VLV_IER); > > Thanks, accepted. > > > > > > > + > > > + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); } > > > + > > > +static void vlv_ved_irq_mask(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; > > > + I915_WRITE(VLV_IER, ier); > > > + POSTING_READ(VLV_IER); > > > + > > > + imr = I915_READ(VLV_IMR); > > > + imr |= VLV_VED_BLOCK_INTERRUPT; > > > + dev_priv->irq_mask = imr; > > > + I915_WRITE(VLV_IMR, imr); > > > + POSTING_READ(VLV_IMR); > > > > Same here. > > Accepted. > > > > > > + > > > + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); } > > > + > > > +static struct irq_chip vlv_ved_irqchip = { > > > + .name = "ipvr_ved_irqchip", > > > + .irq_mask = vlv_ved_irq_mask, > > > + .irq_unmask = vlv_ved_irq_unmask, > > > +}; > > > + > > > +int vlv_ved_irq_init(struct drm_device *dev, int irq) { > > > + struct drm_i915_private *dev_priv = (struct drm_i915_private *) > > dev->dev_private; > > > + WARN_ON(!intel_irqs_enabled(dev_priv)); > > > + irq_set_chip_and_handler_name(irq, > > > + &vlv_ved_irqchip, > > > + handle_simple_irq, > > > + "ipvr_ved_vlv_irq_handler"); > > > + return irq_set_chip_data(irq, dev); } > > > + > > > +void vlv_ved_irq_handler(struct drm_device *dev) { > > > + int ret; > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + if (dev_priv->ved_irq < 0 && printk_ratelimit()) { > > > + DRM_ERROR("invalid ved irq number: %d\n", dev_priv- > > >ved_irq); > > > + return; > > > + } > > > + ret = generic_handle_irq(dev_priv->ved_irq); > > > + if (ret && printk_ratelimit()) { > > > + DRM_ERROR("error handling vlv ved irq: %d\n", ret); > > > + } > > > +} > > > + > > > +/** > > > + * vlv_setup_ved() - setup the bridge between VED driver and i915 > > > + * @dev: the i915 drm device > > > + * > > > + * set up the minimum required resources for the bridge: irq chip, > > > +platform > > > + * resource and platform device. i915 device is set as parent of > > > +the new > > > + * platform device. > > > + * > > > + * Return: 0 if successful. non-zero if allocation/initialization > > > +fails */ int vlv_setup_ved(struct drm_device *dev) { > > > + int irq; > > > + int ret; > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + struct platform_device *platdev; > > > + dev_priv->ved_irq = -1; > > > + dev_priv->ved_platdev = NULL; > > > + > > > + irq = irq_alloc_descs(-1, 0, 1, 0); > > > + if (irq < 0) { > > > + DRM_ERROR("Failed to allocate IRQ desc: %d\n", irq); > > > + ret = -ENOMEM; > > > + goto err; > > > + } > > > + > > > + ret = vlv_ved_irq_init(dev, irq); > > > + if (ret) { > > > + DRM_ERROR("Failed to initialize irqchip for vlv-ved: %d\n", > > ret); > > > + goto err; > > > + } > > > + dev_priv->ved_irq = irq; > > > + > > > + platdev = vlv_ved_platdev_create(dev); > > > + if (IS_ERR(platdev)) { > > > + DRM_ERROR("Failed to create platform device for vlv- > > ved: %ld\n", PTR_ERR(platdev)); > > > + ret = -EINVAL; > > > + goto err; > > > + } > > > + > > > + dev_priv->ved_platdev = platdev; > > > + return 0; > > > +err: > > > + vlv_teardown_ved(dev); > > > + return ret; > > > +} > > > + > > > +/** > > > + * vlv_teardown_ved() - destroy the bridge between VED driver and > > > +i915 > > > + * @dev: the i915 drm device > > > + * > > > + * release all the resources for VED <-> i915 bridge. > > > + */ > > > +void vlv_teardown_ved(struct drm_device *dev) { > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + vlv_ved_platdev_destroy(dev); > > > + dev_priv->ved_platdev = NULL; > > > + if (dev_priv->ved_irq >= 0) { > > > + irq_free_desc(dev_priv->ved_irq); > > > + } > > > + dev_priv->ved_irq = -1; > > > +} > > > -- > > > 1.9.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
