Re: [alsa-devel] [PATCHv2 - CA0132 HDA Codec 1/2] ALSA: Update Creative CA0132 codec to add DSP features.
At Mon, 30 Jul 2012 15:49:00 -0700, Ian Minett wrote:
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.
Ok, we understand. Given that the codec cannot call into the controller's PCM interface to load the DSP image, is there an acceptable way you could recommend for us to add this functionality to the module, without breaking the architecture?
Well, as mentioned earlier, we need to push it up to the controller layer, i.e. hda_intel.c. For example, add a new op for firmware DMA transfer in hda_bus_ops.
Could you describe briefly the procedure for the DMA transfer of CA0132? The code looks too complex to follow.
thanks,
Takashi
The only question is where to implement the DSP loader stuff and how.
Ok, we understand. Given that the codec cannot call into the controller's PCM interface to load the DSP image, is there an acceptable way you could recommend for us to add this functionality to the module, without breaking the architecture?
Well, as mentioned earlier, we need to push it up to the controller layer, i.e. hda_intel.c. For example, add a new op for firmware DMA transfer in hda_bus_ops.
Could you describe briefly the procedure for the DMA transfer of CA0132? The code looks too complex to follow.
Sure - the DSP code download procedure is:
1. The CA0132 driver extracts DSP data segments that need to download to the DSP chip. 2. Each of these data segments will be downloaded to the DSP chip via HDA-link, like audio data. 3. The HDA controller and CA0132 are programmed to transfer the DSP data segments. 4. The HDA controller FIFO must be flushed on every data segment to ensure that there is no previous residual data.
These are the controller ops that the CA0132 driver used for the DMA transfer: #define azx_pcm_open(a) (a->ops->open(a)) #define azx_pcm_close(a) (a->ops->close(a)) #define azx_pcm_prepare(a) (a->ops->prepare(a)) #define azx_pcm_trigger(a, b) (a->ops->trigger(a, b)) #define azx_pcm_hw_free(a) (a->ops->hw_free(a))
Thanks, Ian
At Tue, 31 Jul 2012 19:38:05 -0700, Ian Minett wrote:
The only question is where to implement the DSP loader stuff and how.
Ok, we understand. Given that the codec cannot call into the controller's PCM interface to load the DSP image, is there an acceptable way you could recommend for us to add this functionality to the module, without breaking the architecture?
Well, as mentioned earlier, we need to push it up to the controller layer, i.e. hda_intel.c. For example, add a new op for firmware DMA transfer in hda_bus_ops.
Could you describe briefly the procedure for the DMA transfer of CA0132? The code looks too complex to follow.
Sure - the DSP code download procedure is:
- The CA0132 driver extracts DSP data segments that need to download to the DSP chip.
- Each of these data segments will be downloaded to the DSP chip via HDA-link, like audio data.
- The HDA controller and CA0132 are programmed to transfer the DSP data segments.
So, the data transfer here is just like the normal PCM streaming? For example, if you have an op like
int (*load_dsp)(struct hda_bus *bus, void *buffer, int size);
and calling this for each segment would work? (In addition, we'd need to give some way to determine the stop condition in the codec side.)
- The HDA controller FIFO must be flushed on every data segment to ensure that there is no previous residual data.
Actually FIFO flush might be needed for the audio transfer as well.
thanks,
Takashi
These are the controller ops that the CA0132 driver used for the DMA transfer: #define azx_pcm_open(a) (a->ops->open(a)) #define azx_pcm_close(a) (a->ops->close(a)) #define azx_pcm_prepare(a) (a->ops->prepare(a)) #define azx_pcm_trigger(a, b) (a->ops->trigger(a, b)) #define azx_pcm_hw_free(a) (a->ops->hw_free(a))
Thanks, Ian
So, the data transfer here is just like the normal PCM streaming? For example, if you have an op like
int (*load_dsp)(struct hda_bus *bus, void *buffer, int size);and calling this for each segment would work? (In addition, we'd need to give some way to determine the stop condition in the codec side.)
Hi,
Yes - this approach would work. We would also need to set the data format, and control the stop condition.
For example, by adding the following ops to handle the dsp download process:
/* format is 16-bit Stream Format Structure as defined in HDA spec. pcm_prepare() callback in the codec will be called so that codec driver can obtain the stream tag and format to set up the codec. */ int (*load_dsp_prepare) \ (struct hda_bus *bus, unsigned short format, void *buffer, int size);
/* start the transfer. */ int (*load_dsp_start)(struct hda_bus *bus);
/* stop the transfer and flush the FIFO. */ int (*load_dsp_stop)(struct hda_bus *bus);
Thanks, Ian
At Fri, 3 Aug 2012 20:29:44 -0700, Ian Minett wrote:
So, the data transfer here is just like the normal PCM streaming? For example, if you have an op like
int (*load_dsp)(struct hda_bus *bus, void *buffer, int size);and calling this for each segment would work? (In addition, we'd need to give some way to determine the stop condition in the codec side.)
Hi,
Yes - this approach would work. We would also need to set the data format, and control the stop condition.
For example, by adding the following ops to handle the dsp download process:
/* format is 16-bit Stream Format Structure as defined in HDA spec. pcm_prepare() callback in the codec will be called so that codec driver can obtain the stream tag and format to set up the codec. */ int (*load_dsp_prepare) \ (struct hda_bus *bus, unsigned short format, void *buffer, int size);
/* start the transfer. */ int (*load_dsp_start)(struct hda_bus *bus);
/* stop the transfer and flush the FIFO. */ int (*load_dsp_stop)(struct hda_bus *bus);
Looks good, but I think start and stop can be a single trigger like PCM op, something like:
int (*load_dsp_trigger)(struct hda_bus *bus, bool start);
The second argument could be a generic int like PCM op, but I don't think we'd need pause or suspend/resume command for DSP loader :)
Takashi
Looks good, but I think start and stop can be a single trigger like PCM op, something like:
int (*load_dsp_trigger)(struct hda_bus *bus, bool start);
The second argument could be a generic int like PCM op, but I don't think we'd need pause or suspend/resume command for DSP loader :)
Hi Takashi,
The proposed method for handling the DSP load process looks fine to us, and we can make a start on adding this to the CA0132 driver now.
We can update patch_ca0132.c, but would you be able to assist us with the modifications to the hda_intel.c side at all? We think it might result in a smoother process (and involving fewer submissions :) ) given your familiarity with the low-level architecture in that area.
If not, we'd appreciate any info or pointers you can provide on how and where the necessary modifications need to be made to hda_intel.c.
Thanks again for all your help with this. Ian
At Tue, 7 Aug 2012 17:27:59 -0700, Ian Minett wrote:
Looks good, but I think start and stop can be a single trigger like PCM op, something like:
int (*load_dsp_trigger)(struct hda_bus *bus, bool start);
The second argument could be a generic int like PCM op, but I don't think we'd need pause or suspend/resume command for DSP loader :)
Hi Takashi,
The proposed method for handling the DSP load process looks fine to us, and we can make a start on adding this to the CA0132 driver now.
We can update patch_ca0132.c, but would you be able to assist us with the modifications to the hda_intel.c side at all? We think it might result in a smoother process (and involving fewer submissions :) ) given your familiarity with the low-level architecture in that area.
Sure, just send me a draft version of your patch, so that I can work on it.
If not, we'd appreciate any info or pointers you can provide on how and where the necessary modifications need to be made to hda_intel.c.
You had already some working code but it was calling PCM ops. When you see hda_intel.c, the necessary streaming operations can be much easier implemented there instead of indirect call of PCM ops, if you have a dedicated load_dsp_prepare and load_dsp_trigger. The current azx_pcm_trigger(), for example, looks fairly complex, but it's simply because of the support for linked PCM streams. If you can assume a single stream, it's just a call of azx_stream_start() and azx_stream_stop().
BTW, what would be the license of the firmware? For upstreaming the firmware, you'd need to clarify its license. You can see the license texts in kernel-firmware tree / package about what other vendors give.
thanks,
Takashi
participants (2)
-
Ian Minett -
Takashi Iwai