diff mbox

Oneiric: [PATCH] (UBUNTU: Sauce) Match jack input devices and pcm devices for HDMI

Message ID 4E5BA7BD.6000403@canonical.com
State New
Headers show

Commit Message

David Henningsson Aug. 29, 2011, 2:52 p.m. UTC
Upstream status for this patch is that Takashi went on vacation before 
he commented on the patch, and might not return before kernel freeze.

Anyway, it's needed for jack detection for HDMI to work. So I'd be happy 
if we could apply this for Oneiric (the jack detection patches are 
distro patches anyway), and then I'll have plenty of time to sort out 
with Takashi what to do for P.

Comments

Tim Gardner Aug. 29, 2011, 4:08 p.m. UTC | #1
On 08/29/2011 08:52 AM, David Henningsson wrote:
> Upstream status for this patch is that Takashi went on vacation before
> he commented on the patch, and might not return before kernel freeze.
>
> Anyway, it's needed for jack detection for HDMI to work. So I'd be happy
> if we could apply this for Oneiric (the jack detection patches are
> distro patches anyway), and then I'll have plenty of time to sort out
> with Takashi what to do for P.
>

While the patch looks fine, its difficult for me to ascertain the risk 
of regression. Has this patch had widespread testing? It appears to have 
the possibility for widespread impact. Without some really warm fuzzies 
from you (and a little data to back your claim), I'm inclined to let 
this go through the normal upstream process.

rtg
David Henningsson Aug. 30, 2011, 6:21 a.m. UTC | #2
On 08/29/2011 06:08 PM, Tim Gardner wrote:
> On 08/29/2011 08:52 AM, David Henningsson wrote:
>> Upstream status for this patch is that Takashi went on vacation before
>> he commented on the patch, and might not return before kernel freeze.
>>
>> Anyway, it's needed for jack detection for HDMI to work. So I'd be happy
>> if we could apply this for Oneiric (the jack detection patches are
>> distro patches anyway), and then I'll have plenty of time to sort out
>> with Takashi what to do for P.
>>
>
> While the patch looks fine, its difficult for me to ascertain the risk
> of regression. Has this patch had widespread testing? It appears to have
> the possibility for widespread impact.Without some really warm fuzzies
> from you (and a little data to back your claim), I'm inclined to let
> this go through the normal upstream process.

I'm not sure exactly what "warm fuzzies" you need, but currently:

* It has not had widespread testing. I could obviously test it on all 
machines I have here, something I should probably do anyway, but would 
that be enough for you?

* The upstream version of this patch, for Linux 3.2 (which is slightly 
different, this is a backported version) has received review from 
Stephen Warren@Nvidia, who has recently refactored the HDMI audio 
driver. He believes the patch looks good, but was unsure if he wanted 
more reformatting of the string at the same time.
Tim Gardner Aug. 30, 2011, 1:48 p.m. UTC | #3
On 08/30/2011 12:21 AM, David Henningsson wrote:
> On 08/29/2011 06:08 PM, Tim Gardner wrote:
>> On 08/29/2011 08:52 AM, David Henningsson wrote:
>>> Upstream status for this patch is that Takashi went on vacation before
>>> he commented on the patch, and might not return before kernel freeze.
>>>
>>> Anyway, it's needed for jack detection for HDMI to work. So I'd be happy
>>> if we could apply this for Oneiric (the jack detection patches are
>>> distro patches anyway), and then I'll have plenty of time to sort out
>>> with Takashi what to do for P.
>>>
>>
>> While the patch looks fine, its difficult for me to ascertain the risk
>> of regression. Has this patch had widespread testing? It appears to have
>> the possibility for widespread impact.Without some really warm fuzzies
>> from you (and a little data to back your claim), I'm inclined to let
>> this go through the normal upstream process.
>
> I'm not sure exactly what "warm fuzzies" you need, but currently:
>
> * It has not had widespread testing. I could obviously test it on all
> machines I have here, something I should probably do anyway, but would
> that be enough for you?
>
> * The upstream version of this patch, for Linux 3.2 (which is slightly
> different, this is a backported version) has received review from
> Stephen Warren@Nvidia, who has recently refactored the HDMI audio
> driver. He believes the patch looks good, but was unsure if he wanted
> more reformatting of the string at the same time.
>

So, you're asking to have a patch applied that has general impact on all 
platforms with an HDMI port, which by your own admission has not had 
widespread testing, and has not been agreed to or fully reviewed by 
upstream maintainers.

The kernel is in Beta freeze right now. What would your call be if you 
were in the same position?

rtg
David Henningsson Aug. 31, 2011, 7:18 a.m. UTC | #4
On 08/30/2011 03:48 PM, Tim Gardner wrote:
> On 08/30/2011 12:21 AM, David Henningsson wrote:
>> On 08/29/2011 06:08 PM, Tim Gardner wrote:
>>> On 08/29/2011 08:52 AM, David Henningsson wrote:
>>>> Upstream status for this patch is that Takashi went on vacation before
>>>> he commented on the patch, and might not return before kernel freeze.
>>>>
>>>> Anyway, it's needed for jack detection for HDMI to work. So I'd be
>>>> happy
>>>> if we could apply this for Oneiric (the jack detection patches are
>>>> distro patches anyway), and then I'll have plenty of time to sort out
>>>> with Takashi what to do for P.
>>>>
>>>
>>> While the patch looks fine, its difficult for me to ascertain the risk
>>> of regression. Has this patch had widespread testing? It appears to have
>>> the possibility for widespread impact.Without some really warm fuzzies
>>> from you (and a little data to back your claim), I'm inclined to let
>>> this go through the normal upstream process.
>>
>> I'm not sure exactly what "warm fuzzies" you need, but currently:
>>
>> * It has not had widespread testing. I could obviously test it on all
>> machines I have here, something I should probably do anyway, but would
>> that be enough for you?
>>
>> * The upstream version of this patch, for Linux 3.2 (which is slightly
>> different, this is a backported version) has received review from
>> Stephen Warren@Nvidia, who has recently refactored the HDMI audio
>> driver. He believes the patch looks good, but was unsure if he wanted
>> more reformatting of the string at the same time.
>>
>
> So, you're asking to have a patch applied that has general impact on all
> platforms with an HDMI port, which by your own admission has not had
> widespread testing, and has not been agreed to or fully reviewed by
> upstream maintainers.
>
> The kernel is in Beta freeze right now. What would your call be if you
> were in the same position?

If I were in the same position:

First and foremost, I would start with a warm, welcoming and encouraging 
attitude towards patches. A patch sent here, means someone not only 
wants to improve Linux in general, but also cares in special for Ubuntu, 
and has taken the time and effort to backport a patch to suit Ubuntu's 
particular kernel version. In this case, the patch author is a Canonical 
employee so this is not surprising, but nevertheless, this is a really 
good thing and should be encouraged.

With that positive attitude in mind, I would do a review. The review 
would, of course, take regression risk into account - a kernel bug can 
cause the system to fail in the most horrifying ways - but also weigh 
that against the possible positive outcomes of applying the patch, i e 
why it's needed in the first place. If I'm not qualified to make a 
review, I would find someone who is, to make the opinion for me.

The review in this case would notice that it impacts most people with 
working HDMI audio which means both higher risk and gain. OTOH, the code 
path is simple, which means that successful testing on one machine would 
make it unlikely to fail on another.

If I'm still unsure of regressions even after having done the review, I 
would make sure testing of the patch is done. If I have time, I do the 
testing myself, or if I don't, ask for someone to test it for me, but do 
what I can to help, e g by building a test kernel and provide 
instructions as of how to do the test. If testing with positive result 
is all that's needed before applying the patch, I would communicate that 
clearly, among with a deadline explaining when the testing must be 
completed.
Leann Ogasawara Aug. 31, 2011, 9:35 p.m. UTC | #5
On Wed, 2011-08-31 at 09:18 +0200, David Henningsson wrote:
> On 08/30/2011 03:48 PM, Tim Gardner wrote:
[...]
> > So, you're asking to have a patch applied that has general impact on all
> > platforms with an HDMI port, which by your own admission has not had
> > widespread testing, and has not been agreed to or fully reviewed by
> > upstream maintainers.
> >
> > The kernel is in Beta freeze right now. What would your call be if you
> > were in the same position?
> 
> If I were in the same position:
> 
> First and foremost, I would start with a warm, welcoming and encouraging 
> attitude towards patches. A patch sent here, means someone not only 
> wants to improve Linux in general, but also cares in special for Ubuntu, 
> and has taken the time and effort to backport a patch to suit Ubuntu's 
> particular kernel version. In this case, the patch author is a Canonical 
> employee so this is not surprising, but nevertheless, this is a really 
> good thing and should be encouraged.

Let me just start by saying that we really appreciate all the work that
you've done with regards to the audio subsystem.  Your interaction with
upstream and the bug fixes you've provided for Ubuntu have been much
appreciated and I encourage you to keep up the good work.

> With that positive attitude in mind, I would do a review. The review 
> would, of course, take regression risk into account - a kernel bug can 
> cause the system to fail in the most horrifying ways - but also weigh 
> that against the possible positive outcomes of applying the patch, i e 
> why it's needed in the first place. If I'm not qualified to make a 
> review, I would find someone who is, to make the opinion for me.

Because we do consider you one of our knowledge experts in the audio
area, I believe Tim was wanting a bit more reassurance and confidence
from you with regards to the patch.  Having recognized the potential for
widespread impact, I do feel he was doing his job as one of the
maintainers of the Ubuntu kernel to make sure we're not introducing
regressions, especially being so close to Beta for Oneiric.

> The review in this case would notice that it impacts most people with 
> working HDMI audio which means both higher risk and gain. OTOH, the code 
> path is simple, which means that successful testing on one machine would 
> make it unlikely to fail on another.
> 
> If I'm still unsure of regressions even after having done the review, I 
> would make sure testing of the patch is done. If I have time, I do the 
> testing myself, or if I don't, ask for someone to test it for me, but do 
> what I can to help, e g by building a test kernel and provide 
> instructions as of how to do the test. If testing with positive result 
> is all that's needed before applying the patch, I would communicate that 
> clearly, among with a deadline explaining when the testing must be 
> completed.

I've built a test kernel with your patch applied and placed it at the
following location:

http://people.canonical.com/~ogasawara/diwic/

If you can, could you please test and confirm this resolves the issue at
hand and also doesn't introduce any regressions on a few existing
systems with working HDMI audio (whatever access to systems you have on
hand would suffice).  Please keep in mind that Oneiric Kernel Freeze is
Thurs Sept 15, so if I could get your feedback before then I'd be happy
to try and get this applied before we hit the freeze date.

Thanks in advance,
Leann
David Henningsson Sept. 2, 2011, 3:21 p.m. UTC | #6
On 08/31/2011 11:35 PM, Leann Ogasawara wrote:
> On Wed, 2011-08-31 at 09:18 +0200, David Henningsson wrote:
>> On 08/30/2011 03:48 PM, Tim Gardner wrote:
> [...]
>>> So, you're asking to have a patch applied that has general impact on all
>>> platforms with an HDMI port, which by your own admission has not had
>>> widespread testing, and has not been agreed to or fully reviewed by
>>> upstream maintainers.
>>>
>>> The kernel is in Beta freeze right now. What would your call be if you
>>> were in the same position?
>>
>> If I were in the same position:
>>
>> First and foremost, I would start with a warm, welcoming and encouraging
>> attitude towards patches. A patch sent here, means someone not only
>> wants to improve Linux in general, but also cares in special for Ubuntu,
>> and has taken the time and effort to backport a patch to suit Ubuntu's
>> particular kernel version. In this case, the patch author is a Canonical
>> employee so this is not surprising, but nevertheless, this is a really
>> good thing and should be encouraged.
>
> Let me just start by saying that we really appreciate all the work that
> you've done with regards to the audio subsystem.  Your interaction with
> upstream and the bug fixes you've provided for Ubuntu have been much
> appreciated and I encourage you to keep up the good work.
>
>> With that positive attitude in mind, I would do a review. The review
>> would, of course, take regression risk into account - a kernel bug can
>> cause the system to fail in the most horrifying ways - but also weigh
>> that against the possible positive outcomes of applying the patch, i e
>> why it's needed in the first place. If I'm not qualified to make a
>> review, I would find someone who is, to make the opinion for me.
>
> Because we do consider you one of our knowledge experts in the audio
> area, I believe Tim was wanting a bit more reassurance and confidence
> from you with regards to the patch.  Having recognized the potential for
> widespread impact, I do feel he was doing his job as one of the
> maintainers of the Ubuntu kernel to make sure we're not introducing
> regressions, especially being so close to Beta for Oneiric.
>
>> The review in this case would notice that it impacts most people with
>> working HDMI audio which means both higher risk and gain. OTOH, the code
>> path is simple, which means that successful testing on one machine would
>> make it unlikely to fail on another.
>>
>> If I'm still unsure of regressions even after having done the review, I
>> would make sure testing of the patch is done. If I have time, I do the
>> testing myself, or if I don't, ask for someone to test it for me, but do
>> what I can to help, e g by building a test kernel and provide
>> instructions as of how to do the test. If testing with positive result
>> is all that's needed before applying the patch, I would communicate that
>> clearly, among with a deadline explaining when the testing must be
>> completed.
>
> I've built a test kernel with your patch applied and placed it at the
> following location:
>
> http://people.canonical.com/~ogasawara/diwic/
>
> If you can, could you please test and confirm this resolves the issue at
> hand and also doesn't introduce any regressions on a few existing
> systems with working HDMI audio (whatever access to systems you have on
> hand would suffice).  Please keep in mind that Oneiric Kernel Freeze is
> Thurs Sept 15, so if I could get your feedback before then I'd be happy
> to try and get this applied before we hit the freeze date.

Hi Leann and thank you for your kind words and clear instructions! :-)

Here are the testing results (where 3.0.0-10 means your patched and 
built test kernel installed, and 3.0.0-9 is what comes with 11.10 beta 1) :

Radeon HD 4200 series:

3.0.0-9, radeon driver: no jack input device, no working HDMI audio.
3.0.0-10, radeon driver: no jack input device, no working HDMI audio.
3.0.0-9, fglrx driver: no jack input device, working HDMI audio.
3.0.0-10, fglrx driver: no jack input device, working HDMI audio.

Note: Since there is no jack input device, the code path is not run and 
my patch does not make any difference. Might look into that later (why 
there isn't a jack input device), but that is not on top of the priority 
list ATM.

Intel Arrandale:

3.0.0-9: one jack input device "HDA Intel HDMI/DP" (plugin working, 
unplug does not work), working HDMI audio.
3.0.0-10: one jack input device "HDA Intel HDMI/DP,pcm=3" (plugin 
working but compiz crashed at one point, unplug does not work), working 
HDMI audio.

Note: Patch working as jack input device name changes successfully. The 
compiz crash (reported as bug 839468) was a little strange, so I retried 
but could not reproduce the crash under either kernel. I don't think it 
has anything to do with the HDMI audio patch.

Nvidia GT 430:

3.0.0-9, nouveau driver: four jack input devices "HDA NVidia HDMI/DP" 
(none of them working), no working HDMI audio on any of the four PCMs.
3.0.0-10, nouveau driver: four jack input devices "HDA NVidia 
HDMI/DP,pcm=3", "HDA NVidia HDMI/DP,pcm=7", "HDA NVidia HDMI/DP,pcm=8", 
"HDA NVidia HDMI/DP,pcm=9" (none of them working), no working HDMI audio 
on any of the four PCMs.
3.0.0-10, nvidia driver: four jack input devices "HDA NVidia 
HDMI/DP,pcm=3", "HDA NVidia HDMI/DP,pcm=7", "HDA NVidia HDMI/DP,pcm=8", 
"HDA NVidia HDMI/DP,pcm=9". The pcm=9 indicates working HDMI audio for 
plughw:NVidia,9 (or hdmi:NVidia,3). \o/

Note: I spent four hours trying to install nvidia binary driver before I 
succeeded (thanks to Sarvatt, tjaalton, and tseliot for helping out), 
and one more hour trying to install it under 3.0.0.9.

Final conclusion: patch works as expected and no regressions found.
Leann Ogasawara Sept. 2, 2011, 5:49 p.m. UTC | #7
On Fri, 2011-09-02 at 17:21 +0200, David Henningsson wrote:
> On 08/31/2011 11:35 PM, Leann Ogasawara wrote:
> > On Wed, 2011-08-31 at 09:18 +0200, David Henningsson wrote:
> >> On 08/30/2011 03:48 PM, Tim Gardner wrote:
> > [...]
> >>> So, you're asking to have a patch applied that has general impact on all
> >>> platforms with an HDMI port, which by your own admission has not had
> >>> widespread testing, and has not been agreed to or fully reviewed by
> >>> upstream maintainers.
> >>>
> >>> The kernel is in Beta freeze right now. What would your call be if you
> >>> were in the same position?
> >>
> >> If I were in the same position:
> >>
> >> First and foremost, I would start with a warm, welcoming and encouraging
> >> attitude towards patches. A patch sent here, means someone not only
> >> wants to improve Linux in general, but also cares in special for Ubuntu,
> >> and has taken the time and effort to backport a patch to suit Ubuntu's
> >> particular kernel version. In this case, the patch author is a Canonical
> >> employee so this is not surprising, but nevertheless, this is a really
> >> good thing and should be encouraged.
> >
> > Let me just start by saying that we really appreciate all the work that
> > you've done with regards to the audio subsystem.  Your interaction with
> > upstream and the bug fixes you've provided for Ubuntu have been much
> > appreciated and I encourage you to keep up the good work.
> >
> >> With that positive attitude in mind, I would do a review. The review
> >> would, of course, take regression risk into account - a kernel bug can
> >> cause the system to fail in the most horrifying ways - but also weigh
> >> that against the possible positive outcomes of applying the patch, i e
> >> why it's needed in the first place. If I'm not qualified to make a
> >> review, I would find someone who is, to make the opinion for me.
> >
> > Because we do consider you one of our knowledge experts in the audio
> > area, I believe Tim was wanting a bit more reassurance and confidence
> > from you with regards to the patch.  Having recognized the potential for
> > widespread impact, I do feel he was doing his job as one of the
> > maintainers of the Ubuntu kernel to make sure we're not introducing
> > regressions, especially being so close to Beta for Oneiric.
> >
> >> The review in this case would notice that it impacts most people with
> >> working HDMI audio which means both higher risk and gain. OTOH, the code
> >> path is simple, which means that successful testing on one machine would
> >> make it unlikely to fail on another.
> >>
> >> If I'm still unsure of regressions even after having done the review, I
> >> would make sure testing of the patch is done. If I have time, I do the
> >> testing myself, or if I don't, ask for someone to test it for me, but do
> >> what I can to help, e g by building a test kernel and provide
> >> instructions as of how to do the test. If testing with positive result
> >> is all that's needed before applying the patch, I would communicate that
> >> clearly, among with a deadline explaining when the testing must be
> >> completed.
> >
> > I've built a test kernel with your patch applied and placed it at the
> > following location:
> >
> > http://people.canonical.com/~ogasawara/diwic/
> >
> > If you can, could you please test and confirm this resolves the issue at
> > hand and also doesn't introduce any regressions on a few existing
> > systems with working HDMI audio (whatever access to systems you have on
> > hand would suffice).  Please keep in mind that Oneiric Kernel Freeze is
> > Thurs Sept 15, so if I could get your feedback before then I'd be happy
> > to try and get this applied before we hit the freeze date.
> 
> Hi Leann and thank you for your kind words and clear instructions! :-)
> 
> Here are the testing results (where 3.0.0-10 means your patched and 
> built test kernel installed, and 3.0.0-9 is what comes with 11.10 beta 1) :
> 
> Radeon HD 4200 series:
> 
> 3.0.0-9, radeon driver: no jack input device, no working HDMI audio.
> 3.0.0-10, radeon driver: no jack input device, no working HDMI audio.
> 3.0.0-9, fglrx driver: no jack input device, working HDMI audio.
> 3.0.0-10, fglrx driver: no jack input device, working HDMI audio.
> 
> Note: Since there is no jack input device, the code path is not run and 
> my patch does not make any difference. Might look into that later (why 
> there isn't a jack input device), but that is not on top of the priority 
> list ATM.
> 
> Intel Arrandale:
> 
> 3.0.0-9: one jack input device "HDA Intel HDMI/DP" (plugin working, 
> unplug does not work), working HDMI audio.
> 3.0.0-10: one jack input device "HDA Intel HDMI/DP,pcm=3" (plugin 
> working but compiz crashed at one point, unplug does not work), working 
> HDMI audio.
> 
> Note: Patch working as jack input device name changes successfully. The 
> compiz crash (reported as bug 839468) was a little strange, so I retried 
> but could not reproduce the crash under either kernel. I don't think it 
> has anything to do with the HDMI audio patch.
> 
> Nvidia GT 430:
> 
> 3.0.0-9, nouveau driver: four jack input devices "HDA NVidia HDMI/DP" 
> (none of them working), no working HDMI audio on any of the four PCMs.
> 3.0.0-10, nouveau driver: four jack input devices "HDA NVidia 
> HDMI/DP,pcm=3", "HDA NVidia HDMI/DP,pcm=7", "HDA NVidia HDMI/DP,pcm=8", 
> "HDA NVidia HDMI/DP,pcm=9" (none of them working), no working HDMI audio 
> on any of the four PCMs.
> 3.0.0-10, nvidia driver: four jack input devices "HDA NVidia 
> HDMI/DP,pcm=3", "HDA NVidia HDMI/DP,pcm=7", "HDA NVidia HDMI/DP,pcm=8", 
> "HDA NVidia HDMI/DP,pcm=9". The pcm=9 indicates working HDMI audio for 
> plughw:NVidia,9 (or hdmi:NVidia,3). \o/
> 
> Note: I spent four hours trying to install nvidia binary driver before I 
> succeeded (thanks to Sarvatt, tjaalton, and tseliot for helping out), 
> and one more hour trying to install it under 3.0.0.9.
> 
> Final conclusion: patch works as expected and no regressions found.

Thanks for the testing and the feedback.  I've gone ahead and applied
this to Oneiric.

Thanks,
Leann
David Henningsson Sept. 5, 2011, 1:04 p.m. UTC | #8
On 09/02/2011 07:49 PM, Leann Ogasawara wrote:
> On Fri, 2011-09-02 at 17:21 +0200, David Henningsson wrote:
>> On 08/31/2011 11:35 PM, Leann Ogasawara wrote:
>>> On Wed, 2011-08-31 at 09:18 +0200, David Henningsson wrote:
>>>> On 08/30/2011 03:48 PM, Tim Gardner wrote:
>>> [...]
>>>>> So, you're asking to have a patch applied that has general impact on all
>>>>> platforms with an HDMI port, which by your own admission has not had
>>>>> widespread testing, and has not been agreed to or fully reviewed by
>>>>> upstream maintainers.
>>>>>
>>>>> The kernel is in Beta freeze right now. What would your call be if you
>>>>> were in the same position?
>>>>
>>>> If I were in the same position:
>>>>
>>>> First and foremost, I would start with a warm, welcoming and encouraging
>>>> attitude towards patches. A patch sent here, means someone not only
>>>> wants to improve Linux in general, but also cares in special for Ubuntu,
>>>> and has taken the time and effort to backport a patch to suit Ubuntu's
>>>> particular kernel version. In this case, the patch author is a Canonical
>>>> employee so this is not surprising, but nevertheless, this is a really
>>>> good thing and should be encouraged.
>>>
>>> Let me just start by saying that we really appreciate all the work that
>>> you've done with regards to the audio subsystem.  Your interaction with
>>> upstream and the bug fixes you've provided for Ubuntu have been much
>>> appreciated and I encourage you to keep up the good work.
>>>
>>>> With that positive attitude in mind, I would do a review. The review
>>>> would, of course, take regression risk into account - a kernel bug can
>>>> cause the system to fail in the most horrifying ways - but also weigh
>>>> that against the possible positive outcomes of applying the patch, i e
>>>> why it's needed in the first place. If I'm not qualified to make a
>>>> review, I would find someone who is, to make the opinion for me.
>>>
>>> Because we do consider you one of our knowledge experts in the audio
>>> area, I believe Tim was wanting a bit more reassurance and confidence
>>> from you with regards to the patch.  Having recognized the potential for
>>> widespread impact, I do feel he was doing his job as one of the
>>> maintainers of the Ubuntu kernel to make sure we're not introducing
>>> regressions, especially being so close to Beta for Oneiric.
>>>
>>>> The review in this case would notice that it impacts most people with
>>>> working HDMI audio which means both higher risk and gain. OTOH, the code
>>>> path is simple, which means that successful testing on one machine would
>>>> make it unlikely to fail on another.
>>>>
>>>> If I'm still unsure of regressions even after having done the review, I
>>>> would make sure testing of the patch is done. If I have time, I do the
>>>> testing myself, or if I don't, ask for someone to test it for me, but do
>>>> what I can to help, e g by building a test kernel and provide
>>>> instructions as of how to do the test. If testing with positive result
>>>> is all that's needed before applying the patch, I would communicate that
>>>> clearly, among with a deadline explaining when the testing must be
>>>> completed.
>>>
>>> I've built a test kernel with your patch applied and placed it at the
>>> following location:
>>>
>>> http://people.canonical.com/~ogasawara/diwic/
>>>
>>> If you can, could you please test and confirm this resolves the issue at
>>> hand and also doesn't introduce any regressions on a few existing
>>> systems with working HDMI audio (whatever access to systems you have on
>>> hand would suffice).  Please keep in mind that Oneiric Kernel Freeze is
>>> Thurs Sept 15, so if I could get your feedback before then I'd be happy
>>> to try and get this applied before we hit the freeze date.
>>
>> Hi Leann and thank you for your kind words and clear instructions! :-)
>>
>> Here are the testing results (where 3.0.0-10 means your patched and
>> built test kernel installed, and 3.0.0-9 is what comes with 11.10 beta 1) :
>>
>> Radeon HD 4200 series:
>>
>> 3.0.0-9, radeon driver: no jack input device, no working HDMI audio.
>> 3.0.0-10, radeon driver: no jack input device, no working HDMI audio.
>> 3.0.0-9, fglrx driver: no jack input device, working HDMI audio.
>> 3.0.0-10, fglrx driver: no jack input device, working HDMI audio.
>>
>> Note: Since there is no jack input device, the code path is not run and
>> my patch does not make any difference. Might look into that later (why
>> there isn't a jack input device), but that is not on top of the priority
>> list ATM.
>>
>> Intel Arrandale:
>>
>> 3.0.0-9: one jack input device "HDA Intel HDMI/DP" (plugin working,
>> unplug does not work), working HDMI audio.
>> 3.0.0-10: one jack input device "HDA Intel HDMI/DP,pcm=3" (plugin
>> working but compiz crashed at one point, unplug does not work), working
>> HDMI audio.
>>
>> Note: Patch working as jack input device name changes successfully. The
>> compiz crash (reported as bug 839468) was a little strange, so I retried
>> but could not reproduce the crash under either kernel. I don't think it
>> has anything to do with the HDMI audio patch.
>>
>> Nvidia GT 430:
>>
>> 3.0.0-9, nouveau driver: four jack input devices "HDA NVidia HDMI/DP"
>> (none of them working), no working HDMI audio on any of the four PCMs.
>> 3.0.0-10, nouveau driver: four jack input devices "HDA NVidia
>> HDMI/DP,pcm=3", "HDA NVidia HDMI/DP,pcm=7", "HDA NVidia HDMI/DP,pcm=8",
>> "HDA NVidia HDMI/DP,pcm=9" (none of them working), no working HDMI audio
>> on any of the four PCMs.
>> 3.0.0-10, nvidia driver: four jack input devices "HDA NVidia
>> HDMI/DP,pcm=3", "HDA NVidia HDMI/DP,pcm=7", "HDA NVidia HDMI/DP,pcm=8",
>> "HDA NVidia HDMI/DP,pcm=9". The pcm=9 indicates working HDMI audio for
>> plughw:NVidia,9 (or hdmi:NVidia,3). \o/
>>
>> Note: I spent four hours trying to install nvidia binary driver before I
>> succeeded (thanks to Sarvatt, tjaalton, and tseliot for helping out),
>> and one more hour trying to install it under 3.0.0.9.
>>
>> Final conclusion: patch works as expected and no regressions found.
>
> Thanks for the testing and the feedback.  I've gone ahead and applied
> this to Oneiric.

Thanks! I saw you released it as well, excellent :-)
David Henningsson Sept. 21, 2011, 12:50 p.m. UTC | #9
On 08/29/2011 04:52 PM, David Henningsson wrote:
> Upstream status for this patch is that Takashi went on vacation before
> he commented on the patch, and might not return before kernel freeze.
>
> Anyway, it's needed for jack detection for HDMI to work. So I'd be happy
> if we could apply this for Oneiric (the jack detection patches are
> distro patches anyway), and then I'll have plenty of time to sort out
> with Takashi what to do for P.

FYI, a corresponding patch has now been committed for 3.2 [1], so this 
patch can be dropped in the P cycle (assuming that release will run 3.2 
or later).
diff mbox

Patch

From 658c18c6c7f232fe5ed18d93aeaa7fd7329a31c0 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Mon, 29 Aug 2011 16:43:31 +0200
Subject: [PATCH] ALSA: HDA: hdmi: Emit pcm device index for jack input devices

Needed for userspace to be able to match pcm devices and jack
input devices.

Backport for kernel v3.0.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/patch_hdmi.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index bd0ae69..1687183 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -917,13 +917,6 @@  static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 		return -E2BIG;
 	}
 
-	err = snd_hda_input_jack_add(codec, pin_nid,
-				     SND_JACK_VIDEOOUT, NULL);
-	if (err < 0)
-		return err;
-
-	hdmi_present_sense(codec, pin_nid, &spec->sink_eld[spec->num_pins]);
-
 	spec->pin[spec->num_pins] = pin_nid;
 	spec->num_pins++;
 
@@ -1088,12 +1081,42 @@  static int generic_hdmi_build_pcms(struct hda_codec *codec)
 	return 0;
 }
 
+static int generic_hdmi_build_jack(struct hda_codec *codec, int pin_idx)
+{
+	int err;
+	char hdmi_str[32];
+	struct hdmi_spec *spec = codec->spec;
+	int pin_nid = spec->pin[pin_idx];
+	int cvt_nid = spec->pin_cvt[pin_idx];
+	int cvt_idx, pcmdev;
+	
+	cvt_idx = hda_node_index(spec->cvt, cvt_nid);
+	if (cvt_idx < 0)
+		return cvt_idx;
+	pcmdev = spec->pcm_rec[cvt_idx].device;
+	snprintf(hdmi_str, sizeof(hdmi_str), "HDMI/DP,pcm=%d", pcmdev);
+
+	err = snd_hda_input_jack_add(codec, pin_nid,
+			     SND_JACK_VIDEOOUT, pcmdev > 0 ? hdmi_str : NULL);
+	if (err < 0)
+		return err;
+
+	hdmi_present_sense(codec, pin_nid, &spec->sink_eld[pin_idx]);
+	return 0;
+}
+
 static int generic_hdmi_build_controls(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec = codec->spec;
 	int err;
 	int i;
 
+	for (i = 0; i < spec->num_pins; i++) {
+		err = generic_hdmi_build_jack(codec, i);
+		if (err < 0)
+			return err;
+	}
+
 	for (i = 0; i < codec->num_pcms; i++) {
 		err = snd_hda_create_spdif_out_ctls(codec, spec->cvt[i]);
 		if (err < 0)
-- 
1.7.4.1