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

[Cezary Rojewski <cezary.rojewski@intel.com>]

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@intel.com>
>> ---
>>   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