On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote: > On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote: > > On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote: > > > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote: > > > > The current hda/i915 interface to enable/disable power wells and query > > > > the CD clock rate is based on looking up the relevant i915 module > > > > symbols from the hda driver. By using the component framework we can get > > > > rid of some global state tracking in the i915 driver and pave the way to > > > > fully decouple the two drivers: once support is added to enable/disable > > > > the HDMI functionality dynamically in the hda driver, it can bind/unbind > > > > itself from the i915 component master, without the need to keep a > > > > reference on the i915 module. > > > > > > > > This also gets rid of the problem that currently the i915 driver exposes > > > > the interface only on HSW and BDW, while it's also needed at least on > > > > VLV/CHV. > > > > > > Awesome that you're tackling this, really happy to see these hacks go. > > > Unfortunately I think it's upside down: hda should be the component master > > > and i915 should only register a component. > > > > > > Longer story: The main reason for the component helpers is to be able to > > > magically delay registering the main/master driver until all the > > > components are loaded. Otherwise -EDEFER doesn't work and also the > > > suspend/resume ordering this should result in. Master here means whatever > > > userspace eventually sees as a device node or similar, component is > > > anything really that this userspace interfaces needs to function > > > correctly. > > > > EDEFER doesn't solve the suspend/resume ordering, at least not in the > > general async s/r case. In any case I don't see a problem in making the > > hda component the master and I think it's more logical that way as you > > said, so I changed that. > > Yeah for full async s/r we're screwed as-is. But see below for some crazy > ideas. > > > I think what we need here is then: > > > - Put the current azx_probe into the master_bind callback. The new > > > azx_probe will do nothing else than register the master component and > > > the component match list. To do so it checks the chip flag and if it > > > needs to cooperate with i915 it registers one component match for that. > > > The master_bind (old probe) function calls component_bind_all with the > > > hda_intel pointer as void *data parameter. > > > > I'm not sure this is the best way since by this the i915 module would > > need to be pinned even when no HDMI functionality is used. I think a > > better way would be to let the probe function run as now and > > init/register all the non-HDMI functionality. Then only the HDMI > > functionality would be inited/registered from the bind/unbind hooks. > > Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway > to be able to load the driver. But if we can defer just the hdmi part. > > > But we could go either way even as a follow-up to this patchset. > > > > > - i915 registers a component for the i915 gfx device. It uses the > > > component device to get at i915 sturctures and fills the dev+ops into > > > the hda_intel pointer it gets as void *data. > > > > > > Stuff we then should do on top: > > > - Add deferred probing to azx_probe: Only when all components are there > > > should it actually register. This will take care of all the module load > > > order mess. > > > > I agree that we should only register user interfaces when everything is > > in place. But I'm not sure deferred probe is the best, we could do > > without it by registering HDMI from the component bind hook. > > It's mostly a question whether alsa does support delayed addition of > interface parts. DRM most definitely does not (except for recently added > dp mst connector hotplug). But I guess if the current driver already > delays registering the hdmi part then we're fine. I'm not sure about > whether it's really safe - I spotted not a lot of locking really to make > sure there's no races between i915 loading and userspace trying to access > the hdmi side. Ok, I'm not sure either. If that's not possible, I agree EDEFER would be the best and with that we could also get rid of the request_module() we need now. > > > It should also take care of system suspend/resume ordering and we > > > should be able to delete all the early_resume/late_suspend trickery. > > > > Deferred probe doesn't solve the suspend/resume ordering, we would need > > to have a separate HDMI device that is set as a child to the i915 > > device. Alternatively we could use device_pm_wait_for_dev(). > > I'm not sure whether there's the possibility of deadlocks with > device_pm_wait_for_dev. But if that works I think a component helper to > wait for all components to resume would be really useful. Or we implement > the full-blown pm_ops idea laid out below for components. Yes it could deadlock with pm_async=0 (a debug thing nowadays?) and depending on the bus address of the waiter and wait-for device. At least for us this can't happen (with the fixed PCI addresses for the gfx and audio devices), but I don't know if you can consider this a clean solution. Perhaps by adding a check for this in device_pm_wait_for_dev() and avoiding the deadlock by returning some error would be safe enough. > > > > > Imo we should have things ready up to this point to make sure this > > > refactoring actually works. > > > > I think we could go with this minimal patch with your change to have the > > hda component be master. This only adds the support for components and > > keeps everything else the same. We could consider the bigger changes as > > a follow-up. > > Yeah that was my plan - if EDEFER isn't enough then we keep the early/late > resume/suspend hooks for a bit longer. > > > > Then there's some cool stuff we could do on top: > > > - Register a i915-hda platform devices as a child of the gfx pci device. > > > Besides shuffling around a bit with the interfaces/argument casting and > > > the component match function this doesn't really have a functional > > > impact. But it makes the relationship more clear since hda doesn't > > > really need the entire pci device, but only the small part that does > > > audio. > > > > > > - Replace the hand-rolled power-well interface with runtime pm on that > > > device node. > > > > > > - If system suspend/resume doesn't work automatically with deferred > > > probing (tbh I'm not sure) add pm_ops to the component master. Then add > > > some functions as default implementations for pm_ops for components > > > which simply refcount all component pm_ops calls and call the master > > > pm_ops suspend on the first suspend call and resume on the last resume > > > call. That really should take care of suspend/resume ordering for good. > > > > Yep, these sound good. I think having an HDMI child device is the > > cleanest solution for the s/r ordering issue. > > Ok, sounds like we have a plan. And thanks again for tackling this, I'm > really happy to see this go away. > > Cheers, Daniel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
