| Message ID | 20191217095851.19629-2-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: Enable HDAudio compress | expand |
On Tue, 17 Dec 2019 10:58:45 +0100, Cezary Rojewski wrote: > > Currently only PCM streams can enlist hdac_stream for their data > transfer. Add cstream field to hdac_ext_stream to expose possibility of > compress stream assignment in place of PCM one. > Limited to HOST-type only. > > Rather than copying entire hdac_ext_host_stream_assign, declare separate > PCM and compress wrappers and reuse it for both cases. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > include/sound/hdaudio.h | 1 + > include/sound/hdaudio_ext.h | 2 ++ > sound/hda/ext/hdac_ext_stream.c | 46 +++++++++++++++++++++++++++++---- > 3 files changed, 44 insertions(+), 5 deletions(-) > > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h > index e05b95e83d5a..9a8bf1eb7d69 100644 > --- a/include/sound/hdaudio.h > +++ b/include/sound/hdaudio.h > @@ -481,6 +481,7 @@ struct hdac_stream { > struct snd_pcm_substream *substream; /* assigned substream, > * set in PCM open > */ > + struct snd_compr_stream *cstream; > unsigned int format_val; /* format value to be set in the > * controller and the codec > */ One might use union for pointing to either PCM or compr stream and identify the type with some flag. struct hdac_stream { union { struct snd_pcm_substream *substream; struct snd_compr_stream *cstream; }; bool is_compr; .... But, I'm not advocating for this. Although this makes the stream assignment more handy, it might lead to refer to a wrong object if you don't check the flag properly, too. It really depends on the code. thanks, Takashi
On 2019-12-17 11:19, Takashi Iwai wrote: > On Tue, 17 Dec 2019 10:58:45 +0100, > Cezary Rojewski wrote: >> >> Currently only PCM streams can enlist hdac_stream for their data >> transfer. Add cstream field to hdac_ext_stream to expose possibility of >> compress stream assignment in place of PCM one. >> Limited to HOST-type only. >> >> Rather than copying entire hdac_ext_host_stream_assign, declare separate >> PCM and compress wrappers and reuse it for both cases. >> >> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >> --- >> include/sound/hdaudio.h | 1 + >> include/sound/hdaudio_ext.h | 2 ++ >> sound/hda/ext/hdac_ext_stream.c | 46 +++++++++++++++++++++++++++++---- >> 3 files changed, 44 insertions(+), 5 deletions(-) >> >> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h >> index e05b95e83d5a..9a8bf1eb7d69 100644 >> --- a/include/sound/hdaudio.h >> +++ b/include/sound/hdaudio.h >> @@ -481,6 +481,7 @@ struct hdac_stream { >> struct snd_pcm_substream *substream; /* assigned substream, >> * set in PCM open >> */ >> + struct snd_compr_stream *cstream; >> unsigned int format_val; /* format value to be set in the >> * controller and the codec >> */ > > One might use union for pointing to either PCM or compr stream and > identify the type with some flag. > > struct hdac_stream { > union { > struct snd_pcm_substream *substream; > struct snd_compr_stream *cstream; > }; > bool is_compr; > .... > > But, I'm not advocating for this. Although this makes the stream > assignment more handy, it might lead to refer to a wrong object if you > don't check the flag properly, too. It really depends on the code. > I'm happy with both - existing - and your variant. In essence, this causes simply: s/if (hstream->cstream)/if (hstream->is_compr)/g to occur. In general, I'm strong supporter of a "PCM-compr marriage" idea - both being combined in sense of having similar base in the future so one could make use of "snd_base_stream", checkout the is_compr field and cast into actual type (_pcm_ -or- _compr_) via container_of macro. This is more of a wish or song of the future for now, though. Compress and PCM ops streamlining is not within the scope of probes and requires much more work : )
On 2019-12-17 13:06, Cezary Rojewski wrote: > > > On 2019-12-17 11:19, Takashi Iwai wrote: >> On Tue, 17 Dec 2019 10:58:45 +0100, >> Cezary Rojewski wrote: >>> >>> Currently only PCM streams can enlist hdac_stream for their data >>> transfer. Add cstream field to hdac_ext_stream to expose possibility of >>> compress stream assignment in place of PCM one. >>> Limited to HOST-type only. >>> >>> Rather than copying entire hdac_ext_host_stream_assign, declare separate >>> PCM and compress wrappers and reuse it for both cases. >>> >>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >>> --- >>> include/sound/hdaudio.h | 1 + >>> include/sound/hdaudio_ext.h | 2 ++ >>> sound/hda/ext/hdac_ext_stream.c | 46 +++++++++++++++++++++++++++++---- >>> 3 files changed, 44 insertions(+), 5 deletions(-) >>> >>> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h >>> index e05b95e83d5a..9a8bf1eb7d69 100644 >>> --- a/include/sound/hdaudio.h >>> +++ b/include/sound/hdaudio.h >>> @@ -481,6 +481,7 @@ struct hdac_stream { >>> struct snd_pcm_substream *substream; /* assigned substream, >>> * set in PCM open >>> */ >>> + struct snd_compr_stream *cstream; >>> unsigned int format_val; /* format value to be set in the >>> * controller and the codec >>> */ >> >> One might use union for pointing to either PCM or compr stream and >> identify the type with some flag. >> >> struct hdac_stream { >> union { >> struct snd_pcm_substream *substream; >> struct snd_compr_stream *cstream; >> }; >> bool is_compr; >> .... >> >> But, I'm not advocating for this. Although this makes the stream >> assignment more handy, it might lead to refer to a wrong object if you >> don't check the flag properly, too. It really depends on the code. >> > > I'm happy with both - existing - and your variant. In essence, this > causes simply: s/if (hstream->cstream)/if (hstream->is_compr)/g to occur. > > In general, I'm strong supporter of a "PCM-compr marriage" idea - both > being combined in sense of having similar base in the future so one > could make use of "snd_base_stream", checkout the is_compr field and > cast into actual type (_pcm_ -or- _compr_) via container_of macro. > > This is more of a wish or song of the future for now, though. Compress > and PCM ops streamlining is not within the scope of probes and requires > much more work : ) > After thinking more about it, I'd rather stick to the current approach. Patch 3 of the series ([PATCH 3/7] ALSA: hda: Interrupt servicing and BDL setup for compress streams): (...) /* reset BDL address */ snd_hdac_stream_writel(azx_dev, SD_BDLPL, 0); @@ -486,15 +493,22 @@ int snd_hdac_stream_set_params(struct hdac_stream *azx_dev, unsigned int format_val) { struct snd_pcm_substream *substream = azx_dev->substream; + struct snd_compr_stream *cstream = azx_dev->cstream; unsigned int bufsize, period_bytes; unsigned int no_period_wakeup; int err; - if (!substream) + if (substream) { + bufsize = snd_pcm_lib_buffer_bytes(substream); + period_bytes = snd_pcm_lib_period_bytes(substream); + no_period_wakeup = substream->runtime->no_period_wakeup; + } else if (cstream) { + bufsize = cstream->runtime->buffer_size; + period_bytes = cstream->runtime->fragment_size; + no_period_wakeup = 0; + } else { return -EINVAL; - bufsize = snd_pcm_lib_buffer_bytes(substream); - period_bytes = snd_pcm_lib_period_bytes(substream); - no_period_wakeup = substream->runtime->no_period_wakeup; + } if (bufsize != azx_dev->bufsize || period_bytes != azx_dev->period_bytes || (...) the if/ else if/ else block would have to be reorganized and start with pointer validity first (and return -EINVAL if evaluated to true), e.g.: if (!azx_dev->substream) { return -EINVAL; } else if (axz_dev->is_compr) { // compr stuff } else { // pcm stuff } Now, with union { substream; cstream }; approach, this is valid but may be confusing for a reader - code checks for substream ptr _only_ as additional cstream-check would be redundant. On the other hand: if (substream) { // pcm stuff } else if (cstream) { // compr stuff } else { return -EINVAL; } is clear to everyone. It's true though that only one ptr may be assigned (substream -or- cstream) so union had its point too. I'd value readability over that, though. With that said, I don't see any other suggestions for said series. Should I resend as v2 with no changes (minus "[PATCH 6/7] ASoC: compress: Add pm_runtime support" patch as it has already been accepted by Mark) or leave as is? Czarek
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index e05b95e83d5a..9a8bf1eb7d69 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -481,6 +481,7 @@ struct hdac_stream { struct snd_pcm_substream *substream; /* assigned substream, * set in PCM open */ + struct snd_compr_stream *cstream; unsigned int format_val; /* format value to be set in the * controller and the codec */ diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index ef88b20c7b0a..ec01f2024f0b 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -84,6 +84,8 @@ int snd_hdac_ext_stream_init_all(struct hdac_bus *bus, int start_idx, int num_stream, int dir); void snd_hdac_stream_free_all(struct hdac_bus *bus); void snd_hdac_link_free_all(struct hdac_bus *bus); +struct hdac_ext_stream *snd_hdac_ext_cstream_assign(struct hdac_bus *bus, + struct snd_compr_stream *cstream); struct hdac_ext_stream *snd_hdac_ext_stream_assign(struct hdac_bus *bus, struct snd_pcm_substream *substream, int type); diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c index 6b1b4b834bae..b898b6c0b55d 100644 --- a/sound/hda/ext/hdac_ext_stream.c +++ b/sound/hda/ext/hdac_ext_stream.c @@ -14,6 +14,7 @@ #include <sound/pcm.h> #include <sound/hda_register.h> #include <sound/hdaudio_ext.h> +#include <sound/compress_driver.h> /** * snd_hdac_ext_stream_init - initialize each stream (aka device) @@ -281,8 +282,7 @@ hdac_ext_link_stream_assign(struct hdac_bus *bus, } static struct hdac_ext_stream * -hdac_ext_host_stream_assign(struct hdac_bus *bus, - struct snd_pcm_substream *substream) +hdac_ext_host_stream_assign(struct hdac_bus *bus, int direction) { struct hdac_ext_stream *res = NULL; struct hdac_stream *stream = NULL; @@ -296,7 +296,7 @@ hdac_ext_host_stream_assign(struct hdac_bus *bus, struct hdac_ext_stream *hstream = container_of(stream, struct hdac_ext_stream, hstream); - if (stream->direction != substream->stream) + if (stream->direction != direction) continue; if (!stream->opened) { @@ -310,13 +310,49 @@ hdac_ext_host_stream_assign(struct hdac_bus *bus, spin_lock_irq(&bus->reg_lock); res->hstream.opened = 1; res->hstream.running = 0; - res->hstream.substream = substream; + res->hstream.substream = NULL; + res->hstream.cstream = NULL; spin_unlock_irq(&bus->reg_lock); } return res; } +static struct hdac_ext_stream * +hdac_ext_host_stream_pcm_assign(struct hdac_bus *bus, + struct snd_pcm_substream *substream) +{ + struct hdac_ext_stream *res; + + res = hdac_ext_host_stream_assign(bus, substream->stream); + if (res) + res->hstream.substream = substream; + + return res; +} + +/** + * snd_hdac_ext_cstream_assign - assign a host stream for compress + * @bus: HD-audio core bus + * @cstream: Compress stream to assign + * + * Assign an unused host stream for the given compress stream. + * If no stream is free, NULL is returned. Stream is decoupled + * before assignment. + */ +struct hdac_ext_stream *snd_hdac_ext_cstream_assign(struct hdac_bus *bus, + struct snd_compr_stream *cstream) +{ + struct hdac_ext_stream *res; + + res = hdac_ext_host_stream_assign(bus, cstream->direction); + if (res) + res->hstream.cstream = cstream; + + return res; +} +EXPORT_SYMBOL_GPL(snd_hdac_ext_cstream_assign); + /** * snd_hdac_ext_stream_assign - assign a stream for the PCM * @bus: HD-audio core bus @@ -350,7 +386,7 @@ struct hdac_ext_stream *snd_hdac_ext_stream_assign(struct hdac_bus *bus, return hstream; case HDAC_EXT_STREAM_TYPE_HOST: - return hdac_ext_host_stream_assign(bus, substream); + return hdac_ext_host_stream_pcm_assign(bus, substream); case HDAC_EXT_STREAM_TYPE_LINK: return hdac_ext_link_stream_assign(bus, substream);
Currently only PCM streams can enlist hdac_stream for their data transfer. Add cstream field to hdac_ext_stream to expose possibility of compress stream assignment in place of PCM one. Limited to HOST-type only. Rather than copying entire hdac_ext_host_stream_assign, declare separate PCM and compress wrappers and reuse it for both cases. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- include/sound/hdaudio.h | 1 + include/sound/hdaudio_ext.h | 2 ++ sound/hda/ext/hdac_ext_stream.c | 46 +++++++++++++++++++++++++++++---- 3 files changed, 44 insertions(+), 5 deletions(-)