-
Notifications
You must be signed in to change notification settings - Fork 311
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.
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
fixed bug that copy order incorrect at host component. #9426
base: main
Are you sure you want to change the base?
Conversation
chihualee
commented
Sep 3, 2024
- fixed bug that copy order incorrect at host component.
- a lot of log just print wPtr and rPtr of local-buffer and dma-buffer, it can confirm copy order correctness. It should disable logs manually.
2. a lot of log just print wPtr and rPtr of local-buffer and dma-buffer, it can confirm copy order correctness. It should disable logs manually.
Can one of the admins verify this patch?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple changes in single commit, unclear what this is fixing,
@@ -194,6 +194,8 @@ static uint32_t host_get_copy_bytes_one_shot(struct host_data *hd, struct comp_d | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check existing git commits in this project for examples on how to write git commit. SOF follows similar practises as Linux kernel and Zephyr. One git commit per logical change, proper summary line, explains what bug you are fixing, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please explain what order is incorrect. AFAICT you're only moving the dma_buffer_copy_to() for the one shot case and this will break normal copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranj063
sorry, I am new to GitHub.
As mentioned in https://github.com/orgs/thesofproject/discussions/9362,
For capture stream scenario, I find a bug that copying order incorrect in Host component.
The copy order should be: Local-buffer => DMA-buffer => Host-buffer.
But actually it is first DMA-buffer => Host-buffer, then Local-buffer => DMA-buffer.
I think host_copy_one_shot() is correct for playback, but incorrect for capture.
So my patch is let host copy change into Local-buffer => DMA-buffer => Host-buffer:
Just move (copy from Local-buffer to DMA-buffer)
from host_common_update() to host_copy_one_shot().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kv2019i
sorry, I am new to GitHub.
I think we discuss bug and solution first.
Finally, if the patch is fine I will modify the commit log more normally.
} else { | ||
source = hd->local_buffer; | ||
sink = hd->dma_buffer; | ||
ret = dma_buffer_copy_to(source, sink, hd->process, bytes, DUMMY_CHMAP); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chihualee any update, this part looks wrong as capture uses the DMA buffer for sink, unless your device has no DMA to copy data to host driver from DSP (i.e. you have shared memory) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My capture mechanism is almost the same as original legacy-host,
first data from local buffer to DMA buffer,
then data from DMA buffer to Host buffer.
src/audio/host-legacy.c
Comment on lines -237 to 250
I think this part is doing that capture copying data from local buffer to DMA buffer.
I just move this part to src/audio/host-legacy.c
host_copy_one_shot() around at lines 214
Because it should do first.