On Mon, May 02, 2016 at 11:58:38AM +0100, Chris Wilson wrote: > On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote: > > Imo the low-level irq clearing should all be done in the relevant irq > > setup code in i915_irq.c. Atm we just forgot to do that. I guess you can > > have a bikeshed whether the enginer IMR enable/disable functions should be > > together with the clearing or not, placing them in either file is fine. > > But since we already clear the higher-level IMR registers in i915_irq.c we > > might as well clear the low-level ones too in i915_irq.c. > > That's tricky since that is done before the engines - so how do we get > the various bases to i915_irq.c without duplication? Enabling the irq > for the engines is part of init_hw, so correspondingly putting the > early disable into init looks fine to me. I just don't particularly like that we have hw access in a function that thus far (and at a glance still after this patch) only sets up software state. I'll probably lead to some ugly surprise. That's why I'd like to move this either to engine->init_hw or to i915_irq.c. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
