Skip to content

ASoC: SOF: ipc: introduce cont_update_posn in sof_ipc_stream_params s… #3353

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

Merged
plbossart merged 1 commit into from
May 3, 2022
Merged

ASoC: SOF: ipc: introduce cont_update_posn in sof_ipc_stream_params s… #3353

Show file tree
Hide file tree
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
3 changes: 2 additions & 1 deletion include/sound/sof/stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,9 @@ struct sof_ipc_stream_params {

uint32_t host_period_bytes;
uint16_t no_stream_position; /**< 1 means don't send stream position */
uint8_t cont_update_posn; /**< 1 means continuous update stream position */

Copy link
Collaborator

@dbaluta dbaluta Jan 4, 2022

Choose a reason for hiding this comment

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

Could we just take only 1 byte from reserved area. We are wasting so much space to keep a boolean information.

Copy link
Author

@yaochunhung yaochunhung Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@dbaluta yes, I could modify it as one byte. Thanks

Copy link
Collaborator

@ujfalusi ujfalusi Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We will set the cont_update_posn to one in kernel unconditionally or what is the condition to set it?
Iow, will the use of this new flag going to be:

+ 	if (v->abi_version >= SOF_ABI_VER(3, 21, 0))
+  		msg->cont_update_posn = 1;

If it is then I argue that there is no point of having this flag in the first place and just do it unconditionally in firmware?

Copy link
Author

@yaochunhung yaochunhung Jan 10, 2022

Choose a reason for hiding this comment

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

@ujfalusi Currently I propose to set it in the stream callback function pcm_hw_params e.g.

#define CONTINUOUS_UPDATE_POSITION 1

static int mt8195_dsp_pcm_hw_params(struct snd_sof_dev *sdev,
			  struct snd_pcm_substream *substream,
			  struct snd_pcm_hw_params *params,
			  struct sof_ipc_stream_params *ipc_params)
{
	struct sof_ipc_fw_version *v = &sdev->fw_ready.version;

	if (v->abi_version >= SOF_ABI_VER(3, 21, 0))
		ipc_params->cont_update_posn = CONTINUOUS_UPDATE_POSITION;

	return 0;
}

Copy link
Member

@plbossart plbossart Jan 25, 2022

Choose a reason for hiding this comment

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

I think this should be the default across all platforms, I don't really see a reason why this capability needs to be added 4 times when we all have to support the Chrome conformance suite. @cujomalainey any comments?

Copy link

@cujomalainey cujomalainey Jan 25, 2022

Choose a reason for hiding this comment

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

agreed, given we know the current default does not pass the test, lets push this into the core and replace the existing default

Copy link
Member

@plbossart plbossart Mar 9, 2022

Choose a reason for hiding this comment

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

@yaochunhung can you please add a user for this new capability?

uint16_t reserved[3];
uint8_t reserved[5];
uint16_t chmap[SOF_IPC_MAX_CHANNELS]; /**< channel map - SOF_CHMAP_ */
} __packed;

Expand Down
2 changes: 1 addition & 1 deletion include/uapi/sound/sof/abi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

/* SOF ABI version major, minor and patch numbers */
#define SOF_ABI_MAJOR 3
#define SOF_ABI_MINOR 20
#define SOF_ABI_MINOR 21
#define SOF_ABI_PATCH 0
Copy link
Collaborator

@dbaluta dbaluta Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Did we forget to bump the ABI on Linux side in the past?

Copy link
Member

@lgirdwood lgirdwood Apr 5, 2022

Choose a reason for hiding this comment

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

yes, looks like we need a dedicated person to align with me for FW side.

Copy link
Collaborator

@kv2019i kv2019i Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@dbaluta @lgirdwood @plbossart This is an open issue. I've made proposal in #3298 and #3296 , but we didn't reach consensus and no change was done. I believe @ujfalusi has looked at after me. As per current ABI process https://thesofproject.github.io/latest/contribute/process/abiprocess.html , it is not mandatory to update the kernel headers when the FW ABI is extended. It is allowed to delay the update of the kernel headers and do it only when it is needed. This has certain benefits and has been in use for multiple SOF release cycles, but it indeed may lead to conflicts like this one.

Copy link
Member

@plbossart plbossart Apr 22, 2022

Choose a reason for hiding this comment

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

sorry, I screwed up and should have reset this patch level earlier. I added #3610 to fix this, you'll have to rebase when it's merged. My bad.

Copy link
Author

@yaochunhung yaochunhung Apr 24, 2022

Choose a reason for hiding this comment

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

@plbossart Never mind, I will rebase this PR when #3610 is merged. Thanks.

Copy link
Author

@yaochunhung yaochunhung Apr 26, 2022

Choose a reason for hiding this comment

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

@plbossart I rebase this PR, thanks.


/* SOF ABI version number. Format within 32bit word is MMmmmppp */
Expand Down