Re: [PATCH v2 2/2] ALSA: hda: Transfer firmware in two chunks

[Takashi Iwai <tiwai@xxxxxxx>]

On Fri, 10 Jan 2025 12:33:26 +0100,
Cezary Rojewski wrote:
> 
> 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.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
> ---
>  sound/hda/hdac_stream.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> index 2670792f43b4..18d74a28a246 100644
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -455,6 +455,7 @@ static int setup_bdle(struct hdac_bus *bus,
>  		      struct hdac_stream *azx_dev, __le32 **bdlp,
>  		      int ofs, int size, int with_ioc)
>  {
> +	u32 bdle_size = size / 2;
>  	__le32 *bdl = *bdlp;
>  
>  	while (size > 0) {
> @@ -469,7 +470,7 @@ static int setup_bdle(struct hdac_bus *bus,
>  		bdl[0] = cpu_to_le32((u32)addr);
>  		bdl[1] = cpu_to_le32(upper_32_bits(addr));
>  		/* program the size field of the BDL entry */
> -		chunk = snd_sgbuf_get_chunk_size(dmab, ofs, size);
> +		chunk = snd_sgbuf_get_chunk_size(dmab, ofs, bdle_size);
>  		/* one BDLE cannot cross 4K boundary on CTHDA chips */
>  		if (bus->align_bdle_4k) {
>  			u32 remain = 0x1000 - (ofs & 0xfff);

I still think that it's not best place to change.

For PCM, this workaround isn't needed in most cases, because
setup_bdle() is called per period, and periods_min = 2 for
snd-hda-intel.  Doing extra splitting is superfluous for PCM.

That said, if we need a workaround of the split, it should be done
conditionally for the firmware stream.


thanks,

Takashi