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@xxxxxxxxx>
---
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
