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

[Cezary Rojewski <cezary.rojewski@xxxxxxxxx>]

On 2025-01-10 5:44 PM, Takashi Iwai wrote:
> 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.

Somehow I did not understand your initial point. Now I do. Indeed, I 
should not have altered setup_bdle() in such fashion as it's used by 
_everyone_. Will review the change and come back with something better.

Kind regards,
Czarek