| Message ID | 20250110113326.3809897-3-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: hda: Compilation and firmware-loading fixes | expand |
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. thanks, Takashi
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
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);
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(-)