Skip to content

mailbox: use uncached address to write sw regs #8398

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
lgirdwood merged 1 commit into from
Nov 3, 2023
Merged

mailbox: use uncached address to write sw regs #8398

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
10 changes: 5 additions & 5 deletions src/platform/intel/ace/include/ace/lib/mailbox.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,12 @@ static inline uint64_t mailbox_sw_reg_read64(size_t offset)

static inline void mailbox_sw_regs_write(size_t offset, const void *src, size_t bytes)
{
int regs_write_err __unused = memcpy_s((void *)(MAILBOX_SW_REG_BASE + offset),
MAILBOX_SW_REG_SIZE - offset, src, bytes);
uint32_t __sparse_cache *ptr_c;
uint32_t *ptr;

assert(!regs_write_err);
dcache_writeback_region((__sparse_force void __sparse_cache *)(MAILBOX_SW_REG_BASE +
offset), bytes);
ptr_c = (uint32_t __sparse_cache *)(MAILBOX_SW_REG_BASE + offset);
ptr = cache_to_uncache(ptr_c);
memcpy_s(ptr, MAILBOX_SW_REG_SIZE - offset, src, bytes);
Copy link
Collaborator

@kv2019i kv2019i Oct 26, 2023

Choose a reason for hiding this comment

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

This is quite worrying finding, how come this has impact? Is the register read via cached address by other cores? This still doesn't explain why this PR would have any impact. We should reallly understand why dcache_writeback_region() is not working, we rely on this in many places.

Copy link
Contributor

@marcinszkudlinski marcinszkudlinski Oct 26, 2023

Choose a reason for hiding this comment

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

I'm not 100% sure if this is true - but I think that data inside the sw_mailbox may be modified by the Linux driver. dcache writeback may destroy changes if this is the case.
Cache access to the mailbox doesn't make sense anyway.

and
mailbox_sw_regs_write is used by dai to update llp node. In multi-core test, the updated llp node is invisable to other cores although dcache_writeback_region is used

dcache writeback is not enough, dcache_invalidate is needed on the reader core.

Copy link
Collaborator Author

@RanderWang RanderWang Oct 27, 2023

Choose a reason for hiding this comment

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

Yes, I am also very surprised. The reading method is mailbox_sw_reg_read, so it should be ok. This change use the same method for mailbox_sw_reg_read.

And there is a PR to switch to uncached memory by who ?

Copy link
Collaborator Author

@RanderWang RanderWang Oct 27, 2023

Choose a reason for hiding this comment

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

Linux driver doesn't access llp in mailbox now

Copy link
Collaborator

@lyakh lyakh Oct 27, 2023

Choose a reason for hiding this comment

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

dcache writeback is not enough, dcache_invalidate is needed on the reader core.

This would be a logical explanation, yes. Can we add such an invalidation on the reader side? But in principle - yes, a cached write plus a writeback shouldn't be faster than just a plain simple uncached write. We'd only benefit from cached access there if the same core were accessing that data multiple times without dropping caches - reading or writing (without writing back) data there. But I doubt that we ever do that.

Copy link
Collaborator

@lyakh lyakh Oct 30, 2023

Choose a reason for hiding this comment

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

The reg reads seem to use uncached addresses, so these seem correct. Nevertheless, i don't think the writeback makes given the range of addresses may not be cache aligned, so best to change the register write routines to use uncached addresses.

@kv2019i what kind of registers are those?

Copy link
Collaborator

@kv2019i kv2019i Oct 30, 2023

Choose a reason for hiding this comment

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

@lyakh these are "sw regs", so I guess it can be whatever sw/fw agree upon. Do you think the writeback is safe here?

Copy link
Collaborator

@lyakh lyakh Oct 31, 2023

Choose a reason for hiding this comment

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

@lyakh these are "sw regs", so I guess it can be whatever sw/fw agree upon. Do you think the writeback is safe here?

@kv2019i my understanding of "software registers" is that they're just generic locations in RAM used purely by the software without any direct hardware effects? If that's correct, then at the physical level they should also be handled like ordinary RAM, i.e. all cache management applies?

Copy link
Member

@lgirdwood lgirdwood Oct 31, 2023

Choose a reason for hiding this comment

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

My thoughs here are

  1. cache ops here are not being aligned or forced on cache line boundaries or sizes.
  2. No obvious locking, so can race (and corrupt with 1 above).
  3. The uncache alias is probably fixing this issue as the HW IO arbitration is probably enforcing serialised IO on the memory for all cores (i.e. the HW bus arbitration is a HW spinlock).

Copy link
Collaborator Author

@RanderWang RanderWang Nov 1, 2023

Choose a reason for hiding this comment

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

3. The

My thoughs here are

1. cache ops here are not being aligned or forced on cache line boundaries or sizes.

sw_regs_write may be no cache aligned since the llp slot size is 20 bytes

2. No obvious locking, so can race (and corrupt with 1 above).

we have lock to protect sw_regs_write

3. The uncache alias is probably fixing this issue as the HW IO arbitration is probably enforcing serialised IO on the memory for all cores (i.e. the HW bus arbitration is a HW spinlock).

}

#endif /* __ACE_LIB_MAILBOX_H__ */
Expand Down
10 changes: 5 additions & 5 deletions src/platform/intel/cavs/include/cavs/lib/mailbox.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,12 @@ static inline uint64_t mailbox_sw_reg_read64(size_t offset)

static inline void mailbox_sw_regs_write(size_t offset, const void *src, size_t bytes)
{
int regs_write_err __unused = memcpy_s((void *)(MAILBOX_SW_REG_BASE + offset),
MAILBOX_SW_REG_SIZE - offset, src, bytes);
uint32_t __sparse_cache *ptr_c;
uint32_t *ptr;

assert(!regs_write_err);
dcache_writeback_region((__sparse_force void __sparse_cache *)(MAILBOX_SW_REG_BASE +
offset), bytes);
ptr_c = (uint32_t __sparse_cache *)(MAILBOX_SW_REG_BASE + offset);
ptr = cache_to_uncache(ptr_c);
memcpy_s(ptr, MAILBOX_SW_REG_SIZE - offset, src, bytes);
}

#endif /* __CAVS_LIB_MAILBOX_H__ */
Expand Down