| Message ID | 20231207222944.663893-9-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | soundwire/ASoC: speed-up downloads with BTP/BRA protocol | expand |
On 07-12-23, 16:29, Pierre-Louis Bossart wrote: > Add a convenience pointer to the 'sdw_bus' structure. BPT is a > dedicated stream which will typically not be handled by DAIs or > dailinks. Since there's only one BPT stream per link, storing the > pointer at the link level seems rather natural. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > include/linux/soundwire/sdw.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index e54c5bbd2b91..8db0cd7d0d89 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -965,6 +965,7 @@ struct sdw_master_ops { > * @stream_refcount: number of streams currently using this bus > * @btp_stream_refcount: number of BTP streams currently using this bus (should > * be zero or one, multiple streams per link is not supported). > + * @bpt_stream: pointer stored for convenience. > */ > struct sdw_bus { > struct device *dev; > @@ -996,6 +997,7 @@ struct sdw_bus { > int hw_sync_min_links; > int stream_refcount; > int bpt_stream_refcount; > + struct sdw_stream_runtime *bpt_stream; So we are limiting to single stream? Can we have multiple transfers queued up, which I guess might imply multiple streams?
On 12/18/23 05:55, Vinod Koul wrote: > On 07-12-23, 16:29, Pierre-Louis Bossart wrote: >> Add a convenience pointer to the 'sdw_bus' structure. BPT is a >> dedicated stream which will typically not be handled by DAIs or >> dailinks. Since there's only one BPT stream per link, storing the >> pointer at the link level seems rather natural. >> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> include/linux/soundwire/sdw.h | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h >> index e54c5bbd2b91..8db0cd7d0d89 100644 >> --- a/include/linux/soundwire/sdw.h >> +++ b/include/linux/soundwire/sdw.h >> @@ -965,6 +965,7 @@ struct sdw_master_ops { >> * @stream_refcount: number of streams currently using this bus >> * @btp_stream_refcount: number of BTP streams currently using this bus (should >> * be zero or one, multiple streams per link is not supported). >> + * @bpt_stream: pointer stored for convenience. >> */ >> struct sdw_bus { >> struct device *dev; >> @@ -996,6 +997,7 @@ struct sdw_bus { >> int hw_sync_min_links; >> int stream_refcount; >> int bpt_stream_refcount; >> + struct sdw_stream_runtime *bpt_stream; > > So we are limiting to single stream? Can we have multiple transfers > queued up, which I guess might imply multiple streams? Yes for now there is a BTP/BRA single stream active when there are no audio transfers taking place. This is the only way to guarantee predictable download/resume times. There is no mechanism to queue up transfers, be it from the same peripheral device or different ones. It would be up to the peripheral driver to wait for the BTP stream to be available. Adding a queuing mechanism is a bridge too far for now, most platforms only have 1 or 2 devices only, and a peripheral driver may or may not be ok with delayed downloads. For now the main ask is to reduce download times, a single stream is already a good start. There are other refinements we need to add as well, such as changing clocks to use the fastest gear. I'd like to proceed with baby steps...
On 18-12-23, 14:20, Pierre-Louis Bossart wrote: > > > On 12/18/23 05:55, Vinod Koul wrote: > > On 07-12-23, 16:29, Pierre-Louis Bossart wrote: > >> Add a convenience pointer to the 'sdw_bus' structure. BPT is a > >> dedicated stream which will typically not be handled by DAIs or > >> dailinks. Since there's only one BPT stream per link, storing the > >> pointer at the link level seems rather natural. > >> > >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > >> --- > >> include/linux/soundwire/sdw.h | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > >> index e54c5bbd2b91..8db0cd7d0d89 100644 > >> --- a/include/linux/soundwire/sdw.h > >> +++ b/include/linux/soundwire/sdw.h > >> @@ -965,6 +965,7 @@ struct sdw_master_ops { > >> * @stream_refcount: number of streams currently using this bus > >> * @btp_stream_refcount: number of BTP streams currently using this bus (should > >> * be zero or one, multiple streams per link is not supported). > >> + * @bpt_stream: pointer stored for convenience. > >> */ > >> struct sdw_bus { > >> struct device *dev; > >> @@ -996,6 +997,7 @@ struct sdw_bus { > >> int hw_sync_min_links; > >> int stream_refcount; > >> int bpt_stream_refcount; > >> + struct sdw_stream_runtime *bpt_stream; > > > > So we are limiting to single stream? Can we have multiple transfers > > queued up, which I guess might imply multiple streams? > > > Yes for now there is a BTP/BRA single stream active when there are no > audio transfers taking place. This is the only way to guarantee > predictable download/resume times. > > There is no mechanism to queue up transfers, be it from the same > peripheral device or different ones. It would be up to the peripheral > driver to wait for the BTP stream to be available. > > Adding a queuing mechanism is a bridge too far for now, most platforms > only have 1 or 2 devices only, and a peripheral driver may or may not be > ok with delayed downloads. For now the main ask is to reduce download > times, a single stream is already a good start. There are other > refinements we need to add as well, such as changing clocks to use the > fastest gear. I'd like to proceed with baby steps... Since the API is async in nature, I think it is very reasonable that we add the queue support (a simple list would do) and notify when the transfer is complete..
On 12/21/23 15:39, Vinod Koul wrote: > On 18-12-23, 14:20, Pierre-Louis Bossart wrote: >> >> >> On 12/18/23 05:55, Vinod Koul wrote: >>> On 07-12-23, 16:29, Pierre-Louis Bossart wrote: >>>> Add a convenience pointer to the 'sdw_bus' structure. BPT is a >>>> dedicated stream which will typically not be handled by DAIs or >>>> dailinks. Since there's only one BPT stream per link, storing the >>>> pointer at the link level seems rather natural. >>>> >>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>>> --- >>>> include/linux/soundwire/sdw.h | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h >>>> index e54c5bbd2b91..8db0cd7d0d89 100644 >>>> --- a/include/linux/soundwire/sdw.h >>>> +++ b/include/linux/soundwire/sdw.h >>>> @@ -965,6 +965,7 @@ struct sdw_master_ops { >>>> * @stream_refcount: number of streams currently using this bus >>>> * @btp_stream_refcount: number of BTP streams currently using this bus (should >>>> * be zero or one, multiple streams per link is not supported). >>>> + * @bpt_stream: pointer stored for convenience. >>>> */ >>>> struct sdw_bus { >>>> struct device *dev; >>>> @@ -996,6 +997,7 @@ struct sdw_bus { >>>> int hw_sync_min_links; >>>> int stream_refcount; >>>> int bpt_stream_refcount; >>>> + struct sdw_stream_runtime *bpt_stream; >>> >>> So we are limiting to single stream? Can we have multiple transfers >>> queued up, which I guess might imply multiple streams? >> >> >> Yes for now there is a BTP/BRA single stream active when there are no >> audio transfers taking place. This is the only way to guarantee >> predictable download/resume times. >> >> There is no mechanism to queue up transfers, be it from the same >> peripheral device or different ones. It would be up to the peripheral >> driver to wait for the BTP stream to be available. >> >> Adding a queuing mechanism is a bridge too far for now, most platforms >> only have 1 or 2 devices only, and a peripheral driver may or may not be >> ok with delayed downloads. For now the main ask is to reduce download >> times, a single stream is already a good start. There are other >> refinements we need to add as well, such as changing clocks to use the >> fastest gear. I'd like to proceed with baby steps... > > Since the API is async in nature, I think it is very reasonable that we > add the queue support (a simple list would do) and notify when the > transfer is complete.. On paper queueing is nice/elegant. In practice it's likely that there's a bit of configuration needed before/after each download, e.g. restart the DSP. It would also be an absolute horror to deal with error flow if one of the queued transfers did not succeed. I would be more than happy if we successfully enable a single transfer across multiple platforms, and look at queueing as a optimization. BTW even if there are multiple transfers queued, there's still only one stream active at a given time.
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index e54c5bbd2b91..8db0cd7d0d89 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -965,6 +965,7 @@ struct sdw_master_ops { * @stream_refcount: number of streams currently using this bus * @btp_stream_refcount: number of BTP streams currently using this bus (should * be zero or one, multiple streams per link is not supported). + * @bpt_stream: pointer stored for convenience. */ struct sdw_bus { struct device *dev; @@ -996,6 +997,7 @@ struct sdw_bus { int hw_sync_min_links; int stream_refcount; int bpt_stream_refcount; + struct sdw_stream_runtime *bpt_stream; }; int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
Add a convenience pointer to the 'sdw_bus' structure. BPT is a dedicated stream which will typically not be handled by DAIs or dailinks. Since there's only one BPT stream per link, storing the pointer at the link level seems rather natural. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- include/linux/soundwire/sdw.h | 2 ++ 1 file changed, 2 insertions(+)