[RFC PATCH v4 3/4] ipvr: user mode helper for ipvr drm driver
+ commenters of v1~v3 Thanks, Yao > -Original Message- > From: Sean V Kelley [mailto:seanvk at posteo.de] > Sent: Thursday, January 8, 2015 8:35 > To: Intel-gfx at lists.freedesktop.org > Cc: dri-devel at lists.freedesktop.org; Cheng, Yao; Sean V Kelley > Subject: [RFC PATCH v4 3/4] ipvr: user mode helper for ipvr drm driver > > From: Yao Cheng > > add usermode helper for the ipvr kernel driver. > test_ioctl: test kernel driver by directly ioctl > > v2: > take Emil's comments > - correctly align ipvr_drm.h > > v3: > take Daniel Vetter and Daniel Stone's comments, and implement PRIME > - correctly align ipvr_drm.h > - use __u32 family in ipvr_drm.h > - rip out explicit fence from libdrm_ipvr > - implemented PRIME support > - add relocation fixup implementation > > v4 > bug fixing and add stress test tool > - rename ipvr/test_ioctl.c to ipvr/test_ipvr.c > - implement parallel ioctl stress test in test_ipvr.c > - implement parallel libdrm stress test in test_ipvr.c > - update ipvr_drm.h to keep consistent with kernel change > - remove unused "buffer_ofs/alloc_size/ext_handle" from struct > drm_ipvr_bo > - remove unused arguments for some public functions > - fix a few foolish copy-paste bugs > - fix 32bit compiling issue > > Signed-off-by: Yao Cheng > Signed-off-by: Sean V Kelley > --- > Makefile.am|6 +- > Makefile.sources |1 + > configure.ac | 26 +- > include/drm/ipvr_drm.h | 259 +++ > ipvr/Makefile.am | 57 +++ > ipvr/Makefile.sources |5 + > ipvr/ipvr_bufmgr.h | 132 ++ > ipvr/ipvr_bufmgr_gem.c | 1188 > > ipvr/libdrm_ipvr.pc.in | 11 + > ipvr/test_ipvr.c | 919 + > 10 files changed, 2602 insertions(+), 2 deletions(-) > create mode 100644 include/drm/ipvr_drm.h > create mode 100644 ipvr/Makefile.am > create mode 100644 ipvr/Makefile.sources > create mode 100644 ipvr/ipvr_bufmgr.h > create mode 100644 ipvr/ipvr_bufmgr_gem.c > create mode 100644 ipvr/libdrm_ipvr.pc.in > create mode 100644 ipvr/test_ipvr.c > > diff --git a/Makefile.am b/Makefile.am > index 3cb516c..035d937 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -33,6 +33,10 @@ if HAVE_INTEL > INTEL_SUBDIR = intel > endif > > +if HAVE_IPVR > +IPVR_SUBDIR = ipvr > +endif > + > if HAVE_NOUVEAU > NOUVEAU_SUBDIR = nouveau > endif > @@ -57,7 +61,7 @@ if HAVE_TEGRA > TEGRA_SUBDIR = tegra > endif > > -SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(NOUVEAU_SUBDIR) > $(RADEON_SUBDIR) $(OMAP_SUBDIR) $(EXYNOS_SUBDIR) > $(FREEDRENO_SUBDIR) $(TEGRA_SUBDIR) tests man > +SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(IPVR_SUBDIR) > $(NOUVEAU_SUBDIR) $(RADEON_SUBDIR) $(OMAP_SUBDIR) > $(EXYNOS_SUBDIR) $(FREEDRENO_SUBDIR) $(TEGRA_SUBDIR) tests man > > libdrm_la_LTLIBRARIES = libdrm.la > libdrm_ladir = $(libdir) > diff --git a/Makefile.sources b/Makefile.sources > index 566f7b5..819a0cb 100644 > --- a/Makefile.sources > +++ b/Makefile.sources > @@ -18,6 +18,7 @@ LIBDRM_INCLUDE_H_FILES := \ > include/drm/drm_mode.h \ > include/drm/drm_sarea.h \ > include/drm/i915_drm.h \ > + include/drm/ipvr_drm.h \ > include/drm/mach64_drm.h \ > include/drm/mga_drm.h \ > include/drm/nouveau_drm.h \ > diff --git a/configure.ac b/configure.ac > index c88a1c5..9fea4db 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -68,6 +68,11 @@ AC_ARG_ENABLE(intel, > [Enable support for intel's KMS API (default: auto)]), > [INTEL=$enableval], [INTEL=auto]) > > +AC_ARG_ENABLE(ipvr, > + AS_HELP_STRING([--disable-ipvr], > + [Enable support for valeyview's IPVR hardware decode (default: > auto)]), > + [IPVR=$enableval], [IPVR=auto]) > + > AC_ARG_ENABLE(radeon, > AS_HELP_STRING([--disable-radeon], > [Enable support for radeon's KMS API (default: auto)]), > @@ -209,7 +214,7 @@ if test "x$drm_cv_atomic_primitives" = "xlibatomic- > ops"; then > AC_DEFINE(HAVE_LIB_ATOMIC_OPS, 1, [Enable if you have > libatomic-ops-dev installed]) > fi > > -if test "x$INTEL" != "xno" -o "x$RADEON" != "xno" -o "x$NOUVEAU" != > "xno"; then > +if test "x$INTEL" != "xno" -o "x$IPVR" != "xno" -o "x$RADEON" != "xno" -o > "x$NOUVEAU" != "xno"; then > if test "x$dr
[RFC PATCH V4 1/4] drm/i915: add i915_ved.c to setup bridge for VED
+ commenters of v1~v3
Thanks,
Yao
> -Original Message-
> From: Sean V Kelley [mailto:seanvk at posteo.de]
> Sent: Thursday, January 8, 2015 8:35
> To: Intel-gfx at lists.freedesktop.org
> Cc: dri-devel at lists.freedesktop.org; Cheng, Yao; Sean V Kelley
> Subject: [RFC PATCH V4 1/4] drm/i915: add i915_ved.c to setup bridge for
> VED
>
> From: Yao Cheng
>
> 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.
>
> v2:
> take Daniel & Jani's comments
> - extract change to new file i915_ved.c
> - add kerneldoc
> - change 'ipvr-ved' to 'ipvr-vlv-ved' for extensibility
> - unregister platdev before irq_free_desc
> - add WARN_ON(!intel_irqs_enabled) in irq init code
> - remove unnecessary trace point
> - remove unnecessary BUG_ON
>
> v3:
> take Ville's comments and VED PRIME support
> - add HAS_VED() check
> - add ved struct to make code neat
> - no need to check platform in vlv_irq_handler
> - i915_reg.h update
> - no need to kmalloc for small amount of resource
> - remove unnecessary REG resource
> - follow vlv_display_irqs_install() to implement VED mask/unmask
> - workaround nommu_map_sg issue by set dma_mask to support
> VED PRIME.
>
> v4:
> take Bob's comments
> - add more detail on the use-after-free issue description
> - mask VED irq before removing the child device
>
> Signed-off-by: Yao Cheng
> Signed-off-by: Sean V Kelley
> ---
> 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
> | 12 ++
> drivers/gpu/drm/i915/i915_irq.c | 2 +
> drivers/gpu/drm/i915/i915_reg.h | 4 +
> drivers/gpu/drm/i915/i915_ved.c | 270
>
> 7 files changed, 307 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/i915_ved.c
>
> diff --git a/Documentation/DocBook/drm.tmpl
> b/Documentation/DocBook/drm.tmpl index 56e2a9b..9db989c 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -3867,6 +3867,11 @@ int
> num_ioctls; !Fdrivers/gpu/drm/i915/i915_irq.c
> intel_runtime_pm_disable_interrupts
> !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts
>
> +
> +VED video core integration
> +!Pdrivers/gpu/drm/i915/i915_ved.c VED video core integration
> +!Idrivers/gpu/drm/i915/i915_ved.c
> +
>
>
>Display Hardware Handling diff --git
> a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index
> e4083e4..7d0bbfa 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -84,6 +84,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 887d88f..cd96618 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -828,6 +828,13 @@ int i915_driver_load(struct drm_device *dev,
> unsigned long flags)
> if (IS_GEN5(dev))
> intel_gpu_ips_init(dev_priv);
>
> + if (HAS_VED(dev)) {
> + 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;
> @@ -870,6 +877,10 @@ int i915_driver_unload(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> int ret;
>
> + if (HAS_VED(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 502a01b..aa39d47 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1773,6 +1773,12 @@ struct drm_i915_private {
>
> uint32_t bios_vgacntr;
[RFC PATCH v4 0/4] drm driver for VED in Intel GPU
+ commenters of v1~v3 Thanks, Yao > -Original Message- > From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf > Of Sean V Kelley > Sent: Thursday, January 8, 2015 8:35 > To: Intel-gfx at lists.freedesktop.org > Cc: dri-devel at lists.freedesktop.org > Subject: [RFC PATCH v4 0/4] drm driver for VED in Intel GPU > > > drm/ipvr is a new GEM driver for VED (PowerVR's VPU integrated in Intel > GPU), which extends video capability. > A new Kconfig added for building ipvr driver: > > CONFIG_DRM_IPVR: Build option for ipvr module > > The driver name "ipvr" means the PowerVR's core wrapped by Intel. The > PowerVR VPUs are also integrated by non-i915 platforms such as GMA500, so > we keep ipvr driver and i915 driver separated and independent to each other. > To achieve this we do the minimum change in i915: i915_ved.c added for > setting up the bridge between VED and i915, kerneldoc also updated. > > User mode drm helper "libdrm_ipvr.so" and simple ioctl/execute test is > included. > > one test script "drv_module_reload" in i-g-t also updated to support ipvr > loading and unloading. Due to restriction in Linux platform device model, we > have to manually unload ipvr before unloading i915. > > Yao Cheng (4): > [1/4] drm/i915: add i915_ved.c to setup bridge for VED [2/4] drm/ipvr: drm > driver for VED [3/4] libdrm/ipvr: user mode helper for ipvr drm driver [4/4] > i- > g-t: tests/drv_module_reload: add ipvr support > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC PATCH v4 4/4] tests/drv_module_reload: add ipvr support
+ commenters on v1~v3 When locking issue resolved, this patch can be removed. Thanks, Yao > -Original Message- > From: Sean V Kelley [mailto:seanvk at posteo.de] > Sent: Thursday, January 8, 2015 8:35 > To: Intel-gfx at lists.freedesktop.org > Cc: dri-devel at lists.freedesktop.org; Cheng, Yao; Sean V Kelley > Subject: [RFC PATCH v4 4/4] tests/drv_module_reload: add ipvr support > > From: Yao Cheng > > on vlv, if ipvr is installed, it need be manually unloaded before i915, > otherwise user might run into use-after-free issue. > > v2: > added this patch per Daniel's comment > > v3: > no change > > Signed-off-by: Yao Cheng > Signed-off-by: Sean V Kelley > --- > tests/drv_module_reload | 16 > 1 file changed, 16 insertions(+) > > diff --git a/tests/drv_module_reload b/tests/drv_module_reload index > 5cbff89..82c67bd 100755 > --- a/tests/drv_module_reload > +++ b/tests/drv_module_reload > @@ -24,6 +24,14 @@ rmmod snd_hda_intel &> /dev/null > > #ignore errors in ips - gen5 only > rmmod intel_ips &> /dev/null > + > +# vlv only for now: > +# due to platform device model limitation, need unload ipvr manually if > +lsmod | grep ipvr &> /dev/null ; then > + echo Need manually unload ipvr.ko. > + rmmod ipvr > +fi > + > rmmod i915 > #ignore errors in intel-gtt, often built-in rmmod intel-gtt &> /dev/null @@ > - > 31,6 +39,11 @@ rmmod intel-gtt &> /dev/null rmmod drm_kms_helper &> > /dev/null rmmod drm &> /dev/null > > +if lsmod | grep ipvr &> /dev/null ; then > + echo WARNING: ipvr.ko still loaded! > + exit 1 > +fi > + > if lsmod | grep i915 &> /dev/null ; then > echo WARNING: i915.ko still loaded! > exit 1 > @@ -41,6 +54,9 @@ fi > modprobe i915 > echo 1 > /sys/class/vtconsole/vtcon1/bind > > +# for vlv, load VED driver > +modprobe ipvr > + > modprobe snd_hda_intel > > # try to run something > -- > 2.1.0
[RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support
> -Original Message- > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Monday, January 5, 2015 16:40 > To: Cheng, Yao > Cc: Daniel Vetter; Thierry Reding; intel-gfx at lists.freedesktop.org; dri- > devel at lists.freedesktop.org; Kelley, Sean V; Chehab, John; > emil.l.velikov at gmail.com; Jiang, Fei; Beckett, Robert; Barbalho, Rafael > Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support > > On Sun, Dec 21, 2014 at 02:40:24PM +, Cheng, Yao wrote: > > > -Original Message- > > > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] > > > Sent: Thursday, December 18, 2014 19:21 > > > To: Thierry Reding > > > Cc: Cheng, Yao; intel-gfx at lists.freedesktop.org; dri- > > > devel at lists.freedesktop.org; Kelley, Sean V; Chehab, John; > > > emil.l.velikov at gmail.com; Jiang, Fei > > > Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr > > > support > > > > > > On Thu, Dec 18, 2014 at 11:04 AM, Thierry Reding > > > > > > wrote: > > > >> I double checked the symptom and found it was a deadlock on > > > drm_global_mutex. > > > >> When i915_driver_load() registers the platform device while ipvr > > > >> module > > > is in the system, ipvr's probe() function tries to lock > > > drm_global_mutex which was already held by i915. > > > >> I think either of the following 2 actions need to be moved to a > > > >> bottom half > > > e.g. a work queue: > > > >> platform_device_add () call in i915_ved.c (called during > > > i915_driver_load()) > > > >> drm_dev_register() call during ipvr's probe() Which one > > > >> makes more sense? pls kindly advise (I personally prefer the former > one.). > > > > > > > > Yes, that's somewhat ugly, but I don't see a way around that. I'd > > > > also think that moving platform_device_add() to a workqueue would > > > > be the best option here. > > > > > > Or we simply kill drm_global_mutex for platform drivers that don't > > > use the - > > > >probe hook. It should work when they have a correct order betwen > > > drm_dev_alloc and _register and all the code in between. So just > > > ditch the - > > > >load callback in teh ipvr driver and rework the load sequence as > > > >suggested > > > somewhere else and this is fixed already. No need for bottom halfs I > think. > > > > Daniel, sorry I didn't quite understand "platform drivers that don't > > use the probe hook". For initialization, the ipvr platform driver's > > probe() is called in following 2 possible paths: > > 1. ipvr installed before i915. In this case, ipvr's probe() is called > > inside i915_driver_load() and falls into the drm_global_mutex dead lock. > > 2. i915 installed before ipvr. In this case, ipvr's probe() is called > > without drm_global_mutex held by i915 and no dead lock issue. > > If we kill drm_global_mutex, will path 2 run into issue? And in your > > suggestion, how to rework the load sequence? Do you mean calling > > ipvr's > > load() callback directly during platform driver probe()? > > Hm right it's not that simple really. What we need in more detail is: > - Move the mutex_lock(_global_mutex) out of drm_dev_register into > all the callers. If a driver has a ->load() callback it most likely is > racy with the usual load ordering issues. > > - Rework ipvr to no longer have a ->load callback. Insteaed use the > following sequence (in the platform ->probe callback): > > drm_dev_alloc(); > ipvr_load(); > drm_dev_register(); > > With that ordering we don't need the additional guarantees that > drm_global_mutex provides and we can avoid to take that lock around > drm_dev_registrer() call in the ipvr code. Thanks for the detailed explanation, Daniel! That sounds to be a small refactor on drm core, and need change many drm drivers: nouveau, tegra, udl. Should it be a standalone RFC patch? > > This should resolve the deadlock I hope. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support
> -Original Message- > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] > Sent: Thursday, December 18, 2014 19:21 > To: Thierry Reding > Cc: Cheng, Yao; intel-gfx at lists.freedesktop.org; dri- > devel at lists.freedesktop.org; Kelley, Sean V; Chehab, John; > emil.l.velikov at gmail.com; Jiang, Fei > Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support > > On Thu, Dec 18, 2014 at 11:04 AM, Thierry Reding > wrote: > >> I double checked the symptom and found it was a deadlock on > drm_global_mutex. > >> When i915_driver_load() registers the platform device while ipvr module > is in the system, ipvr's probe() function tries to lock drm_global_mutex which > was already held by i915. > >> I think either of the following 2 actions need to be moved to a bottom half > e.g. a work queue: > >> platform_device_add () call in i915_ved.c (called during > i915_driver_load()) > >> drm_dev_register() call during ipvr's probe() Which one makes > >> more sense? pls kindly advise (I personally prefer the former one.). > > > > Yes, that's somewhat ugly, but I don't see a way around that. I'd also > > think that moving platform_device_add() to a workqueue would be the > > best option here. > > Or we simply kill drm_global_mutex for platform drivers that don't use the - > >probe hook. It should work when they have a correct order betwen > drm_dev_alloc and _register and all the code in between. So just ditch the - > >load callback in teh ipvr driver and rework the load sequence as suggested > somewhere else and this is fixed already. No need for bottom halfs I think. Daniel, sorry I didn't quite understand "platform drivers that don't use the probe hook". For initialization, the ipvr platform driver's probe() is called in following 2 possible paths: 1. ipvr installed before i915. In this case, ipvr's probe() is called inside i915_driver_load() and falls into the drm_global_mutex dead lock. 2. i915 installed before ipvr. In this case, ipvr's probe() is called without drm_global_mutex held by i915 and no dead lock issue. If we kill drm_global_mutex, will path 2 run into issue? And in your suggestion, how to rework the load sequence? Do you mean calling ipvr's load() callback directly during platform driver probe()? > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support
> -Original Message- > From: Thierry Reding [mailto:thierry.reding at gmail.com] > Sent: Wednesday, December 17, 2014 16:13 > To: Cheng, Yao > Cc: Daniel Vetter; intel-gfx at lists.freedesktop.org; dri- > devel at lists.freedesktop.org; daniel.vetter at ffwll.ch; Kelley, Sean V; > Chehab, > John; emil.l.velikov at gmail.com; Jiang, Fei > Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support Thanks Thiery for the suggestion, pls see my inline comments > > > Thierry/Daniel, the actual symptom is, after "rmmod i915", though > > drm_drv_release() is also called on the child device "ipvr", I still > > see the module exist in the system (check it by "lsmod"). > > Which module? ipvr or i915? The ipvr module still exist by checking "lsmod" after rmmod i915 > > > This causes issue when I modprobe i915 and ipvr again later. > > What issue are you seeing? If your driver can't deal with a situation where > it's > probed again after being removed then you have a bug. > I double checked the symptom and found it was a deadlock on drm_global_mutex. When i915_driver_load() registers the platform device while ipvr module is in the system, ipvr's probe() function tries to lock drm_global_mutex which was already held by i915. I think either of the following 2 actions need to be moved to a bottom half e.g. a work queue: platform_device_add () call in i915_ved.c (called during i915_driver_load()) drm_dev_register() call during ipvr's probe() Which one makes more sense? pls kindly advise (I personally prefer the former one.). > > I don't understand why this happens but I believe what Daniel said: > > "grabbing a module refcount for the platform device doesn't work (it > > would pin the module forever)" > > What I'd expect to happen is this: > > # modprobe i915 > i915 registers a platform devices > # modprobe ipvr > driver core probes ipvr device > # modprobe -r i915 > i915 removes the platform device (ipvr's ->remove() is called) > > I guess if you don't do anything else, then indeed the ipvr module will stay > around, but the above should work idempotently, that is you should be able > to repeat it an unlimited number of times and nothing should break. > > In fact you should be able to run the following in any permutation without > causing a crash: > > # modprobe i915 > # modprobe ipvr > # modprobe -r ipvr > # modprobe -r i915 > > If any permutation results in a crash you have a bug. I assume all the permutations will work after fixing the deadlock. > > Thierry
[RFC PATCH v3 3/4] ipvr: user mode helper for ipvr drm driver
Add Jon/Bob/Raf for detail review. > -Original Message- > From: Cheng, Yao > Sent: Saturday, November 22, 2014 3:09 > To: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; > daniel.vetter at ffwll.ch; Kelley, Sean V; Chehab, John > Cc: Jiang, Fei; dh.herrmann at gmail.com; jani.nikula at linux.intel.com; > emil.l.velikov at gmail.com; ville.syrjala at linux.intel.com; > jbarnes at virtuousgeek.org; daniel at fooishbar.org; Cheng, Yao > Subject: [RFC PATCH v3 3/4] ipvr: user mode helper for ipvr drm driver > > add usermode helper for the ipvr kernel driver. > test_ioctl: test kernel driver by directly ioctl > > v2: > take Emil's comments > - correctly align ipvr_drm.h > > v3: > take Daniel Vetter and Daniel Stone's comments, and implement PRIME > - correctly align ipvr_drm.h > - use __u32 family in ipvr_drm.h > - rip out explicit fence from libdrm_ipvr > - implemented PRIME support > - add relocation fixup implementation > > Signed-off-by: Yao Cheng > --- > Makefile.am|6 +- > Makefile.sources |1 + > configure.ac | 26 +- > include/drm/ipvr_drm.h | 278 +++ > ipvr/Makefile.am | 57 +++ > ipvr/Makefile.sources |5 + > ipvr/ipvr_bufmgr.h | 149 ++ > ipvr/ipvr_bufmgr_gem.c | 1200 > > ipvr/libdrm_ipvr.pc.in | 11 + > ipvr/test_ioctl.c | 423 + > 10 files changed, 2154 insertions(+), 2 deletions(-) > create mode 100644 include/drm/ipvr_drm.h > create mode 100644 ipvr/Makefile.am > create mode 100644 ipvr/Makefile.sources > create mode 100644 ipvr/ipvr_bufmgr.h > create mode 100644 ipvr/ipvr_bufmgr_gem.c > create mode 100644 ipvr/libdrm_ipvr.pc.in > create mode 100644 ipvr/test_ioctl.c > > diff --git a/Makefile.am b/Makefile.am > index 3952a88..2227add 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -33,6 +33,10 @@ if HAVE_INTEL > INTEL_SUBDIR = intel > endif > > +if HAVE_IPVR > +IPVR_SUBDIR = ipvr > +endif > + > if HAVE_NOUVEAU > NOUVEAU_SUBDIR = nouveau > endif > @@ -53,7 +57,7 @@ if HAVE_FREEDRENO > FREEDRENO_SUBDIR = freedreno > endif > > -SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(NOUVEAU_SUBDIR) > $(RADEON_SUBDIR) $(OMAP_SUBDIR) $(EXYNOS_SUBDIR) > $(FREEDRENO_SUBDIR) tests man > +SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(IPVR_SUBDIR) > $(NOUVEAU_SUBDIR) $(RADEON_SUBDIR) $(OMAP_SUBDIR) > $(EXYNOS_SUBDIR) $(FREEDRENO_SUBDIR) tests man > > libdrm_la_LTLIBRARIES = libdrm.la > libdrm_ladir = $(libdir) > diff --git a/Makefile.sources b/Makefile.sources > index d86fb2a..96f8c60 100644 > --- a/Makefile.sources > +++ b/Makefile.sources > @@ -18,6 +18,7 @@ LIBDRM_INCLUDE_H_FILES := \ > include/drm/drm_mode.h \ > include/drm/drm_sarea.h \ > include/drm/i915_drm.h \ > + include/drm/ipvr_drm.h \ > include/drm/mach64_drm.h \ > include/drm/mga_drm.h \ > include/drm/nouveau_drm.h \ > diff --git a/configure.ac b/configure.ac > index ee59b03..6dcf1b2 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -68,6 +68,11 @@ AC_ARG_ENABLE(intel, > [Enable support for intel's KMS API (default: auto)]), > [INTEL=$enableval], [INTEL=auto]) > > +AC_ARG_ENABLE(ipvr, > + AS_HELP_STRING([--disable-ipvr], > + [Enable support for baytrail's hardware VP8 decode (default: > auto)]), > + [IPVR=$enableval], [IPVR=auto]) > + > AC_ARG_ENABLE(radeon, > AS_HELP_STRING([--disable-radeon], > [Enable support for radeon's KMS API (default: auto)]), > @@ -204,7 +209,7 @@ if test "x$drm_cv_atomic_primitives" = "xlibatomic- > ops"; then > AC_DEFINE(HAVE_LIB_ATOMIC_OPS, 1, [Enable if you have > libatomic-ops-dev installed]) > fi > > -if test "x$INTEL" != "xno" -o "x$RADEON" != "xno" -o "x$NOUVEAU" != > "xno"; then > +if test "x$INTEL" != "xno" -o "x$IPVR" != "xno" -o "x$RADEON" != "xno" -o > "x$NOUVEAU" != "xno"; then > if test "x$drm_cv_atomic_primitives" = "xnone"; then > if test "x$INTEL" != "xauto"; then > if test "x$INTEL" != "xno"; then > @@ -214,6 +219,14 @@ if test "x$INTEL" != "xno" -o "x$RADEON" != "xno" -o > "x$NOUVEAU" != "xno"; then > AC_MSG_WARN([Disabling libdrm_intel
[RFC PATCH v3 2/4] drm/ipvr: drm driver for VED
Add Jon/Bob/Raf for detail review.
> -Original Message-
> From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf
> Of Cheng, Yao
> Sent: Thursday, November 27, 2014 19:49
> To: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> daniel.vetter at ffwll.ch; Kelley, Sean V; Chehab, John
> Cc: emil.l.velikov at gmail.com; Jiang, Fei
> Subject: RE: [RFC PATCH v3 2/4] drm/ipvr: drm driver for VED
>
> > -Original Message-
> > From: Cheng, Yao
> > Sent: Saturday, November 22, 2014 3:07
> > To: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> > daniel.vetter at ffwll.ch; Kelley, Sean V; Chehab, John
> > Cc: Jiang, Fei; dh.herrmann at gmail.com; jani.nikula at linux.intel.com;
> > emil.l.velikov at gmail.com; ville.syrjala at linux.intel.com;
> > jbarnes at virtuousgeek.org; daniel at fooishbar.org; Cheng, Yao
> > Subject: [RFC PATCH v3 2/4] drm/ipvr: drm driver for VED
> > +typedef struct drm_ipvr_private {
> > + struct drm_device *dev;
> > + struct pci_dev *pci_root;
> > +
> > + /* IMG video context */
> > + struct list_head ipvr_ctx_list;
>
> The current design leads to ctx leak. There's no ctx_list for each file
> struct, so
> each create_context_ioctl causes one ctx leak.
> Need to move the ctx_list from dev_private to file_private.
>
> > + spinlock_t ipvr_ctx_lock;
> > + struct idr ipvr_ctx_idr;
> > + struct ipvr_context default_ctx;
> > +
> > + /* PM related */
> > + atomic_t pending_events;
> > +
> > + /* exec related */
> > + struct ipvr_validate_context validate_ctx;
> > +
> > + /* IMG MMU specific */
> > + struct ipvr_mmu_driver *mmu;
> > + /*struct ipvr_mmu_pd *pf_pd;*/
> > + atomic_t ipvr_mmu_invaldc;
> > +
> > + /* GEM mm related */
> > + struct ipvr_gem_stat ipvr_stat;
> > + struct kmem_cache *ipvr_bo_slab;
> > + struct ipvr_address_space addr_space;
> > +
> > + /* fence related */
> > + u32 last_seq;
> > + wait_queue_head_t fence_queue;
> > + struct ipvr_fence_driver fence_drv;
> > +
> > + /* MMIO window shared from parent device */
> > + u8 __iomem* reg_base;
> > +
> > + /*
> > +* VED specific
> > +*/
> > + struct ved_private *ved_private;
> > +}drm_ipvr_private_t;
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support
> -Original Message- > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Monday, November 24, 2014 21:15 > To: Thierry Reding > Cc: Daniel Vetter; Cheng, Yao; intel-gfx at lists.freedesktop.org; dri- > devel at lists.freedesktop.org; daniel.vetter at ffwll.ch; Kelley, Sean V; > Chehab, > John; emil.l.velikov at gmail.com; Jiang, Fei > Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support > > On Mon, Nov 24, 2014 at 10:55:46AM +0100, Thierry Reding wrote: > > On Fri, Nov 21, 2014 at 09:36:33PM +0100, Daniel Vetter wrote: > > > On Fri, Nov 21, 2014 at 09:27:04PM +0100, Thierry Reding wrote: > > > > On Sat, Nov 22, 2014 at 03:10:01AM +0800, Yao Cheng wrote: > > > > > on vlv, if ipvr is installed, it need be manually unloaded > > > > > before i915, otherwise user might run into use-after-free issue. > > > > > > > > Huh? That doesn't sound right. What exactly is it that's going wrong? > > > > You should never have to do this. If you do you're almost > > > > certainly doing something wrong in the kernel module. > > > > > > It's the hilarity called platform devices. Removing them is somewhat > > > racy, so doing that upfront makes the entire thing a bit safer. The > > > use after free is on the text, since grabbing a module refcount for > > > the platform device doesn't work (it would pin the module forever). > > > > I don't understand what the issue is here. I've used platform devices > > quite extensively on ARM and I've never encountered a situation where > > they were insufficient (or racy for that matter). > > > > If I understand correctly what this commit tries to achieve, then it > > unloads one module before another module that it depends on so that > > the dependency can be removed subsequently without causing a crash. > > That sounds really brittle to me. How are you going to document this > > for users so that they don't accidentally go and unload the i915 > > module and crash their system? > > Module unloading taints your kernel and isn't an end-user supported feature. > That simple ;-) > > Also afaik the problem is that you actually can't unload i915 until you've > unloaded the subordinate driver, since i915 registering the platform driver > prevents unload. Or at least that was my understanding, I didn't test this > myself. I just asked whether the unload script still works and apparently it > breaks. > > I guess what's different with ARM is that DT creates all the platform devices, > and not modules themselves? > -Daniel Thierry/Daniel, the actual symptom is, after "rmmod i915", though drm_drv_release() is also called on the child device "ipvr", I still see the module exist in the system (check it by "lsmod"). This causes issue when I modprobe i915 and ipvr again later. I don't understand why this happens but I believe what Daniel said: "grabbing a module refcount for the platform device doesn't work (it would pin the module forever)". > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[Intel-gfx] [RFC PATCH v3 1/4] drm/i915: add i915_ved.c to setup bridge for VED
> -Original Message-
> From: Beckett, Robert
> Sent: Saturday, November 29, 2014 0:59
> To: Cheng, Yao; intel-gfx at lists.freedesktop.org; dri-
> devel at lists.freedesktop.org; daniel.vetter at ffwll.ch; Kelley, Sean V;
> Chehab,
> John
> Cc: emil.l.velikov at gmail.com; Jiang, Fei; dh.herrmann at gmail.com;
> daniel at fooishbar.org
> Subject: Re: [Intel-gfx] [RFC PATCH v3 1/4] drm/i915: add i915_ved.c to setup
> bridge for VED
> > + * 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.
>
> Can you go in to more detail on what you consider to be the restriction in the
> platform device model?
>
> When removing the device via platform_device_unregister, it will call the
> remove callback of any drivers handling this device (via the bus remove
> function). It is then up to the driver to ensure no further usage of the
> device
> from which it is being removed. This usually involves removing all user input
> vectors, disabling interrupts involved and flushing/canceling any delayed
> work. This should prevent any further use of the device by the driver.
>
> The driver's remove function is called in a direct call chain from
> platform_device_unregister, so by the time it returns, there should be no
> further chance of accesses.
>
Bob, thx for your review comments.
The symptom is, after "rmmod i915", though drm_drv_release() is also called on
the child device "ipvr", I still see the module exist in the system (check it
by "lsmod"). This causes issue when I modprobe i915 and ipvr again later.
Actually I don't understand why this restriction exists, but accroding to
Daniel, grabbing a module refcount for the platform device doesn't work (it
would pin the module forever).
> > +void vlv_teardown_ved(struct drm_device *dev) {
> > + struct drm_i915_private *dev_priv = dev->dev_private;
>
> you may want to mask the i915 interrupt for the VED block before removing
> the device.
>
Thx, will add it.
> > + vlv_ved_platdev_destroy(dev);
> > + if (dev_priv->ved.irq >= 0)
> > + irq_free_desc(dev_priv->ved.irq);
> > +}
> >
>
>
> Generally it is a nicely interfaced child device.
>
[RFC PATCH v3 2/4] drm/ipvr: drm driver for VED
> -Original Message-
> From: Cheng, Yao
> Sent: Saturday, November 22, 2014 3:07
> To: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> daniel.vetter at ffwll.ch; Kelley, Sean V; Chehab, John
> Cc: Jiang, Fei; dh.herrmann at gmail.com; jani.nikula at linux.intel.com;
> emil.l.velikov at gmail.com; ville.syrjala at linux.intel.com;
> jbarnes at virtuousgeek.org; daniel at fooishbar.org; Cheng, Yao
> Subject: [RFC PATCH v3 2/4] drm/ipvr: drm driver for VED
> +typedef struct drm_ipvr_private {
> + struct drm_device *dev;
> + struct pci_dev *pci_root;
> +
> + /* IMG video context */
> + struct list_head ipvr_ctx_list;
The current design leads to ctx leak. There's no ctx_list for each file struct,
so each create_context_ioctl causes one ctx leak.
Need to move the ctx_list from dev_private to file_private.
> + spinlock_t ipvr_ctx_lock;
> + struct idr ipvr_ctx_idr;
> + struct ipvr_context default_ctx;
> +
> + /* PM related */
> + atomic_t pending_events;
> +
> + /* exec related */
> + struct ipvr_validate_context validate_ctx;
> +
> + /* IMG MMU specific */
> + struct ipvr_mmu_driver *mmu;
> + /*struct ipvr_mmu_pd *pf_pd;*/
> + atomic_t ipvr_mmu_invaldc;
> +
> + /* GEM mm related */
> + struct ipvr_gem_stat ipvr_stat;
> + struct kmem_cache *ipvr_bo_slab;
> + struct ipvr_address_space addr_space;
> +
> + /* fence related */
> + u32 last_seq;
> + wait_queue_head_t fence_queue;
> + struct ipvr_fence_driver fence_drv;
> +
> + /* MMIO window shared from parent device */
> + u8 __iomem* reg_base;
> +
> + /*
> + * VED specific
> + */
> + struct ved_private *ved_private;
> +}drm_ipvr_private_t;
[RFC PATCH v3 2/4] drm/ipvr: drm driver for VED
> -Original Message-
> From: Cheng, Yao
> Sent: Saturday, November 22, 2014 3:07
> To: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> daniel.vetter at ffwll.ch; Kelley, Sean V; Chehab, John
> Cc: Jiang, Fei; dh.herrmann at gmail.com; jani.nikula at linux.intel.com;
> emil.l.velikov at gmail.com; ville.syrjala at linux.intel.com;
> jbarnes at virtuousgeek.org; daniel at fooishbar.org; Cheng, Yao
> Subject: [RFC PATCH v3 2/4] drm/ipvr: drm driver for VED
>
> +static void
> +ipvr_drm_preclose(struct drm_device *dev, struct drm_file *file_priv)
> +{
> + /* if user didn't destory ctx explicitly, remove ctx here */
> + struct drm_ipvr_private *dev_priv;
> + struct drm_ipvr_file_private *ipvr_fpriv;
> + struct ved_private *ved_priv;
> + struct ipvr_context *ipvr_ctx = NULL;
> + unsigned long irq_flags;
> +
> + IPVR_DEBUG_ENTRY("enter\n");
> + dev_priv = dev->dev_private;
> + ipvr_fpriv = file_priv->driver_priv;
> + ved_priv = dev_priv->ved_private;
> +
> + if (ipvr_fpriv->ctx_id == IPVR_CONTEXT_INVALID_ID)
> + return;
> + ipvr_ctx = (struct ipvr_context *)
> + idr_find(_priv->ipvr_ctx_idr, ipvr_fpriv->ctx_id);
Need protection on ipvr_ctx_idr. Same to the other idr related code.
> + if (!ipvr_ctx || (ipvr_ctx->ipvr_fpriv != ipvr_fpriv)) {
> + IPVR_DEBUG_GENERAL("ctx for id %d has already
> destroyed\n",
> + ipvr_fpriv->ctx_id);
> + return;
> + }
> +
> + /**
> + * fixme: remove this work-around (WA the issue that calling
> + * close() with queued cmd might cause state machine issue).
> + * we should wait for only the cmds sent from contexts in this file
> + * instead of all cmds
> + */
> + ipvr_fence_wait_empty_locked(dev_priv);
> +
> + IPVR_DEBUG_PM("Video:remove context type 0x%x\n", ipvr_ctx-
> >ctx_type);
> + mutex_lock(_priv->ved_mutex);
> + if (ved_priv->ipvr_ctx == ipvr_ctx )
> + ved_priv->ipvr_ctx = NULL;
> + mutex_unlock(_priv->ved_mutex);
> +
> + spin_lock_irqsave(_priv->ipvr_ctx_lock, irq_flags);
> + list_del(_ctx->head);
> + ipvr_fpriv->ctx_id = IPVR_CONTEXT_INVALID_ID;
> + spin_unlock_irqrestore(_priv->ipvr_ctx_lock, irq_flags);
> +
> + idr_remove(_priv->ipvr_ctx_idr, ipvr_ctx->ctx_id);
> +
> + kfree(ipvr_ctx );
> + kfree(ipvr_fpriv);
> +}
[RFC PATCH v3 2/4] drm/ipvr: drm driver for VED
> -Original Message-
> From: Cheng, Yao
> Sent: Saturday, November 22, 2014 3:07
> To: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> daniel.vetter at ffwll.ch; Kelley, Sean V; Chehab, John
> Cc: Jiang, Fei; dh.herrmann at gmail.com; jani.nikula at linux.intel.com;
> emil.l.velikov at gmail.com; ville.syrjala at linux.intel.com;
> jbarnes at virtuousgeek.org; daniel at fooishbar.org; Cheng, Yao
> Subject: [RFC PATCH v3 2/4] drm/ipvr: drm driver for VED
>
> +
> +int ipvr_gem_mmap_offset_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv)
> +{
> + int ret;
> + struct drm_ipvr_gem_mmap_offset *args = data;
> + struct drm_ipvr_gem_object *obj;
> +
> + IPVR_DEBUG_ENTRY("getting mmap offset for BO %u.\n", args-
> >handle);
> + obj = to_ipvr_bo(drm_gem_object_lookup(dev, file_priv, args-
> >handle));
> +
> + /* create map offset */
> + ret = drm_gem_create_mmap_offset(>base);
> + if (ret) {
> + IPVR_ERROR("could not allocate mmap offset: %d\n", ret);
> + return ret;
> + }
> + args->offset = drm_vma_node_offset_addr(>base.vma_node);
> + return 0;
> +}
I missed an unreferenced here.
[RFC PATCH 3/3] libdrm: user mode helper for ipvr drm driver
Hi Daniel, weâve resolved this in patch v2. From: Daniel Stone [mailto:dan...@fooishbar.org] Sent: Wednesday, October 29, 2014 0:56 To: Jiang, Fei Cc: Emil Velikov; Cheng, Yao; intel-gfx at lists.freedesktop.org; Vetter, Daniel; dri-devel at lists.freedesktop.org Subject: Re: [RFC PATCH 3/3] libdrm: user mode helper for ipvr drm driver Hi, On 17 October 2014 01:36, Jiang, Fei mailto:fei.jiang at intel.com>> wrote: Thanks for Emil's suggestion. You are right, we need make sure structure size aligned on 8 bytes, which is important for 32bit-64bit compatible case. While you're at it, please don't use enum as a type inside ioctls, since the size can vary by compiler. Please use a uint32_t or whatever instead, assigning enum values to that. Cheers, Daniel Fei -Original Message- From: Emil Velikov [mailto:emil.l.velikov at gmail.com<mailto:emil.l.veli...@gmail.com>] Sent: Thursday, October 16, 2014 11:20 PM To: Cheng, Yao; intel-gfx at lists.freedesktop.org<mailto:intel-gfx at lists.freedesktop.org> Cc: emil.l.velikov at gmail.com<mailto:emil.l.velikov at gmail.com>; Jiang, Fei; dri-devel at lists.freedesktop.org<mailto:dri-devel at lists.freedesktop.org>; Vetter, Daniel Subject: Re: [RFC PATCH 3/3] libdrm: user mode helper for ipvr drm driver On 16/10/14 15:33, Cheng, Yao wrote: > Hi Emil, > Sorry, what do you mean by "correctly aligned"? does it mean the paddings in > this data structure? > Afaict for compatibility reasons the struct size have to be "aligned" (multiple of 8 bytes), or if you prefer - the struct is missing the required padding :) I've only skimmed through the patch so it may be that other structs are having this issue. Cheers, Emil >> -Original Message- >> From: Emil Velikov [mailto:emil.l.velikov at gmail.com<mailto:emil.l.velikov >> at gmail.com>] >> Sent: Wednesday, October 15, 2014 5:24 PM >> To: Cheng, Yao; intel-gfx at lists.freedesktop.org<mailto:intel-gfx at >> lists.freedesktop.org> >> Cc: emil.l.velikov at gmail.com<mailto:emil.l.velikov at gmail.com>; Jiang, >> Fei; >> dri-devel at lists.freedesktop.org<mailto:dri-devel at >> lists.freedesktop.org>; Vetter, Daniel >> Subject: Re: [RFC PATCH 3/3] libdrm: user mode helper for ipvr drm >> driver >> >> Hi Yao, >> >> struct drm_ipvr_gem_userptr does not seem to be correctly aligned - >> is that intentional ? Might be worth checking if anything else in >> ipvr_drm.h and ipvr_bufmgr.h is in the same boat. >> >> Cheers, >> Emil >> ___ dri-devel mailing list dri-devel at lists.freedesktop.org<mailto:dri-devel at lists.freedesktop.org> http://lists.freedesktop.org/mailman/listinfo/dri-devel -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20141029/abc06763/attachment.html>
[Intel-gfx] [RFC PATCH v2 2/4] drm/ipvr: drm driver for VED
CC David for notifying the patch update: Add the missing v2 changelog: Take David's comment: add mmap support, remove the MMAP_IOCTL and add MMAP_OFFSET_IOCTL Take David's comment: remove postclose() and move code to preclose() Take David's comment: set NULL to set_busid Forgot to take David's comment (Sorry, will add it in v3 patch): Replace drm_platform_init/drm_put_dev with drm_dev_alloc, drm_dev_register, drm_dev_unregister and drm_dev_unref > -Original Message- > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Wednesday, October 22, 2014 16:51 > To: Cheng, Yao > Cc: Daniel Vetter; intel-gfx at lists.freedesktop.org; dri- > devel at lists.freedesktop.org; Kelley, Sean V; Vetter, Daniel; Abel, Michael > J; > Jiang, Fei; Rao, Ram R; David Herrmann > Subject: Re: [Intel-gfx] [RFC PATCH v2 2/4] drm/ipvr: drm driver for VED > > On Wed, Oct 22, 2014 at 06:37:16AM +, Cheng, Yao wrote: > > > -Original Message- > > > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of > > > Daniel Vetter > > > Sent: Tuesday, October 21, 2014 5:08 PM > > > To: Cheng, Yao > > > Cc: intel-gfx at lists.freedesktop.org; > > > dri-devel at lists.freedesktop.org; Kelley, Sean V; Vetter, Daniel; > > > Abel, Michael J; Jiang, Fei; Rao, Ram R > > > Subject: Re: [Intel-gfx] [RFC PATCH v2 2/4] drm/ipvr: drm driver for > > > VED > > > > > > On Tue, Oct 21, 2014 at 02:36:42PM +0800, Yao Cheng wrote: > > > > Probes VED and creates a new drm device for hardware accelerated > > > > video decoding. > > > > Currently support VP8 decoding on valleyview. > > > > > > > > Signed-off-by: Yao Cheng > > > > > > The in-patch changelog here is missing, and there's also no > > > indication in the cover letter for what changes you've made. On a > > > quick look you've incorporated some of David's feedback, but not all > > > of it. That's not good, since if you only partially apply review > > > feedback then you essentially force reviewers to read the entire > > > patch again, which is a good way to driver them away. Also you > > > should Cc: (in the sob section of the patch) all the people who have > commented on your patch already. > > > > Oops, sorry for not following the upstreaming rules :( I might have > > overlooked some of David's comment..have to learn more about the > > rules. For this version, I'll add changelog by replying my patch with > > cc to those commenters, I assume this is not too late > > > > > > > > With that out of the way some high-level review: > > > - I think we need the full libva implementation to review the interfaces > > > properly. At least the little libdrm test program doesn't seem to fully > > > exercise it all. > > > > The libva driver need some time to be fully open sourced, but I can > > upload the code to Sean's private github repo for your access. I'll > > sync with Sean and you internally. > > It doesn't need to be the final libva driver of course, just something so that > people can look at the userspace side. So upload to some github account is > perfectly ok. > > Or do you mean we still have legal review pending on those patches? In that > case I think we need to wait for that to complete first. > > > > - The ioctl structs need to be cleaned up. You can't use uint32_t and > > > similar typedefs since they can clash with userspace. You must use > __u32 > > > and friends. Also, some of the padding fields arent' really required - > > > if you only have 4byte types then you don't need to align to 8 bytes. > > > > > > - Input validation on ioctls looks spotty at best. E.g. if you have any > > > padding fields you need to check that they are 0, otherwise we can't > > > ever reuse them as flags fields. And on principle _all_ input fields > > > must be validated first. > > > > > > For some good guidelines for ioctls see > > > http://blog.ffwll.ch/2013/11/botching-up-ioctls.html > > > > > > > Thanks for pointing me to the ioctl instruction... I'll read it > > carefully and update the ioctl interfaces... > > > > > - Locking seems to be inexistent in places, at least some of the idr > > > manipulation very much looks like it's done lock-free. That doesn't work > > > well. > > > > Yes, probably we haven't considered all the scenarios carefully, is it > > possible to review them in an
[Intel-gfx] [RFC PATCH v2 1/4] drm/i915: add i915_ved.c to setup bridge for VED
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 at lists.freedesktop.org] On Behalf
> Of Cheng, Yao
> Sent: Wednesday, October 22, 2014 15:09
> To: Ville Syrj?l?
> Cc: Rao, Ram R; intel-gfx at lists.freedesktop.org; dri-
> devel at lists.freedesktop.org; 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 at linux.intel.com]
> > Sent: Tuesday, October 21, 2014 6:30 PM
> > To: Cheng, Yao
> > Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> > 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
> > > ---
> > > 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;
> > > !Fdrivers/gpu/drm/i915/i915_irq.c
> > > intel_runtime_pm_disable_interrupts
> > > !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts
> > >
> > > +
> > > +VED video core integration
> > > +!Pdrivers/gpu/drm/i915/i915_ved.c VED video core integration
> > > +!Idrivers/gpu/drm/i915/i915_ved.c
> > > +
> > >
> > >
> > >Display Hardware Handling 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.
>
> Accept
[Intel-gfx] [RFC PATCH v2 2/4] drm/ipvr: drm driver for VED
> > > > > > With that out of the way some high-level review: > > > - I think we need the full libva implementation to review the interfaces > > > properly. At least the little libdrm test program doesn't seem to fully > > > exercise it all. > > > > The libva driver need some time to be fully open sourced, but I can > > upload the code to Sean's private github repo for your access. I'll > > sync with Sean and you internally. > > It doesn't need to be the final libva driver of course, just something so that > people can look at the userspace side. So upload to some github account is > perfectly ok. > > Or do you mean we still have legal review pending on those patches? In that > case I think we need to wait for that to complete first. I seeyes you're right, it's still under legal review. We'll put it to internet as soon as being approved. > > > - Locking seems to be inexistent in places, at least some of the idr > > > manipulation very much looks like it's done lock-free. That doesn't work > > > well. > > > > Yes, probably we haven't considered all the scenarios carefully, is it > > possible to review them in an internal discussion? > > Imo no need for private review since I didn't spot anything fundamentally > wrong. It's just a lot of small details, and for those I think m-l review is > a good > tool. But someone needs to do that, and I don't really have the time for it. I see, thanks. > > > > - You implement file-descriptor based fences, but then also have the > more > > > gem-traditional wait ioctl working on buffer objects. That's a bit a > > > funky mix of implicit and explicit fencing. Furthermore adding new > > > private fence objects isn't a good idea now that everyon is talking > > > about de-staging android syncpts as the official userspace interface. > > > > > > Also, your userspace patches don't use this, so maybe we can just rip it > > > all out? > > > > Currently the libdrm_ipvr.so uses both the WAIT IOCTL and FD style > > fence... At beginning, both drm_ipvr_gem_bo_alloc() and > > drm_ipvr_gem_bo_wait() use the WAIT IOCTL. > > In drm_ipvr_gem_bo_alloc(), libdrm_ipvr tries to return an existing > > free BO instead of requesting kernel via IOCTL, like libdrm_intel does. > > Eventually we think the status query on multiple BOs is inefficient, > > so we added the FD style fence to let libdrm_ipvr call select() to do > > a batch query. I'm fine to drop one and keep the other. Which one is > > preferred by GEM? The WAIT_IOCTL or the FD fence? Or do you suggest > > directly use the Android syncpts? > > The wait ioctl is the usual approach with gem drivers. Explicit fencing is > still in > flux like I've said, so charging ahead and locking down an interface doesn't > seem like a good idea. And I'd be _really_ surprised if you can benchmark the > benefits of explicit fencing, so I don't think you can even justify the added > complexity. Understood...We didn't do real benchmark, the "inefficient" just means the logic in code. Will double-check the perf, and rip out the FD-based fence in v3 patch if no real benefit. > > > > - I'm a bit unclear on your usage of vxd_/pvr_ prefixes. > > > > > > > Thanks for pointing out this, shall I add some description about this in > > next > patch (in git commit message)? > > We use different prefixes to distinguish the function scope, like we used to > do on GMA series (Android product): > > ved: decoding function only > > vec: encoding function only (for future extension) > > vsp: post-processing runction only (for future extension) > > ipvr: common for all encoding/decoding/postproc > > Yeah, explaining this kind of stuff in the commit message would be great. > Or just go ahead and add a new vxd section in the drm docbook (like we > already have for i915) and add such high-level information there. Thanks, will add this in v3 patch. > > > > The driver is fairly big and I don't really have the time to do a > > > full blown review of even just the interfaces. I think we need to > > > have some internal discussions about how to do this, but meanwhile > > > we can cover some of the high-level bits. > > > > > > > This is great, I'll talk with Sean on how to run this. > > Yeah, we need to internally figure out how to do the review. Thx I asked Sean to co-ordinate this :) > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[Intel-gfx] [RFC PATCH v2 1/4] drm/i915: add i915_ved.c to setup bridge for VED
> > > Please resend with a patch changelog to account for my review > comments. > > > And Ville's. Plus cc us both. And if there's anything you didn't > > > address, you must reply to the review and we need to further discuss this. > > > > > > > Daniel, I see, thanks for the instruction. > > Do you mean resending the [RFC PATCH v2] with changelog and cc list? > > Or adding changelog/cc when sending [RFC PATCH v3]? > > I think you could just add the per-patch changelog for both v2 and v3 when > sending out v3. > -Daniel Got it! Many thanks! > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[Intel-gfx] [RFC PATCH v2 1/4] drm/i915: add i915_ved.c to setup bridge for VED
> -Original Message-
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, October 21, 2014 8:09 PM
> To: Cheng, Yao
> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> 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
>
> Please resend with a patch changelog to account for my review comments.
> And Ville's. Plus cc us both. And if there's anything you didn't address, you
> must reply to the review and we need to further discuss this.
>
Daniel, I see, thanks for the instruction.
Do you mean resending the [RFC PATCH v2] with changelog and cc list?
Or adding changelog/cc when sending [RFC PATCH v3]?
> Thanks, Daniel
> > ---
> > 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;
> > !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_disable_interrupts
> > !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts
> >
> > +
> > +VED video core integration
> > +!Pdrivers/gpu/drm/i915/i915_ved.c VED video core integration
> > +!Idrivers/gpu/drm/i915/i915_ved.c
> > +
> >
> >
> >Display Hardware Handling 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)) {
> > + 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;
> > +
> > /* Old dri1 support in
[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 at linux.intel.com]
> Sent: Tuesday, October 21, 2014 6:30 PM
> To: Cheng, Yao
> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> 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
> > ---
> > 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;
> > !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_disable_interrupts
> > !Fdrivers/gpu/drm/i915/i915_irq.c intel_runtime_pm_enable_interrupts
> >
> > +
> > +VED video core integration
> > +!Pdrivers/gpu/drm/i915/i915_ved.c VED video core integration
> > +!Idrivers/gpu/drm/i915/i915_ved.c
> > +
> >
> >
> >Display Hardware Handling 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! */
>
[RFC PATCH 3/3] libdrm: user mode helper for ipvr drm driver
Thx Emil. Understood now. Will re-scan the data structures and update. > -Original Message- > From: Emil Velikov [mailto:emil.l.velikov at gmail.com] > Sent: Thursday, October 16, 2014 11:20 PM > To: Cheng, Yao; intel-gfx at lists.freedesktop.org > Cc: emil.l.velikov at gmail.com; Jiang, Fei; dri-devel at > lists.freedesktop.org; > Vetter, Daniel > Subject: Re: [RFC PATCH 3/3] libdrm: user mode helper for ipvr drm driver > > On 16/10/14 15:33, Cheng, Yao wrote: > > Hi Emil, > > Sorry, what do you mean by "correctly aligned"? does it mean the paddings > in this data structure? > > > Afaict for compatibility reasons the struct size have to be "aligned" > (multiple of 8 bytes), or if you prefer - the struct is missing the required > padding :) I've only skimmed through the patch so it may be that other > structs are having this issue. > > Cheers, > Emil > > >> -Original Message- > >> From: Emil Velikov [mailto:emil.l.velikov at gmail.com] > >> Sent: Wednesday, October 15, 2014 5:24 PM > >> To: Cheng, Yao; intel-gfx at lists.freedesktop.org > >> Cc: emil.l.velikov at gmail.com; Jiang, Fei; > >> dri-devel at lists.freedesktop.org; Vetter, Daniel > >> Subject: Re: [RFC PATCH 3/3] libdrm: user mode helper for ipvr drm > >> driver > >> > >> Hi Yao, > >> > >> struct drm_ipvr_gem_userptr does not seem to be correctly aligned - > >> is that intentional ? Might be worth checking if anything else in > >> ipvr_drm.h and ipvr_bufmgr.h is in the same boat. > >> > >> Cheers, > >> Emil > >>
[Intel-gfx] [RFC PATCH 1/3] drm/i915: add vxd392 bridge in i915
> 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(_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);
> > +
[Intel-gfx] [RFC PATCH 0/3] drm driver for baytrail's vxd392
Hi Daniel, this patch [RFC PATCH 3/3] libdrm: user mode helper for ipvr drm driver adds the ipvr part into libdrm. Do you mean this patch is missing? > -Original Message- > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Thursday, October 16, 2014 7:46 PM > To: Cheng, Yao > Cc: intel-gfx at lists.freedesktop.org; Jiang, Fei; dri- > devel at lists.freedesktop.org; Vetter, Daniel > Subject: Re: [Intel-gfx] [RFC PATCH 0/3] drm driver for baytrail's vxd392 > > On Mon, Oct 13, 2014 at 08:15:00PM +0800, Yao Cheng wrote: > > drm/ipvr is a new GEM driver for baytrail's vxd392, which accelerates VP8 > video decoding. > > The driver name "ipvr" means the PowerVR's IP wrapped by Intel. In the > future, ipvr may support other platforms such as Merrifield. > > Code is placed at drivers/gpu/drm/ipvr and the following two new Kconfig > are added: > > CONFIG_DRM_IPVR: Build option for ipvr module > > CONFIG_DRM_IPVR_EC: Experimental feature of error concealment > > > > User mode drm helper "libdrm_ipvr.so" and simple test are also included. > > git repos/patches for the userspace side seems to be missing. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[RFC PATCH 3/3] libdrm: user mode helper for ipvr drm driver
Hi Emil, Sorry, what do you mean by "correctly aligned"? does it mean the paddings in this data structure? > -Original Message- > From: Emil Velikov [mailto:emil.l.velikov at gmail.com] > Sent: Wednesday, October 15, 2014 5:24 PM > To: Cheng, Yao; intel-gfx at lists.freedesktop.org > Cc: emil.l.velikov at gmail.com; Jiang, Fei; dri-devel at > lists.freedesktop.org; > Vetter, Daniel > Subject: Re: [RFC PATCH 3/3] libdrm: user mode helper for ipvr drm driver > > Hi Yao, > > struct drm_ipvr_gem_userptr does not seem to be correctly aligned - is > that intentional ? Might be worth checking if anything else in > ipvr_drm.h and ipvr_bufmgr.h is in the same boat. > > Cheers, > Emil >
[Intel-gfx] [RFC PATCH 2/3] drm/ipvr: drm driver for vxd392
Hi Daniel/Hermann,
> -Original Message-
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Thursday, October 16, 2014 6:29 PM
> To: Cheng, Yao
> Cc: David Herrmann; Vetter, Daniel; Intel Graphics Development; Jiang, Fei;
> dri-devel at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [RFC PATCH 2/3] drm/ipvr: drm driver for vxd392
>
> On Wed, Oct 15, 2014 at 02:14:33AM +, Cheng, Yao wrote:
> > Hi Herrmann
> >
> > > -Original Message-
> > > From: David Herrmann [mailto:dh.herrmann at gmail.com]
> > > Sent: Monday, October 13, 2014 10:27 PM
> > > To: Cheng, Yao
> > > Cc: Intel Graphics Development; Jiang, Fei;
> > > dri-devel at lists.freedesktop.org; Vetter, Daniel
> > > Subject: Re: [RFC PATCH 2/3] drm/ipvr: drm driver for vxd392
> > >
> > > Hi
> > >
> > > > +static struct drm_ioctl_desc ipvr_gem_ioctls[] = {
> > > > + DRM_IOCTL_DEF_DRV(IPVR_CONTEXT_CREATE,
> > > > + ipvr_context_create_ioctl, DRM_UNLOCKED),
> > > > + DRM_IOCTL_DEF_DRV(IPVR_CONTEXT_DESTROY,
> > > > + ipvr_context_destroy_ioctl, DRM_UNLOCKED),
> > > > + DRM_IOCTL_DEF_DRV(IPVR_MISC,
> > > > + ipvr_misc_ioctl, DRM_AUTH),
> > > > + DRM_IOCTL_DEF_DRV(IPVR_GEM_EXECBUFFER,
> > > > + ipvr_gem_execbuffer, DRM_AUTH|DRM_UNLOCKED),
> > > > + DRM_IOCTL_DEF_DRV(IPVR_GEM_BUSY,
> > > > + ipvr_gem_busy_ioctl, DRM_AUTH|DRM_UNLOCKED),
> > > > + DRM_IOCTL_DEF_DRV(IPVR_GEM_CREATE,
> > > > + ipvr_gem_create_ioctl, DRM_UNLOCKED),
> > > > + DRM_IOCTL_DEF_DRV(IPVR_GEM_MMAP,
> > > > + ipvr_gem_mmap_ioctl, DRM_UNLOCKED),
> > >
> > > Why do you need this ioctl? mmap() should work perfectly fine. I
> > > don't see why you require people to use a ipvr specific ioctl to map
> buffers.
> >
> > Many thanks to your comments, in our existing libdrm helper and
> > userspace drivers, mmap_ioctl was the interface of mapping objects. We
> > continued using the ioctl way for compatibility. Is it mandatory to
> > implement mmap() to replace mmap_ioctl for GEM drivers?
>
> Yeah, the new way is to have a ioctl in the drm driver to create an mmap
> offset (look at the gtt_mmap_offset ioctl for i915), and the use mmap on the
> drm fd. Doing a direct driver ioctl to forward the mmap is deprecated.
>
> The reason for that is that a) this is the new standard way used by all drm
> drivers b) if you use the standard mmap system call debug tools like valgrind
> will understand it without any special actions.
>
> I'll do a patch to i915 so that people stop copy-pasting that old misdesign.
> -Daniel
Accepted :) I will update the patch to implement the mmap interface and remove
the legacy MMAP_IOCTL.
BTW I didn't see a field to get mmap_offset in struct drm_gem_open, I guess
something like a new "GET_MMAP_OFFSET_IOCTL" need be added to support mapping
flinked/primed BO, is it?
>
> >
> > >
> > > > + DRM_IOCTL_DEF_DRV(IPVR_SYNC_CPU,
> > > > + ipvr_sync_cpu_ioctl, DRM_UNLOCKED),
> > > > + DRM_IOCTL_DEF_DRV(IPVR_GEM_WAIT,
> > > > + ipvr_gem_wait_ioctl, DRM_AUTH|DRM_UNLOCKED),
> > > > + DRM_IOCTL_DEF_DRV(IPVR_GEM_USERPTR,
> > > > + ipvr_gem_userptr_ioctl, DRM_UNLOCKED), };
> > > > +
> > > > +static void ipvr_gem_init(struct drm_device *dev) {
> > > > + struct drm_ipvr_private *dev_priv = dev->dev_private;
> > > > +
> > > > + dev_priv->ipvr_bo_slab =
> kmem_cache_create("ipvr_gem_object",
> > > > + sizeof(union drm_ipvr_gem_objects), 0,
> > > > + SLAB_HWCACHE_ALIGN, NULL);
> > > > +
> > > > + INIT_LIST_HEAD(_priv->ipvr_mm.unbound_list);
> > > > + INIT_LIST_HEAD(_priv->ipvr_mm.bound_list);
> > > > + spin_lock_init(_priv->ipvr_mm.object_stat_lock);
> > > > +
> > > > + dev_priv->ipvr_mm.interruptible = true; }
> > > > +
> > > > +static void ipvr_gem_setup_mmu(struct drm_device *dev,
> > > > + unsigned long linear_start,
> > > > + unsigned lo
[RFC PATCH 2/3] drm/ipvr: drm driver for vxd392
Hi Herrmann
> -Original Message-
> From: David Herrmann [mailto:dh.herrmann at gmail.com]
> Sent: Monday, October 13, 2014 10:27 PM
> To: Cheng, Yao
> Cc: Intel Graphics Development; Jiang, Fei; dri-devel at
> lists.freedesktop.org;
> Vetter, Daniel
> Subject: Re: [RFC PATCH 2/3] drm/ipvr: drm driver for vxd392
>
> Hi
>
> > +static struct drm_ioctl_desc ipvr_gem_ioctls[] = {
> > + DRM_IOCTL_DEF_DRV(IPVR_CONTEXT_CREATE,
> > + ipvr_context_create_ioctl, DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(IPVR_CONTEXT_DESTROY,
> > + ipvr_context_destroy_ioctl, DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(IPVR_MISC,
> > + ipvr_misc_ioctl, DRM_AUTH),
> > + DRM_IOCTL_DEF_DRV(IPVR_GEM_EXECBUFFER,
> > + ipvr_gem_execbuffer, DRM_AUTH|DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(IPVR_GEM_BUSY,
> > + ipvr_gem_busy_ioctl, DRM_AUTH|DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(IPVR_GEM_CREATE,
> > + ipvr_gem_create_ioctl, DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(IPVR_GEM_MMAP,
> > + ipvr_gem_mmap_ioctl, DRM_UNLOCKED),
>
> Why do you need this ioctl? mmap() should work perfectly fine. I don't see
> why you require people to use a ipvr specific ioctl to map buffers.
Many thanks to your comments, in our existing libdrm helper and userspace
drivers, mmap_ioctl was the interface of mapping objects. We continued using
the ioctl way for compatibility. Is it mandatory to implement mmap() to replace
mmap_ioctl for GEM drivers?
>
> > + DRM_IOCTL_DEF_DRV(IPVR_SYNC_CPU,
> > + ipvr_sync_cpu_ioctl, DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(IPVR_GEM_WAIT,
> > + ipvr_gem_wait_ioctl, DRM_AUTH|DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(IPVR_GEM_USERPTR,
> > + ipvr_gem_userptr_ioctl, DRM_UNLOCKED), };
> > +
> > +static void ipvr_gem_init(struct drm_device *dev) {
> > + struct drm_ipvr_private *dev_priv = dev->dev_private;
> > +
> > + dev_priv->ipvr_bo_slab = kmem_cache_create("ipvr_gem_object",
> > + sizeof(union drm_ipvr_gem_objects), 0,
> > + SLAB_HWCACHE_ALIGN, NULL);
> > +
> > + INIT_LIST_HEAD(_priv->ipvr_mm.unbound_list);
> > + INIT_LIST_HEAD(_priv->ipvr_mm.bound_list);
> > + spin_lock_init(_priv->ipvr_mm.object_stat_lock);
> > +
> > + dev_priv->ipvr_mm.interruptible = true; }
> > +
> > +static void ipvr_gem_setup_mmu(struct drm_device *dev,
> > + unsigned long linear_start,
> > + unsigned long linear_end,
> > + unsigned long tiling_start,
> > + unsigned long tiling_end) {
> > + /* Let GEM Manage all of the aperture.
> > +*
> > +* However, leave one page at the end still bound to the scratch
> > page.
> > +* There are a number of places where hardware apparently
> prefetches
> > +* past the end of the object, and we've seen multiple hangs with
> > the
> > +* GPU head pointer stuck in a batchbuffer bound at last page of the
> > +* aperture. One page should be enough to keep any prefetching
> inside
> > +* of the aperture.
> > +*/
> > + struct drm_ipvr_private *dev_priv = dev->dev_private;
> > + struct ipvr_address_space *addr_space = _priv->addr_space;
> > +
> > + /* todo: add sanity check */
> > + addr_space->dev = dev_priv->dev;
> > + INIT_LIST_HEAD(_space->active_list);
> > + INIT_LIST_HEAD(_space->inactive_list);
> > +
> > + /* Subtract the guard page ... */
> > + drm_mm_init(_space->linear_mm, linear_start,
> > + linear_end - linear_start - PAGE_SIZE);
> > + dev_priv->addr_space.linear_start = linear_start;
> > + dev_priv->addr_space.linear_total = linear_end - linear_start;
> > +
> > + drm_mm_init(_space->tiling_mm, tiling_start,
> > + tiling_end - tiling_start - PAGE_SIZE);
> > + dev_priv->addr_space.tiling_start = tiling_start;
> > + dev_priv->addr_space.tiling_total = tiling_end - tiling_start;
> > +}
> > +
> > +static void ipvr_do_takedown(struct drm_device *dev)
[Intel-gfx] [RFC PATCH 0/3] drm driver for baytrail's vxd392
Thx Kelley/Reding for comments, Yes, the vxd392 is a totally different IP and it also exist on Intel's non-GEN platforms such as Merrifield. If we put it inside i915 there'll be issue if we someday supports Merrifield. -Original Message- From: Sean V Kelley [mailto:sean.v.kel...@intel.com] Sent: Tuesday, October 14, 2014 11:51 PM To: Thierry Reding Cc: Cheng, Yao; Vetter, Daniel; intel-gfx at lists.freedesktop.org; Jiang, Fei; DRI mailing list Subject: Re: [Intel-gfx] [RFC PATCH 0/3] drm driver for baytrail's vxd392 On Tue, Oct 14, 2014 at 4:53 AM, Thierry Reding wrote: > On Mon, Oct 13, 2014 at 08:15:00PM +0800, Yao Cheng wrote: >> drm/ipvr is a new GEM driver for baytrail's vxd392, which accelerates VP8 >> video decoding. >> The driver name "ipvr" means the PowerVR's IP wrapped by Intel. In the >> future, ipvr may support other platforms such as Merrifield. >> Code is placed at drivers/gpu/drm/ipvr and the following two new Kconfig are >> added: >> CONFIG_DRM_IPVR: Build option for ipvr module >> CONFIG_DRM_IPVR_EC: Experimental feature of error concealment >> >> User mode drm helper "libdrm_ipvr.so" and simple test are also included. >> >> Yao Cheng (3): >> [1/3] drm/i915: add vxd392 bridge in i915 on baytrail [2/3] >> drm/ipvr: ipvr drm driver for vxd392 > > If this is Intel-specific, why doesn't it live under the i915 driver? It is an entirely unrelated HW IP block, VXD392. Nothing to do with GEN aside from DRM based. Sean > > Thierry > > ___ > Intel-gfx mailing list > Intel-gfx at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Sean V. Kelley Open Source Technology Center / SSG Intel Corp.
[Intel-gfx] [RFC PATCH 1/3] drm/i915: add vxd392 bridge in i915
Nikula, thx for reminder, will update it.
-Original Message-
From: Jani Nikula [mailto:jani.nik...@linux.intel.com]
Sent: Tuesday, October 14, 2014 9:27 PM
To: Cheng, Yao; intel-gfx at lists.freedesktop.org
Cc: Jiang, Fei; dri-devel at lists.freedesktop.org; Vetter, Daniel
Subject: Re: [Intel-gfx] [RFC PATCH 1/3] drm/i915: add vxd392 bridge in i915
On Mon, 13 Oct 2014, 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
> ---
> 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_INDEX0x21
>
> +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);
> + 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);
> +}
> +
> void i
