Re: [alsa-devel] [PATCH 1/7] ALSA: hda: Allow for compress stream to hdac_ext_stream assignment

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

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