> -----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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
