Re: [RFC PATCH 1/3] drm/i915: add vxd392 bridge in i915

[Daniel Vetter <daniel@ffwll.ch>]

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