diff mbox series

[v3] ALSA: hda: Transfer firmware in two chunks

Message ID 20250114125813.11567-1-cezary.rojewski@intel.com (mailing list archive)
State Superseded
Headers show
Series [v3] ALSA: hda: Transfer firmware in two chunks | expand

Commit Message

Cezary Rojewski Jan. 14, 2025, 12:58 p.m. UTC
As per specification, SDxLVI shall be at least 1 i.e.: two chunks to
perform a valid transfer. This is true for the PCM transfer code but
not firmware-transfer one.

Technical background:
- the LVI > 0 rule shall be obeyed in PCM transfer
- HW permits LVI == 0 when transfer is SW-controlled (SPIB)
- FW download is not a PCM transfer and is SW-controlled (SPIB)

The above is the fundament which AudioDSP firmware loading functions
have been built upon and worked since 2016. The presented changes are to
align the loading flows and avoid rising more questions in the future.

Achieve the goal by splitting snd_hdac_stream_setup_periods() into
substream-dependent and -independent part. Let snd_hdac_dsp_prepare()
utilize the latter so that both DSP-loading and PCM flows utilize same
BLDE setup loop which already takes care of cutting the buffer based on
azx_dev->period_bytes.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/hdaudio.h |  2 ++
 sound/hda/hdac_stream.c | 64 ++++++++++++++++++++++++-----------------
 2 files changed, 40 insertions(+), 26 deletions(-)

Comments

Takashi Iwai Jan. 14, 2025, 1:15 p.m. UTC | #1
On Tue, 14 Jan 2025 13:58:13 +0100,
Cezary Rojewski wrote:
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -577,6 +577,8 @@ struct hdac_stream *snd_hdac_get_stream(struct hdac_bus *bus,
>  
>  int snd_hdac_stream_setup(struct hdac_stream *azx_dev, bool code_loading);
>  void snd_hdac_stream_cleanup(struct hdac_stream *azx_dev);
> +int snd_hdac_stream_setup_bdle(struct hdac_stream *azx_dev, struct snd_dma_buffer *dmab,
> +			       struct snd_pcm_runtime *runtime);

Will you use this function by other driver in future?
Otherwise it makes little sense to export this function.


thanks,

Takashi
Cezary Rojewski Jan. 14, 2025, 6:24 p.m. UTC | #2
On 2025-01-14 2:15 PM, Takashi Iwai wrote:
> On Tue, 14 Jan 2025 13:58:13 +0100,
> Cezary Rojewski wrote:
>> --- a/include/sound/hdaudio.h
>> +++ b/include/sound/hdaudio.h
>> @@ -577,6 +577,8 @@ struct hdac_stream *snd_hdac_get_stream(struct hdac_bus *bus,
>>   
>>   int snd_hdac_stream_setup(struct hdac_stream *azx_dev, bool code_loading);
>>   void snd_hdac_stream_cleanup(struct hdac_stream *azx_dev);
>> +int snd_hdac_stream_setup_bdle(struct hdac_stream *azx_dev, struct snd_dma_buffer *dmab,
>> +			       struct snd_pcm_runtime *runtime);
> 
> Will you use this function by other driver in future?
> Otherwise it makes little sense to export this function.

While we do have some refactor-related plans for hdac_stream.c, I'm 
treating that as a separate subject and thus exporting the symbol should 
not be present here. Sending v4, thank you!

Czarek
diff mbox series

Patch

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index b098ceadbe74..3a3cfb832ddc 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -577,6 +577,8 @@  struct hdac_stream *snd_hdac_get_stream(struct hdac_bus *bus,
 
 int snd_hdac_stream_setup(struct hdac_stream *azx_dev, bool code_loading);
 void snd_hdac_stream_cleanup(struct hdac_stream *azx_dev);
+int snd_hdac_stream_setup_bdle(struct hdac_stream *azx_dev, struct snd_dma_buffer *dmab,
+			       struct snd_pcm_runtime *runtime);
 int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev);
 int snd_hdac_stream_set_params(struct hdac_stream *azx_dev,
 				unsigned int format_val);
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index 2670792f43b4..da6c48eaf29c 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -492,32 +492,21 @@  static int setup_bdle(struct hdac_bus *bus,
 }
 
 /**
- * snd_hdac_stream_setup_periods - set up BDL entries
+ * snd_hdac_stream_setup_bdle - set up BDL entries
  * @azx_dev: HD-audio core stream to set up
+ * @dmab: allocated DMA buffer
+ * @runtime: substream runtime, optional
  *
  * Set up the buffer descriptor table of the given stream based on the
  * period and buffer sizes of the assigned PCM substream.
  */
-int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
+int snd_hdac_stream_setup_bdle(struct hdac_stream *azx_dev, struct snd_dma_buffer *dmab,
+			       struct snd_pcm_runtime *runtime)
 {
 	struct hdac_bus *bus = azx_dev->bus;
-	struct snd_pcm_substream *substream = azx_dev->substream;
-	struct snd_compr_stream *cstream = azx_dev->cstream;
-	struct snd_pcm_runtime *runtime = NULL;
-	struct snd_dma_buffer *dmab;
-	__le32 *bdl;
 	int i, ofs, periods, period_bytes;
 	int pos_adj, pos_align;
-
-	if (substream) {
-		runtime = substream->runtime;
-		dmab = snd_pcm_get_dma_buf(substream);
-	} else if (cstream) {
-		dmab = snd_pcm_get_dma_buf(cstream);
-	} else {
-		WARN(1, "No substream or cstream assigned\n");
-		return -EINVAL;
-	}
+	__le32 *bdl;
 
 	/* reset BDL address */
 	snd_hdac_stream_writel(azx_dev, SD_BDLPL, 0);
@@ -571,6 +560,34 @@  int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
 		azx_dev->bufsize, period_bytes);
 	return -EINVAL;
 }
+EXPORT_SYMBOL_GPL(snd_hdac_stream_setup_bdle);
+
+/**
+ * snd_hdac_stream_setup_periods - set up BDL entries
+ * @azx_dev: HD-audio core stream to set up
+ *
+ * Set up the buffer descriptor table of the given stream based on the
+ * period and buffer sizes of the assigned PCM substream.
+ */
+int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
+{
+	struct snd_pcm_substream *substream = azx_dev->substream;
+	struct snd_compr_stream *cstream = azx_dev->cstream;
+	struct snd_pcm_runtime *runtime = NULL;
+	struct snd_dma_buffer *dmab;
+
+	if (substream) {
+		runtime = substream->runtime;
+		dmab = snd_pcm_get_dma_buf(substream);
+	} else if (cstream) {
+		dmab = snd_pcm_get_dma_buf(cstream);
+	} else {
+		WARN(1, "No substream or cstream assigned\n");
+		return -EINVAL;
+	}
+
+	return snd_hdac_stream_setup_bdle(azx_dev, dmab, runtime);
+}
 EXPORT_SYMBOL_GPL(snd_hdac_stream_setup_periods);
 
 /**
@@ -923,7 +940,6 @@  int snd_hdac_dsp_prepare(struct hdac_stream *azx_dev, unsigned int format,
 			 unsigned int byte_size, struct snd_dma_buffer *bufp)
 {
 	struct hdac_bus *bus = azx_dev->bus;
-	__le32 *bdl;
 	int err;
 
 	snd_hdac_dsp_lock(azx_dev);
@@ -943,18 +959,14 @@  int snd_hdac_dsp_prepare(struct hdac_stream *azx_dev, unsigned int format,
 
 	azx_dev->substream = NULL;
 	azx_dev->bufsize = byte_size;
-	azx_dev->period_bytes = byte_size;
+	/* It is recommended to transfer the firmware in two or more chunks. */
+	azx_dev->period_bytes = byte_size / 2;
 	azx_dev->format_val = format;
+	azx_dev->no_period_wakeup = 1;
 
 	snd_hdac_stream_reset(azx_dev);
 
-	/* reset BDL address */
-	snd_hdac_stream_writel(azx_dev, SD_BDLPL, 0);
-	snd_hdac_stream_writel(azx_dev, SD_BDLPU, 0);
-
-	azx_dev->frags = 0;
-	bdl = (__le32 *)azx_dev->bdl.area;
-	err = setup_bdle(bus, bufp, azx_dev, &bdl, 0, byte_size, 0);
+	err = snd_hdac_stream_setup_bdle(azx_dev, bufp, NULL);
 	if (err < 0)
 		goto error;