Skip to content

topology: Ensure buffers get allocated on core 0 - #5097

Merged
lgirdwood merged 1 commit into
thesofproject:mainfrom
lkoenig:dev/buffer_on_core
Dec 22, 2021
Merged

lgirdwood merged 1 commit into
thesofproject:mainfrom
lkoenig:dev/buffer_on_core

Conversation

@lkoenig

@lkoenig lkoenig commented Dec 16, 2021

Copy link
Copy Markdown
Contributor

Pass the core number to the buffer instanciation so buffers can be
allocated on the same core the pipeline is scheduled.

Signed-off-by: Lionel Koenig lionelk@google.com

@lyakh

lyakh commented Dec 16, 2021

Copy link
Copy Markdown
Collaborator

Actually we recently discussed that buffers don't need their own core appropriation. The most common multi-core use-case is when complete pipelines are assigned to secondary cores. In those cases buffers logically are handled on the same cores. Not sure about cases when individual components are assigned to alternate cores, but in those cases inter-core buffers should anyway be used by both cores, so, shouldn't matter which of them allocates them?

@lgirdwood

lgirdwood commented Dec 16, 2021 •

Copy link
Copy Markdown
Member

@lkoenig does this fix an issue on your branch as @ranj063 did look at this a few months back as reported above by @lyakh. I think main branch has all the multicore updates that might mean this update may not needed (unless we have a regression) although it could also be the branch you are using is missing a fix that's in main.

@cujomalainey

Copy link
Copy Markdown
Contributor

@lgirdwood this was something we decided to send as we spotted it when we were chasing down multicore problems on TGL-013 when 2 pipelines are scheduled differently or the DAI is scheduled on a different core. Maybe we should drop the parameter in W_BUFFER as it looks suspicious.

@lkoenig

lkoenig commented Dec 16, 2021 •

Copy link
Copy Markdown
Contributor Author

My take on that is either:

  1. either we honor core request for buffer.
  2. we make sure the core number does not make it to the buffer.
    This change attempt to adresses 1.

@ranj063

ranj063 commented Dec 16, 2021 •

Copy link
Copy Markdown
Collaborator

@lkoenig this is change is OK but it really achieves only one thing ie when a pipeline is scheduled to run entirely on a secondary core, assigning the buffer core to have the same core ID as the pipeline core will keep the components as not shared. Buffers are all allocated on the runtime shared heap , so assigning the core here should make no difference to the allocation itself.

Eventually though, we agreed to change the logic for determining how components are identifed as shared or not-shared depending on what other components they are connected to and if they are on the same core or different cores.

@gkbldcig

Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@lgirdwood

Copy link
Copy Markdown
Member

@lkoenig do we still need this ?

@cujomalainey

Copy link
Copy Markdown
Contributor

@lkoenig do we still need this ?

@lgirdwood @ranj063 so what does that agreement mean for this API as this is a bit confusing when debugging multicore issues

@lgirdwood

Copy link
Copy Markdown
Member

@lkoenig do we still need this ?

@lgirdwood @ranj063 so what does that agreement mean for this API as this is a bit confusing when debugging multicore issues

Agree, it is a tad confusing. I think we do need to remove the core ID from the buffer topology config and just plain ignore it in the FW code (as this wont break ABI).
@lkoenig were you seeing any logic that was breaking without this or was any extra cache WB/INV taking place (due to any confusion about the data being copied between cores ?)

@lkoenig

lkoenig commented Dec 21, 2021

Copy link
Copy Markdown
Contributor Author

@lkoenig do we still need this ?

@lgirdwood @ranj063 so what does that agreement mean for this API as this is a bit confusing when debugging multicore issues

Agree, it is a tad confusing. I think we do need to remove the core ID from the buffer topology config and just plain ignore it in the FW code (as this wont break ABI). @lkoenig were you seeing any logic that was breaking without this or was any extra cache WB/INV taking place (due to any confusion about the data being copied between cores ?)

@lgirdwood I did not see any logic broken by the buffer allocation. I felt weird that everything for the pipeline was allcated on core 1 but the buffers where on core 0. I am happy to remove instead all the CORE allocation for the buffers if that is a better way.

@lgirdwood

Copy link
Copy Markdown
Member

. I am happy to remove instead all the CORE allocation for the buffers if that is a better way.

Ack - lets remove and make it easier for developers to follow :)

Ensure all buffers got allocated by core 0

Signed-off-by: Lionel Koenig <lionelk@google.com>
@lkoenig lkoenig changed the title topology: Ensure buffers get allocated on core. topology: Ensure buffers get allocated on core 0 Dec 22, 2021
@lkoenig

lkoenig commented Dec 22, 2021

Copy link
Copy Markdown
Contributor Author

@lgirdwood I updated the PR accordingly.

@lgirdwood
lgirdwood merged commit 996a067 into thesofproject:main Dec 22, 2021
@lkoenig
lkoenig deleted the dev/buffer_on_core branch January 10, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants