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?

Quoted 2 lines · Jyri Sarha

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)

Quoted 2 lines · Jyri Sarha

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)

Quoted 9 lines

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).

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'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.

Quoted 4 lines · Russell King - ARM Linux

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

Quoted 8 lines · Vinod Koul

Quoted 4 lines · Russell King - ARM Linux

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: <[email protected]>

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.)

Quoted 18 lines · Russell King - ARM Linux

Quoted 8 lines · Vinod Koul

Quoted 4 lines · Russell King - ARM Linux

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: <[email protected]>

Quoted 7 lines

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...

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.

Quoted 2 lines · Vinod Koul

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.

Quoted 23 lines · Russell King - ARM Linux

Quoted 2 lines · Vinod Koul

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.

Quoted 4 lines

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.

Quoted 25 lines · Takashi Iwai

Quoted 23 lines · Russell King - ARM Linux

Quoted 2 lines · Vinod Koul

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.

Quoted 8 lines

Quoted 4 lines

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...

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 3 lines

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...

Quoted 2 lines

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

Quoted 13 lines · Vinod Koul

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 3 lines

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...

Quoted 5 lines

Quoted 2 lines

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...

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 3 lines

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...

Quoted 2 lines

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

Quoted 13 lines · Vinod Koul

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 3 lines

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...

Quoted 5 lines

Quoted 2 lines

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.)

Signature · 2 lines

FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Quoted 40 lines · Russell King - ARM Linux

Quoted 13 lines · Vinod Koul

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 3 lines

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...

Quoted 5 lines

Quoted 2 lines

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;
        }

Quoted 1 line

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.

Quoted 67 lines · Takashi Iwai

Quoted 40 lines · Russell King - ARM Linux

Quoted 13 lines · Vinod Koul

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 3 lines

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...

Quoted 5 lines

Quoted 2 lines

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;
        }

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 69 lines · Russell King - ARM Linux

Quoted 67 lines · Takashi Iwai

Quoted 40 lines · Russell King - ARM Linux

Quoted 13 lines · Vinod Koul

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 3 lines

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...

Quoted 5 lines

Quoted 2 lines

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;
        }

Quoted 1 line

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.

Quoted 6 lines

Quoted 3 lines

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 :)

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 94 lines · Takashi Iwai

Quoted 69 lines · Russell King - ARM Linux

Quoted 67 lines · Takashi Iwai

Quoted 40 lines · Russell King - ARM Linux

Quoted 13 lines · Vinod Koul

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 3 lines

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...

Quoted 5 lines

Quoted 2 lines

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;
        }

Quoted 1 line

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.

Quoted 6 lines

Quoted 3 lines

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 :)

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 3 lines

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).

Quoted 111 lines · Takashi Iwai

Quoted 94 lines · Takashi Iwai

Quoted 69 lines · Russell King - ARM Linux

Quoted 67 lines · Takashi Iwai

Quoted 40 lines · Russell King - ARM Linux

Quoted 13 lines · Vinod Koul

Quoted 12 lines · Russell King - ARM Linux

Quoted 11 lines

Quoted 3 lines

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...

Quoted 5 lines

Quoted 2 lines

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;
        }

Quoted 1 line

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.

Quoted 6 lines

Quoted 3 lines

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 :)

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 3 lines

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)) {

Quoted 2 lines

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.

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.

Quoted 4 lines · Takashi Iwai

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.

Quoted 2 lines · Russell King - ARM Linux

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 :)

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 1 line

I don't think this code has been thought through at all.

True, more hardening needed.

Quoted 20 lines · Takashi Iwai

Quoted 2 lines · Russell King - ARM Linux

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 :)

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

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.

Quoted 1 line

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.

Quoted 2 lines · Russell King - ARM Linux

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..

Quoted 7 lines · Vinod Koul

Quoted 2 lines · Russell King - ARM Linux

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 [email protected] which was two of my replies ago in this sub-thread.

Quoted 29 lines · Russell King - ARM Linux

Quoted 7 lines · Vinod Koul

Quoted 2 lines · Russell King - ARM Linux

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.

Quoted 2 lines

I said this in my message [email protected] 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.

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.)

Quoted 31 lines · Takashi Iwai

Quoted 29 lines · Russell King - ARM Linux

Quoted 7 lines · Vinod Koul

Quoted 2 lines · Russell King - ARM Linux

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.

Quoted 6 lines

Quoted 2 lines

I said this in my message [email protected] 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 ^.

Quoted 3 lines

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

                                              ^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

linkage between the slave ops and master ops to allow for generic

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

slave drivers (like tda998x) to be used with different master drivers

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

(armada, tilcdc, etc). In theory, you can register the master device

^^^^^^^^^^^^^^^^^^^^^

Quoted 4 lines

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.

Quoted 43 lines · Russell King - ARM Linux

Quoted 31 lines · Takashi Iwai

Quoted 29 lines · Russell King - ARM Linux

Quoted 7 lines · Vinod Koul

Quoted 2 lines · Russell King - ARM Linux

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.

Quoted 6 lines

Quoted 2 lines

I said this in my message [email protected] 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)

Quoted 6 lines

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!

Quoted 18 lines

Here's the relevant paragraph from the above referenced message, and to make the appropriate bit stand out, I've underlined it with ^.

Quoted 3 lines

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

                                              ^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

linkage between the slave ops and master ops to allow for generic

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

slave drivers (like tda998x) to be used with different master drivers

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

(armada, tilcdc, etc). In theory, you can register the master device

^^^^^^^^^^^^^^^^^^^^^

Quoted 4 lines

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.)

Quoted 6 lines

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.

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.

Quoted 79 lines · Takashi Iwai

Quoted 43 lines · Russell King - ARM Linux

Quoted 31 lines · Takashi Iwai

Quoted 29 lines · Russell King - ARM Linux

Quoted 7 lines · Vinod Koul

Quoted 2 lines · Russell King - ARM Linux

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.

Quoted 6 lines

Quoted 2 lines

I said this in my message [email protected] 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)

Quoted 6 lines

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!

Quoted 18 lines

Here's the relevant paragraph from the above referenced message, and to make the appropriate bit stand out, I've underlined it with ^.

Quoted 3 lines

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

                                              ^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

linkage between the slave ops and master ops to allow for generic

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

slave drivers (like tda998x) to be used with different master drivers

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

(armada, tilcdc, etc). In theory, you can register the master device

^^^^^^^^^^^^^^^^^^^^^

Quoted 4 lines

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.)

Quoted 6 lines

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.

Quoted 5 lines

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...

Quoted 10 lines

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.

Quoted 6 lines

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(-)

drivers/base/component.c +116 -26

index f748430bb654..d72fe9bc7033 100644
@@ -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);

Signature · 7 lines

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 7 lines · Jyri Sarha

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).

Quoted 2 lines · Jyri Sarha

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)

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!

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?

- 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.

Quoted 4 lines · Arnaud Pouliquen

- 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.

Quoted 7 lines

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.

Quoted 4 lines

So how should we proceed? Are we still waiting for some feedback from DRM/video side?

Quoted 3 lines

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.

Quoted 3 lines

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)

Quoted 2 lines · Arnaud Pouliquen

- 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.