2013/7/24 Daniel Vetter <daniel@xxxxxxxx>: > On Tue, Jul 23, 2013 at 07:33:40PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> >> Hi >> >> This patch series is based on emails I sent a few days ago, with subject >> "proposals/questions about the IRQ registers". The biggest goal of the series is >> to have an unified way to initialize/reset IRQ regsiters. After this series, >> when you add new interrupts to our driver you won't need to think about doing >> the correct reads, writes and posting_reads, all you'll need to worry about is >> to call the macros everyone else calls. So now either all the registers are >> initialized correctly or all the registers are initialized wrongly. >> >> The first 10 patches do all the rework on i915+. The changes should be >> incremental and any non-trivial changes are on separate patches, so bisecting >> regressions should be very easy. It is worth mentioning that patches 2, 6, 9 and >> 10 touch VLV-specific code, and I don't have a way to test this code. >> >> On my intial patches the i8xx code was changed along with the i915+ code, but >> Ben suggested I shouldn't be touching i8xx code, so patches 11-15 do the i8xx >> work. I don't know if we really want these patches, but they're here anyway, so >> feel free to give them a NACK, I won't complain. In case we decide to merge the >> patches, git bisect should easily spot regressions. >> >> Patches tested on Haswell (constant use for 2 weeks), SandyBridge (a few hours >> of usage) and Atom (only booted and then suspend/resumed once). > > My first question was why you didn't use name##IMR, name##IER, name##IIR > in the macros, since that would match what we have in -internal and imo > would look more natural. But then I've noticed that for IIR, IER & IMR we > don't have a prefix ... I still think the I in the name looks a bit funny > though (e.g. GTI). Maybe we should add an I915_ prefix then we could > remove the I. Yeah, you guessed the reason. If we use name##IMR then we could replace "INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);" with "INTEL_IRQ_REG_INIT(, false, enable_mask, dev_priv->irq_mask);", but I guess your suggestion of renaming is probably better. The problem with renaming to I915_IIR is that the registers also exists on i8xx, so I'd vote for another name (GEN2_IIR?). > > The other thing I'm unclear about is the do_iir flag, which in the end > seems to clear IIR bits in uninstall but not in preinstall. Iirc Ben's > proposal was to do the double IIR clear in preinstall, and I tend to agree > with him. Can you please elaborate on this? Please look at the final result of the series, we remove the flag on later patches (you just made me notice I forgot to remove it on the reg_init functions, but we don't really use it anymore). At the end of the series we do the double-IIR clear at both preinstall and uninstall. I wanted to keep one functional change at each patch. > > Maybe one patch on top to add some kerneldoc to the new magic macros would > be good so that we can explain once what the thinking between the sequence > is. Will do. Thanks Daniel and Chris for the fast reviews! > > Cheers, Daniel >> > >> Thanks, >> Paulo >> >> Paulo Zanoni (15): >> drm/i915: add INTEL_IRQ_REG_RESET >> drm/i915: change how VLV_IIR is reset >> drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET >> drm/i915: really clear the IIR registers >> drm/i915: add INTEL_IRQ_REG_INIT >> drm/i915: use INTEL_IRQ_REG_INIT on VLV too >> drm/i915: reset the IIR registers at preinstall >> drm/i915: WARN if IIR is not zero at irq_postinstall >> drm/i915: remove additional zerogin of VLV_IIR at postinstall >> drm/i915: remove extra clearing of GTIIR from VLV irq preinstall >> drm/i915: add INTEL_IRQ_REG_RESET16 >> drm/i915: really clear the IIR registers on i8xx >> drm/i915: add INTEL_IRQ_REG_INIT16 >> drm/i915: reset the i8xx IIR registers at preinstall >> drm/i915: WARN if IIR is not zero at i8xx irq_postinstall >> >> drivers/gpu/drm/i915/i915_irq.c | 144 +++++++++++++++++----------------------- >> 1 file changed, 62 insertions(+), 82 deletions(-) >> >> -- >> 1.8.1.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
