compress: adding support for hardware dependent params

compress: adding support for hardware dependent params

[Banajit Goswami]

Hi,

 We have a requirement, where we need to set a number of hardware specific
parameters for compressed offload use-cases, before starting a stream. As
these parameters need to be set once for the entire session, we would
like to explore the best way to set these additional parameters somewhere
before calling trigger() function.
 Below are few of the proposed solutions that we could think of at this
point for getting this done-


1. Adding new IOCTL call: Add a new ioctl control in compress-offload.c
file to handle hardware specific parameters setting. This new ioctl
control will call into soc-compress.c and eventually call into platform
driver ioctl to set those hardware specific parameters. In this case, we
will add a new structure, let's say "snd_compr_hw_ctrl_params", with the
same fields as "snd_compr_metadata" struct.


2. Adding a function hw_dependant_hw_params(), which will be called after
hw_params() and before prepare() function. The new function will
eventually call into the platform driver to get the hardware specific
parameters set. Once implemented, this would look something like-
  hw_params();
  hw_dependant_hw_params();
  prepare();


3. Add a function "hw_dependant_hw_params()", as _part_ of the hw_params()
function. This new function will be called after the (existing) generic
parameters are set. So once implemented, the proposed function calls would
look like-
  hw_params() {
	generic_hw_params();
	hw_dependant_hw_params();
  }
  prepare();


We would like to get feedback on these proposed solutions and get a
discussion going to see how best these requirements can be fitted in the
current Compressed ALSA framework (for mainlining).

Thanks and Regards,
Banajit Goswami

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: compress: adding support for hardware dependent params

[Pierre-Louis Bossart]

>
>   We have a requirement, where we need to set a number of hardware specific
> parameters for compressed offload use-cases, before starting a stream. As
> these parameters need to be set once for the entire session, we would
> like to explore the best way to set these additional parameters somewhere
> before calling trigger() function.

I think you need to provide more context here. The compressed API 
provides a fair amount of information on the parameters needed to handle 
the stream - maybe too much in most cases -, I don't really get the 
rationale for hardware-specific parameters needed specifically for 
compressed data that you wouldn't need for PCM.
I am also not clear on how we are going to agree on new hw_params that 
are hardware specific. Shouldn't this sort of requirement be handled 
through a control interface?

Re: compress: adding support for hardware dependent params

[noman pouigt]

>  We have a requirement, where we need to set a number of hardware specific
> parameters for compressed offload use-cases, before starting a stream. As
> these parameters need to be set once for the entire session, we would
> like to explore the best way to set these additional parameters somewhere
> before calling trigger() function.
>  Below are few of the proposed solutions that we could think of at this
> point for getting this done-
As you have not mentioned what kind of metadata you are talking about
I am assuming if your meta data can fit in below structure then we already
have a code in upstream which takes care of your need.

140 struct snd_compr_metadata {
141          __u32 key;
142          __u32 value[8];
143 };

Structure snd_compr_ops has set_metadata and get_metadata callbacks
which you can use for your need.

 88  * struct snd_compr_ops: compressed path DSP operations
 89  * @open: Open the compressed stream
 90  * This callback is mandatory and shall keep dsp ready to receive the stream
 91  * parameter
 92  * @free: Close the compressed stream, mandatory
 93  * @set_params: Sets the compressed stream parameters, mandatory
 94  * This can be called in during stream creation only to set codec params
 95  * and the stream properties
 96  * @get_params: retrieve the codec parameters, mandatory
 97  * @trigger: Trigger operations like start, pause, resume, drain, stop.
 98  * This callback is mandatory
 99  * @pointer: Retrieve current h/w pointer information. Mandatory
100  * @copy: Copy the compressed data to/from userspace, Optional
101  * Can't be implemented if DSP supports mmap
102  * @mmap: DSP mmap method to mmap DSP memory
103  * @ack: Ack for DSP when data is written to audio buffer, Optional
104  * Not valid if copy is implemented
105  * @get_caps: Retrieve DSP capabilities, mandatory
106  * @get_codec_caps: Retrieve capabilities for a specific codec, mandatory
107  */

It would be nice if someone can send a patch here to describe set and
get_metadata callbacks in compress_driver.h

>
>
> 1. Adding new IOCTL call: Add a new ioctl control in compress-offload.c
> file to handle hardware specific parameters setting. This new ioctl
> control will call into soc-compress.c and eventually call into platform
> driver ioctl to set those hardware specific parameters. In this case, we
> will add a new structure, let's say "snd_compr_hw_ctrl_params", with the
> same fields as "snd_compr_metadata" struct.
>
>
> 2. Adding a function hw_dependant_hw_params(), which will be called after
> hw_params() and before prepare() function. The new function will
> eventually call into the platform driver to get the hardware specific
> parameters set. Once implemented, this would look something like-
>   hw_params();
>   hw_dependant_hw_params();
>   prepare();
>
>
> 3. Add a function "hw_dependant_hw_params()", as _part_ of the hw_params()
> function. This new function will be called after the (existing) generic
> parameters are set. So once implemented, the proposed function calls would
> look like-
>   hw_params() {
>         generic_hw_params();
>         hw_dependant_hw_params();
>   }
>   prepare();
>
>
> We would like to get feedback on these proposed solutions and get a
> discussion going to see how best these requirements can be fitted in the
> current Compressed ALSA framework (for mainlining).
Can you explain your meta data?

Be the change you want to see in the world:Gandhi

Re: compress: adding support for hardware dependent params

[Vinod Koul]

On Mon, 2014-02-03 at 01:01 -0800, noman pouigt wrote:
> >  We have a requirement, where we need to set a number of hardware specific
> > parameters for compressed offload use-cases, before starting a stream. As
> > these parameters need to be set once for the entire session, we would
> > like to explore the best way to set these additional parameters somewhere
> > before calling trigger() function.
> >  Below are few of the proposed solutions that we could think of at this
> > point for getting this done-
> As you have not mentioned what kind of metadata you are talking about
> I am assuming if your meta data can fit in below structure then we already
> have a code in upstream which takes care of your need.
No this is for audio file metadata which should be passed to decoder
running in your DSP.

Yes as you and Pierre said in the absence of any specifics in the
parameters it would be very difficult to comment on how to go about it.
For offload specific case, you only need to know how to configure the
decoder, which this API tries to do quite well. If you have something HW
specific that should be same issue in PCM handling too, how do you do it
there. I am suspecting the problem statement isn't correct here!
> 
> 140 struct snd_compr_metadata {
> 141          __u32 key;
> 142          __u32 value[8];
> 143 };
> 
> Structure snd_compr_ops has set_metadata and get_metadata callbacks
> which you can use for your need.
> 
>  88  * struct snd_compr_ops: compressed path DSP operations
>  89  * @open: Open the compressed stream
>  90  * This callback is mandatory and shall keep dsp ready to receive the stream
>  91  * parameter
>  92  * @free: Close the compressed stream, mandatory
>  93  * @set_params: Sets the compressed stream parameters, mandatory
>  94  * This can be called in during stream creation only to set codec params
>  95  * and the stream properties
>  96  * @get_params: retrieve the codec parameters, mandatory
>  97  * @trigger: Trigger operations like start, pause, resume, drain, stop.
>  98  * This callback is mandatory
>  99  * @pointer: Retrieve current h/w pointer information. Mandatory
> 100  * @copy: Copy the compressed data to/from userspace, Optional
> 101  * Can't be implemented if DSP supports mmap
> 102  * @mmap: DSP mmap method to mmap DSP memory
> 103  * @ack: Ack for DSP when data is written to audio buffer, Optional
> 104  * Not valid if copy is implemented
> 105  * @get_caps: Retrieve DSP capabilities, mandatory
> 106  * @get_codec_caps: Retrieve capabilities for a specific codec, mandatory
> 107  */
> 
> It would be nice if someone can send a patch here to describe set and
> get_metadata callbacks in compress_driver.h
Thanks for pointing, will do the needful

> >
> > 1. Adding new IOCTL call: Add a new ioctl control in compress-offload.c
> > file to handle hardware specific parameters setting. This new ioctl
> > control will call into soc-compress.c and eventually call into platform
> > driver ioctl to set those hardware specific parameters. In this case, we
> > will add a new structure, let's say "snd_compr_hw_ctrl_params", with the
> > same fields as "snd_compr_metadata" struct.
> >
> >
> > 2. Adding a function hw_dependant_hw_params(), which will be called after
> > hw_params() and before prepare() function. The new function will
> > eventually call into the platform driver to get the hardware specific
> > parameters set. Once implemented, this would look something like-
> >   hw_params();
> >   hw_dependant_hw_params();
> >   prepare();
> >
> >
> > 3. Add a function "hw_dependant_hw_params()", as _part_ of the hw_params()
> > function. This new function will be called after the (existing) generic
> > parameters are set. So once implemented, the proposed function calls would
> > look like-
> >   hw_params() {
> >         generic_hw_params();
> >         hw_dependant_hw_params();
> >   }
> >   prepare();
> >
> >
> > We would like to get feedback on these proposed solutions and get a
> > discussion going to see how best these requirements can be fitted in the
> > current Compressed ALSA framework (for mainlining).
> Can you explain your meta data?
> 
> Be the change you want to see in the world:Gandhi


-- 
~Vinod

Re: compress: adding support for hardware dependent params

[Banajit Goswami]

Hi,

>>  We have a requirement, where we need to set a number of hardware
>> specific
>> parameters for compressed offload use-cases, before starting a stream.
>> As
>> these parameters need to be set once for the entire session, we would
>> like to explore the best way to set these additional parameters
>> somewhere
>> before calling trigger() function.
>>  Below are few of the proposed solutions that we could think of at this
>> point for getting this done-
> As you have not mentioned what kind of metadata you are talking about
> I am assuming if your meta data can fit in below structure then we already
> have a code in upstream which takes care of your need.
>
> 140 struct snd_compr_metadata {
> 141          __u32 key;
> 142          __u32 value[8];
> 143 };
>
> Structure snd_compr_ops has set_metadata and get_metadata callbacks
> which you can use for your need.
>

 We can probably use the same snd_compr_metadata structure for setting the
hardware specific parameters, but the only issue is that, currently this
structure is being used only from compress_set_gapless_metadata() in
external/tinycompress/compress.c to set metadata specific to gapless
audio. The parameters we are looking to set is not specific to gapless
audio, but rather a few hardware/board specific parameters (for passing
to our driver)

I think, the best way to use the same structure and have the new
requirement fulfilled is that-

1. Define a new KEY inside the enum in
external/kernel-headers/original/sound/compress_offload.h

enum {
	SNDRV_COMPRESS_ENCODER_PADDING = 1,
	SNDRV_COMPRESS_ENCODER_DELAY = 2,
	SNDRV_COMPRESS_HW_SPECIFIC_METADATA = 3,
};

2. Have a new generic function (not specific to gapless audio) in
external/tinycompress/compress.c to call driver IOCTL function, to set the
H/W specific parameters-

int compress_set_metadata(struct compress *compress,
	struct compr_gapless_mdata *mdata)
{
	struct snd_compr_metadata metadata;
	int version;

	if (!is_compress_ready(compress))
		return oops(compress, ENODEV, "device not ready");

	version = get_compress_version(compress);
	if (version <= 0)
		return -1;

	if (version < SNDRV_PROTOCOL_VERSION(0, 1, 1))
		return oops(compress, ENXIO, "gapless apis not supported in kernel");

	metadata.key = SNDRV_COMPRESS_HW_SPECIFIC_METADATA;
	metadata.value[0] = mdata->hw_specific_metadata;
	if (ioctl(compress->fd, SNDRV_COMPRESS_SET_METADATA, &metadata))
		return oops(compress, errno, "can't set metadata for stream\n");

	return 0;
}

Let me know your comment on this.

Thanks,
Banajit

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: compress: adding support for hardware dependent params

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 679 bytes --]

On Sun, Mar 16, 2014 at 05:55:37AM -0000, Banajit Goswami wrote:

> audio. The parameters we are looking to set is not specific to gapless
> audio, but rather a few hardware/board specific parameters (for passing
> to our driver)

Why not just use ALSA controls for this - that's the normal way of doing
things like this, if the parameters aren't part of the stream data it's
not obvious to me why they're being set on the stream.  I'm also a bit
concerned about the idea that applications would need to support another
custom API for passing opaque blobs to drivers, we already have bytes
controls.  Can you give any examples of the sort of thing that would be
configured here?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]