soundwire: send multi sections in one BPT stream #5541
Conversation
bardliao
requested review from
Copilot,
plbossart,
ranj063,
shumingfan and
ujfalusi
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the SoundWire BPT (Bulk Register Access over Point-to-Point) implementation to support sending multiple register sections in a single BPT stream instead of requiring separate streams for each section.
- Refactors the BPT message structure to use an array of sections instead of single address/length/buffer fields
- Updates buffer size calculations to iterate over all sections and accumulate totals
- Modifies DMA buffer preparation and response checking to handle multiple sections sequentially
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| drivers/soundwire/bus.h | Defines new sdw_bpt_section structure and refactors sdw_bpt_msg to use section arrays |
| drivers/soundwire/bus.c | Updates message length validation to calculate total length across all sections |
| drivers/soundwire/cadence_master.h | Updates function signatures to accept section arrays instead of individual parameters |
| drivers/soundwire/cadence_master.c | Implements multi-section support in DMA buffer preparation and response checking |
| drivers/soundwire/intel_ace2x.c | Updates buffer size calculations and DMA preparation calls for multi-section support |
| drivers/soundwire/debugfs.c | Adds section allocation and initialization for debugging interface |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (buffer_size == total_num_bytes && (i + 1) < num_frames) { | ||
| sec_index++; | ||
| if (sec_index >= num_sec) { | ||
| dev_err(dev, "%s: incorrect section index %d i %d\n", | ||
| __func__, sec_index, i); | ||
| return -EINVAL; | ||
| } | ||
| p_buf = sec[sec_index].buf; | ||
| buffer_size = sec[sec_index].len; | ||
| total_num_bytes = 0; | ||
| } |
There was a problem hiding this comment.
The condition buffer_size == total_num_bytes may incorrectly trigger section advancement. This logic assumes sections are processed sequentially with exact byte counts, but the comparison should account for accumulated data across frames. Consider using a running total per section instead.
There was a problem hiding this comment.
buffer_size is the buffer size of each section and total_num_bytes is the buffer size that has been written. They should be exactly the same when a section is sent. Then reset buffer_size and total_num_bytes for the next section.
|
@bardliao this PR is not easy to review. Can you break it down by splitting into a couple of different patches and incrementally add the sections. For example, introduce the concept of sections first, then modify the current code so that you have only one section and then finally add the logic for multiple sections |
bardliao
force-pushed
the
bra-multi-sections
branch
2 times, most recently
from
cada244 to
d7ac43f
Compare
@ranj063 Updated. |
drivers/soundwire/bus.h
Outdated
|
|
||
| /** | ||
| * struct sdw_btp_msg - Message structure | ||
| * struct sdw_btp_section - Message structure |
There was a problem hiding this comment.
Message section structure?
drivers/soundwire/bus.h
Outdated
| * but a practical limit of SDW_BPT_MSG_MAX_BYTES is enforced. | ||
| * @dev_num: Slave device number | ||
| * @flags: transfer flags, indicate if xfer is read or write | ||
| * @buf: message data buffer (filled by host for write, filled |
drivers/soundwire/intel_ace2x.c
Outdated
| ret = sdw_cdns_check_read_response(cdns->dev, sdw->bpt_ctx.dmab_rx_bdl.area, | ||
| sdw->bpt_ctx.pdi1_buffer_size, | ||
| msg->sec[0].buf, msg->sec[0].len, sdw->bpt_ctx.num_frames, | ||
| msg->sec, 1, sdw->bpt_ctx.num_frames, |
There was a problem hiding this comment.
you set msg->sections to 1 in the first patch isnt it? Why not use that here and the above 2 places?
There was a problem hiding this comment.
msg->sections = 1 was set in debugfs which provides an example to use the helpers. And intel_ace2x_bpt_open_stream() didn't find the buffer size for other sections other than the first section yet. No matter what msg->sections' is set, it can handle 1 section. That's why I pass 1here and will change tomsg->sections' when it can handle other sections in the next commit.
Thanks @bardliao. A couple of minor comments only now |
Currently we send a BRA message with a start address with continuous registers in a BPT stream. However, a codec may need to write different register sections shortly. Introduce a register section in struct sdw_btp_msg which contain register start address, length, and buffer. This commit uses only 1 section for each BPT message. And we need to add up all BPT section lengh and check if it reach maximum BPT bytes. No function changes. Signed-off-by: Bard Liao <[email protected]>
We can get start_register, data_size, and buffer data from the new sdw_bpt_section parameter. Also, handle all register sections in the cdns BRA helpers. No function changes as section number is 1. Signed-off-by: Bard Liao <[email protected]>
Calculate required PDI buffer and pass the section number to the cdns BPT helpers. Signed-off-by: Bard Liao <[email protected]>
bardliao
force-pushed
the
bra-multi-sections
branch
from
d7ac43f to
1d279be
Compare
bardliao
changed the title
3 participants
Currently we send a BRA message with a start address with continuous registers in a BPT stream. However, a codec may need to write different register sections shortly. It makes more sense to send different register sections in a BPT stream and no need to close/open the BPT stream repeatedly.