On Thu, Oct 16, 2014 at 03:05:07PM +0000, Cheng, Yao wrote: > > 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. Yeah, i915 driver core section sounds like the right place. Btw there's a small blog post from me with some tips for how to go about this: http://blog.ffwll.ch/2014/06/documentation-for-drmi915.html > > > + > > > +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. i-g-t is the i915 kernel driver regression test suite. For a quick intro see: http://blog.ffwll.ch/2013/08/recent-drmi915-testsuite-improvements.html The igt repo is at http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/ The README file in there also has some good information. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
