Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
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
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
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
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Tue, Mar 15, 2016 at 04:14:19PM -0500, Pierre-Louis Bossart wrote: > On 3/15/16 11:21 AM, Vinod Koul wrote: > >On Tue, Mar 15, 2016 at 03:35:45PM +0200, Ville Syrjälä wrote: > I understand the benefits of a parent/child device/subdevice model. What I > don't see is whether we need the component framework at all here? > It was used in the case of HDaudio since both i915 and HDaudio controllers > get probed at different times, here the HDMI interface is just a bunch of > registers/DMA from the same hardware so the whole master/client interface > exposed by the component framework to 'bind' independent drivers is a bit > overkill? > >>> > >>>I haven't read the patches, but using component.c when you instead can > >>>model it with parent/child smells like abuse. Component.c is meant for > >>>when you have multiple devices all over the device tree that in aggregate > >>>constitute the overall logical driver. This doesn't seem to be the case > >>>here. > > > >Right I do think that might be the case. > > > >>We still need the eld notify hooks so that i915 can inform the audio > >>driver about the state of the display. Whether that's best done via > >>the component stuff or something else I don't know. > > > >There is already work ongoing by ARM folks for the interface, should we > >reuse that [1]. I would certainly argue reusing rather than redfeining a > >new one would be better. > > > >Btw this interface seems to rely on display side implemting these ops. > > My understanding is that it's an interface for external encoders > that have an I2S or S/PDIF link. Such external encoders appear as > ASoC codecs that can be bound to the SoC with DT bindings. I don't > see what we could reuse here? That is one of the intended usages. Right now three folks are developing on that, TI which seems to be encoder case, then sti (don't know if that off chip or not) and mediatek. The point here is that we can use the same interface here too if we are not going i915 way. Possibly we might want to modify or add to this and not make it ASoC dependent as this driver won't be an ASoC one. But yes I haven't looked closely enough to say that if this should be the way or not. I wanted to pint our an alternate interface being developed which can be possible reused in non i915 case Thanks -- ~Vinod ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On 3/15/16 11:21 AM, Vinod Koul wrote: On Tue, Mar 15, 2016 at 03:35:45PM +0200, Ville Syrjälä wrote: I understand the benefits of a parent/child device/subdevice model. What I don't see is whether we need the component framework at all here? It was used in the case of HDaudio since both i915 and HDaudio controllers get probed at different times, here the HDMI interface is just a bunch of registers/DMA from the same hardware so the whole master/client interface exposed by the component framework to 'bind' independent drivers is a bit overkill? I haven't read the patches, but using component.c when you instead can model it with parent/child smells like abuse. Component.c is meant for when you have multiple devices all over the device tree that in aggregate constitute the overall logical driver. This doesn't seem to be the case here. Right I do think that might be the case. We still need the eld notify hooks so that i915 can inform the audio driver about the state of the display. Whether that's best done via the component stuff or something else I don't know. There is already work ongoing by ARM folks for the interface, should we reuse that [1]. I would certainly argue reusing rather than redfeining a new one would be better. Btw this interface seems to rely on display side implemting these ops. My understanding is that it's an interface for external encoders that have an I2S or S/PDIF link. Such external encoders appear as ASoC codecs that can be bound to the SoC with DT bindings. I don't see what we could reuse here? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Tue, Mar 15, 2016 at 03:35:45PM +0200, Ville Syrjälä wrote: > > > I understand the benefits of a parent/child device/subdevice model. What I > > > don't see is whether we need the component framework at all here? > > > It was used in the case of HDaudio since both i915 and HDaudio controllers > > > get probed at different times, here the HDMI interface is just a bunch of > > > registers/DMA from the same hardware so the whole master/client interface > > > exposed by the component framework to 'bind' independent drivers is a bit > > > overkill? > > > > I haven't read the patches, but using component.c when you instead can > > model it with parent/child smells like abuse. Component.c is meant for > > when you have multiple devices all over the device tree that in aggregate > > constitute the overall logical driver. This doesn't seem to be the case > > here. Right I do think that might be the case. > We still need the eld notify hooks so that i915 can inform the audio > driver about the state of the display. Whether that's best done via > the component stuff or something else I don't know. There is already work ongoing by ARM folks for the interface, should we reuse that [1]. I would certainly argue reusing rather than redfeining a new one would be better. Btw this interface seems to rely on display side implemting these ops. Thanks -- ~Vinod [1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105526.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Tue, Mar 15, 2016 at 03:35:45PM +0200, Ville Syrjälä wrote: > On Tue, Mar 15, 2016 at 09:39:48AM +0100, Daniel Vetter wrote: > > On Mon, Mar 14, 2016 at 12:27:51PM -0500, Pierre-Louis Bossart wrote: > > > On 3/14/16 10:30 AM, Ville Syrjälä wrote: > > > >On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: > > > >>On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > > > >>>On 3/11/16 1:09 PM, 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. > > > >>> > > > >>>we don't want to expose this interface when HDAudio is present and > > > >>>enabled so we would need to add a test for this. > > > >>>Also it looks like you want the creation of the platform device done in > > > >>>i915, we were thinking of doing this as part of the audio drivers but > > > >>>in > > > >>>the end this looks equivalent. In both cases we would end-up ignoring > > > >>>the HAD022A8 HID and not use acpi for this extension > > > >> > > > >>Well, if you have a device you can hang off from then i915 should need > > > >>to register it I suppose. Though that would make the interrupt > > > >>forwarding perhaps less nice. There's also the suspend/resume order > > > >>dependency to deal with if there's no parent/child relationship between > > > >>the devices. > > > > > > > >Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar > > > >to get at the registers, which would look a bit funkly at least. > > > > > > I understand the benefits of a parent/child device/subdevice model. What I > > > don't see is whether we need the component framework at all here? > > > It was used in the case of HDaudio since both i915 and HDaudio controllers > > > get probed at different times, here the HDMI interface is just a bunch of > > > registers/DMA from the same hardware so the whole master/client interface > > > exposed by the component framework to 'bind' independent drivers is a bit > > > overkill? > > > > I haven't read the patches, but using component.c when you instead can > > model it with parent/child smells like abuse. Component.c is meant for > > when you have multiple devices all over the device tree that in aggregate > > constitute the overall logical driver. This doesn't seem to be the case > > here. > > We still need the eld notify hooks so that i915 can inform the audio > driver about the state of the display. Whether that's best done via > the component stuff or something else I don't know. Hm right. I guess we could stuff it into the platform data stuff maybe instead, so the driver could get at the i915/snd interface directly? -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
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Tue, Mar 15, 2016 at 09:39:48AM +0100, Daniel Vetter wrote: > On Mon, Mar 14, 2016 at 12:27:51PM -0500, Pierre-Louis Bossart wrote: > > On 3/14/16 10:30 AM, Ville Syrjälä wrote: > > >On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: > > >>On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > > >>>On 3/11/16 1:09 PM, 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. > > >>> > > >>>we don't want to expose this interface when HDAudio is present and > > >>>enabled so we would need to add a test for this. > > >>>Also it looks like you want the creation of the platform device done in > > >>>i915, we were thinking of doing this as part of the audio drivers but in > > >>>the end this looks equivalent. In both cases we would end-up ignoring > > >>>the HAD022A8 HID and not use acpi for this extension > > >> > > >>Well, if you have a device you can hang off from then i915 should need > > >>to register it I suppose. Though that would make the interrupt > > >>forwarding perhaps less nice. There's also the suspend/resume order > > >>dependency to deal with if there's no parent/child relationship between > > >>the devices. > > > > > >Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar > > >to get at the registers, which would look a bit funkly at least. > > > > I understand the benefits of a parent/child device/subdevice model. What I > > don't see is whether we need the component framework at all here? > > It was used in the case of HDaudio since both i915 and HDaudio controllers > > get probed at different times, here the HDMI interface is just a bunch of > > registers/DMA from the same hardware so the whole master/client interface > > exposed by the component framework to 'bind' independent drivers is a bit > > overkill? > > I haven't read the patches, but using component.c when you instead can > model it with parent/child smells like abuse. Component.c is meant for > when you have multiple devices all over the device tree that in aggregate > constitute the overall logical driver. This doesn't seem to be the case > here. We still need the eld notify hooks so that i915 can inform the audio driver about the state of the display. Whether that's best done via the component stuff or something else I don't know. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
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. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Mon, Mar 14, 2016 at 12:27:51PM -0500, Pierre-Louis Bossart wrote: > On 3/14/16 10:30 AM, Ville Syrjälä wrote: > >On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: > >>On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > >>>On 3/11/16 1:09 PM, 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. > >>> > >>>we don't want to expose this interface when HDAudio is present and > >>>enabled so we would need to add a test for this. > >>>Also it looks like you want the creation of the platform device done in > >>>i915, we were thinking of doing this as part of the audio drivers but in > >>>the end this looks equivalent. In both cases we would end-up ignoring > >>>the HAD022A8 HID and not use acpi for this extension > >> > >>Well, if you have a device you can hang off from then i915 should need > >>to register it I suppose. Though that would make the interrupt > >>forwarding perhaps less nice. There's also the suspend/resume order > >>dependency to deal with if there's no parent/child relationship between > >>the devices. > > > >Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar > >to get at the registers, which would look a bit funkly at least. > > I understand the benefits of a parent/child device/subdevice model. What I > don't see is whether we need the component framework at all here? > It was used in the case of HDaudio since both i915 and HDaudio controllers > get probed at different times, here the HDMI interface is just a bunch of > registers/DMA from the same hardware so the whole master/client interface > exposed by the component framework to 'bind' independent drivers is a bit > overkill? I haven't read the patches, but using component.c when you instead can model it with parent/child smells like abuse. Component.c is meant for when you have multiple devices all over the device tree that in aggregate constitute the overall logical driver. This doesn't seem to be the case here. -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
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
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. > > Another bit we didn't fully do for VED is abstracting away the dma mapping > stuff, because x86 dma abstraction sucks (compared to arm). Not sure, but > this might have been fixed meanwhile - if we can set up a dma_ops that the > subdevice would use, we should do so (instead of the page_to_pfn hacks VED > used). This one might be a bit a problem - on byt we got away with pfn_to_page because no iommu at all, but that's not a good idea really. Definitely need to reevaluate this again. Iirc there's been some talk of just walking up a chain of platform devices until the core x86 dma code finds something with dma support, then use that. -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
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
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. -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
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On 3/14/16 10:30 AM, Ville Syrjälä wrote: On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: On 3/11/16 1:09 PM, 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. we don't want to expose this interface when HDAudio is present and enabled so we would need to add a test for this. Also it looks like you want the creation of the platform device done in i915, we were thinking of doing this as part of the audio drivers but in the end this looks equivalent. In both cases we would end-up ignoring the HAD022A8 HID and not use acpi for this extension Well, if you have a device you can hang off from then i915 should need to register it I suppose. Though that would make the interrupt forwarding perhaps less nice. There's also the suspend/resume order dependency to deal with if there's no parent/child relationship between the devices. Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar to get at the registers, which would look a bit funkly at least. I understand the benefits of a parent/child device/subdevice model. What I don't see is whether we need the component framework at all here? It was used in the case of HDaudio since both i915 and HDaudio controllers get probed at different times, here the HDMI interface is just a bunch of registers/DMA from the same hardware so the whole master/client interface exposed by the component framework to 'bind' independent drivers is a bit overkill? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: > On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > > On 3/11/16 1:09 PM, 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. > > > > we don't want to expose this interface when HDAudio is present and > > enabled so we would need to add a test for this. > > Also it looks like you want the creation of the platform device done in > > i915, we were thinking of doing this as part of the audio drivers but in > > the end this looks equivalent. In both cases we would end-up ignoring > > the HAD022A8 HID and not use acpi for this extension > > Well, if you have a device you can hang off from then i915 should need > to register it I suppose. Though that would make the interrupt > forwarding perhaps less nice. There's also the suspend/resume order > dependency to deal with if there's no parent/child relationship between > the devices. Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar to get at the registers, which would look a bit funkly at least. > > > > > > 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. > > > > > -- > Ville Syrjälä > Intel OTC > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > On 3/11/16 1:09 PM, 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. > > we don't want to expose this interface when HDAudio is present and > enabled so we would need to add a test for this. > Also it looks like you want the creation of the platform device done in > i915, we were thinking of doing this as part of the audio drivers but in > the end this looks equivalent. In both cases we would end-up ignoring > the HAD022A8 HID and not use acpi for this extension Well, if you have a device you can hang off from then i915 should need to register it I suppose. Though that would make the interrupt forwarding perhaps less nice. There's also the suspend/resume order dependency to deal with if there's no parent/child relationship between the devices. > > > 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. > > -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On 3/11/16 1:09 PM, 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. we don't want to expose this interface when HDAudio is present and enabled so we would need to add a test for this. Also it looks like you want the creation of the platform device done in i915, we were thinking of doing this as part of the audio drivers but in the end this looks equivalent. In both cases we would end-up ignoring the HAD022A8 HID and not use acpi for this extension 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. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
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. 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. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
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. Another bit we didn't fully do for VED is abstracting away the dma mapping stuff, because x86 dma abstraction sucks (compared to arm). Not sure, but this might have been fixed meanwhile - if we can set up a dma_ops that the subdevice would use, we should do so (instead of the page_to_pfn hacks VED used). -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
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
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. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
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? If yes, would there be two component framework links, one between i915/audio driver and one between subdevice/audio driver. I am way beyond my comfort zone, bear with me if this is silly. Thanks. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Fri, Mar 04, 2016 at 08:50:41PM -0600, Pierre-Louis Bossart wrote: > Add header files for interface available on Baytrail and CherryTrail > > Initial code was downloaded from https://github.com/01org/baytrailaudio/ > ...and had the changes to .config stripped and the revert on sound/init.c > done by David Henningson > > Clean-up, port to 4.4 and intel-drm by Pierre Bossart > > Signed-off-by: David Henningsson> Signed-off-by: Pierre-Louis Bossart > --- > drivers/gpu/drm/i915/hdmi_audio_if.h | 122 > +++ > drivers/gpu/drm/i915/i915_drv.h | 31 + > drivers/gpu/drm/i915/i915_reg.h | 7 ++ > drivers/gpu/drm/i915/intel_drv.h | 11 > 4 files changed, 171 insertions(+) > create mode 100644 drivers/gpu/drm/i915/hdmi_audio_if.h > > diff --git a/drivers/gpu/drm/i915/hdmi_audio_if.h > b/drivers/gpu/drm/i915/hdmi_audio_if.h > new file mode 100644 > index 000..f968028 > --- /dev/null > +++ b/drivers/gpu/drm/i915/hdmi_audio_if.h > @@ -0,0 +1,122 @@ > +/* > + * Copyright (c) 2010, Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * Authors: > + * jim liu > + * Uma Shankar > + */ > + > + > +#ifndef __HDMI_AUDIO_IF_H > +#define __HDMI_AUDIO_IF_H > + > +#include > +#include > + > +/* HDMI AUDIO INTERRUPT TYPE */ > +#define HDMI_AUDIO_UNDERRUN (1UL<<0) > +#define HDMI_AUDIO_BUFFER_DONE (1UL<<1) > + > +/* the monitor type HDMI or DVI */ > +#define MONITOR_TYPE_HDMI 1 > +#define MONITOR_TYPE_DVI 2 > + > +extern int i915_hdmi_state; > +extern int i915_notify_had; > + > +enum had_caps_list { > + HAD_GET_ELD = 1, > + HAD_GET_SAMPLING_FREQ, > + HAD_GET_DISPLAY_RATE, > + HAD_GET_HDCP_STATUS, > + HAD_GET_AUDIO_STATUS, > + HAD_SET_ENABLE_AUDIO, > + HAD_SET_DISABLE_AUDIO, > + HAD_SET_ENABLE_AUDIO_INT, > + HAD_SET_DISABLE_AUDIO_INT, > + OTHERS_TBD, > +}; > + > +enum had_event_type { > + HAD_EVENT_HOT_PLUG = 1, > + HAD_EVENT_HOT_UNPLUG, > + HAD_EVENT_MODE_CHANGING, > + HAD_EVENT_PM_CHANGING, > + HAD_EVENT_AUDIO_BUFFER_DONE, > + HAD_EVENT_AUDIO_BUFFER_UNDERRUN, > + HAD_EVENT_QUERY_IS_AUDIO_BUSY, > + HAD_EVENT_QUERY_IS_AUDIO_SUSPENDED, Kinda hard to see where everything gets used due to the way the patches are split up. 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. > +}; > + > +/* > + * HDMI Display Controller Audio Interface > + * > + */ > +typedef int (*had_event_call_back) (enum had_event_type event_type, > + void *ctxt_info); > + > +struct hdmi_audio_registers_ops { > + int (*hdmi_audio_read_register)(uint32_t reg_addr, uint32_t *data); > + int (*hdmi_audio_write_register)(uint32_t reg_addr, uint32_t data); > + int (*hdmi_audio_read_modify)(uint32_t reg_addr, uint32_t data, > + uint32_t mask); > +}; > + > +struct hdmi_audio_query_set_ops { > + int (*hdmi_audio_get_caps)(enum had_caps_list query_element, > + void *capabilties); > + int (*hdmi_audio_set_caps)(enum had_caps_list set_element, > + void *capabilties); > +}; > + > +typedef struct hdmi_audio_event { > + int type; > +} hdmi_audio_event_t; > + > +struct snd_intel_had_interface { > + const char *name; > + int (*query)(void *had_data, hdmi_audio_event_t event); > + int (*suspend)(void *had_data, hdmi_audio_event_t event); > + int (*resume)(void *had_data); > +}; > + > +struct hdmi_audio_priv { > + struct drm_device *dev; > + u32 hdmib_reg; > + > + bool is_hdcp_supported; > + bool hdmi_hpd_connected; > + int monitor_type; > + void *context; > +}; > + > +extern void i915_hdmi_audio_init(struct hdmi_audio_priv *p_hdmi_priv); > + > +extern bool mid_hdmi_audio_is_busy(struct drm_device *dev); > +extern bool mid_hdmi_audio_suspend(struct drm_device *dev); > +extern void mid_hdmi_audio_resume(struct drm_device *dev); > +extern void mid_hdmi_audio_signal_event(struct
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Fri, Mar 04, 2016 at 08:50:41PM -0600, Pierre-Louis Bossart wrote: > Add header files for interface available on Baytrail and CherryTrail > > Initial code was downloaded from https://github.com/01org/baytrailaudio/ > ...and had the changes to .config stripped and the revert on sound/init.c > done by David Henningson > > Clean-up, port to 4.4 and intel-drm by Pierre Bossart > > Signed-off-by: David Henningsson> Signed-off-by: Pierre-Louis Bossart > --- > drivers/gpu/drm/i915/hdmi_audio_if.h | 122 > +++ > drivers/gpu/drm/i915/i915_drv.h | 31 + > drivers/gpu/drm/i915/i915_reg.h | 7 ++ > drivers/gpu/drm/i915/intel_drv.h | 11 > 4 files changed, 171 insertions(+) > create mode 100644 drivers/gpu/drm/i915/hdmi_audio_if.h > > diff --git a/drivers/gpu/drm/i915/hdmi_audio_if.h > b/drivers/gpu/drm/i915/hdmi_audio_if.h > new file mode 100644 > index 000..f968028 > --- /dev/null > +++ b/drivers/gpu/drm/i915/hdmi_audio_if.h > @@ -0,0 +1,122 @@ > +/* > + * Copyright (c) 2010, Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * Authors: > + * jim liu > + * Uma Shankar > + */ > + > + > +#ifndef __HDMI_AUDIO_IF_H > +#define __HDMI_AUDIO_IF_H > + > +#include > +#include > + > +/* HDMI AUDIO INTERRUPT TYPE */ > +#define HDMI_AUDIO_UNDERRUN (1UL<<0) > +#define HDMI_AUDIO_BUFFER_DONE (1UL<<1) > + > +/* the monitor type HDMI or DVI */ > +#define MONITOR_TYPE_HDMI 1 > +#define MONITOR_TYPE_DVI 2 > + > +extern int i915_hdmi_state; > +extern int i915_notify_had; > + > +enum had_caps_list { > + HAD_GET_ELD = 1, > + HAD_GET_SAMPLING_FREQ, > + HAD_GET_DISPLAY_RATE, > + HAD_GET_HDCP_STATUS, > + HAD_GET_AUDIO_STATUS, > + HAD_SET_ENABLE_AUDIO, > + HAD_SET_DISABLE_AUDIO, > + HAD_SET_ENABLE_AUDIO_INT, > + HAD_SET_DISABLE_AUDIO_INT, > + OTHERS_TBD, > +}; > + > +enum had_event_type { > + HAD_EVENT_HOT_PLUG = 1, > + HAD_EVENT_HOT_UNPLUG, > + HAD_EVENT_MODE_CHANGING, > + HAD_EVENT_PM_CHANGING, > + HAD_EVENT_AUDIO_BUFFER_DONE, > + HAD_EVENT_AUDIO_BUFFER_UNDERRUN, > + HAD_EVENT_QUERY_IS_AUDIO_BUSY, > + HAD_EVENT_QUERY_IS_AUDIO_SUSPENDED, > +}; > + > +/* > + * HDMI Display Controller Audio Interface > + * > + */ > +typedef int (*had_event_call_back) (enum had_event_type event_type, > + void *ctxt_info); > + > +struct hdmi_audio_registers_ops { > + int (*hdmi_audio_read_register)(uint32_t reg_addr, uint32_t *data); > + int (*hdmi_audio_write_register)(uint32_t reg_addr, uint32_t data); > + int (*hdmi_audio_read_modify)(uint32_t reg_addr, uint32_t data, > + uint32_t mask); > +}; Do all the register actually live within the device 2 mmio bar? If so I think what we might want to do here is go for a proper subdevice approach like we tried to do for the VED on BYT. VED sort of dried up and no one cared (not sure the userspae part was even published), but I think the approach was sane. See here: https://lists.freedesktop.org/archives/intel-gfx/2015-January/058137.html > + > +struct hdmi_audio_query_set_ops { > + int (*hdmi_audio_get_caps)(enum had_caps_list query_element, > + void *capabilties); > + int (*hdmi_audio_set_caps)(enum had_caps_list set_element, > + void *capabilties); > +}; > + > +typedef struct hdmi_audio_event { > + int type; > +} hdmi_audio_event_t; > + > +struct snd_intel_had_interface { > + const char *name; > + int (*query)(void *had_data, hdmi_audio_event_t event); > + int (*suspend)(void *had_data, hdmi_audio_event_t event); > + int (*resume)(void *had_data); > +}; > + > +struct hdmi_audio_priv { > + struct drm_device *dev; > + u32 hdmib_reg; > + > + bool is_hdcp_supported; > + bool hdmi_hpd_connected; > + int monitor_type; > + void *context; > +}; > + > +extern void i915_hdmi_audio_init(struct hdmi_audio_priv *p_hdmi_priv); > + > +extern bool mid_hdmi_audio_is_busy(struct drm_device *dev); > +extern bool mid_hdmi_audio_suspend(struct drm_device *dev); > +extern void mid_hdmi_audio_resume(struct drm_device *dev); > +extern void mid_hdmi_audio_signal_event(struct drm_device *dev, > + enum had_event_type event); > + > +/* Added for HDMI Audio */ > +extern void hdmi_get_eld(uint8_t *eld); > +extern int i915_enable_hdmi_audio_int(struct drm_device *dev); >
[Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
Add header files for interface available on Baytrail and CherryTrail Initial code was downloaded from https://github.com/01org/baytrailaudio/ ...and had the changes to .config stripped and the revert on sound/init.c done by David Henningson Clean-up, port to 4.4 and intel-drm by Pierre Bossart Signed-off-by: David HenningssonSigned-off-by: Pierre-Louis Bossart --- drivers/gpu/drm/i915/hdmi_audio_if.h | 122 +++ drivers/gpu/drm/i915/i915_drv.h | 31 + drivers/gpu/drm/i915/i915_reg.h | 7 ++ drivers/gpu/drm/i915/intel_drv.h | 11 4 files changed, 171 insertions(+) create mode 100644 drivers/gpu/drm/i915/hdmi_audio_if.h diff --git a/drivers/gpu/drm/i915/hdmi_audio_if.h b/drivers/gpu/drm/i915/hdmi_audio_if.h new file mode 100644 index 000..f968028 --- /dev/null +++ b/drivers/gpu/drm/i915/hdmi_audio_if.h @@ -0,0 +1,122 @@ +/* + * Copyright (c) 2010, Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * Authors: + * jim liu + * Uma Shankar + */ + + +#ifndef __HDMI_AUDIO_IF_H +#define __HDMI_AUDIO_IF_H + +#include +#include + +/* HDMI AUDIO INTERRUPT TYPE */ +#define HDMI_AUDIO_UNDERRUN (1UL<<0) +#define HDMI_AUDIO_BUFFER_DONE (1UL<<1) + +/* the monitor type HDMI or DVI */ +#define MONITOR_TYPE_HDMI 1 +#define MONITOR_TYPE_DVI 2 + +extern int i915_hdmi_state; +extern int i915_notify_had; + +enum had_caps_list { + HAD_GET_ELD = 1, + HAD_GET_SAMPLING_FREQ, + HAD_GET_DISPLAY_RATE, + HAD_GET_HDCP_STATUS, + HAD_GET_AUDIO_STATUS, + HAD_SET_ENABLE_AUDIO, + HAD_SET_DISABLE_AUDIO, + HAD_SET_ENABLE_AUDIO_INT, + HAD_SET_DISABLE_AUDIO_INT, + OTHERS_TBD, +}; + +enum had_event_type { + HAD_EVENT_HOT_PLUG = 1, + HAD_EVENT_HOT_UNPLUG, + HAD_EVENT_MODE_CHANGING, + HAD_EVENT_PM_CHANGING, + HAD_EVENT_AUDIO_BUFFER_DONE, + HAD_EVENT_AUDIO_BUFFER_UNDERRUN, + HAD_EVENT_QUERY_IS_AUDIO_BUSY, + HAD_EVENT_QUERY_IS_AUDIO_SUSPENDED, +}; + +/* + * HDMI Display Controller Audio Interface + * + */ +typedef int (*had_event_call_back) (enum had_event_type event_type, + void *ctxt_info); + +struct hdmi_audio_registers_ops { + int (*hdmi_audio_read_register)(uint32_t reg_addr, uint32_t *data); + int (*hdmi_audio_write_register)(uint32_t reg_addr, uint32_t data); + int (*hdmi_audio_read_modify)(uint32_t reg_addr, uint32_t data, + uint32_t mask); +}; + +struct hdmi_audio_query_set_ops { + int (*hdmi_audio_get_caps)(enum had_caps_list query_element, + void *capabilties); + int (*hdmi_audio_set_caps)(enum had_caps_list set_element, + void *capabilties); +}; + +typedef struct hdmi_audio_event { + int type; +} hdmi_audio_event_t; + +struct snd_intel_had_interface { + const char *name; + int (*query)(void *had_data, hdmi_audio_event_t event); + int (*suspend)(void *had_data, hdmi_audio_event_t event); + int (*resume)(void *had_data); +}; + +struct hdmi_audio_priv { + struct drm_device *dev; + u32 hdmib_reg; + + bool is_hdcp_supported; + bool hdmi_hpd_connected; + int monitor_type; + void *context; +}; + +extern void i915_hdmi_audio_init(struct hdmi_audio_priv *p_hdmi_priv); + +extern bool mid_hdmi_audio_is_busy(struct drm_device *dev); +extern bool mid_hdmi_audio_suspend(struct drm_device *dev); +extern void mid_hdmi_audio_resume(struct drm_device *dev); +extern void mid_hdmi_audio_signal_event(struct drm_device *dev, + enum had_event_type event); + +/* Added for HDMI Audio */ +extern void hdmi_get_eld(uint8_t *eld); +extern int i915_enable_hdmi_audio_int(struct drm_device *dev); +extern int i915_disable_hdmi_audio_int(struct drm_device *dev); +extern int mid_hdmi_audio_setup( + had_event_call_back audio_callbacks, + struct hdmi_audio_registers_ops *reg_ops, + struct hdmi_audio_query_set_ops *query_ops); +extern int mid_hdmi_audio_register( + struct snd_intel_had_interface *driver, + void *had_data); + +#endif /* __HDMI_AUDIO_IF_H */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2cb0a41..5dceb5b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -53,6 +53,7 @@ #include #include #include "intel_guc.h" +#include