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