From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [RFC PATCH 1/3] drm/i915: add vxd392 bridge in i915 Date: Sun, 19 Oct 2014 13:27:11 +0200 Message-ID: <20141019112711.GL26941@phenom.ffwll.local> References: <1413202502-19026-1-git-send-email-yao.cheng@intel.com> <1413202502-19026-2-git-send-email-yao.cheng@intel.com> <20141016101508.GJ2655@bremse.labri.fr> <8FF7D634BEE4C2428EFFAB6B7E919E4B017BCFE7@shsmsx102.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <8FF7D634BEE4C2428EFFAB6B7E919E4B017BCFE7@shsmsx102.ccr.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Cheng, Yao" Cc: "dri-devel@lists.freedesktop.org" , "Vetter, Daniel" , "intel-gfx@lists.freedesktop.org" , "Jiang, Fei" List-Id: dri-devel@lists.freedesktop.org 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