Re: [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface

[Daniel Vetter <daniel@ffwll.ch>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx