Skip to content

[RFC] ipc: topology: Introduce POST_PROCESS component#3284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dbaluta wants to merge 1 commit into from
Closed

[RFC] ipc: topology: Introduce POST_PROCESS component #3284

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/include/ipc/topology.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -47,6 +47,7 @@ enum sof_comp_type {
SOF_COMP_ASRC, /**< Asynchronous sample rate converter */
SOF_COMP_DCBLOCK,
SOF_COMP_SMART_AMP, /**< smart amplifier component */
SOF_COMP_POST_PROCESS,

@lgirdwoodlgirdwoodAug 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we just call this PROCESS now that we are using UUID. @plbossart ? This would cover all cases.

@plbossartplbossartAug 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I thought we'd stop adding such types.

@dbalutadbalutaAug 10, 2020•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart@lgirdwood

Wait a second, so should I refrain from adding a new type to sof_comp_type and use the new UUID approach for components right? (I need to revise the UUID PR again).

Also, what about the "subtypes" (flavours) of PROCESS component like (mp3, aac, etc). Can I still use enum sof_ipc_process_type for that, right?

@lgirdwoodlgirdwoodAug 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaluta the intention is to move away from kernel updates to minimise upstreaming (since the kernel does not care about general processing components). Having said that the ALSA compressed PCM may need to know the media type or does it not care and just acts as a passthrough ? @plbossart can you comment, does compressed ALSA API need to know media types ?

@paulstelian97paulstelian97Aug 11, 2020•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only needs to know enough to translate the userspace request to properly select the correct type. I think a good idea is to just have the enum copied and the value of the type field outright passed through directly to the firmware without the kernel interpreting/translating it in any way.

@dbalutadbalutaAug 11, 2020•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood when ALSA compressed device is created, ALSA needs to know the media type. But the PROCESS component on the FW side is created much early when the topology is parsed. Current infrastructure allows to set both component type (PROCESS) and process component type (mp3/aac etc) when the topology is created.

See sof_ipc_comp_process.comp.type and sof_ipc_comp_process.type.

/* generic "effect", "codec" or proprietary processing component */
struct sof_ipc_comp_process {
struct sof_ipc_comp comp;
struct sof_ipc_comp_config config;
uint32_t size;	/**< size of bespoke data section in bytes */
uint32_t type;	/**< sof_ipc_process_type */
/* reserved for future use */
uint32_t reserved[7];
unsigned char data[0];
} __attribute__((packed));

I think for the first implementation we will use by default:

  • sof_ipc_comp_process.comp.type = SOF_COMP_PRCESS (this is read from topology file)
  • sof_ipc_comp_process.type = SOF_PROCESS_CODEC_MP3 (this is harcoded) (1)

(1) ideally will be received by ALSA Compress API from userspace and will be set when we start a stream.

@plbossartplbossartAug 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaluta this is an interesting one: i think the compressed API lets you specify the codec type at run time, when setting the params. So in theory it's possible to have a generic decoder that can handle multiple formats. Setting a unique format at the topology creation time is possible, but would be a restriction (just like setting a single frequency for a PCM device is permitted but isn't always required).

/* keep FILEREAD/FILEWRITE as the last ones */
SOF_COMP_FILEREAD = 10000, /**< host test based file IO */
SOF_COMP_FILEWRITE = 10001, /**< host test based file IO */
Expand DownExpand Up@@ -220,6 +221,23 @@ struct sof_ipc_comp_tone {
int32_t ramp_step;
} __attribute__((packed));

/** \brief Types of processing components */
enum sof_ipc_process_type {
SOF_PROCESS_NONE = 0, /**< None */
SOF_PROCESS_EQFIR, /**< Intel FIR */
SOF_PROCESS_EQIIR, /**< Intel IIR */
SOF_PROCESS_KEYWORD_DETECT, /**< Keyword Detection */
SOF_PROCESS_KPB, /**< KeyPhrase Buffer Manager */
SOF_PROCESS_CHAN_SELECTOR, /**< Channel Selector */
SOF_PROCESS_MUX,
SOF_PROCESS_DEMUX,
SOF_PROCESS_DCBLOCK,
SOF_PROCESS_SMART_AMP, /**< Smart Amplifier */
SOF_PROCESS_CODEC_PCM, /**< Codecs */

@cujomalaineycujomalaineyAug 11, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second the idea of having the system be a single codec and have the implementation be selected at runtime

@paulstelian97paulstelian97Aug 12, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the ALSA Compress interface requires this runtime selection anyway.

SOF_PROCESS_CODEC_MP3,
SOF_PROCESS_CODEC_AAC,
};

/* generic "effect", "codec" or proprietary processing component */
struct sof_ipc_comp_process {
struct sof_ipc_comp comp;
Expand Down