HDMI codec, way forward?

HDMI codec, way forward?

[Jyri Sarha]

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.html
[2] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/098564.html
[3] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-September/097667.html
[4] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-October/098275.html

Re: HDMI codec, way forward?

[Lars-Peter Clausen]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDMI codec, way forward?

[Russell King - ARM Linux]

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.

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

Re: HDMI codec, way forward?

[Arnaud Pouliquen]

> 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

Re: HDMI codec, way forward?

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2293 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: HDMI codec, way forward?

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 904 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: HDMI codec, way forward?

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 517 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: HDMI codec, way forward?

[Vinod Koul]

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.html
[2]: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/097042.html

-- 
~Vinod

Re: HDMI codec, way forward?

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 773 bytes --]

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

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: HDMI codec, way forward?

[Vinod Koul]

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

-- 
~Vinod

Re: HDMI codec, way forward?

[Russell King - ARM Linux]

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

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

Re: HDMI codec, way forward?

[Vinod Koul]

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

-- 
~Vinod

Re: HDMI codec, way forward?

[Russell King - ARM Linux]

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.

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

Re: HDMI codec, way forward?

[Russell King - ARM Linux]

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.

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

Re: HDMI codec, way forward?

[Jyri Sarha]

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

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDMI codec, way forward?

[Russell King - ARM Linux]

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

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

Re: HDMI codec, way forward?

[Jyri Sarha]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDMI codec, way forward?

[Vinod Koul]

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

-- 
~Vinod

Re: [alsa-devel] HDMI codec, way forward?

[Russell King - ARM Linux]

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.

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

Re: HDMI codec, way forward?

[Vinod Koul]

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

-- 
~Vinod

Re: [alsa-devel] HDMI codec, way forward?

[Jani Nikula]

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.



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] HDMI codec, way forward?

[Russell King - ARM Linux]

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

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

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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.
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] HDMI codec, way forward?

[Russell King - ARM Linux]

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.

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

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDMI codec, way forward?

[Russell King - ARM Linux]

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.

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

Re: [alsa-devel] HDMI codec, way forward?

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDMI codec, way forward?

[Vinod Koul]

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

-- 
~Vinod

Re: HDMI codec, way forward?

[Vinod Koul]

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

-- 
~Vinod

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: HDMI codec, way forward?

[Russell King - ARM Linux]

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.

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

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] HDMI codec, way forward?

[Russell King - ARM Linux]

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.

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

Re: [alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] HDMI codec, way forward?

[Russell King - ARM Linux]

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


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

Re: [alsa-devel] HDMI codec, way forward?

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel