At Fri, 27 Jul 2012 13:21:05 -0700, Ian Minett wrote:
Another possibility is to fork hda_intel.c and move the Creative's controller chip driver there (not meant the code in patch_ca0132.c). You can strip down many workarounds found in hda_intel.c (e.g. bdl_pos_adj, position_fix, probing errors, vga_switcheroo, etc) as a bonus. And the new controller driver can still use the same hda_codec.c and the rest as long as it uses the same structure.
The CA0132 chip can be connected to and work with any other, non- Creative HDA controllers. So, forking hda_intel.c for a Creative- specific controller won't work in the case where our chip is using a different HDA controller.
So, the DSP part is specific to the codec and not the controller? OK, then forking the controller doesn't work.
Do you have any suggestions or thoughts on how we can add the spin_lock_irqxxx(), or an alternative approach, into non-specific controller code in order to facilitate the codec DSP download? Since the download isn't controller-specific, it doesn't sound right to move it into controller code such as hda_intel.c either.
No, no. The biggest problem is that the codec driver is calling what the controller driver should do -- prepare, trigger, whatever -- manually. This is the layer violation.
One thought we have is to handle the spin_lock_irqxxx() as a special case or quirk for the CA0132 codec. Using an approach similar to 'needs_damn_long_delay' in the CA0110 patch, we could define another flag e.g. 'needs_trigger_irqsave' in hda_codec.h struct hda_bus{}. In the CA0132 patch, we would set this flag, which would then be read in hda_intel.c,azx_pcm_trigger(). Either spin_lock_irqxxx() or the current spin_lock() would then be called depending on the flag condition. What is your opinion on an approach like this? Would this be acceptable?
No. The fact that you are calling the PCM functions is already wrong. The irqsave() is just a side-effect by that, and we can forget about it completely now.
The only question is where to implement the DSP loader stuff and how.
But, still one concern is that people may very likely build the kernel without taking the firmware. Then suddenly it breaks just by the kernel update -- this shouldn't happen. So, we'd need a kernel config, or at least a module parameter, to enable/disable the new DSP stuff. When it's off, the whole DSP isn't requested and it behaves just likes a standard HD-audio.
The driver allows for audio playback to still function as normal in the absence of the DSP firmware image, the only effect will be that there will be no DSP processing applied to the stream. The DSP effect and volume controls and WUH recording capability won't be available.
Then we'd need some flag to enable/disable firmware loading, too.
Finally, can I send the binary firmware patch to the list? I sent it shortly after the previous patch submission but it didn't appear to get through. Is it ok to send directly to you?
It's up to you. If the file is too big, you can send it directly to me and I'll push it to git tree.
thanks,
Takashi
Thanks very much, Ian
Takashi Iwai <tiwai@suse.de> To 07/26/2012 01:06 Ian Minett <ian_minett@creativelabs.com> AM cc alsa-devel@alsa-project.org Subject Re: [PATCHv2 - CA0132 HDA Codec 1/2] ALSA: Update Creative CA0132 codec to add DSP features.At Wed, 25 Jul 2012 11:01:19 -0700, Ian Minett wrote:
From: Ian Minett ian_minett@creativelabs.com
Hi, Thanks again for the feedback - we've reworked the patch accordingly, to remove floats, tidy the use of macros and add the MODULE_FIRMWARE() lines.
You are right - since it is a pretty big change introducing all the DSP functionality it isn't trivial to break it into multiple small patches. I've added more info on what has been changed below in the git commit message.
The reason we need to modify the trigger callback in hda_intel.c is that the DSP code is downloaded from the host (driver) to the CA0132 chip via the HD-link. This means that the trigger callback is called from the codec driver.
Well, this is the biggest problem. What the patch does is a layer violation. Such an operation must not be performed in the codec driver. I guess you know it very well when you look again how ugly hacks you need for setting up a fake PCM instance and so on.
One possible way would be to move these DSP loading code into hda_intel.c. But then it bloats up the generic controller driver.
Another possibility is to fork hda_intel.c and move the Creative's controller chip driver there (not meant the code in patch_ca0132.c). You can strip down many workarounds found in hda_intel.c (e.g. bdl_pos_adj, position_fix, probing errors, vga_switcheroo, etc) as a bonus. And the new controller driver can still use the same hda_codec.c and the rest as long as it uses the same structure.
I will submit the DSP firmware blob (ctefx.bin) as a separate patch against the alsa-firmware repository, for intended upstreaming to linux-firmware package. The CA0132 codec uses request_firmware() loader to find the DSP binary in the /lib/firmware directory (the firmware isn't required for building the CA0132 module). If this isn't the correct way to submit firmware binaries, we'd appreciate any info on what the correct method for this is.
Yes, this is the correct way.
But, still one concern is that people may very likely build the kernel without taking the firmware. Then suddenly it breaks just by the kernel update -- this shouldn't happen.
So, we'd need a kernel config, or at least a module parameter, to enable/disable the new DSP stuff. When it's off, the whole DSP isn't requested and it behaves just likes a standard HD-audio.
thanks,
Takashi
ForwardSourceID:NT0001C0EA [1.2 <text/html; US-ASCII (quoted-printable)>]