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

[Daniel Vetter]

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