Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > The current hda/i915 interface to enable/disable power wells and query
> > > the CD clock rate is based on looking up the relevant i915 module
> > > symbols from the hda driver. By using the component framework we can get
> > > rid of some global state tracking in the i915 driver and pave the way to
> > > fully decouple the two drivers: once support is added to enable/disable
> > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > itself from the i915 component master, without the need to keep a
> > > reference on the i915 module.
> > > 
> > > This also gets rid of the problem that currently the i915 driver exposes
> > > the interface only on HSW and BDW, while it's also needed at least on
> > > VLV/CHV.
> > 
> > Awesome that you're tackling this, really happy to see these hacks go.
> > Unfortunately I think it's upside down: hda should be the component master
> > and i915 should only register a component.
> > 
> > Longer story: The main reason for the component helpers is to be able to
> > magically delay registering the main/master driver until all the
> > components are loaded. Otherwise -EDEFER doesn't work and also the
> > suspend/resume ordering this should result in. Master here means whatever
> > userspace eventually sees as a device node or similar, component is
> > anything really that this userspace interfaces needs to function
> > correctly.
> 
> EDEFER doesn't solve the suspend/resume ordering, at least not in the
> general async s/r case. In any case I don't see a problem in making the
> hda component the master and I think it's more logical that way as you
> said, so I changed that.

Yeah for full async s/r we're screwed as-is. But see below for some crazy
ideas.

> > I think what we need here is then:
> > - Put the current azx_probe into the master_bind callback. The new
> >   azx_probe will do nothing else than register the master component and
> >   the component match list. To do so it checks the chip flag and if it
> >   needs to cooperate with i915 it registers one component match for that.
> >   The master_bind (old probe) function calls component_bind_all with the
> >   hda_intel pointer as void *data parameter.
> 
> I'm not sure this is the best way since by this the i915 module would
> need to be pinned even when no HDMI functionality is used. I think a
> better way would be to let the probe function run as now and
> init/register all the non-HDMI functionality. Then only the HDMI
> functionality would be inited/registered from the bind/unbind hooks.

Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
to be able to load the driver. But if we can defer just the hdmi part.

> But we could go either way even as a follow-up to this patchset.
> 
> > - i915 registers a component for the i915 gfx device. It uses the
> >   component device to get at i915 sturctures and fills the dev+ops into
> >   the hda_intel pointer it gets as void *data.
> >
> > Stuff we then should do on top:
> > - Add deferred probing to azx_probe: Only when all components are there
> >   should it actually register. This will take care of all the module load
> >   order mess.
> 
> I agree that we should only register user interfaces when everything is
> in place. But I'm not sure deferred probe is the best, we could do
> without it by registering HDMI from the component bind hook.

It's mostly a question whether alsa does support delayed addition of
interface parts. DRM most definitely does not (except for recently added
dp mst connector hotplug). But I guess if the current driver already
delays registering the hdmi part then we're fine. I'm not sure about
whether it's really safe - I spotted not a lot of locking really to make
sure there's no races between i915 loading and userspace trying to access
the hdmi side.

> >   It should also take care of system suspend/resume ordering and we
> >   should be able to delete all the early_resume/late_suspend trickery.
> 
> Deferred probe doesn't solve the suspend/resume ordering, we would need
> to have a separate HDMI device that is set as a child to the i915
> device. Alternatively we could use device_pm_wait_for_dev().

I'm not sure whether there's the possibility of deadlocks with
device_pm_wait_for_dev. But if that works I think a component helper to
wait for all components to resume would be really useful. Or we implement
the full-blown pm_ops idea laid out below for components.
> 
> > Imo we should have things ready up to this point to make sure this
> > refactoring actually works.
> 
> I think we could go with this minimal patch with your change to have the
> hda component be master. This only adds the support for components and
> keeps everything else the same. We could consider the bigger changes as
> a follow-up.

Yeah that was my plan - if EDEFER isn't enough then we keep the early/late
resume/suspend hooks for a bit longer.

> > Then there's some cool stuff we could do on top:
> > - Register a i915-hda platform devices as a child of the gfx pci device.
> >   Besides shuffling around a bit with the interfaces/argument casting and
> >   the component match function this doesn't really have a functional
> >   impact. But it makes the relationship more clear since hda doesn't
> >   really need the entire pci device, but only the small part that does
> >   audio.
> > 
> > - Replace the hand-rolled power-well interface with runtime pm on that
> >   device node.
> > 
> > - If system suspend/resume doesn't work automatically with deferred
> >   probing (tbh I'm not sure) add pm_ops to the component master. Then add
> >   some functions as default implementations for pm_ops for components
> >   which simply refcount all component pm_ops calls and call the master
> >   pm_ops suspend on the first suspend call and resume on the last resume
> >   call. That really should take care of suspend/resume ordering for good.
> 
> Yep, these sound good. I think having an HDMI child device is the
> cleanest solution for the s/r ordering issue.

Ok, sounds like we have a plan. And thanks again for tackling this, I'm
really happy to see this go away.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx