At Wed, 07 Jan 2015 21:49:46 +0200,
Imre Deak wrote:
>
> On Tue, 2015-01-06 at 11:25 +0100, Takashi Iwai wrote:
> > At Mon, 05 Jan 2015 19:25:09 +0200,
> > Imre Deak wrote:
> > >
> > > On Mon, 2015-01-05 at 16:35 +0100, Takashi Iwai wrote:
> > > > At Mon, 05 Jan 2015 17:29:34 +0200,
> > > > Imre Deak wrote:
> > > > >
> > > > > Hi Mengdong, Takashi,
> > > > >
> > > > > On Tue, 2014-12-09 at 18:33 +0100, Takashi Iwai wrote:
> > > > > > At Tue, 09 Dec 2014 18:56:07 +0200,
> > > > > > Imre Deak wrote:
> > > > > > >
> > > > > > > On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
> > > > > > > > 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.
> > > > > > >
> > > > > > > Ok, I'm not sure either. If that's not possible, I agree EDEFER
> > > > > > > would be
> > > > > > > the best and with that we could also get rid of the
> > > > > > > request_module() we
> > > > > > > need now.
> > > > > >
> > > > > > The probe of HD-audio driver is done in a work anyway, so changing
> > > > > > the
> > > > > > code to sync with component should be feasible.
> > > > > >
> > > > > > I'm off today (and yesterday was a Christmas party :), so had little
> > > > > > time for review so far. Will check the series tomorrow.
> > > > >
> > > > > could one of you help reviewing this patchset? Note that there is a v2
> > > > > version with some individual patches having a v3 already.
> > > >
> > > > Is there any git branch containing the latest patches that is easily
> > > > applicable to 3.19-rc (or linux-next)?
> > >
> > > I applied them on 3.19-rc2 and pushed the branch to:
> > > https://github.com/ideak/linux/commits/audio-component
> >
> > Thanks! Just reading through it (but couldn't build/test it as today
> > is again a holiday here :) The patches looks good but a few
> > nitpicking:
> >
> > - audio_component shouldn't be added into struct azx;
> > struct azx is a common object used by Tegra HD-audio, too, so keep
> > it clean. There is struct hda_intel defined in hda_intel.c, which
> > inherits struct azx.
> >
> > That is, we need to cut struct hda_intel definition out to a header
> > file (e.g. hda_intel.h) and include it in hda_i915.c. I think we
> > can merge the contents of hda_i915.h into hda_intel.h, too.
>
> Ok, I did the above and also changed the hda_i915 interface to accept
> hda_intel instead of chip as that makes now more sense.
>
> > - The removal of include/drm/i915_powerwell.h should be folded into
> > the last patch, so that we can separate the changes in sound/* and
> > drm/i915/* in each patch.
>
> Oops, that would've also caused a bisect error. I moved the removal to
> the last patch.
>
> I tested this on HSW, seems to work ok. I updated the github branch, if
> you don't see more issues, I will post the new version to the list.
Thanks. I tried pulling your branch and did a quick test. It looks
working well.
After reading the commits again, the following minor issues came to my
mind:
- request_module("i915") may return an error if i915 is built-in.
We can simply ignore the return value there since the binding
condition is checked at the next sooner or later.
- pr_xxx() can be replaced with dev_xxx() as we pass the hda object to
each function.
Other than that, all patches look good. Feel free to take my
reviewed-by tag.
Reviewed-by: Takashi Iwai <ti...@suse.de>
thanks,
Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
