[alsa-devel] HDMI codec, way forward?
After reading the ELCE Audio mini conf minutes [1] I gather that HDMI audio was not discussed after all.
My conclusion from the Lars-Peter's latest mail [2] related to the subject is that the wind is currently blowing slightly in favour of my version of the hdmi codec [3], or at least not in favour of Arnaud's version [4].
So how should we proceed? Are we still waiting for some feedback from DRM/video side?
There was not too many clear suggestions to my latest patch series [3], so I do not see too much point in sending yet another version of that series.
Arnaud, if I'd try to make a patch series that would implement sti HDMI audio with my HDMI codec, would you be willing try that out?
Best regards, Jyri
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/098847.htm... [2] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/098564.htm... [3] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-September/097667.h... [4] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/098275.htm...
On 10/16/2015 01:50 PM, Jyri Sarha wrote:
After reading the ELCE Audio mini conf minutes [1] I gather that HDMI audio was not discussed after all.
It was discussed, but rather shortly and only in the context of the HDA. (Adding Vinod to Cc, who presented yet another approach to the problem last week)
My conclusion from the Lars-Peter's latest mail [2] related to the subject is that the wind is currently blowing slightly in favour of my version of the hdmi codec [3], or at least not in favour of Arnaud's version [4].
So how should we proceed? Are we still waiting for some feedback from DRM/video side?
What needs to happen is that everybody who is working on HDMI audio support should get their heads together and come up with a common solution that works for everybody, rather than having everybody trying to push their own solution and put the maintainer in the spotlight of having to choose one (Mark has been asking for this for the past half year or so).
- Lars
On Fri, 16 Oct 2015 14:31:43 +0200, Lars-Peter Clausen wrote:
On 10/16/2015 01:50 PM, Jyri Sarha wrote:
After reading the ELCE Audio mini conf minutes [1] I gather that HDMI audio was not discussed after all.
It was discussed, but rather shortly and only in the context of the HDA. (Adding Vinod to Cc, who presented yet another approach to the problem last week)
Or maybe Mengdong? In anyway, HD-audio/i915 has already some hooks in component to communicate between graphics and audio stacks. The direct ELD passing is still missing but easy to implement on its top (there was a patchset in the past).
Note that these are suppose to be a stop gap until the final solid common infrastructure is built up.
Takashi
On Fri, Oct 16, 2015 at 02:31:43PM +0200, Lars-Peter Clausen wrote:
On 10/16/2015 01:50 PM, Jyri Sarha wrote:
After reading the ELCE Audio mini conf minutes [1] I gather that HDMI audio was not discussed after all.
It was discussed, but rather shortly and only in the context of the HDA. (Adding Vinod to Cc, who presented yet another approach to the problem last week)
My conclusion from the Lars-Peter's latest mail [2] related to the subject is that the wind is currently blowing slightly in favour of my version of the hdmi codec [3], or at least not in favour of Arnaud's version [4].
So how should we proceed? Are we still waiting for some feedback from DRM/video side?
What needs to happen is that everybody who is working on HDMI audio support should get their heads together and come up with a common solution that works for everybody, rather than having everybody trying to push their own solution and put the maintainer in the spotlight of having to choose one (Mark has been asking for this for the past half year or so).
Sorry, but how about people try to come up with _a solution_ first. There's been loads of talk about this, lots of drivers trying to do integrated or driver specific approaches, but only one or two attempts at doing something driver independent.
If all the effort that's been put into discussion was put into sorting out a solution, maybe we'd be a little further forward than we are now.
I've sent one proposal which uses a notifier to inform audio and CEC drivers about state changes in the HDMI video side, and had precisely zero replies to it - people seemed to prefer discussing stuff rather than reviewing code and coming up with actual solutions.
On Fri, Oct 16, 2015 at 02:37:17PM +0100, Russell King - ARM Linux wrote:
If all the effort that's been put into discussion was put into sorting out a solution, maybe we'd be a little further forward than we are now.
I've sent one proposal which uses a notifier to inform audio and CEC drivers about state changes in the HDMI video side, and had precisely zero replies to it - people seemed to prefer discussing stuff rather than reviewing code and coming up with actual solutions.
I agree entirely with the above. FWIW I'm not reviewing code so much because I don't feel I have enough HDMI-specific knowledge to make sure things are general enough and the lack of discussion makes me unsure that it's really helping - I don't want to give the impression that it's a case of addressing some code level comments I might have when I'm not sure we're even travelling in the right direction.
On Fri, Oct 16, 2015 at 02:37:17PM +0100, Russell King - ARM Linux wrote:
I've sent one proposal which uses a notifier to inform audio and CEC drivers about state changes in the HDMI video side, and had precisely zero replies to it - people seemed to prefer discussing stuff rather than reviewing code and coming up with actual solutions.
Do you mind sending me a pointer to this series, I would like to read this up
On Sun, Oct 18, 2015 at 08:38:34PM +0530, Vinod Koul wrote:
On Fri, Oct 16, 2015 at 02:37:17PM +0100, Russell King - ARM Linux wrote:
I've sent one proposal which uses a notifier to inform audio and CEC drivers about state changes in the HDMI video side, and had precisely zero replies to it - people seemed to prefer discussing stuff rather than reviewing code and coming up with actual solutions.
Do you mind sending me a pointer to this series, I would like to read this up
It isn't a series. It's a prototype patch that I posted in one of the previous threads earlier this month:
Date: Tue, 6 Oct 2015 19:51:57 +0100 Subject: Re: [alsa-devel] [PATCH RFC V2 0/5] another generic audio hdmi codec proposal Message-ID: 20151006185157.GT21513@n2100.arm.linux.org.uk
As I've said there, audio is not the only issue here, CEC also needs to have access to much the same information that audio needs.
CEC needs to know two things: when the HDMI sink is disconnected, and when the HDMI sink's EDID is available (specifically, CEC needs to know the HDMI physical address for itself, which is passed as a number via the EDID HDMI vendor block.)
On Sun, Oct 18, 2015 at 04:20:48PM +0100, Russell King - ARM Linux wrote:
On Sun, Oct 18, 2015 at 08:38:34PM +0530, Vinod Koul wrote:
On Fri, Oct 16, 2015 at 02:37:17PM +0100, Russell King - ARM Linux wrote:
I've sent one proposal which uses a notifier to inform audio and CEC drivers about state changes in the HDMI video side, and had precisely zero replies to it - people seemed to prefer discussing stuff rather than reviewing code and coming up with actual solutions.
Do you mind sending me a pointer to this series, I would like to read this up
It isn't a series. It's a prototype patch that I posted in one of the previous threads earlier this month:
Date: Tue, 6 Oct 2015 19:51:57 +0100 Subject: Re: [alsa-devel] [PATCH RFC V2 0/5] another generic audio hdmi codec proposal Message-ID: 20151006185157.GT21513@n2100.arm.linux.org.uk
As I've said there, audio is not the only issue here, CEC also needs to have access to much the same information that audio needs.
CEC needs to know two things: when the HDMI sink is disconnected, and when the HDMI sink's EDID is available (specifically, CEC needs to know the HDMI physical address for itself, which is passed as a number via the EDID HDMI vendor block.)
Right but can I ask why you didn't try making video as component and then CEC, audio and others receive the notification over this. I don't know if there are any limitations to this since you wrote component bits you are the best person to comment...
On Sun, Oct 18, 2015 at 09:43:29PM +0530, Vinod Koul wrote:
On Sun, Oct 18, 2015 at 04:20:48PM +0100, Russell King - ARM Linux wrote:
On Sun, Oct 18, 2015 at 08:38:34PM +0530, Vinod Koul wrote:
On Fri, Oct 16, 2015 at 02:37:17PM +0100, Russell King - ARM Linux wrote:
I've sent one proposal which uses a notifier to inform audio and CEC drivers about state changes in the HDMI video side, and had precisely zero replies to it - people seemed to prefer discussing stuff rather than reviewing code and coming up with actual solutions.
Do you mind sending me a pointer to this series, I would like to read this up
It isn't a series. It's a prototype patch that I posted in one of the previous threads earlier this month:
Date: Tue, 6 Oct 2015 19:51:57 +0100 Subject: Re: [alsa-devel] [PATCH RFC V2 0/5] another generic audio hdmi codec proposal Message-ID: 20151006185157.GT21513@n2100.arm.linux.org.uk
As I've said there, audio is not the only issue here, CEC also needs to have access to much the same information that audio needs.
CEC needs to know two things: when the HDMI sink is disconnected, and when the HDMI sink's EDID is available (specifically, CEC needs to know the HDMI physical address for itself, which is passed as a number via the EDID HDMI vendor block.)
Right but can I ask why you didn't try making video as component and then CEC, audio and others receive the notification over this. I don't know if there are any limitations to this since you wrote component bits you are the best person to comment...
I don't understand what you're suggesting; I've not been able to look at anything you've suggested (as you've referred to it as URLs) and I'm up to my eyeballs with horribly crappy crypto drivers on two different SoCs.
On Sun, Oct 18, 2015 at 09:43:29PM +0530, Vinod Koul wrote:
Right but can I ask why you didn't try making video as component and then CEC, audio and others receive the notification over this.
Okay, I think I see what you're getting at. No, I don't want to tie this stuff into the component helpers because that's the wrong approach.
The component helper is purely about combining several struct devices into one larger component for a subsystem which deals with card-level components. It's not about cross-subsystem stuff.
The problem with using it for cross-subsystem stuff is that it becomes too tightly bound together: why should the graphics side get torn down if you unload the audio or CEC driver?
Audio and CEC are rather optional for HDMI - HDMI can work without audio and CEC being present. However, audio can't be conveyed across the link without the video side being configured. So, it makes sense to allow the CEC and audio parts to be loaded separately (possibly as modules) while having the video parts built-in to the kernel - especially if you want to use the HDMI output as the console.
Binding CEC and audio into the component helper alongside the video part will mean that nothing will come up until all the components are present, and everything will be torn down when any one of those components are removed. Clearly, that's undesirable.
On Sun, 18 Oct 2015 19:16:42 +0200, Russell King - ARM Linux wrote:
On Sun, Oct 18, 2015 at 09:43:29PM +0530, Vinod Koul wrote:
Right but can I ask why you didn't try making video as component and then CEC, audio and others receive the notification over this.
Okay, I think I see what you're getting at. No, I don't want to tie this stuff into the component helpers because that's the wrong approach.
The component helper is purely about combining several struct devices into one larger component for a subsystem which deals with card-level components. It's not about cross-subsystem stuff.
The problem with using it for cross-subsystem stuff is that it becomes too tightly bound together: why should the graphics side get torn down if you unload the audio or CEC driver?
Audio and CEC are rather optional for HDMI - HDMI can work without audio and CEC being present. However, audio can't be conveyed across the link without the video side being configured. So, it makes sense to allow the CEC and audio parts to be loaded separately (possibly as modules) while having the video parts built-in to the kernel - especially if you want to use the HDMI output as the console.
Binding CEC and audio into the component helper alongside the video part will mean that nothing will come up until all the components are present, and everything will be torn down when any one of those components are removed. Clearly, that's undesirable.
Currently i915/audio component works as you described. The audio is optional and HDMI graphics works without audio, while HDMI HD-audio mandates i915 graphics.
Takashi
On Mon, Oct 19, 2015 at 03:20:30PM +0200, Takashi Iwai wrote:
On Sun, 18 Oct 2015 19:16:42 +0200, Russell King - ARM Linux wrote:
On Sun, Oct 18, 2015 at 09:43:29PM +0530, Vinod Koul wrote:
Right but can I ask why you didn't try making video as component and then CEC, audio and others receive the notification over this.
Okay, I think I see what you're getting at. No, I don't want to tie this stuff into the component helpers because that's the wrong approach.
The component helper is purely about combining several struct devices into one larger component for a subsystem which deals with card-level components. It's not about cross-subsystem stuff.
The problem with using it for cross-subsystem stuff is that it becomes too tightly bound together: why should the graphics side get torn down if you unload the audio or CEC driver?
Audio and CEC are rather optional for HDMI - HDMI can work without audio and CEC being present. However, audio can't be conveyed across the link without the video side being configured. So, it makes sense to allow the CEC and audio parts to be loaded separately (possibly as modules) while having the video parts built-in to the kernel - especially if you want to use the HDMI output as the console.
Binding CEC and audio into the component helper alongside the video part will mean that nothing will come up until all the components are present, and everything will be torn down when any one of those components are removed. Clearly, that's undesirable.
Currently i915/audio component works as you described. The audio is optional and HDMI graphics works without audio, while HDMI HD-audio mandates i915 graphics.
Right, but we also add additional interface on top of this to allow things like ensuring display is on when audio wants to run and now notification for events.
I don't see a reason why this can be used as single interface to bind as well as talk to display from various components, unless I missed obvious which prevent us from doing this in non i915 cases...
On Tue, Oct 20, 2015 at 09:08:09AM +0530, Vinod Koul wrote:
On Mon, Oct 19, 2015 at 03:20:30PM +0200, Takashi Iwai wrote:
On Sun, 18 Oct 2015 19:16:42 +0200, Russell King - ARM Linux wrote:
On Sun, Oct 18, 2015 at 09:43:29PM +0530, Vinod Koul wrote:
Right but can I ask why you didn't try making video as component and then CEC, audio and others receive the notification over this.
Okay, I think I see what you're getting at. No, I don't want to tie this stuff into the component helpers because that's the wrong approach.
The component helper is purely about combining several struct devices into one larger component for a subsystem which deals with card-level components. It's not about cross-subsystem stuff.
The problem with using it for cross-subsystem stuff is that it becomes too tightly bound together: why should the graphics side get torn down if you unload the audio or CEC driver?
Audio and CEC are rather optional for HDMI - HDMI can work without audio and CEC being present. However, audio can't be conveyed across the link without the video side being configured. So, it makes sense to allow the CEC and audio parts to be loaded separately (possibly as modules) while having the video parts built-in to the kernel - especially if you want to use the HDMI output as the console.
Binding CEC and audio into the component helper alongside the video part will mean that nothing will come up until all the components are present, and everything will be torn down when any one of those components are removed. Clearly, that's undesirable.
Currently i915/audio component works as you described. The audio is optional and HDMI graphics works without audio, while HDMI HD-audio mandates i915 graphics.
Right, but we also add additional interface on top of this to allow things like ensuring display is on when audio wants to run and now notification for events.
I don't see a reason why this can be used as single interface to bind as well as talk to display from various components, unless I missed obvious which prevent us from doing this in non i915 cases...
Maybe I can comment more specifically if I saw the code. Right now all I'm aware of is this idea without any code, and I don't like it.
On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
Currently i915/audio component works as you described. The audio is optional and HDMI graphics works without audio, while HDMI HD-audio mandates i915 graphics.
Right, but we also add additional interface on top of this to allow things like ensuring display is on when audio wants to run and now notification for events.
I don't see a reason why this can be used as single interface to bind as well as talk to display from various components, unless I missed obvious which prevent us from doing this in non i915 cases...
Maybe I can comment more specifically if I saw the code. Right now all I'm aware of is this idea without any code, and I don't like it.
Ok, i will be post my patches tomorrow. FWIW uses interface in sound/hda/hdac_i915.c for display power up/down
On Tue, 20 Oct 2015, Vinod Koul vinod.koul@intel.com wrote:
On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
Currently i915/audio component works as you described. The audio is optional and HDMI graphics works without audio, while HDMI HD-audio mandates i915 graphics.
Right, but we also add additional interface on top of this to allow things like ensuring display is on when audio wants to run and now notification for events.
I don't see a reason why this can be used as single interface to bind as well as talk to display from various components, unless I missed obvious which prevent us from doing this in non i915 cases...
Maybe I can comment more specifically if I saw the code. Right now all I'm aware of is this idea without any code, and I don't like it.
Ok, i will be post my patches tomorrow. FWIW uses interface in sound/hda/hdac_i915.c for display power up/down
Side note, some of the Intel HD audio controller registers are in a power domain controlled by i915. The audio drivers needs to do power get/put, otherwise the audio driver loses its marbles when i915 switches off power. OTOH audio needs to be a good citizen so i915 can reach low power states.
Just a shout from the back, unsure if this is relevant at this point...
BR, Jani.
On Wed, Oct 21, 2015 at 12:11:08PM +0300, Jani Nikula wrote:
On Tue, 20 Oct 2015, Vinod Koul vinod.koul@intel.com wrote:
On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
Currently i915/audio component works as you described. The audio is optional and HDMI graphics works without audio, while HDMI HD-audio mandates i915 graphics.
Right, but we also add additional interface on top of this to allow things like ensuring display is on when audio wants to run and now notification for events.
I don't see a reason why this can be used as single interface to bind as well as talk to display from various components, unless I missed obvious which prevent us from doing this in non i915 cases...
Maybe I can comment more specifically if I saw the code. Right now all I'm aware of is this idea without any code, and I don't like it.
Ok, i will be post my patches tomorrow. FWIW uses interface in sound/hda/hdac_i915.c for display power up/down
Side note, some of the Intel HD audio controller registers are in a power domain controlled by i915. The audio drivers needs to do power get/put, otherwise the audio driver loses its marbles when i915 switches off power. OTOH audio needs to be a good citizen so i915 can reach low power states.
Just a shout from the back, unsure if this is relevant at this point...
We are putting the get/put calls in driver's suspend and resume handler, that way we would suspend when not in use and ensure display suspends too...
On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote:
On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
Currently i915/audio component works as you described. The audio is optional and HDMI graphics works without audio, while HDMI HD-audio mandates i915 graphics.
Right, but we also add additional interface on top of this to allow things like ensuring display is on when audio wants to run and now notification for events.
I don't see a reason why this can be used as single interface to bind as well as talk to display from various components, unless I missed obvious which prevent us from doing this in non i915 cases...
Maybe I can comment more specifically if I saw the code. Right now all I'm aware of is this idea without any code, and I don't like it.
Ok, i will be post my patches tomorrow. FWIW uses interface in sound/hda/hdac_i915.c for display power up/down
This:
component_match_add(dev, &match, hdac_component_master_match, bus); ret = component_master_add_with_match(dev, &hdac_component_master_ops, match); if (ret < 0) goto out_err;
/* * Atm, we don't support deferring the component binding, so make sure * i915 is loaded and that the binding successfully completes. */ request_module("i915");
if (!acomp->ops) { ret = -ENODEV; goto out_master_del; }
is really not nice.
The whole point of the component helper is that you don't care when the slaves come along. The above code does care about that, because it expects that the master and slaves will be bound either inside component_master_add_with_match(), or via that request_module (which may happen asynchronously) and ends up setting ->ops.
This won't work for systems in the presence of deferred probing, since there's only a weak connection between loading a module and the driver successfully probing its device.
If you arrange for the audio device to finish probing in the master's bind callback, that would be _far_ better, and would be in line with how the component helper is supposed to work. It also means that if your slave (the video side) goes away, you'll know about it as the unbind callback will be called (at which point you should tear down the audio device.)
However, there's a lurking problem: you can't register the same struct device as a slave more than once into the component helpers - that's because it's designed to look for devices. There's intentionally no linkage between the slave ops and master ops to allow for generic slave drivers (like tda998x) to be used with different master drivers (armada, tilcdc, etc). In theory, you can register the master device of one componentised system as a slave device of another, but that requires a much more complicated locking setup in component.c (I have patches for that, but I've resisted sending them because it makes the locking very messy.)
On Wed, 21 Oct 2015 11:27:44 +0200, Russell King - ARM Linux wrote:
On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote:
On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
Currently i915/audio component works as you described. The audio is optional and HDMI graphics works without audio, while HDMI HD-audio mandates i915 graphics.
Right, but we also add additional interface on top of this to allow things like ensuring display is on when audio wants to run and now notification for events.
I don't see a reason why this can be used as single interface to bind as well as talk to display from various components, unless I missed obvious which prevent us from doing this in non i915 cases...
Maybe I can comment more specifically if I saw the code. Right now all I'm aware of is this idea without any code, and I don't like it.
Ok, i will be post my patches tomorrow. FWIW uses interface in sound/hda/hdac_i915.c for display power up/down
This:
component_match_add(dev, &match, hdac_component_master_match, bus); ret = component_master_add_with_match(dev, &hdac_component_master_ops, match); if (ret < 0) goto out_err; /* * Atm, we don't support deferring the component binding, so make sure * i915 is loaded and that the binding successfully completes. */ request_module("i915"); if (!acomp->ops) { ret = -ENODEV; goto out_master_del; }is really not nice.
Yeah, admittedly it's a really ugly workaround. It's coded in that way just to make our lives easier: it works well in most cases for i915 / hd-audio.
Actually, it's easy to modify the hda code in the right way, i.e. to defer via component bind ops. However, one drawback is that it's not trivial to handle the fallback case; namely, HD-audio might not need i915 binding, depending on the chip model and the configuration.
Braswell and Skylake have a (virtually) single audio controller as default, and this manages both the analog and HDMI/DP codecs. The i915 interaction is required only for the latter codecs, and thus for the former, it's optional. If we defer binding, the instantiation won't happen unless it's bound with i915. So, if i915 isn't initialized (e.g. by booting with nomodeset option), the whole audio will be gone, too. OTOH, Haswell and Braswell have a dedicated HDA controller for HDMI/DP, and they must be disabled (or fail) unless bound with graphics.
It's a corner case, so we might better ignore this. But it'd be certainly a "regression" -- at least, I used to test this in that way sometimes in the past. So it remains in the current way as is.
It's one of the reasons I mentioned it being a stop gap. But, I think the concept, passing ops via component, is actually working, and should be applicable to other drm/audio drivers. That's the point.
thanks,
Takashi
The whole point of the component helper is that you don't care when the slaves come along. The above code does care about that, because it expects that the master and slaves will be bound either inside component_master_add_with_match(), or via that request_module (which may happen asynchronously) and ends up setting ->ops.
This won't work for systems in the presence of deferred probing, since there's only a weak connection between loading a module and the driver successfully probing its device.
If you arrange for the audio device to finish probing in the master's bind callback, that would be _far_ better, and would be in line with how the component helper is supposed to work. It also means that if your slave (the video side) goes away, you'll know about it as the unbind callback will be called (at which point you should tear down the audio device.)
However, there's a lurking problem: you can't register the same struct device as a slave more than once into the component helpers - that's because it's designed to look for devices. There's intentionally no linkage between the slave ops and master ops to allow for generic slave drivers (like tda998x) to be used with different master drivers (armada, tilcdc, etc). In theory, you can register the master device of one componentised system as a slave device of another, but that requires a much more complicated locking setup in component.c (I have patches for that, but I've resisted sending them because it makes the locking very messy.)
-- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
On Wed, Oct 21, 2015 at 03:41:44PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 11:27:44 +0200, Russell King - ARM Linux wrote:
On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote:
On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
Currently i915/audio component works as you described. The audio is optional and HDMI graphics works without audio, while HDMI HD-audio mandates i915 graphics.
Right, but we also add additional interface on top of this to allow things like ensuring display is on when audio wants to run and now notification for events.
I don't see a reason why this can be used as single interface to bind as well as talk to display from various components, unless I missed obvious which prevent us from doing this in non i915 cases...
Maybe I can comment more specifically if I saw the code. Right now all I'm aware of is this idea without any code, and I don't like it.
Ok, i will be post my patches tomorrow. FWIW uses interface in sound/hda/hdac_i915.c for display power up/down
This:
component_match_add(dev, &match, hdac_component_master_match, bus); ret = component_master_add_with_match(dev, &hdac_component_master_ops, match); if (ret < 0) goto out_err; /* * Atm, we don't support deferring the component binding, so make sure * i915 is loaded and that the binding successfully completes. */ request_module("i915"); if (!acomp->ops) { ret = -ENODEV; goto out_master_del; }is really not nice.
Yeah, admittedly it's a really ugly workaround. It's coded in that way just to make our lives easier: it works well in most cases for i915 / hd-audio.
Actually, it's easy to modify the hda code in the right way, i.e. to defer via component bind ops. However, one drawback is that it's not trivial to handle the fallback case; namely, HD-audio might not need i915 binding, depending on the chip model and the configuration.
Braswell and Skylake have a (virtually) single audio controller as default, and this manages both the analog and HDMI/DP codecs. The i915 interaction is required only for the latter codecs, and thus for the former, it's optional. If we defer binding, the instantiation won't happen unless it's bound with i915. So, if i915 isn't initialized (e.g. by booting with nomodeset option), the whole audio will be gone, too. OTOH, Haswell and Braswell have a dedicated HDA controller for HDMI/DP, and they must be disabled (or fail) unless bound with graphics.
It's a corner case, so we might better ignore this. But it'd be certainly a "regression" -- at least, I used to test this in that way sometimes in the past. So it remains in the current way as is.
It's one of the reasons I mentioned it being a stop gap. But, I think the concept, passing ops via component, is actually working, and should be applicable to other drm/audio drivers. That's the point.
It's only the point if you can code it up properly, which from what I read in that file, it isn't.
Build the i915 DRM drivers as a module, load up the system, and then try removing the i915 DRM module and see what happens to the audio part. For starters, you have no protection what so ever against acomp->ops or acomp->dev becoming NULL - it's hellishly racy.
Secondly, you reject the initialisation if acomp->ops isn't set, but you allow acomp->ops to later become unset by the i915 DRM module being removed. If you can cope with acomp->ops being unset at a random point during the audio driver's use, why can't you cope with it being set at some random point later?
I don't think this code has been thought through at all.
On Wed, 21 Oct 2015 16:03:07 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 03:41:44PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 11:27:44 +0200, Russell King - ARM Linux wrote:
On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote:
On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
> Currently i915/audio component works as you described. The audio is > optional and HDMI graphics works without audio, while HDMI HD-audio > mandates i915 graphics.
Right, but we also add additional interface on top of this to allow things like ensuring display is on when audio wants to run and now notification for events.
I don't see a reason why this can be used as single interface to bind as well as talk to display from various components, unless I missed obvious which prevent us from doing this in non i915 cases...
Maybe I can comment more specifically if I saw the code. Right now all I'm aware of is this idea without any code, and I don't like it.
Ok, i will be post my patches tomorrow. FWIW uses interface in sound/hda/hdac_i915.c for display power up/down
This:
component_match_add(dev, &match, hdac_component_master_match, bus); ret = component_master_add_with_match(dev, &hdac_component_master_ops, match); if (ret < 0) goto out_err; /* * Atm, we don't support deferring the component binding, so make sure * i915 is loaded and that the binding successfully completes. */ request_module("i915"); if (!acomp->ops) { ret = -ENODEV; goto out_master_del; }is really not nice.
Yeah, admittedly it's a really ugly workaround. It's coded in that way just to make our lives easier: it works well in most cases for i915 / hd-audio.
Actually, it's easy to modify the hda code in the right way, i.e. to defer via component bind ops. However, one drawback is that it's not trivial to handle the fallback case; namely, HD-audio might not need i915 binding, depending on the chip model and the configuration.
Braswell and Skylake have a (virtually) single audio controller as default, and this manages both the analog and HDMI/DP codecs. The i915 interaction is required only for the latter codecs, and thus for the former, it's optional. If we defer binding, the instantiation won't happen unless it's bound with i915. So, if i915 isn't initialized (e.g. by booting with nomodeset option), the whole audio will be gone, too. OTOH, Haswell and Braswell have a dedicated HDA controller for HDMI/DP, and they must be disabled (or fail) unless bound with graphics.
It's a corner case, so we might better ignore this. But it'd be certainly a "regression" -- at least, I used to test this in that way sometimes in the past. So it remains in the current way as is.
It's one of the reasons I mentioned it being a stop gap. But, I think the concept, passing ops via component, is actually working, and should be applicable to other drm/audio drivers. That's the point.
It's only the point if you can code it up properly, which from what I read in that file, it isn't.
An idea can fly without coding, too :)
Build the i915 DRM drivers as a module, load up the system, and then try removing the i915 DRM module and see what happens to the audio part. For starters, you have no protection what so ever against acomp->ops or acomp->dev becoming NULL - it's hellishly racy.
Yes, very likely.
Secondly, you reject the initialisation if acomp->ops isn't set, but you allow acomp->ops to later become unset by the i915 DRM module being removed. If you can cope with acomp->ops being unset at a random point during the audio driver's use, why can't you cope with it being set at some random point later?
Setting/unsetting on the fly would be picky because the code does refcounting. Maybe an easier option is to inc/dec module usage count appropriately in use.
I don't think this code has been thought through at all.
True, more hardening needed.
thanks,
Takashi
On Wed, 21 Oct 2015 16:10:31 +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 16:03:07 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 03:41:44PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 11:27:44 +0200, Russell King - ARM Linux wrote:
On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote:
On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
> > Currently i915/audio component works as you described. The audio is > > optional and HDMI graphics works without audio, while HDMI HD-audio > > mandates i915 graphics. > > Right, but we also add additional interface on top of this to allow > things like ensuring display is on when audio wants to run and now > notification for events. > > I don't see a reason why this can be used as single interface to bind as > well as talk to display from various components, unless I missed obvious > which prevent us from doing this in non i915 cases...
Maybe I can comment more specifically if I saw the code. Right now all I'm aware of is this idea without any code, and I don't like it.
Ok, i will be post my patches tomorrow. FWIW uses interface in sound/hda/hdac_i915.c for display power up/down
This:
component_match_add(dev, &match, hdac_component_master_match, bus); ret = component_master_add_with_match(dev, &hdac_component_master_ops, match); if (ret < 0) goto out_err; /* * Atm, we don't support deferring the component binding, so make sure * i915 is loaded and that the binding successfully completes. */ request_module("i915"); if (!acomp->ops) { ret = -ENODEV; goto out_master_del; }is really not nice.
Yeah, admittedly it's a really ugly workaround. It's coded in that way just to make our lives easier: it works well in most cases for i915 / hd-audio.
Actually, it's easy to modify the hda code in the right way, i.e. to defer via component bind ops. However, one drawback is that it's not trivial to handle the fallback case; namely, HD-audio might not need i915 binding, depending on the chip model and the configuration.
Braswell and Skylake have a (virtually) single audio controller as default, and this manages both the analog and HDMI/DP codecs. The i915 interaction is required only for the latter codecs, and thus for the former, it's optional. If we defer binding, the instantiation won't happen unless it's bound with i915. So, if i915 isn't initialized (e.g. by booting with nomodeset option), the whole audio will be gone, too. OTOH, Haswell and Braswell have a dedicated HDA controller for HDMI/DP, and they must be disabled (or fail) unless bound with graphics.
It's a corner case, so we might better ignore this. But it'd be certainly a "regression" -- at least, I used to test this in that way sometimes in the past. So it remains in the current way as is.
It's one of the reasons I mentioned it being a stop gap. But, I think the concept, passing ops via component, is actually working, and should be applicable to other drm/audio drivers. That's the point.
It's only the point if you can code it up properly, which from what I read in that file, it isn't.
An idea can fly without coding, too :)
Build the i915 DRM drivers as a module, load up the system, and then try removing the i915 DRM module and see what happens to the audio part. For starters, you have no protection what so ever against acomp->ops or acomp->dev becoming NULL - it's hellishly racy.
Yes, very likely.
Secondly, you reject the initialisation if acomp->ops isn't set, but you allow acomp->ops to later become unset by the i915 DRM module being removed. If you can cope with acomp->ops being unset at a random point during the audio driver's use, why can't you cope with it being set at some random point later?
Setting/unsetting on the fly would be picky because the code does refcounting. Maybe an easier option is to inc/dec module usage count appropriately in use.
... and actually we pin at master bind:
static int hdac_component_master_bind(struct device *dev) { ..... /* * Atm, we don't support dynamic unbinding initiated by the child * component, so pin its containing module until we unbind. */ if (!try_module_get(acomp->ops->owner)) {
so unloading i915 module won't happen (but other method for unbinding still possible).
Takashi
On Wed, Oct 21, 2015 at 04:37:07PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 16:10:31 +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 16:03:07 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 03:41:44PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 11:27:44 +0200, Russell King - ARM Linux wrote:
On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote:
On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote: > > > Currently i915/audio component works as you described. The audio is > > > optional and HDMI graphics works without audio, while HDMI HD-audio > > > mandates i915 graphics. > > > > Right, but we also add additional interface on top of this to allow > > things like ensuring display is on when audio wants to run and now > > notification for events. > > > > I don't see a reason why this can be used as single interface to bind as > > well as talk to display from various components, unless I missed obvious > > which prevent us from doing this in non i915 cases... > > Maybe I can comment more specifically if I saw the code. Right now all > I'm aware of is this idea without any code, and I don't like it.
Ok, i will be post my patches tomorrow. FWIW uses interface in sound/hda/hdac_i915.c for display power up/down
This:
component_match_add(dev, &match, hdac_component_master_match, bus); ret = component_master_add_with_match(dev, &hdac_component_master_ops, match); if (ret < 0) goto out_err; /* * Atm, we don't support deferring the component binding, so make sure * i915 is loaded and that the binding successfully completes. */ request_module("i915"); if (!acomp->ops) { ret = -ENODEV; goto out_master_del; }is really not nice.
Yeah, admittedly it's a really ugly workaround. It's coded in that way just to make our lives easier: it works well in most cases for i915 / hd-audio.
Actually, it's easy to modify the hda code in the right way, i.e. to defer via component bind ops. However, one drawback is that it's not trivial to handle the fallback case; namely, HD-audio might not need i915 binding, depending on the chip model and the configuration.
Braswell and Skylake have a (virtually) single audio controller as default, and this manages both the analog and HDMI/DP codecs. The i915 interaction is required only for the latter codecs, and thus for the former, it's optional. If we defer binding, the instantiation won't happen unless it's bound with i915. So, if i915 isn't initialized (e.g. by booting with nomodeset option), the whole audio will be gone, too. OTOH, Haswell and Braswell have a dedicated HDA controller for HDMI/DP, and they must be disabled (or fail) unless bound with graphics.
It's a corner case, so we might better ignore this. But it'd be certainly a "regression" -- at least, I used to test this in that way sometimes in the past. So it remains in the current way as is.
It's one of the reasons I mentioned it being a stop gap. But, I think the concept, passing ops via component, is actually working, and should be applicable to other drm/audio drivers. That's the point.
It's only the point if you can code it up properly, which from what I read in that file, it isn't.
An idea can fly without coding, too :)
Build the i915 DRM drivers as a module, load up the system, and then try removing the i915 DRM module and see what happens to the audio part. For starters, you have no protection what so ever against acomp->ops or acomp->dev becoming NULL - it's hellishly racy.
Yes, very likely.
Secondly, you reject the initialisation if acomp->ops isn't set, but you allow acomp->ops to later become unset by the i915 DRM module being removed. If you can cope with acomp->ops being unset at a random point during the audio driver's use, why can't you cope with it being set at some random point later?
Setting/unsetting on the fly would be picky because the code does refcounting. Maybe an easier option is to inc/dec module usage count appropriately in use.
... and actually we pin at master bind:
static int hdac_component_master_bind(struct device *dev) { ..... /* * Atm, we don't support dynamic unbinding initiated by the child * component, so pin its containing module until we unbind. */ if (!try_module_get(acomp->ops->owner)) {
so unloading i915 module won't happen (but other method for unbinding still possible).
Pinning the module only prevents the code from disappearing, not the driver itself from disappearing. You can still just manually unbind through sysfs. The real fix is really what Russell described. -Daniel
On Wed, 21 Oct 2015 17:36:01 +0200, Daniel Vetter wrote:
On Wed, Oct 21, 2015 at 04:37:07PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 16:10:31 +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 16:03:07 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 03:41:44PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 11:27:44 +0200, Russell King - ARM Linux wrote:
On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote: > On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote: > > > > Currently i915/audio component works as you described. The audio is > > > > optional and HDMI graphics works without audio, while HDMI HD-audio > > > > mandates i915 graphics. > > > > > > Right, but we also add additional interface on top of this to allow > > > things like ensuring display is on when audio wants to run and now > > > notification for events. > > > > > > I don't see a reason why this can be used as single interface to bind as > > > well as talk to display from various components, unless I missed obvious > > > which prevent us from doing this in non i915 cases... > > > > Maybe I can comment more specifically if I saw the code. Right now all > > I'm aware of is this idea without any code, and I don't like it. > > Ok, i will be post my patches tomorrow. FWIW uses interface in > sound/hda/hdac_i915.c for display power up/down
This:
component_match_add(dev, &match, hdac_component_master_match, bus); ret = component_master_add_with_match(dev, &hdac_component_master_ops, match); if (ret < 0) goto out_err; /* * Atm, we don't support deferring the component binding, so make sure * i915 is loaded and that the binding successfully completes. */ request_module("i915"); if (!acomp->ops) { ret = -ENODEV; goto out_master_del; }is really not nice.
Yeah, admittedly it's a really ugly workaround. It's coded in that way just to make our lives easier: it works well in most cases for i915 / hd-audio.
Actually, it's easy to modify the hda code in the right way, i.e. to defer via component bind ops. However, one drawback is that it's not trivial to handle the fallback case; namely, HD-audio might not need i915 binding, depending on the chip model and the configuration.
Braswell and Skylake have a (virtually) single audio controller as default, and this manages both the analog and HDMI/DP codecs. The i915 interaction is required only for the latter codecs, and thus for the former, it's optional. If we defer binding, the instantiation won't happen unless it's bound with i915. So, if i915 isn't initialized (e.g. by booting with nomodeset option), the whole audio will be gone, too. OTOH, Haswell and Braswell have a dedicated HDA controller for HDMI/DP, and they must be disabled (or fail) unless bound with graphics.
It's a corner case, so we might better ignore this. But it'd be certainly a "regression" -- at least, I used to test this in that way sometimes in the past. So it remains in the current way as is.
It's one of the reasons I mentioned it being a stop gap. But, I think the concept, passing ops via component, is actually working, and should be applicable to other drm/audio drivers. That's the point.
It's only the point if you can code it up properly, which from what I read in that file, it isn't.
An idea can fly without coding, too :)
Build the i915 DRM drivers as a module, load up the system, and then try removing the i915 DRM module and see what happens to the audio part. For starters, you have no protection what so ever against acomp->ops or acomp->dev becoming NULL - it's hellishly racy.
Yes, very likely.
Secondly, you reject the initialisation if acomp->ops isn't set, but you allow acomp->ops to later become unset by the i915 DRM module being removed. If you can cope with acomp->ops being unset at a random point during the audio driver's use, why can't you cope with it being set at some random point later?
Setting/unsetting on the fly would be picky because the code does refcounting. Maybe an easier option is to inc/dec module usage count appropriately in use.
... and actually we pin at master bind:
static int hdac_component_master_bind(struct device *dev) { ..... /* * Atm, we don't support dynamic unbinding initiated by the child * component, so pin its containing module until we unbind. */ if (!try_module_get(acomp->ops->owner)) {
so unloading i915 module won't happen (but other method for unbinding still possible).
Pinning the module only prevents the code from disappearing, not the driver itself from disappearing. You can still just manually unbind through sysfs.
Yes, that's what I mentioned in parenthesis in the above :) We have always a brute force kill method.
The real fix is really what Russell described.
Right... I guess the fix is fairly trivial, though. All accesses to audio components are done via only helper functions, so we'd just need some lock there to protect from the unbind.
Below is a test patch I cooked quickly. This is the third patch applied after other two more patches: a cleanup patch and a patch for deferred probe of HD-audio with component.
Takashi
--- 8< --- From: Takashi Iwai tiwai@suse.de Subject: [PATCH 3/3] ALSA: hda - Add mutex protection at unbinding slave
For a safer unbinding, each access to audio component ops is protected via a new mutex, hdac_bus.audio_component_lock.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/hdaudio.h | 1 + sound/hda/hdac_i915.c | 63 +++++++++++++++++++++++++++++++++++-------------- 2 files changed, 46 insertions(+), 18 deletions(-)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index e2b712c90d3f..b88443c33aee 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -303,6 +303,7 @@ struct hdac_bus {
/* i915 component interface */ struct i915_audio_component *audio_component; + struct mutex audio_component_lock; int i915_power_refcount; };
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index f6fc16cfd02c..625e74ad2d12 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -26,17 +26,31 @@ struct hdac_gfx_component { const struct component_master_ops *ops; };
+static struct i915_audio_component *bus_get_acomp(struct hdac_bus *bus) +{ + mutex_lock(&bus->audio_component_lock); + return bus->audio_component; +} + +static void bus_put_acomp(struct hdac_bus *bus) +{ + mutex_lock(&bus->audio_component_lock); +} + int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable) { - struct i915_audio_component *acomp = bus->audio_component; + struct i915_audio_component *acomp = bus_get_acomp(bus); + int ret = 0;
- if (!acomp || !acomp->ops) - return -ENODEV; + if (!acomp || !acomp->ops) { + ret = -ENODEV; + goto out; + }
if (!acomp->ops->codec_wake_override) { dev_warn(bus->dev, "Invalid codec wake callback\n"); - return 0; + goto out; }
dev_dbg(bus->dev, "%s codec wakeup\n", @@ -44,16 +58,21 @@ int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
acomp->ops->codec_wake_override(acomp->dev, enable);
- return 0; + out: + bus_put_acomp(bus); + return ret; } EXPORT_SYMBOL_GPL(snd_hdac_set_codec_wakeup);
int snd_hdac_display_power(struct hdac_bus *bus, bool enable) { - struct i915_audio_component *acomp = bus->audio_component; + struct i915_audio_component *acomp = bus_get_acomp(bus); + int ret = 0;
- if (!acomp || !acomp->ops) - return -ENODEV; + if (!acomp || !acomp->ops) { + ret = -ENODEV; + goto out; + }
dev_dbg(bus->dev, "display power %s\n", enable ? "enable" : "disable"); @@ -70,18 +89,22 @@ int snd_hdac_display_power(struct hdac_bus *bus, bool enable) acomp->ops->put_power(acomp->dev); }
- return 0; + out: + bus_put_acomp(bus); + return ret; } EXPORT_SYMBOL_GPL(snd_hdac_display_power);
int snd_hdac_get_display_clk(struct hdac_bus *bus) { - struct i915_audio_component *acomp = bus->audio_component; + struct i915_audio_component *acomp = bus_get_acomp(bus); + int ret;
if (!acomp || !acomp->ops) - return -ENODEV; - - return acomp->ops->get_cdclk_freq(acomp->dev); + ret = -ENODEV; + ret = acomp->ops->get_cdclk_freq(acomp->dev); + bus_put_acomp(bus); + return ret; } EXPORT_SYMBOL_GPL(snd_hdac_get_display_clk);
@@ -89,14 +112,14 @@ static int hdac_component_master_bind(struct device *dev) { struct hdac_device *codec = dev_to_hdac_dev(dev); struct hdac_bus *bus = codec->bus; - struct i915_audio_component *acomp = bus->audio_component; + struct i915_audio_component *acomp = bus_get_acomp(bus); struct hdac_gfx_component *hda_comp = container_of(acomp, struct hdac_gfx_component, acomp); int ret;
ret = component_bind_all(dev, acomp); if (ret < 0) - return ret; + goto out_unlock;
if (WARN_ON(!(acomp->dev && acomp->ops && acomp->ops->get_power && acomp->ops->put_power && acomp->ops->get_cdclk_freq))) { @@ -125,7 +148,8 @@ static int hdac_component_master_bind(struct device *dev)
out_unbind: component_unbind_all(dev, acomp); - + out_unlock: + bus_put_acomp(bus); return ret; }
@@ -133,7 +157,7 @@ static void hdac_component_master_unbind(struct device *dev) { struct hdac_device *codec = dev_to_hdac_dev(dev); struct hdac_bus *bus = codec->bus; - struct i915_audio_component *acomp = bus->audio_component; + struct i915_audio_component *acomp = bus_get_acomp(bus); struct hdac_gfx_component *hda_comp = container_of(acomp, struct hdac_gfx_component, acomp);
@@ -142,6 +166,7 @@ static void hdac_component_master_unbind(struct device *dev) module_put(acomp->ops->owner); component_unbind_all(dev, acomp); WARN_ON(acomp->ops || acomp->dev); + bus_put_acomp(bus); }
static const struct component_master_ops hdac_component_master_ops = { @@ -159,12 +184,13 @@ int snd_hdac_i915_register_notifier(struct hdac_device *codec, const struct i915_audio_component_audio_ops *aops) { struct hdac_bus *bus = codec->bus; - struct i915_audio_component *acomp = bus->audio_component; + struct i915_audio_component *acomp = bus_get_acomp(bus);
if (WARN_ON(!acomp)) return -ENODEV;
acomp->audio_ops = aops; + bus_put_acomp(bus); return 0; } EXPORT_SYMBOL_GPL(snd_hdac_i915_register_notifier); @@ -179,6 +205,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus, hda_comp = kzalloc(sizeof(*hda_comp), GFP_KERNEL); if (!hda_comp) return -ENOMEM; + mutex_init(&bus->audio_component_lock); bus->audio_component = &hda_comp->acomp; hda_comp->ops = codec_ops;
On Wed, 21 Oct 2015 18:46:23 +0200, Takashi Iwai wrote:
Below is a test patch I cooked quickly. This is the third patch applied after other two more patches: a cleanup patch and a patch for deferred probe of HD-audio with component.
And this is the patch to defer the probe.
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH 2/3] ALSA: hda - Implement deferred probe for i915 audio component
For Intel controllers that mandate i915 binding, the probe is deferred and done through the component master bind ops. The controller driver needs to pass its ops to continue the probe. For the legacy HDA, we just call the existing azx_probe_continue() there.
A possible drawback is that there might be no longer fallback working when the i915 graphics is unavailable, e.g. boot with nomodeset option, on SKL/BSW.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/hda_i915.h | 7 ++++-- sound/hda/hdac_i915.c | 60 ++++++++++++++++++++++------------------------- sound/pci/hda/hda_intel.c | 45 +++++++++++++++++++---------------- 3 files changed, 58 insertions(+), 54 deletions(-)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h index 7b19f1f8cc23..b42449af6cd2 100644 --- a/include/sound/hda_i915.h +++ b/include/sound/hda_i915.h @@ -4,13 +4,15 @@ #ifndef __SOUND_HDA_I915_H #define __SOUND_HDA_I915_H
+#include <linux/component.h> #include <drm/i915_component.h>
#ifdef CONFIG_SND_HDA_I915 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable); int snd_hdac_display_power(struct hdac_bus *bus, bool enable); int snd_hdac_get_display_clk(struct hdac_bus *bus); -int snd_hdac_i915_init(struct hdac_bus *bus); +int snd_hdac_i915_init(struct hdac_bus *bus, + const struct component_master_ops *codec_ops); int snd_hdac_i915_exit(struct hdac_bus *bus); int snd_hdac_i915_register_notifier(struct hdac_device *codec, const struct i915_audio_component_audio_ops *); @@ -27,7 +29,8 @@ static inline int snd_hdac_get_display_clk(struct hdac_bus *bus) { return 0; } -static inline int snd_hdac_i915_init(struct hdac_bus *bus) +static inline int snd_hdac_i915_init(struct hdac_bus *bus, + const struct component_master_ops *codec_ops) { return -ENODEV; } diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index e36ed18f23c4..f6fc16cfd02c 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -21,6 +21,11 @@ #include <sound/hdaudio.h> #include <sound/hda_i915.h>
+struct hdac_gfx_component { + struct i915_audio_component acomp; + const struct component_master_ops *ops; +}; + int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable) { struct i915_audio_component *acomp = bus->audio_component; @@ -85,6 +90,8 @@ static int hdac_component_master_bind(struct device *dev) struct hdac_device *codec = dev_to_hdac_dev(dev); struct hdac_bus *bus = codec->bus; struct i915_audio_component *acomp = bus->audio_component; + struct hdac_gfx_component *hda_comp = + container_of(acomp, struct hdac_gfx_component, acomp); int ret;
ret = component_bind_all(dev, acomp); @@ -106,6 +113,14 @@ static int hdac_component_master_bind(struct device *dev) goto out_unbind; }
+ if (hda_comp->ops && hda_comp->ops->bind) { + ret = hda_comp->ops->bind(dev); + if (ret < 0) { + module_put(acomp->ops->owner); + goto out_unbind; + } + } + return 0;
out_unbind: @@ -119,7 +134,11 @@ static void hdac_component_master_unbind(struct device *dev) struct hdac_device *codec = dev_to_hdac_dev(dev); struct hdac_bus *bus = codec->bus; struct i915_audio_component *acomp = bus->audio_component; + struct hdac_gfx_component *hda_comp = + container_of(acomp, struct hdac_gfx_component, acomp);
+ if (hda_comp->ops && hda_comp->ops->unbind) + hda_comp->ops->unbind(dev); module_put(acomp->ops->owner); component_unbind_all(dev, acomp); WARN_ON(acomp->ops || acomp->dev); @@ -150,45 +169,22 @@ int snd_hdac_i915_register_notifier(struct hdac_device *codec, } EXPORT_SYMBOL_GPL(snd_hdac_i915_register_notifier);
-int snd_hdac_i915_init(struct hdac_bus *bus) +int snd_hdac_i915_init(struct hdac_bus *bus, + const struct component_master_ops *codec_ops) { struct component_match *match = NULL; struct device *dev = bus->dev; - struct i915_audio_component *acomp; - int ret; + struct hdac_gfx_component *hda_comp;
- acomp = kzalloc(sizeof(*acomp), GFP_KERNEL); - if (!acomp) + hda_comp = kzalloc(sizeof(*hda_comp), GFP_KERNEL); + if (!hda_comp) return -ENOMEM; - bus->audio_component = acomp; + bus->audio_component = &hda_comp->acomp; + hda_comp->ops = codec_ops;
component_match_add(dev, &match, hdac_component_master_match, bus); - ret = component_master_add_with_match(dev, &hdac_component_master_ops, - match); - if (ret < 0) - goto out_err; - - /* - * Atm, we don't support deferring the component binding, so make sure - * i915 is loaded and that the binding successfully completes. - */ - request_module("i915"); - - if (!acomp->ops) { - ret = -ENODEV; - goto out_master_del; - } - dev_dbg(dev, "bound to i915 component master\n"); - - return 0; -out_master_del: - component_master_del(dev, &hdac_component_master_ops); -out_err: - kfree(acomp); - bus->audio_component = NULL; - dev_err(dev, "failed to add i915 component master (%d)\n", ret); - - return ret; + return component_master_add_with_match(dev, &hdac_component_master_ops, + match); } EXPORT_SYMBOL_GPL(snd_hdac_i915_init);
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index c38c68f57938..78deda018894 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1884,6 +1884,20 @@ static const struct hda_controller_ops pci_hda_ops = { .link_power = azx_intel_link_power, };
+#ifdef CONFIG_SND_HDA_I915 +static int azx_i915_bind(struct device *dev) +{ + struct snd_card *card = dev_get_drvdata(dev); + struct azx *chip = card->private_data; + + return azx_probe_continue(chip); +} + +static const struct component_master_ops component_ops = { + .bind = azx_i915_bind, +}; +#endif /* CONFIG_SND_HDA_I915 */ + static int azx_probe(struct pci_dev *pci, const struct pci_device_id *pci_id) { @@ -1943,10 +1957,19 @@ static int azx_probe(struct pci_dev *pci, } #endif /* CONFIG_SND_HDA_PATCH_LOADER */
-#ifndef CONFIG_SND_HDA_I915 - if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) + if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) { +#ifdef CONFIG_SND_HDA_I915 + /* HSW/BDW controllers need this power */ + if (CONTROLLER_IN_GPU(pci)) + hda->need_i915_power = 1; + err = snd_hdac_i915_init(azx_bus(chip), &component_ops); + if (err < 0) + goto out_free; + schedule_probe = false; /* continue via bind ops */ +#else dev_err(card->dev, "Haswell must build in CONFIG_SND_HDA_I915\n"); #endif + }
if (schedule_probe) schedule_work(&hda->probe_work); @@ -1983,23 +2006,6 @@ static int azx_probe_continue(struct azx *chip) * display codec needs the power and it can be released after probe. */ if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) { - /* HSW/BDW controllers need this power */ - if (CONTROLLER_IN_GPU(pci)) - hda->need_i915_power = 1; - - err = snd_hdac_i915_init(bus); - if (err < 0) { - /* if the controller is bound only with HDMI/DP - * (for HSW and BDW), we need to abort the probe; - * for other chips, still continue probing as other - * codecs can be on the same link. - */ - if (CONTROLLER_IN_GPU(pci)) - goto out_free; - else - goto skip_i915; - } - err = snd_hdac_display_power(bus, true); if (err < 0) { dev_err(chip->card->dev, @@ -2008,7 +2014,6 @@ static int azx_probe_continue(struct azx *chip) } }
- skip_i915: err = azx_first_init(chip); if (err < 0) goto out_free;
On Wed, 21 Oct 2015 18:48:06 +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 18:46:23 +0200, Takashi Iwai wrote:
Below is a test patch I cooked quickly. This is the third patch applied after other two more patches: a cleanup patch and a patch for deferred probe of HD-audio with component.
And this is the patch to defer the probe.
... and the first patch, a sort of cleanup, is here (sorry for the mess).
I put these three in test/hda-i915 branch of my sound git tree.
Takashi
---- 8< ---- From: Takashi Iwai tiwai@suse.de Subject: [PATCH 1/3] ALSA: hda - Pass hdac_device object at notifier registration
For dropping the local static variable in hdac_i915.c, add an argument to pass the hdac_device object to snd-Hdac_i915_register_notifier(). This allows us (in theory) having individual audio component per bus.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/hda_i915.h | 6 ++++-- sound/hda/hdac_i915.c | 21 +++++++++++++-------- sound/pci/hda/patch_hdmi.c | 5 +++-- 3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h index 930b41e5acf4..7b19f1f8cc23 100644 --- a/include/sound/hda_i915.h +++ b/include/sound/hda_i915.h @@ -12,7 +12,8 @@ int snd_hdac_display_power(struct hdac_bus *bus, bool enable); int snd_hdac_get_display_clk(struct hdac_bus *bus); int snd_hdac_i915_init(struct hdac_bus *bus); int snd_hdac_i915_exit(struct hdac_bus *bus); -int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *); +int snd_hdac_i915_register_notifier(struct hdac_device *codec, + const struct i915_audio_component_audio_ops *); #else static inline int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable) { @@ -34,7 +35,8 @@ static inline int snd_hdac_i915_exit(struct hdac_bus *bus) { return 0; } -static inline int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *ops) +static inline int snd_hdac_i915_register_notifier(struct hdac_device *codec, + const struct i915_audio_component_audio_ops *ops) { return -ENODEV; } diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index 55c3df4458f7..e36ed18f23c4 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -21,8 +21,6 @@ #include <sound/hdaudio.h> #include <sound/hda_i915.h>
-static struct i915_audio_component *hdac_acomp; - int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable) { struct i915_audio_component *acomp = bus->audio_component; @@ -84,7 +82,9 @@ EXPORT_SYMBOL_GPL(snd_hdac_get_display_clk);
static int hdac_component_master_bind(struct device *dev) { - struct i915_audio_component *acomp = hdac_acomp; + struct hdac_device *codec = dev_to_hdac_dev(dev); + struct hdac_bus *bus = codec->bus; + struct i915_audio_component *acomp = bus->audio_component; int ret;
ret = component_bind_all(dev, acomp); @@ -116,7 +116,9 @@ out_unbind:
static void hdac_component_master_unbind(struct device *dev) { - struct i915_audio_component *acomp = hdac_acomp; + struct hdac_device *codec = dev_to_hdac_dev(dev); + struct hdac_bus *bus = codec->bus; + struct i915_audio_component *acomp = bus->audio_component;
module_put(acomp->ops->owner); component_unbind_all(dev, acomp); @@ -134,12 +136,16 @@ static int hdac_component_master_match(struct device *dev, void *data) return !strcmp(dev->driver->name, "i915"); }
-int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *aops) +int snd_hdac_i915_register_notifier(struct hdac_device *codec, + const struct i915_audio_component_audio_ops *aops) { - if (WARN_ON(!hdac_acomp)) + struct hdac_bus *bus = codec->bus; + struct i915_audio_component *acomp = bus->audio_component; + + if (WARN_ON(!acomp)) return -ENODEV;
- hdac_acomp->audio_ops = aops; + acomp->audio_ops = aops; return 0; } EXPORT_SYMBOL_GPL(snd_hdac_i915_register_notifier); @@ -155,7 +161,6 @@ int snd_hdac_i915_init(struct hdac_bus *bus) if (!acomp) return -ENOMEM; bus->audio_component = acomp; - hdac_acomp = acomp;
component_match_add(dev, &match, hdac_component_master_match, bus); ret = component_master_add_with_match(dev, &hdac_component_master_ops, diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index f503a883bef3..fbd60ea98f19 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2216,7 +2216,7 @@ static void generic_hdmi_free(struct hda_codec *codec) int pin_idx;
if (is_haswell_plus(codec) || is_valleyview_plus(codec)) - snd_hdac_i915_register_notifier(NULL); + snd_hdac_i915_register_notifier(&codec->core, NULL);
for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); @@ -2381,7 +2381,8 @@ static int patch_generic_hdmi(struct hda_codec *codec) codec->depop_delay = 0; spec->i915_audio_ops.audio_ptr = codec; spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify; - snd_hdac_i915_register_notifier(&spec->i915_audio_ops); + snd_hdac_i915_register_notifier(&codec->core, + &spec->i915_audio_ops); }
if (hdmi_parse_codec(codec) < 0) {
On Wed, Oct 21, 2015 at 04:10:31PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 16:03:07 +0200, Russell King - ARM Linux wrote:
It's only the point if you can code it up properly, which from what I read in that file, it isn't.
An idea can fly without coding, too :)
Build the i915 DRM drivers as a module, load up the system, and then try removing the i915 DRM module and see what happens to the audio part. For starters, you have no protection what so ever against acomp->ops or acomp->dev becoming NULL - it's hellishly racy.
Yes, very likely.
Secondly, you reject the initialisation if acomp->ops isn't set, but you allow acomp->ops to later become unset by the i915 DRM module being removed. If you can cope with acomp->ops being unset at a random point during the audio driver's use, why can't you cope with it being set at some random point later?
Setting/unsetting on the fly would be picky because the code does refcounting. Maybe an easier option is to inc/dec module usage count appropriately in use.
I don't think this code has been thought through at all.
True, more hardening needed.
In any case, this doesn't (and can't) solve the CEC problem, so it's not a solution to the problem at hand.
On Wed, Oct 21, 2015 at 03:37:47PM +0100, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 04:10:31PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 16:03:07 +0200, Russell King - ARM Linux wrote:
It's only the point if you can code it up properly, which from what I read in that file, it isn't.
An idea can fly without coding, too :)
Build the i915 DRM drivers as a module, load up the system, and then try removing the i915 DRM module and see what happens to the audio part. For starters, you have no protection what so ever against acomp->ops or acomp->dev becoming NULL - it's hellishly racy.
Yes, very likely.
Secondly, you reject the initialisation if acomp->ops isn't set, but you allow acomp->ops to later become unset by the i915 DRM module being removed. If you can cope with acomp->ops being unset at a random point during the audio driver's use, why can't you cope with it being set at some random point later?
Setting/unsetting on the fly would be picky because the code does refcounting. Maybe an easier option is to inc/dec module usage count appropriately in use.
I don't think this code has been thought through at all.
True, more hardening needed.
I will try to fix this once I am done with Skylake drivers.. Plus adding new features like ELD in on my plan for this interface
In any case, this doesn't (and can't) solve the CEC problem, so it's not a solution to the problem at hand.
Sorry am not sure I follow the reasons for that, wouldn't CEC be another slave in such an interface? I though component fwk did allow us to have multiple slaves..
On Wed, Oct 21, 2015 at 09:49:28PM +0530, Vinod Koul wrote:
On Wed, Oct 21, 2015 at 03:37:47PM +0100, Russell King - ARM Linux wrote:
In any case, this doesn't (and can't) solve the CEC problem, so it's not a solution to the problem at hand.
Sorry am not sure I follow the reasons for that, wouldn't CEC be another slave in such an interface? I though component fwk did allow us to have multiple slaves..
Not with the way you're using the component helper here.
I guess that not all my message is being read, because people keep replying half-way down my messages...
You can only register a struct device _once_ as a slave device.
With the way you're using it here for audio, you're registering the i915 DRM device as a slave component device, and the audio side as the master. That means the audio master can bind to the DRM slave component device.
You can't then have a CEC master bind to the i915 DRM slave device (it's already bound to the audio master device), and you can't register the i915 DRM device as a second slave component device. It becomes indistinguishable from the first, and there's no way to tell which of the two different 'ops' structures should be used with which master.
I said this in my message 20151021140307.GE32532@n2100.arm.linux.org.uk which was two of my replies ago in this sub-thread.
On Wed, 21 Oct 2015 19:34:37 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 09:49:28PM +0530, Vinod Koul wrote:
On Wed, Oct 21, 2015 at 03:37:47PM +0100, Russell King - ARM Linux wrote:
In any case, this doesn't (and can't) solve the CEC problem, so it's not a solution to the problem at hand.
Sorry am not sure I follow the reasons for that, wouldn't CEC be another slave in such an interface? I though component fwk did allow us to have multiple slaves..
Not with the way you're using the component helper here.
I guess that not all my message is being read, because people keep replying half-way down my messages...
You can only register a struct device _once_ as a slave device.
With the way you're using it here for audio, you're registering the i915 DRM device as a slave component device, and the audio side as the master. That means the audio master can bind to the DRM slave component device.
You can't then have a CEC master bind to the i915 DRM slave device (it's already bound to the audio master device), and you can't register the i915 DRM device as a second slave component device. It becomes indistinguishable from the first, and there's no way to tell which of the two different 'ops' structures should be used with which master.
I said this in my message 20151021140307.GE32532@n2100.arm.linux.org.uk which was two of my replies ago in this sub-thread.
Can't the limitation of single slave dev be extended simply? e.g. add some matching semantics to component_master_add_child() like a shared key in both master and slave, and let assign only the matched slave.
I might think of the problem too easy, but didn't see any obvious restriction in the code except for that...
thanks,
Takashi
On Wed, Oct 21, 2015 at 07:59:06PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 19:34:37 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 09:49:28PM +0530, Vinod Koul wrote:
On Wed, Oct 21, 2015 at 03:37:47PM +0100, Russell King - ARM Linux wrote:
In any case, this doesn't (and can't) solve the CEC problem, so it's not a solution to the problem at hand.
Sorry am not sure I follow the reasons for that, wouldn't CEC be another slave in such an interface? I though component fwk did allow us to have multiple slaves..
Not with the way you're using the component helper here.
I guess that not all my message is being read, because people keep replying half-way down my messages...
You can only register a struct device _once_ as a slave device.
With the way you're using it here for audio, you're registering the i915 DRM device as a slave component device, and the audio side as the master. That means the audio master can bind to the DRM slave component device.
You can't then have a CEC master bind to the i915 DRM slave device (it's already bound to the audio master device), and you can't register the i915 DRM device as a second slave component device. It becomes indistinguishable from the first, and there's no way to tell which of the two different 'ops' structures should be used with which master.
I said this in my message 20151021140307.GE32532@n2100.arm.linux.org.uk which was two of my replies ago in this sub-thread.
Can't the limitation of single slave dev be extended simply? e.g. add some matching semantics to component_master_add_child() like a shared key in both master and slave, and let assign only the matched slave.
I've already explained that in the email message I referred to by message ID above (which was a reply to one of your previous messages)
This is why I find email such a poor communication medium - I'm quite sure most people only read half an email message before hitting reply, and then they stick their reply after where they stopped reading and never bother reading the rest of the message. Normally, at this point, I'll start getting grumpy and sending frustrated emails... which would eventually deride into flames, but let's _try_ to keep this civil.
Here's the relevant paragraph from the above referenced message, and to make the appropriate bit stand out, I've underlined it with ^.
However, there's a lurking problem: you can't register the same struct device as a slave more than once into the component helpers - that's because it's designed to look for devices. There's intentionally no
^^^^^^^^^^^^^^^^^^^^^^^^
linkage between the slave ops and master ops to allow for generic
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
slave drivers (like tda998x) to be used with different master drivers
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(armada, tilcdc, etc). In theory, you can register the master device
^^^^^^^^^^^^^^^^^^^^^
of one componentised system as a slave device of another, but that requires a much more complicated locking setup in component.c (I have patches for that, but I've resisted sending them because it makes the locking very messy.)
In a subsequent sentence, I also address the issue that would occur if any of the already componentised DRM drivers were to attempt what you're doing in i915 - although I say it's solvable, I've resisted that because it makes the locking in there _much_ more hairy, and I'm now not certain that my more complex locking implementation would even cope with that scenario.
On Wed, 21 Oct 2015 20:19:38 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 07:59:06PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 19:34:37 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 09:49:28PM +0530, Vinod Koul wrote:
On Wed, Oct 21, 2015 at 03:37:47PM +0100, Russell King - ARM Linux wrote:
In any case, this doesn't (and can't) solve the CEC problem, so it's not a solution to the problem at hand.
Sorry am not sure I follow the reasons for that, wouldn't CEC be another slave in such an interface? I though component fwk did allow us to have multiple slaves..
Not with the way you're using the component helper here.
I guess that not all my message is being read, because people keep replying half-way down my messages...
You can only register a struct device _once_ as a slave device.
With the way you're using it here for audio, you're registering the i915 DRM device as a slave component device, and the audio side as the master. That means the audio master can bind to the DRM slave component device.
You can't then have a CEC master bind to the i915 DRM slave device (it's already bound to the audio master device), and you can't register the i915 DRM device as a second slave component device. It becomes indistinguishable from the first, and there's no way to tell which of the two different 'ops' structures should be used with which master.
I said this in my message 20151021140307.GE32532@n2100.arm.linux.org.uk which was two of my replies ago in this sub-thread.
Can't the limitation of single slave dev be extended simply? e.g. add some matching semantics to component_master_add_child() like a shared key in both master and slave, and let assign only the matched slave.
I've already explained that in the email message I referred to by message ID above (which was a reply to one of your previous messages)
This is why I find email such a poor communication medium - I'm quite sure most people only read half an email message before hitting reply, and then they stick their reply after where they stopped reading and never bother reading the rest of the message. Normally, at this point, I'll start getting grumpy and sending frustrated emails... which would eventually deride into flames, but let's _try_ to keep this civil.
Thank you for patience!
Here's the relevant paragraph from the above referenced message, and to make the appropriate bit stand out, I've underlined it with ^.
However, there's a lurking problem: you can't register the same struct device as a slave more than once into the component helpers - that's because it's designed to look for devices. There's intentionally no
^^^^^^^^^^^^^^^^^^^^^^^^linkage between the slave ops and master ops to allow for generic
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
slave drivers (like tda998x) to be used with different master drivers
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(armada, tilcdc, etc). In theory, you can register the master device
^^^^^^^^^^^^^^^^^^^^^
of one componentised system as a slave device of another, but that requires a much more complicated locking setup in component.c (I have patches for that, but I've resisted sending them because it makes the locking very messy.)
In a subsequent sentence, I also address the issue that would occur if any of the already componentised DRM drivers were to attempt what you're doing in i915 - although I say it's solvable, I've resisted that because it makes the locking in there _much_ more hairy, and I'm now not certain that my more complex locking implementation would even cope with that scenario.
I understood that the original component design wasn't for cross subsystems. OTOH I wondered why it can't be extended for wider use -- that was the simple question; I haven't seen so complex locking in some upstream drm drivers code through a quick glance, so the mess about locking you mentioned wasn't clear to me. Now point taken.
(Still I think it's possible to have multiple components binds for such cases, but I won't insist on it :)
To be noted, a weak dependency is still necessary for i915 audio case, and a simple notifier type registration won't work, unfortunately. GPU power adjustment is mandatory even at probing the audio hardware to judge whether HDMI codec is present or not. Meanwhile the dependency to the graphics must not be tight, either. The very same audio driver is for a controller without graphics, too. That's the reason we took the component as an alternative implementation, which is a bit cleaner than the direct symbol resolving in the earlier code.
So, if any better solution that covers a use case like i915/hda is present, we're willing to switch. As repeatedly written, the current implementation is a stop gap. Although this doesn't look too bad, there are still some obvious pitfalls as you pointed out. We can paper over them (maybe I'll do for 4.4), but a fundamentally better solution would be more welcome, of course.
thanks,
Takashi
On Wed, Oct 21, 2015 at 10:37:27PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 20:19:38 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 07:59:06PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 19:34:37 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 09:49:28PM +0530, Vinod Koul wrote:
On Wed, Oct 21, 2015 at 03:37:47PM +0100, Russell King - ARM Linux wrote:
In any case, this doesn't (and can't) solve the CEC problem, so it's not a solution to the problem at hand.
Sorry am not sure I follow the reasons for that, wouldn't CEC be another slave in such an interface? I though component fwk did allow us to have multiple slaves..
Not with the way you're using the component helper here.
I guess that not all my message is being read, because people keep replying half-way down my messages...
You can only register a struct device _once_ as a slave device.
With the way you're using it here for audio, you're registering the i915 DRM device as a slave component device, and the audio side as the master. That means the audio master can bind to the DRM slave component device.
You can't then have a CEC master bind to the i915 DRM slave device (it's already bound to the audio master device), and you can't register the i915 DRM device as a second slave component device. It becomes indistinguishable from the first, and there's no way to tell which of the two different 'ops' structures should be used with which master.
I said this in my message 20151021140307.GE32532@n2100.arm.linux.org.uk which was two of my replies ago in this sub-thread.
Can't the limitation of single slave dev be extended simply? e.g. add some matching semantics to component_master_add_child() like a shared key in both master and slave, and let assign only the matched slave.
I've already explained that in the email message I referred to by message ID above (which was a reply to one of your previous messages)
This is why I find email such a poor communication medium - I'm quite sure most people only read half an email message before hitting reply, and then they stick their reply after where they stopped reading and never bother reading the rest of the message. Normally, at this point, I'll start getting grumpy and sending frustrated emails... which would eventually deride into flames, but let's _try_ to keep this civil.
Thank you for patience!
Here's the relevant paragraph from the above referenced message, and to make the appropriate bit stand out, I've underlined it with ^.
However, there's a lurking problem: you can't register the same struct device as a slave more than once into the component helpers - that's because it's designed to look for devices. There's intentionally no
^^^^^^^^^^^^^^^^^^^^^^^^linkage between the slave ops and master ops to allow for generic
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
slave drivers (like tda998x) to be used with different master drivers
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(armada, tilcdc, etc). In theory, you can register the master device
^^^^^^^^^^^^^^^^^^^^^
of one componentised system as a slave device of another, but that requires a much more complicated locking setup in component.c (I have patches for that, but I've resisted sending them because it makes the locking very messy.)
In a subsequent sentence, I also address the issue that would occur if any of the already componentised DRM drivers were to attempt what you're doing in i915 - although I say it's solvable, I've resisted that because it makes the locking in there _much_ more hairy, and I'm now not certain that my more complex locking implementation would even cope with that scenario.
I understood that the original component design wasn't for cross subsystems. OTOH I wondered why it can't be extended for wider use -- that was the simple question; I haven't seen so complex locking in some upstream drm drivers code through a quick glance, so the mess about locking you mentioned wasn't clear to me. Now point taken.
You can find my patch for the "more complex locking" problem at the end of this mail. This may not apply to the current version of component.c, but illustrates at least some of the problem of getting rid of the global component_mutex lock, which would be necessary to allow a binding master to call back into the component helper to then register a slave. Today, that action would deadlock on component_mutex.
I'm not actually convinced that even this patch is correct as it stands...
To be noted, a weak dependency is still necessary for i915 audio case, and a simple notifier type registration won't work, unfortunately. GPU power adjustment is mandatory even at probing the audio hardware to judge whether HDMI codec is present or not. Meanwhile the dependency to the graphics must not be tight, either. The very same audio driver is for a controller without graphics, too. That's the reason we took the component as an alternative implementation, which is a bit cleaner than the direct symbol resolving in the earlier code.
So, if any better solution that covers a use case like i915/hda is present, we're willing to switch. As repeatedly written, the current implementation is a stop gap. Although this doesn't look too bad, there are still some obvious pitfalls as you pointed out. We can paper over them (maybe I'll do for 4.4), but a fundamentally better solution would be more welcome, of course.
Well, I guess it's fine as a temporary stop-gap, but what I don't want to see is this stop-gap becoming more common. It may work for your i915 case, but it won't work everywhere, so it can't become a generic solution to this problem.
drivers/base/component.c | 142 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 116 insertions(+), 26 deletions(-)
diff --git a/drivers/base/component.c b/drivers/base/component.c index f748430bb654..d72fe9bc7033 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -30,7 +30,10 @@ struct component_match { struct master { struct list_head node; struct list_head components; + struct kref kref; + struct mutex mutex; bool bound; + bool dead;
const struct component_master_ops *ops; struct device *dev; @@ -49,8 +52,33 @@ struct component {
static DEFINE_MUTEX(component_mutex); static LIST_HEAD(component_list); +static DEFINE_MUTEX(master_mutex); static LIST_HEAD(masters);
+static void master_release(struct kref *kref) __releases(master_mutex) +{ + struct master *master = container_of(kref, struct master, kref); + + list_del(&master->node); + mutex_unlock(&master_mutex); + WARN_ON(!list_empty(&master->components)); + kfree(master); +} + +static void master_release_nolock(struct kref *kref) +{ + struct master *master = container_of(kref, struct master, kref); + + list_del(&master->node); + WARN_ON(!list_empty(&master->components)); + kfree(master); +} + +static void master_norelease(struct kref *kref) +{ + WARN_ON(kref); +} + static struct master *__master_find(struct device *dev, const struct component_master_ops *ops) { @@ -63,9 +91,30 @@ static struct master *__master_find(struct device *dev, return NULL; }
+/* + * Find the master structure for this device, and make sure that the + * per-master lock is held. This is used from within the master device + * drivers bind and unbind callbacks, which should only be called from + * paths where the per-master lock is already held. + */ +static struct master *master_find_locked(struct device *dev, + const struct component_master_ops *ops) +{ + struct master *master; + + mutex_lock(&master_mutex); + master = __master_find(dev, NULL); + WARN_ON(master && !mutex_is_locked(&master->mutex)); + mutex_unlock(&master_mutex); + + return master; +} + /* Attach an unattached component to a master. */ static void component_attach_master(struct master *master, struct component *c) { + kref_get(&master->kref); + c->master = master;
list_add_tail(&c->master_node, &master->components); @@ -77,6 +126,9 @@ static void component_detach_master(struct master *master, struct component *c) list_del(&c->master_node);
c->master = NULL; + + /* This kref_put should never release the master */ + kref_put(&master->kref, master_norelease); }
/* @@ -90,6 +142,7 @@ int component_master_add_child(struct master *master, struct component *c; int ret = -ENXIO;
+ mutex_lock(&component_mutex); list_for_each_entry(c, &component_list, node) { if (c->master && c->master != master) continue; @@ -101,6 +154,7 @@ int component_master_add_child(struct master *master, break; } } + mutex_unlock(&component_mutex);
return ret; } @@ -137,6 +191,7 @@ static int find_components(struct master *master) /* Detach all attached components from this master */ static void master_remove_components(struct master *master) { + mutex_lock(&component_mutex); while (!list_empty(&master->components)) { struct component *c = list_first_entry(&master->components, struct component, master_node); @@ -145,6 +200,7 @@ static void master_remove_components(struct master *master)
component_detach_master(master, c); } + mutex_unlock(&component_mutex); }
/* @@ -201,20 +257,43 @@ static int try_to_bring_up_master(struct master *master,
static int try_to_bring_up_masters(struct component *component) { - struct master *m; + struct master *m, *last = NULL; int ret = 0;
+ mutex_lock(&master_mutex); list_for_each_entry(m, &masters, node) { + if (last) { + kref_put(&last->kref, master_release_nolock); + last = NULL; + } + + if (m->dead || m->bound) + continue; + + kref_get(&m->kref); + mutex_unlock(&master_mutex); + + mutex_lock(&m->mutex); ret = try_to_bring_up_master(m, component); + mutex_unlock(&m->mutex); + + mutex_lock(&master_mutex); + last = m; + if (ret != 0) break; }
+ if (last) + kref_put(&last->kref, master_release_nolock); + mutex_unlock(&master_mutex); + return ret; }
static void take_down_master(struct master *master) { + mutex_lock(&master->mutex); if (master->bound) { master->ops->unbind(master->dev); devres_release_group(master->dev, NULL); @@ -222,6 +301,7 @@ static void take_down_master(struct master *master) }
master_remove_components(master); + mutex_unlock(&master->mutex); }
static size_t component_match_size(size_t num) @@ -307,20 +387,25 @@ int component_master_add_with_match(struct device *dev, master->dev = dev; master->ops = ops; master->match = match; + mutex_init(&master->mutex); INIT_LIST_HEAD(&master->components); + kref_init(&master->kref);
- /* Add to the list of available masters. */ - mutex_lock(&component_mutex); + /* + * Add to the list of available masters. This is so + * the component code below can find this master. + */ + mutex_lock(&master_mutex); list_add(&master->node, &masters); + mutex_unlock(&master_mutex);
+ mutex_lock(&master->mutex); ret = try_to_bring_up_master(master, NULL); + mutex_unlock(&master->mutex);
- if (ret < 0) { - /* Delete off the list if we weren't successful */ - list_del(&master->node); - kfree(master); - } - mutex_unlock(&component_mutex); + /* Delete off the list if we weren't successful */ + if (ret < 0) + kref_put_mutex(&master->kref, master_release, &master_mutex);
return ret < 0 ? ret : 0; } @@ -338,15 +423,17 @@ void component_master_del(struct device *dev, { struct master *master;
- mutex_lock(&component_mutex); + mutex_lock(&master_mutex); master = __master_find(dev, ops); + if (master) + master->dead = true; + mutex_unlock(&master_mutex); + if (master) { take_down_master(master);
- list_del(&master->node); - kfree(master); + kref_put_mutex(&master->kref, master_release, &master_mutex); } - mutex_unlock(&component_mutex); } EXPORT_SYMBOL_GPL(component_master_del);
@@ -364,12 +451,9 @@ static void component_unbind(struct component *component,
void component_unbind_all(struct device *master_dev, void *data) { - struct master *master; + struct master *master = master_find_locked(master_dev, NULL); struct component *c;
- WARN_ON(!mutex_is_locked(&component_mutex)); - - master = __master_find(master_dev, NULL); if (!master) return;
@@ -432,13 +516,10 @@ static int component_bind(struct component *component, struct master *master,
int component_bind_all(struct device *master_dev, void *data) { - struct master *master; + struct master *master = master_find_locked(master_dev, NULL); struct component *c; int ret = 0;
- WARN_ON(!mutex_is_locked(&component_mutex)); - - master = __master_find(master_dev, NULL); if (!master) return -EINVAL;
@@ -474,14 +555,16 @@ int component_add(struct device *dev, const struct component_ops *ops)
mutex_lock(&component_mutex); list_add_tail(&component->node, &component_list); + mutex_unlock(&component_mutex);
ret = try_to_bring_up_masters(component); if (ret < 0) { + mutex_lock(&component_mutex); list_del(&component->node); + mutex_unlock(&component_mutex);
kfree(component); } - mutex_unlock(&component_mutex);
return ret < 0 ? ret : 0; } @@ -490,21 +573,28 @@ EXPORT_SYMBOL_GPL(component_add); void component_del(struct device *dev, const struct component_ops *ops) { struct component *c, *component = NULL; + struct master *master = NULL;
mutex_lock(&component_mutex); list_for_each_entry(c, &component_list, node) if (c->dev == dev && c->ops == ops) { + master = c->master; + if (master) + kref_get(&master->kref); list_del(&c->node); component = c; break; } + mutex_unlock(&component_mutex);
- if (component && component->master) - take_down_master(component->master); + if (WARN_ON(!component)) + return;
- mutex_unlock(&component_mutex); + if (master) { + take_down_master(master); + kref_put_mutex(&master->kref, master_release, &master_mutex); + }
- WARN_ON(!component); kfree(component); } EXPORT_SYMBOL_GPL(component_del);
On Wed, Oct 21, 2015 at 10:04:15PM +0100, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 10:37:27PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 20:19:38 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 07:59:06PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 19:34:37 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 09:49:28PM +0530, Vinod Koul wrote:
On Wed, Oct 21, 2015 at 03:37:47PM +0100, Russell King - ARM Linux wrote: > In any case, this doesn't (and can't) solve the CEC problem, so it's not > a solution to the problem at hand.
Sorry am not sure I follow the reasons for that, wouldn't CEC be another slave in such an interface? I though component fwk did allow us to have multiple slaves..
Not with the way you're using the component helper here.
I guess that not all my message is being read, because people keep replying half-way down my messages...
You can only register a struct device _once_ as a slave device.
With the way you're using it here for audio, you're registering the i915 DRM device as a slave component device, and the audio side as the master. That means the audio master can bind to the DRM slave component device.
You can't then have a CEC master bind to the i915 DRM slave device (it's already bound to the audio master device), and you can't register the i915 DRM device as a second slave component device. It becomes indistinguishable from the first, and there's no way to tell which of the two different 'ops' structures should be used with which master.
I said this in my message 20151021140307.GE32532@n2100.arm.linux.org.uk which was two of my replies ago in this sub-thread.
Can't the limitation of single slave dev be extended simply? e.g. add some matching semantics to component_master_add_child() like a shared key in both master and slave, and let assign only the matched slave.
I've already explained that in the email message I referred to by message ID above (which was a reply to one of your previous messages)
This is why I find email such a poor communication medium - I'm quite sure most people only read half an email message before hitting reply, and then they stick their reply after where they stopped reading and never bother reading the rest of the message. Normally, at this point, I'll start getting grumpy and sending frustrated emails... which would eventually deride into flames, but let's _try_ to keep this civil.
Thank you for patience!
Here's the relevant paragraph from the above referenced message, and to make the appropriate bit stand out, I've underlined it with ^.
However, there's a lurking problem: you can't register the same struct device as a slave more than once into the component helpers - that's because it's designed to look for devices. There's intentionally no
^^^^^^^^^^^^^^^^^^^^^^^^linkage between the slave ops and master ops to allow for generic
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
slave drivers (like tda998x) to be used with different master drivers
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(armada, tilcdc, etc). In theory, you can register the master device
^^^^^^^^^^^^^^^^^^^^^
of one componentised system as a slave device of another, but that requires a much more complicated locking setup in component.c (I have patches for that, but I've resisted sending them because it makes the locking very messy.)
In a subsequent sentence, I also address the issue that would occur if any of the already componentised DRM drivers were to attempt what you're doing in i915 - although I say it's solvable, I've resisted that because it makes the locking in there _much_ more hairy, and I'm now not certain that my more complex locking implementation would even cope with that scenario.
I understood that the original component design wasn't for cross subsystems. OTOH I wondered why it can't be extended for wider use -- that was the simple question; I haven't seen so complex locking in some upstream drm drivers code through a quick glance, so the mess about locking you mentioned wasn't clear to me. Now point taken.
You can find my patch for the "more complex locking" problem at the end of this mail. This may not apply to the current version of component.c, but illustrates at least some of the problem of getting rid of the global component_mutex lock, which would be necessary to allow a binding master to call back into the component helper to then register a slave. Today, that action would deadlock on component_mutex.
I'm not actually convinced that even this patch is correct as it stands...
To be noted, a weak dependency is still necessary for i915 audio case, and a simple notifier type registration won't work, unfortunately. GPU power adjustment is mandatory even at probing the audio hardware to judge whether HDMI codec is present or not. Meanwhile the dependency to the graphics must not be tight, either. The very same audio driver is for a controller without graphics, too. That's the reason we took the component as an alternative implementation, which is a bit cleaner than the direct symbol resolving in the earlier code.
So, if any better solution that covers a use case like i915/hda is present, we're willing to switch. As repeatedly written, the current implementation is a stop gap. Although this doesn't look too bad, there are still some obvious pitfalls as you pointed out. We can paper over them (maybe I'll do for 4.4), but a fundamentally better solution would be more welcome, of course.
Well, I guess it's fine as a temporary stop-gap, but what I don't want to see is this stop-gap becoming more common. It may work for your i915 case, but it won't work everywhere, so it can't become a generic solution to this problem.
My gut feeling is that component isn't the right framework for something generic. It's really good to build something very specific (and i915.ko binding to the i915-specific side of snd-hda is such a case I think). But for a generic audio-over-hdmi framework I think we need to have proper abstraction at that level, with a set of "give me the hdmi endpoint I want" functions for the audio side, and corresponding register functions for the gfx side. Similar to how you get your gpios, regulators and whatever else. -Daniel
drivers/base/component.c | 142 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 116 insertions(+), 26 deletions(-)
diff --git a/drivers/base/component.c b/drivers/base/component.c index f748430bb654..d72fe9bc7033 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -30,7 +30,10 @@ struct component_match { struct master { struct list_head node; struct list_head components;
struct kref kref;
struct mutex mutex; bool bound;
bool dead;
const struct component_master_ops *ops; struct device *dev;
@@ -49,8 +52,33 @@ struct component {
static DEFINE_MUTEX(component_mutex); static LIST_HEAD(component_list); +static DEFINE_MUTEX(master_mutex); static LIST_HEAD(masters);
+static void master_release(struct kref *kref) __releases(master_mutex) +{
- struct master *master = container_of(kref, struct master, kref);
- list_del(&master->node);
- mutex_unlock(&master_mutex);
- WARN_ON(!list_empty(&master->components));
- kfree(master);
+}
+static void master_release_nolock(struct kref *kref) +{
- struct master *master = container_of(kref, struct master, kref);
- list_del(&master->node);
- WARN_ON(!list_empty(&master->components));
- kfree(master);
+}
+static void master_norelease(struct kref *kref) +{
- WARN_ON(kref);
+}
static struct master *__master_find(struct device *dev, const struct component_master_ops *ops) { @@ -63,9 +91,30 @@ static struct master *__master_find(struct device *dev, return NULL; }
+/*
- Find the master structure for this device, and make sure that the
- per-master lock is held. This is used from within the master device
- drivers bind and unbind callbacks, which should only be called from
- paths where the per-master lock is already held.
- */
+static struct master *master_find_locked(struct device *dev,
- const struct component_master_ops *ops)
+{
- struct master *master;
- mutex_lock(&master_mutex);
- master = __master_find(dev, NULL);
- WARN_ON(master && !mutex_is_locked(&master->mutex));
- mutex_unlock(&master_mutex);
- return master;
+}
/* Attach an unattached component to a master. */ static void component_attach_master(struct master *master, struct component *c) {
kref_get(&master->kref);
c->master = master;
list_add_tail(&c->master_node, &master->components);
@@ -77,6 +126,9 @@ static void component_detach_master(struct master *master, struct component *c) list_del(&c->master_node);
c->master = NULL;
- /* This kref_put should never release the master */
- kref_put(&master->kref, master_norelease);
}
/* @@ -90,6 +142,7 @@ int component_master_add_child(struct master *master, struct component *c; int ret = -ENXIO;
- mutex_lock(&component_mutex); list_for_each_entry(c, &component_list, node) { if (c->master && c->master != master) continue;
@@ -101,6 +154,7 @@ int component_master_add_child(struct master *master, break; } }
mutex_unlock(&component_mutex);
return ret;
} @@ -137,6 +191,7 @@ static int find_components(struct master *master) /* Detach all attached components from this master */ static void master_remove_components(struct master *master) {
- mutex_lock(&component_mutex); while (!list_empty(&master->components)) { struct component *c = list_first_entry(&master->components, struct component, master_node);
@@ -145,6 +200,7 @@ static void master_remove_components(struct master *master)
component_detach_master(master, c);}
- mutex_unlock(&component_mutex);
}
/* @@ -201,20 +257,43 @@ static int try_to_bring_up_master(struct master *master,
static int try_to_bring_up_masters(struct component *component) {
- struct master *m;
struct master *m, *last = NULL; int ret = 0;
mutex_lock(&master_mutex); list_for_each_entry(m, &masters, node) {
if (last) {kref_put(&last->kref, master_release_nolock);last = NULL;}if (m->dead || m->bound)continue;kref_get(&m->kref);mutex_unlock(&master_mutex);mutex_lock(&m->mutex);ret = try_to_bring_up_master(m, component);
mutex_unlock(&m->mutex);mutex_lock(&master_mutex);last = m;if (ret != 0) break; }
if (last)
kref_put(&last->kref, master_release_nolock);mutex_unlock(&master_mutex);
return ret;
}
static void take_down_master(struct master *master) {
- mutex_lock(&master->mutex); if (master->bound) { master->ops->unbind(master->dev); devres_release_group(master->dev, NULL);
@@ -222,6 +301,7 @@ static void take_down_master(struct master *master) }
master_remove_components(master);
- mutex_unlock(&master->mutex);
}
static size_t component_match_size(size_t num) @@ -307,20 +387,25 @@ int component_master_add_with_match(struct device *dev, master->dev = dev; master->ops = ops; master->match = match;
- mutex_init(&master->mutex); INIT_LIST_HEAD(&master->components);
- kref_init(&master->kref);
- /* Add to the list of available masters. */
- mutex_lock(&component_mutex);
/*
* Add to the list of available masters. This is so* the component code below can find this master.*/mutex_lock(&master_mutex); list_add(&master->node, &masters);
mutex_unlock(&master_mutex);
mutex_lock(&master->mutex); ret = try_to_bring_up_master(master, NULL);
mutex_unlock(&master->mutex);
- if (ret < 0) {
/* Delete off the list if we weren't successful */list_del(&master->node);kfree(master);- }
- mutex_unlock(&component_mutex);
/* Delete off the list if we weren't successful */
if (ret < 0)
kref_put_mutex(&master->kref, master_release, &master_mutex);return ret < 0 ? ret : 0;
} @@ -338,15 +423,17 @@ void component_master_del(struct device *dev, { struct master *master;
- mutex_lock(&component_mutex);
- mutex_lock(&master_mutex); master = __master_find(dev, ops);
- if (master)
master->dead = true;- mutex_unlock(&master_mutex);
- if (master) { take_down_master(master);
list_del(&master->node);kfree(master);
kref_put_mutex(&master->kref, master_release, &master_mutex);
- mutex_unlock(&component_mutex);
} EXPORT_SYMBOL_GPL(component_master_del);
@@ -364,12 +451,9 @@ static void component_unbind(struct component *component,
void component_unbind_all(struct device *master_dev, void *data) {
- struct master *master;
- struct master *master = master_find_locked(master_dev, NULL); struct component *c;
- WARN_ON(!mutex_is_locked(&component_mutex));
- master = __master_find(master_dev, NULL); if (!master) return;
@@ -432,13 +516,10 @@ static int component_bind(struct component *component, struct master *master,
int component_bind_all(struct device *master_dev, void *data) {
- struct master *master;
- struct master *master = master_find_locked(master_dev, NULL); struct component *c; int ret = 0;
- WARN_ON(!mutex_is_locked(&component_mutex));
- master = __master_find(master_dev, NULL); if (!master) return -EINVAL;
@@ -474,14 +555,16 @@ int component_add(struct device *dev, const struct component_ops *ops)
mutex_lock(&component_mutex); list_add_tail(&component->node, &component_list);
mutex_unlock(&component_mutex);
ret = try_to_bring_up_masters(component); if (ret < 0) {
mutex_lock(&component_mutex);list_del(&component->node);
mutex_unlock(&component_mutex);kfree(component); }
mutex_unlock(&component_mutex);
return ret < 0 ? ret : 0;
} @@ -490,21 +573,28 @@ EXPORT_SYMBOL_GPL(component_add); void component_del(struct device *dev, const struct component_ops *ops) { struct component *c, *component = NULL;
struct master *master = NULL;
mutex_lock(&component_mutex); list_for_each_entry(c, &component_list, node) if (c->dev == dev && c->ops == ops) {
master = c->master;if (master)kref_get(&master->kref); list_del(&c->node); component = c; break;}
mutex_unlock(&component_mutex);
- if (component && component->master)
take_down_master(component->master);
- if (WARN_ON(!component))
return;
- mutex_unlock(&component_mutex);
- if (master) {
take_down_master(master);kref_put_mutex(&master->kref, master_release, &master_mutex);- }
- WARN_ON(!component); kfree(component);
} EXPORT_SYMBOL_GPL(component_del);
-- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Oct 16, 2015 at 02:31:43PM +0200, Lars-Peter Clausen wrote:
On 10/16/2015 01:50 PM, Jyri Sarha wrote:
My conclusion from the Lars-Peter's latest mail [2] related to the subject is that the wind is currently blowing slightly in favour of my version of the hdmi codec [3], or at least not in favour of Arnaud's version [4].
So how should we proceed? Are we still waiting for some feedback from DRM/video side?
That too, though given that they have never commented on these serieses at all as far as I remember and are generally not very vocal IME I'm probably OK with just applying something.
What needs to happen is that everybody who is working on HDMI audio support should get their heads together and come up with a common solution that works for everybody, rather than having everybody trying to push their own solution and put the maintainer in the spotlight of having to choose one (Mark has been asking for this for the past half year or so).
Longer, I think. In this context things like the work that Russell has done with the EDID handling is really great - making common code that other people are able to adopt and use, and where I can see that there's reasonably widespread buy in. There was some debate about the differences between your patch set and the very similar patch set that Arnaud posted which was good to see but it felt like that petered out rather than drew to a conclusion, I'd at least expect to see a new version appear that the pair of you can hopefully agree on or some active statement that you both support one or the other version that's there now.
It's one thing if there's unresolvable differences that someone just needs to take a call on but right now it just feels like it's more that there's a lack of engagement. What I don't want to do is merge something and then find a few months later that it needs big reworks (as opposed to extensions) because it's missed some important things that are a problem for large classes of hardware. Having separate approaches for completely different classes of hardware would be fine I think but we need to understand what they are and ensure that as far as possible the user experience outside the kernel is consistent.
On Fri, Oct 16, 2015 at 02:31:43PM +0200, Lars-Peter Clausen wrote:
On 10/16/2015 01:50 PM, Jyri Sarha wrote:
After reading the ELCE Audio mini conf minutes [1] I gather that HDMI audio was not discussed after all.
It was discussed, but rather shortly and only in the context of the HDA. (Adding Vinod to Cc, who presented yet another approach to the problem last week)
Thanks for adding me :)
Based on the links sent by Jyri, I did look up the series from Jyri and Arnaud. I have tried to summarize the approach but please do correct me if I got something wrong
There are similarities but both are trying to add an interface between audio and display.
Jyri's approach to add generic IEC code makes sense and drives reuse. I kind of didn't like adding rates and formats for DAIs, if they are placeholders then it is okay but otherwise we should read and set them from ELD. Also I have reservations about hdmi_codec_ops and this being set by drm driver, why not use component interface for these things and let data be shared!
Arnaud's approach to add N,CTS helpers is great. But having calls to drm from audio side for drm enabling wouldn't have been required if we have component interface here.
If we have these calls on a component interface and use these as generic ops for the audio-drm interface rather than i915 which is implemented today we don't need to export symbols and create a dependency between these two side.
Also I would again like you folks to review the HDA HDMI ASoC driver RFC I posted recently [1]. We use component interface and have been adding more callback to interface. David added notification [2] and we plan to add ELD query as well soonish.
[1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/098730.htm... [2]: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/097042.html
On 10/18/15 18:02, Vinod Koul wrote:
Jyri's approach to add generic IEC code makes sense and drives reuse. I kind of didn't like adding rates and formats for DAIs, if they are placeholders then it is okay but otherwise we should read and set them from ELD. Also I
The idea is to provide all possible formats and rates to the ASoC core at the DAI registration phase. Then at the stream startup the currently valid ELD is scanned and and ALSA constraints are set according to it (with snd_pcm_hw_constraint_eld()).
have reservations about hdmi_codec_ops and this being set by drm driver, why not use component interface for these things and let data be shared!
Russell already listed why the component interface is not good for registering optional subsystems. Any particular reason why passing function pointers over pdata should be considered bad? Can you suggest an alternative?
Best regards, Jyri
After reading the ELCE Audio mini conf minutes [1] I gather that HDMI audio was not discussed after all.
My conclusion from the Lars-Peter's latest mail [2] related to the subject is that the wind is currently blowing slightly in favour of my version of the hdmi codec [3], or at least not in favour of Arnaud's version [4].
Based on previous discussions and lack of feedback from DRM community, This make sens for me.
So how should we proceed? Are we still waiting for some feedback from DRM/video side?
There was not too many clear suggestions to my latest patch series [3], so I do not see too much point in sending yet another version of that series.
For me, some points need to be clarified: - Do we need the abort callback. Is there a uses cases that makes it mandatory (some timeout mechanism already exists to stop audio...)
- Does trigger is needed when CPU-DAI is master on bus? Otherwise how to inform HDMI that I2S bus is no more clocked? I'm not sure that mute is sufficient for all hardwares... For instance for my hardware CTS parameter is hardware computed based on I2S L/R clock.
- IEC controls to support compress mode. This should be handled by codec driver in I2S mode, but not in SPDIF mode.
Arnaud, if I'd try to make a patch series that would implement sti HDMI audio with my HDMI codec, would you be willing try that out?
hmm...more simple that i do the stuff for sti platform as some pieces of code are missing today in kernel (Device tree part). I will try to find time next week to make a prototype for test and give you a feedback.
Additional point: We should also define some new helper functions:
- For N and CTS HDMI parameters: In my RFC i have proposed helper function, don't know if that matches with other platforms need...
- For IEC standard controls (could be added in core/pcm_iec958.c)
BR, Arnaud
On Fri, Oct 16, 2015 at 03:51:40PM +0200, Arnaud Pouliquen wrote:
- Does trigger is needed when CPU-DAI is master on bus?
Otherwise how to inform HDMI that I2S bus is no more clocked? I'm not sure that mute is sufficient for all hardwares... For instance for my hardware CTS parameter is hardware computed based on I2S L/R clock.
I'd expect us to be always calling trigger - is there some debate on this? Obviously since it's called in atomic context it can be difficult to use but we should be notifying.
On Fri, Oct 16, 2015 at 03:15:25PM +0100, Mark Brown wrote:
On Fri, Oct 16, 2015 at 03:51:40PM +0200, Arnaud Pouliquen wrote:
- Does trigger is needed when CPU-DAI is master on bus?
Otherwise how to inform HDMI that I2S bus is no more clocked? I'm not sure that mute is sufficient for all hardwares... For instance for my hardware CTS parameter is hardware computed based on I2S L/R clock.
I'd expect us to be always calling trigger - is there some debate on this? Obviously since it's called in atomic context it can be difficult to use but we should be notifying.
If the question is only on trigger being non atomic, we certainly allow that now, we can specify that by having dailinks's nonatomic flag as true
On 10/16/15 16:51, Arnaud Pouliquen wrote:
After reading the ELCE Audio mini conf minutes [1] I gather that HDMI audio was not discussed after all.
My conclusion from the Lars-Peter's latest mail [2] related to the subject is that the wind is currently blowing slightly in favour of my version of the hdmi codec [3], or at least not in favour of Arnaud's version [4].
Based on previous discussions and lack of feedback from DRM community, This make sens for me.
So how should we proceed? Are we still waiting for some feedback from DRM/video side?
There was not too many clear suggestions to my latest patch series [3], so I do not see too much point in sending yet another version of that series.
For me, some points need to be clarified:
- Do we need the abort callback. Is there a uses cases that makes it
mandatory (some timeout mechanism already exists to stop audio...)
I am not currently using the abort_cb my self, so it can be dropped as well. Is should not be needed for codec DAI implementations, as long as the CPU-DAI is the i2s master.
- Does trigger is needed when CPU-DAI is master on bus?
Otherwise how to inform HDMI that I2S bus is no more clocked? I'm not sure that mute is sufficient for all hardwares... For instance for my hardware CTS parameter is hardware computed based on I2S L/R clock.
The most of the codec drivers do not implement the trigger callback as the stream start and stop timing is usually handled at the CPU-DAI. Codec is just prepared (quite often even without prepare callback) and the stream start/stop is triggered at CPU DAI end. However, I should probably still implement the trigger callback and let the video side driver decide if it needs it or not.
- IEC controls to support compress mode. This should be handled by codec
driver in I2S mode, but not in SPDIF mode.
Arnaud, if I'd try to make a patch series that would implement sti HDMI audio with my HDMI codec, would you be willing try that out?
hmm...more simple that i do the stuff for sti platform as some pieces of code are missing today in kernel (Device tree part). I will try to find time next week to make a prototype for test and give you a feedback.
That would be even better. Just let me know I can help you in any way.
Additional point: We should also define some new helper functions:
- For N and CTS HDMI parameters: In my RFC i have proposed helper
function, don't know if that matches with other platforms need...
Absolutely, but I don't think these helpers should have any direct association to ASoC side. So they are in a way orthogonal to the ASoC side HDMI codec implementation. But very relevant to the video side driver registering the HDMI codec.
- For IEC standard controls (could be added in core/pcm_iec958.c)
Oh, I did not even realize that there are predefined special kcontrols for handling the status bits. Adding those sounds like right thing to do.
Cheers, Jyr
On Mon, Oct 19, 2015 at 04:10:32PM +0300, Jyri Sarha wrote:
On 10/16/15 16:51, Arnaud Pouliquen wrote:
- For IEC standard controls (could be added in core/pcm_iec958.c)
Oh, I did not even realize that there are predefined special kcontrols for handling the status bits. Adding those sounds like right thing to do.
And this is an example of stuff done properly: it's a library that drivers can opt into using if they so desire. It can be used with ALSA drivers, and it can be used with ASoC drivers too. Or, if a driver wants to do something more complex, it can opt not to use it at all, but make use of everything else. It can pick-and-choose. :)
participants (9)
-
Arnaud Pouliquen -
Daniel Vetter -
Jani Nikula -
Jyri Sarha -
Lars-Peter Clausen -
Mark Brown -
Russell King - ARM Linux -
Takashi Iwai -
Vinod Koul