[PATCH] ALSA: compress: Add DSP private metadata interface

[PATCH] ALSA: compress: Add DSP private metadata interface

[Tim Sheridan]

Provides an area to communicate with a DSP to implement support for
weird and wonderful features of DSP hardware that aren't supported in
the compress offload API.

Signed-off-by: Tim Sheridan <tim.sheridan@imgtec.com>
---
 include/uapi/sound/compress_offload.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index e00d8cb..65ac445 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -30,7 +30,7 @@
 #include <sound/compress_params.h>
 
 
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 2)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 3)
 /**
  * struct snd_compressed_buffer - compressed buffer
  * @fragment_size: size of buffer fragment in bytes
@@ -150,6 +150,8 @@ struct snd_compr_metadata {
  * SNDRV_COMPRESS_SET_PARAMS: Set codec and stream parameters
  * Note: only codec params can be changed runtime and stream params cant be
  * SNDRV_COMPRESS_GET_PARAMS: Query codec params
+ * SNDRV_COMPRESS_SET_DSP_PRIV: Set private DSP metadata
+ * SNDRV_COMPRESS_GET_DSP_PRIV: Get private DSP metadata
  * SNDRV_COMPRESS_TSTAMP: get the current timestamp value
  * SNDRV_COMPRESS_AVAIL: get the current buffer avail value.
  * This also queries the tstamp properties
@@ -171,6 +173,8 @@ struct snd_compr_metadata {
 						 struct snd_compr_metadata)
 #define SNDRV_COMPRESS_GET_METADATA	_IOWR('C', 0x15,\
 						 struct snd_compr_metadata)
+#define SNDRV_COMPRESS_SET_DSP_PRIV	_IOW('C', 0x16, struct snd_compr_metadata)
+#define SNDRV_COMPRESS_GET_DSP_PRIV	_IOWR('C', 0x17, struct snd_compr_metadata)
 #define SNDRV_COMPRESS_TSTAMP		_IOR('C', 0x20, struct snd_compr_tstamp)
 #define SNDRV_COMPRESS_AVAIL		_IOR('C', 0x21, struct snd_compr_avail)
 #define SNDRV_COMPRESS_PAUSE		_IO('C', 0x30)
-- 
1.7.1

Re: [PATCH] ALSA: compress: Add DSP private metadata interface

[Takashi Iwai]

Add Vinod to Cc.

On Mon, 22 Feb 2016 18:54:31 +0100,
Tim Sheridan wrote:
> 
> Provides an area to communicate with a DSP to implement support for
> weird and wonderful features of DSP hardware that aren't supported in
> the compress offload API.
> 
> Signed-off-by: Tim Sheridan <tim.sheridan@imgtec.com>

Don't you have the code actually implement this ioctl?
Or, is there anything I miss?


thanks,

Takashi

> ---
>  include/uapi/sound/compress_offload.h |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index e00d8cb..65ac445 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -30,7 +30,7 @@
>  #include <sound/compress_params.h>
>  
>  
> -#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 2)
> +#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 3)
>  /**
>   * struct snd_compressed_buffer - compressed buffer
>   * @fragment_size: size of buffer fragment in bytes
> @@ -150,6 +150,8 @@ struct snd_compr_metadata {
>   * SNDRV_COMPRESS_SET_PARAMS: Set codec and stream parameters
>   * Note: only codec params can be changed runtime and stream params cant be
>   * SNDRV_COMPRESS_GET_PARAMS: Query codec params
> + * SNDRV_COMPRESS_SET_DSP_PRIV: Set private DSP metadata
> + * SNDRV_COMPRESS_GET_DSP_PRIV: Get private DSP metadata
>   * SNDRV_COMPRESS_TSTAMP: get the current timestamp value
>   * SNDRV_COMPRESS_AVAIL: get the current buffer avail value.
>   * This also queries the tstamp properties
> @@ -171,6 +173,8 @@ struct snd_compr_metadata {
>  						 struct snd_compr_metadata)
>  #define SNDRV_COMPRESS_GET_METADATA	_IOWR('C', 0x15,\
>  						 struct snd_compr_metadata)
> +#define SNDRV_COMPRESS_SET_DSP_PRIV	_IOW('C', 0x16, struct snd_compr_metadata)
> +#define SNDRV_COMPRESS_GET_DSP_PRIV	_IOWR('C', 0x17, struct snd_compr_metadata)
>  #define SNDRV_COMPRESS_TSTAMP		_IOR('C', 0x20, struct snd_compr_tstamp)
>  #define SNDRV_COMPRESS_AVAIL		_IOR('C', 0x21, struct snd_compr_avail)
>  #define SNDRV_COMPRESS_PAUSE		_IO('C', 0x30)
> -- 
> 1.7.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] ALSA: compress: Add DSP private metadata interface

[Vinod Koul]

On Tue, Feb 23, 2016 at 10:31:18AM +0100, Takashi Iwai wrote:
> Add Vinod to Cc.
> 
> On Mon, 22 Feb 2016 18:54:31 +0100,
> Tim Sheridan wrote:
> > 
> > Provides an area to communicate with a DSP to implement support for
> > weird and wonderful features of DSP hardware that aren't supported in
> > the compress offload API.

And compress API was never intended for that :)

Can you tell me the awesome features you would like to support using this
API and I would try to provide you a standard alsa way to do that...

The intention of compress API is to provide a streaming interface for data
transfer and nothing more. To implement DSP features and control them, you
need to look at alsa control interface

> > 
> > Signed-off-by: Tim Sheridan <tim.sheridan@imgtec.com>

Btw there was another person from imgtec who had similar request last year,
never heard back from him after he was supposed to fix stuff in his driver
and come back

> 
> Don't you have the code actually implement this ioctl?
> Or, is there anything I miss?
> 
> 
> thanks,
> 
> Takashi
> 
> > ---
> >  include/uapi/sound/compress_offload.h |    6 +++++-
> >  1 files changed, 5 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> > index e00d8cb..65ac445 100644
> > --- a/include/uapi/sound/compress_offload.h
> > +++ b/include/uapi/sound/compress_offload.h
> > @@ -30,7 +30,7 @@
> >  #include <sound/compress_params.h>
> >  
> >  
> > -#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 2)
> > +#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 3)
> >  /**
> >   * struct snd_compressed_buffer - compressed buffer
> >   * @fragment_size: size of buffer fragment in bytes
> > @@ -150,6 +150,8 @@ struct snd_compr_metadata {
> >   * SNDRV_COMPRESS_SET_PARAMS: Set codec and stream parameters
> >   * Note: only codec params can be changed runtime and stream params cant be
> >   * SNDRV_COMPRESS_GET_PARAMS: Query codec params
> > + * SNDRV_COMPRESS_SET_DSP_PRIV: Set private DSP metadata
> > + * SNDRV_COMPRESS_GET_DSP_PRIV: Get private DSP metadata
> >   * SNDRV_COMPRESS_TSTAMP: get the current timestamp value
> >   * SNDRV_COMPRESS_AVAIL: get the current buffer avail value.
> >   * This also queries the tstamp properties
> > @@ -171,6 +173,8 @@ struct snd_compr_metadata {
> >  						 struct snd_compr_metadata)
> >  #define SNDRV_COMPRESS_GET_METADATA	_IOWR('C', 0x15,\
> >  						 struct snd_compr_metadata)
> > +#define SNDRV_COMPRESS_SET_DSP_PRIV	_IOW('C', 0x16, struct snd_compr_metadata)
> > +#define SNDRV_COMPRESS_GET_DSP_PRIV	_IOWR('C', 0x17, struct snd_compr_metadata)
> >  #define SNDRV_COMPRESS_TSTAMP		_IOR('C', 0x20, struct snd_compr_tstamp)
> >  #define SNDRV_COMPRESS_AVAIL		_IOR('C', 0x21, struct snd_compr_avail)
> >  #define SNDRV_COMPRESS_PAUSE		_IO('C', 0x30)
> > -- 
> > 1.7.1
> > 
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > 

-- 
~Vinod

Re: [PATCH] ALSA: compress: Add DSP private metadata interface

[Tim Sheridan]

Hi Takashi/Vinod,

> > Don't you have the code actually implement this ioctl?
> > Or, is there anything I miss?

Yup, not implementing it anywhere in the kernel tree. Just submitting the patch for the API to support implementing it in the out-of-tree IMG AXD driver. Apologies if I'm breaking some kind of rule here requiring an in-tree usage of a new API feature -- it might be better to submit this as part of a revised AXD patch series which implements this ioctl.

> And compress API was never intended for that :)
> 
> Can you tell me the awesome features you would like to support using this
> API and I would try to provide you a standard alsa way to do that...
> 
> The intention of compress API is to provide a streaming interface for data
> transfer and nothing more. To implement DSP features and control them,
> you need to look at alsa control interface

AXD's got various features which require mode setting (e.g. for selecting between multi-device synchronization mechanisms that it supports). This would need an enum control, but I guess that this could be fudged with mutually exclusive switch controls for each of the enum values. AXD's also got numeric configuration parameters which it needs to be able to receive (e.g. parameters for stream synchronization heuristics, audio filter coefficients, etc.). Maybe a volume control with a domain wide enough could be used for this kind of configuration parameter. It's rather horrible, but might be fine for while the AXD driver is out of tree and there aren't more appropriate ALSA control types (e.g. enum and non-volume integer). :-/

> Btw there was another person from imgtec who had similar request last year,
> never heard back from him after he was supposed to fix stuff in his driver
> and come back

Yeah, Qais left IMG before being able to finish up his fixes for the AXD driver (I think the last patch series was sent in August 2015). I've only got a limited amount of time for working on AXD, but if I get the time/orders from above me in IMG, a revised patch series could appear again. :-)

Many thanks,
Tim

Re: [PATCH] ALSA: compress: Add DSP private metadata interface

[Pierre-Louis Bossart]

> AXD's got various features which require mode setting (e.g. for
> selecting between multi-device synchronization mechanisms that it
> supports). This would need an enum control, but I guess that this
> could be fudged with mutually exclusive switch controls for each of
> the enum values. AXD's also got numeric configuration parameters
> which it needs to be able to receive (e.g. parameters for stream
> synchronization heuristics, audio filter coefficients, etc.). Maybe a
> volume control with a domain wide enough could be used for this kind
> of configuration parameter. It's rather horrible, but might be fine
> for while the AXD driver is out of tree and there aren't more
> appropriate ALSA control types (e.g. enum and non-volume integer).
> :-/

I am not too sure I understand what you refer to as synchronization or 
multi-device synchronization mechanisms. Since you mentioned you only 
need an initial time, I wonder if this is conceptually related to the 
start_at() functionality discussed last year for PCM?

If you have a set of implementation-defined mechanisms related to 
algorithms, why not use an ALSA binary control where you define you own 
syntax and content and use the alsa control layer as a 'dumb' pipe 
between user and kernel. That's what everyone does for DSP control.

Re: [PATCH] ALSA: compress: Add DSP private metadata interface

[Tim Sheridan]

Hi Pierre-Louis,

> I am not too sure I understand what you refer to as synchronization or multi-
> device synchronization mechanisms. Since you mentioned you only need an
> initial time, I wonder if this is conceptually related to the
> start_at() functionality discussed last year for PCM?

Yup, it is related to the start_at() functionality. Before start_at() playback can happen, the AXD DSP must be switched into the start_at mode.

> If you have a set of implementation-defined mechanisms related to
> algorithms, why not use an ALSA binary control where you define you own
> syntax and content and use the alsa control layer as a 'dumb' pipe between
> user and kernel. That's what everyone does for DSP control.

Ah, thanks very much for that advice. This looks like a good option (looking how it's being done for filter parameters in sound/soc/intel/atom).

Cheers,
Tim

Re: [PATCH] ALSA: compress: Add DSP private metadata interface

[Vinod Koul]

On Tue, Feb 23, 2016 at 03:40:29PM +0000, Tim Sheridan wrote:
> Hi Pierre-Louis,
> 
> > I am not too sure I understand what you refer to as synchronization or multi-
> > device synchronization mechanisms. Since you mentioned you only need an
> > initial time, I wonder if this is conceptually related to the
> > start_at() functionality discussed last year for PCM?
> 
> Yup, it is related to the start_at() functionality. Before start_at() playback can happen, the AXD DSP must be switched into the start_at mode.
> 
> > If you have a set of implementation-defined mechanisms related to
> > algorithms, why not use an ALSA binary control where you define you own
> > syntax and content and use the alsa control layer as a 'dumb' pipe between
> > user and kernel. That's what everyone does for DSP control.
> 
> Ah, thanks very much for that advice. This looks like a good option (looking how it's being done for filter parameters in sound/soc/intel/atom).

Also please note that we can now avoid hard coding these in kernel. You can
define a topology graph for DSP and use alsa topology to create a topology
binary which is loaded by driver and passed to ASoC topology core which
creates these elements as well as DAPM graph

This way you can change graphs and customize them very easily!

Thanks
-- 
~Vinod