On Wed, Mar 16, 2016 at 06:43:38PM +0200, Ville Syrjälä wrote: > On Tue, Mar 15, 2016 at 03:30:55PM +0200, Ville Syrjälä wrote: > > On Tue, Mar 15, 2016 at 09:35:26AM +0100, Daniel Vetter wrote: > > > On Mon, Mar 14, 2016 at 04:20:19PM +0200, Ville Syrjälä wrote: > > > > On Mon, Mar 14, 2016 at 10:04:00AM +0100, Daniel Vetter wrote: > > > > > On Fri, Mar 11, 2016 at 09:09:12PM +0200, Ville Syrjälä wrote: > > > > > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > > > > > > Thanks for the review Ville > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > Kinda hard to see where everything gets used due to the way the patches > > > > > > > > are split up. > > > > > > > > > > > > > > Yes, it's far from great... > > > > > > > > > > > > > > > At least the hotplug/mode change events are not needed. We only have the > > > > > > > > two points where i915 should inform the audio driver about this stuff, > > > > > > > > and those are the intel_audio_code_enable/disable(). For that we > > > > > > > > already have the .pin_eld_notify() hook. > > > > > > > > > > > > > > > > The interrupt stuff should mostly vanish from i915 with the subdevice > > > > > > > > approach. As in i915 would just call the interrupt handler of the audio > > > > > > > > driver based on the LPE bits in IIR, and the audio driver can then do > > > > > > > > whatever it wants based on its own status register. > > > > > > > > > > > > > > Are you saying that the subdevice would provide a read/write interface > > > > > > > for the audio driver to look at display registers, and the i915 driver > > > > > > > would only provide a notification interface (EDID and interrupts) to the > > > > > > > audio driver? > > > > > > > > > > > > The audio driver would simply ioremap the appropriate range of > > > > > > registers itself. > > > > > > > > > > > > > If yes, would there be two component framework links, one between > > > > > > > i915/audio driver and one between subdevice/audio driver. > > > > > > > > > > > > Yeah sort of. i915 registers the platform device for the audio, the > > > > > > audio driver can then bind to the device via the platform driver .probe > > > > > > callback. It can then register with the audio component stuff at some > > > > > > point to get the relevant notifications on the display state. When > > > > > > i915 gets unloaded we remove the platform device, at which point the > > > > > > audio driver's platform driver .remove() callback gets invoked and > > > > > > it should unregister/cleanup everything. > > > > > > > > > > > > I just tried to frob around with the VED code a bit, and got it to load > > > > > > at least. It's not quite happy about reloading i915 while the ipvr > > > > > > driver was loaded though. Not sure what's going on there, but I do > > > > > > think this approach should work. So the VED patch could serve as a > > > > > > decent enough model to follow. > > > > > > > > > > platform devices registerd by modules are apparently inherently racy and > > > > > in an unfixable way. At least I remember something like that from VED > > > > > discussion. > > > > > > > > > > In short you _must_ unload VED manually before unloading i915, or it all > > > > > goes boom. If this is the only thing that went boom it's acceptable. > > > > > > > > VED goes boom due drm_global_mutex deadlock at least if you load > > > > i915 while ipvr is already loaded. I didn't check to hard if there > > > > were any booms on pure unload so far. > > > > > > Oh right another boom that happened, but can be avoided by dropping the > > > ->load callback and directly calling drm_dev_alloc/register. Shouldn't be > > > a problem with a non-drm driver though. > > > > > > > Anyways, I was a bit worried about the races so I did a pair of > > > > small test modules to try out this model, and to me it looked to > > > > work so far. I just unregistered the platform device from the parent > > > > pci driver .remove() hook, and it got blocked until the platform > > > > driver's .remove() hook was done. > > > > > > > > In any case if this is broken, then I assume mfd must be broken. And > > > > that thing is at least used quite extensively. > > > > > > Hm, I don't remember the exact details, but iirc the problem was that the > > > struct device is refcounted, but you can't add a module ref for the vtable > > > is has (specifically the final release method) since that would result in > > > a ref-loop. Usually it works, but if someone keeps the device alive > > > (through sysfs or whatever) and you manage to unload everything before > > > that last ref gets dropped it goes boom. > > > > > > So "works", but not in a safe way. > > > > I was worried that it's something like that. I guess I'll need to try > > grab a ref on something in my test module and see how it fares. > > At least a sysfs attribute on the child device didn't cause any > explosions in my tests. I slept for a while in the sysfs store function, > and while doing that removed the module for the parent device. The > platform_device_unregister() in the parent .remove() blocked until the > sysfs store was done. Hm, maybe it's been fixed by now, that discussion about platform devices created by modules was a while ago. -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
