[alsa-devel] [PATCHv2 - CA0132 HDA Codec 1/2] ALSA: Update Creative CA0132 codec to add DSP features.

[Takashi Iwai]

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 at suse.de>                                           
>                                                                                                  To
>              07/26/2012 01:06          Ian Minett <ian_minett at creativelabs.com>
>              AM                                                                                  cc
>                                        alsa-devel at 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 at 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)>]
>