diff mbox series

[v4,06/11] ASoC: SOF: Intel: Account for compress streams when servicing IRQs

Message ID 20200131123115.14247-7-cezary.rojewski@intel.com (mailing list archive)
State New, archived
Headers show
Series ASoC: SOF: Data probing | expand

Commit Message

Cezary Rojewski Jan. 31, 2020, 12:31 p.m. UTC 
Update stream irq handler definition to correctly set hdac_stream
current position when servicing stream interrupts for compress streams.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/hdaudio.h          |  1 +
 sound/soc/sof/intel/hda-stream.c | 26 ++++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

Comments

Pierre-Louis Bossart Jan. 31, 2020, 4:39 p.m. UTC | #1 
Maybe be a paranoid check but the types used in this patch seem to need 
additional work:

> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 9a8bf1eb7d69..9a24d57f0cf2 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -496,6 +496,7 @@ struct hdac_stream {
>   	bool locked:1;
>   	bool stripe:1;			/* apply stripe control */
>   
> +	unsigned long curr_pos;

'long' is an error-prone definition...

>   	/* timestamp */
>   	unsigned long start_wallclk;	/* start + minimum wallclk */
>   	unsigned long period_wallclk;	/* wallclk for period */
> diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
> index c0ab9bb2a797..c8920a60e346 100644
> --- a/sound/soc/sof/intel/hda-stream.c
> +++ b/sound/soc/sof/intel/hda-stream.c
> @@ -571,6 +571,23 @@ bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev)
>   	return ret;
>   }
>   
> +static void hda_dsp_set_bytes_transferred(struct hdac_stream *hstream,
> +					  u64 buffer_size)
> +{
> +	unsigned int prev_pos;
> +	int pos, num_bytes;
> +
> +	div_u64_rem(hstream->curr_pos, buffer_size, &prev_pos);

... here you use it as a u64 value so I guess the intent was to use more 
than 32 bits?

But then the u64 for the 'buffer size' argument is also not consistent 
with the definition of div_u64_rem, it's got to be u32, or you wanted to 
use div64_64_rem?

static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)

prev_pos should also be declared as u32 to avoid any ambiguity.

> +	pos = snd_hdac_stream_get_pos_posbuf(hstream);
> +
> +	if (pos < prev_pos)
> +		num_bytes = (buffer_size - prev_pos) +  pos;
> +	else
> +		num_bytes = pos - prev_pos;
> +
> +	hstream->curr_pos += num_bytes;

... and here it's a never ending-increment that is likely to hit a 
32-bit ceiling.

I remember we made a mistake some time back on compressed stuff and 
updated the counters to rely on 64 bits. Unless this is a hardware 
counter (and then we should use u32 and deal with overflow), we should 
use u64, no?
Cezary Rojewski Feb. 18, 2020, 2:49 p.m. UTC | #2 
On 2020-01-31 17:39, Pierre-Louis Bossart wrote:
> Maybe be a paranoid check but the types used in this patch seem to need 
> additional work:
> 
>> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
>> index 9a8bf1eb7d69..9a24d57f0cf2 100644
>> --- a/include/sound/hdaudio.h
>> +++ b/include/sound/hdaudio.h
>> @@ -496,6 +496,7 @@ struct hdac_stream {
>>       bool locked:1;
>>       bool stripe:1;            /* apply stripe control */
>> +    unsigned long curr_pos;
> 
> 'long' is an error-prone definition...
> 
>>       /* timestamp */
>>       unsigned long start_wallclk;    /* start + minimum wallclk */
>>       unsigned long period_wallclk;    /* wallclk for period */
>> diff --git a/sound/soc/sof/intel/hda-stream.c 
>> b/sound/soc/sof/intel/hda-stream.c
>> index c0ab9bb2a797..c8920a60e346 100644
>> --- a/sound/soc/sof/intel/hda-stream.c
>> +++ b/sound/soc/sof/intel/hda-stream.c
>> @@ -571,6 +571,23 @@ bool hda_dsp_check_stream_irq(struct snd_sof_dev 
>> *sdev)
>>       return ret;
>>   }
>> +static void hda_dsp_set_bytes_transferred(struct hdac_stream *hstream,
>> +                      u64 buffer_size)
>> +{
>> +    unsigned int prev_pos;
>> +    int pos, num_bytes;
>> +
>> +    div_u64_rem(hstream->curr_pos, buffer_size, &prev_pos);
> 
> ... here you use it as a u64 value so I guess the intent was to use more 
> than 32 bits?
> 
> But then the u64 for the 'buffer size' argument is also not consistent 
> with the definition of div_u64_rem, it's got to be u32, or you wanted to 
> use div64_64_rem?
> 
> static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
> 
> prev_pos should also be declared as u32 to avoid any ambiguity.
> 
>> +    pos = snd_hdac_stream_get_pos_posbuf(hstream);
>> +
>> +    if (pos < prev_pos)
>> +        num_bytes = (buffer_size - prev_pos) +  pos;
>> +    else
>> +        num_bytes = pos - prev_pos;
>> +
>> +    hstream->curr_pos += num_bytes;
> 
> ... and here it's a never ending-increment that is likely to hit a 
> 32-bit ceiling.
> 
> I remember we made a mistake some time back on compressed stuff and 
> updated the counters to rely on 64 bits. Unless this is a hardware 
> counter (and then we should use u32 and deal with overflow), we should 
> use u64, no?

All addressed in v5, thanks for review!
diff mbox series

Patch

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 9a8bf1eb7d69..9a24d57f0cf2 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -496,6 +496,7 @@  struct hdac_stream {
 	bool locked:1;
 	bool stripe:1;			/* apply stripe control */
 
+	unsigned long curr_pos;
 	/* timestamp */
 	unsigned long start_wallclk;	/* start + minimum wallclk */
 	unsigned long period_wallclk;	/* wallclk for period */
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index c0ab9bb2a797..c8920a60e346 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -571,6 +571,23 @@  bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev)
 	return ret;
 }
 
+static void hda_dsp_set_bytes_transferred(struct hdac_stream *hstream,
+					  u64 buffer_size)
+{
+	unsigned int prev_pos;
+	int pos, num_bytes;
+
+	div_u64_rem(hstream->curr_pos, buffer_size, &prev_pos);
+	pos = snd_hdac_stream_get_pos_posbuf(hstream);
+
+	if (pos < prev_pos)
+		num_bytes = (buffer_size - prev_pos) +  pos;
+	else
+		num_bytes = pos - prev_pos;
+
+	hstream->curr_pos += num_bytes;
+}
+
 static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)
 {
 	struct sof_intel_hda_dev *sof_hda = bus_to_sof_hda(bus);
@@ -588,14 +605,19 @@  static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)
 			snd_hdac_stream_writeb(s, SD_STS, sd_status);
 
 			active = true;
-			if (!s->substream ||
+			if ((!s->substream && !s->cstream) ||
 			    !s->running ||
 			    (sd_status & SOF_HDA_CL_DMA_SD_INT_COMPLETE) == 0)
 				continue;
 
 			/* Inform ALSA only in case not do that with IPC */
-			if (sof_hda->no_ipc_position)
+			if (s->substream && sof_hda->no_ipc_position) {
 				snd_sof_pcm_period_elapsed(s->substream);
+			} else if (s->cstream) {
+				hda_dsp_set_bytes_transferred(s,
+					s->cstream->runtime->buffer_size);
+				snd_compr_fragment_elapsed(s->cstream);
+			}
 		}
 	}