[PATCH v4 1/2] ASoC: SOF: introduce no_stream_position so host_period_bytes preserves its data

[PATCH v4 1/2] ASoC: SOF: introduce no_stream_position so host_period_bytes preserves its data

[Rajwa, Marcin]

From: Marcin Rajwa <marcin.rajwa@linux.intel.com>

Change the use of host_period_bytes. So far this field was used
as an bool value indicating whether FW should send stream position
update. With this patch we use host_period_bytes to provide firmware
information about the frequency of host interrupts aimed to read
its input buffer. This is accoring to ALSA definition of 'FramePeriod'.
Knowing this firmware can safely copy large/irregular chunks of data
(like data comming from i.e draining task) without the risk of buffer
overflow.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

---
  include/sound/sof/stream.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/sound/sof/stream.h b/include/sound/sof/stream.h
index 643f175cb479..06af4ecb2584 100644
--- a/include/sound/sof/stream.h
+++ b/include/sound/sof/stream.h
@@ -83,10 +83,10 @@ struct sof_ipc_stream_params {
      uint16_t sample_valid_bytes;
      uint16_t sample_container_bytes;

-    /* for notifying host period has completed - 0 means no period IRQ */
      uint32_t host_period_bytes;
+    uint16_t no_stream_position; /* 1 means no IPC for position update */

-    uint32_t reserved[2];
+    uint16_t reserved[3];
      uint16_t chmap[SOF_IPC_MAX_CHANNELS];    /**< channel map - 
SOF_CHMAP_ */
  } __packed;


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH v4 2/2] ASoC: SOF: Intel: fix reset of host_period_bytes

[Rajwa, Marcin]

From: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch prevents the reset of host period bytes
and uses no_stream_position to record requests
for stream position.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
  sound/soc/sof/intel/hda-pcm.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c
index 9b730f183529..956407cf59ea 100644
--- a/sound/soc/sof/intel/hda-pcm.c
+++ b/sound/soc/sof/intel/hda-pcm.c
@@ -116,9 +116,9 @@ int hda_dsp_pcm_hw_params(struct snd_sof_dev *sdev,
      /* disable SPIB, to enable buffer wrap for stream */
      hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_DISABLE, 0);

-    /* set host_period_bytes to 0 if no IPC position */
+    /* update no_stream_position flag for ipc params */
      if (hda && hda->no_ipc_position)
-        ipc_params->host_period_bytes = 0;
+        ipc_params->no_stream_position = 1;

      ipc_params->stream_tag = hstream->stream_tag;
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v4 1/2] ASoC: SOF: introduce no_stream_position so host_period_bytes preserves its data

[Pierre-Louis Bossart]

On 7/30/19 5:16 AM, Rajwa, Marcin wrote:
> From: Marcin Rajwa <marcin.rajwa@linux.intel.com>
> 
> Change the use of host_period_bytes. So far this field was used
> as an bool value indicating whether FW should send stream position
> update. With this patch we use host_period_bytes to provide firmware
> information about the frequency of host interrupts aimed to read
> its input buffer. This is accoring to ALSA definition of 'FramePeriod'.

according to the

> Knowing this firmware can safely copy large/irregular chunks of data

why irregular? ALSA periods are pretty regular and predictable.

> (like data comming from i.e draining task) without the risk of buffer

coming

Please proof-read your commit messages (and use an editor which 
spell-checks for you), typos and misleading information don't exactly 
boost trust in the suggested patch, regardless of its merits.

> overflow.
> 
> Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> 
> ---
>   include/sound/sof/stream.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sound/sof/stream.h b/include/sound/sof/stream.h
> index 643f175cb479..06af4ecb2584 100644
> --- a/include/sound/sof/stream.h
> +++ b/include/sound/sof/stream.h
> @@ -83,10 +83,10 @@ struct sof_ipc_stream_params {
>       uint16_t sample_valid_bytes;
>       uint16_t sample_container_bytes;
> 
> -    /* for notifying host period has completed - 0 means no period IRQ */
>       uint32_t host_period_bytes;
> +    uint16_t no_stream_position; /* 1 means no IPC for position update */
> 
> -    uint32_t reserved[2];
> +    uint16_t reserved[3];
>       uint16_t chmap[SOF_IPC_MAX_CHANNELS];    /**< channel map - 
> SOF_CHMAP_ */
>   } __packed;
> 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v4 1/2] ASoC: SOF: introduce no_stream_position so host_period_bytes preserves its data

[Rajwa, Marcin]

On 7/30/2019 2:51 PM, Pierre-Louis Bossart wrote:

>
>
> On 7/30/19 5:16 AM, Rajwa, Marcin wrote:
>> From: Marcin Rajwa <marcin.rajwa@linux.intel.com>
>>
>> Change the use of host_period_bytes. So far this field was used
>> as an bool value indicating whether FW should send stream position
>> update. With this patch we use host_period_bytes to provide firmware
>> information about the frequency of host interrupts aimed to read
>> its input buffer. This is accoring to ALSA definition of 'FramePeriod'.
>
> according to the
>
>> Knowing this firmware can safely copy large/irregular chunks of data
>
> why irregular? ALSA periods are pretty regular and predictable.

I did not say ALSA periods are irregular I said that sometimes (like in 
case of draining) firmware needs to copy irregular amount of that.

What I mean by "irregular" is not equal to ALSA period or multiple of 
periods.

>
>> (like data comming from i.e draining task) without the risk of buffer
>
> coming
>
> Please proof-read your commit messages (and use an editor which 
> spell-checks for you), typos and misleading information don't exactly 
> boost trust in the suggested patch, regardless of its merits.


Sorry for typos. Should I correct them and resend again or correct here 
as we discuss?

>
>> overflow.
>>
>> Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
>> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
>> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
>>
>> ---
>>   include/sound/sof/stream.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/sound/sof/stream.h b/include/sound/sof/stream.h
>> index 643f175cb479..06af4ecb2584 100644
>> --- a/include/sound/sof/stream.h
>> +++ b/include/sound/sof/stream.h
>> @@ -83,10 +83,10 @@ struct sof_ipc_stream_params {
>>       uint16_t sample_valid_bytes;
>>       uint16_t sample_container_bytes;
>>
>> -    /* for notifying host period has completed - 0 means no period 
>> IRQ */
>>       uint32_t host_period_bytes;
>> +    uint16_t no_stream_position; /* 1 means no IPC for position 
>> update */
>>
>> -    uint32_t reserved[2];
>> +    uint16_t reserved[3];
>>       uint16_t chmap[SOF_IPC_MAX_CHANNELS];    /**< channel map - 
>> SOF_CHMAP_ */
>>   } __packed;
>>
>>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v4 1/2] ASoC: SOF: introduce no_stream_position so host_period_bytes preserves its data

[Pierre-Louis Bossart]

>>> Change the use of host_period_bytes. So far this field was used
>>> as an bool value indicating whether FW should send stream position
>>> update. With this patch we use host_period_bytes to provide firmware
>>> information about the frequency of host interrupts aimed to read
>>> its input buffer. This is accoring to ALSA definition of 'FramePeriod'.
>>
>> according to the
>>
>>> Knowing this firmware can safely copy large/irregular chunks of data
>>
>> why irregular? ALSA periods are pretty regular and predictable.
> 
> I did not say ALSA periods are irregular I said that sometimes (like in 
> case of draining) firmware needs to copy irregular amount of that.
> 
> What I mean by "irregular" is not equal to ALSA period or multiple of 
> periods.

Marcin, in the v2 review this is what we discussed. The formatting may 
be off so please refer to the emails directly should this be difficult 
to read:

"
 >>>
 >>> Before I provide more feedback, can you clarify if the 
'host_period_bytes' is the same value as the ALSA period size (in 
bytes)? And what would be the value when the no_irq mode is used?
 >>
 >> Yes, this is the same value. It is obtained by 
*params_period_bytes**()* in kernel.
 >
 > ok good. I was afraid this would be a different concept.
 >
 > So what you are saying is that the draining happens by bursts whose 
size is tied to the period defined by the host, yes?
Yes. We try to fill host buffer as much as we can to gain fast draining 
but in the same time we give host time to read it.
"

I cannot reconcile the two threads, is the draining tied to the ALSA 
period size or not?

Care to clarify?

> 
>>
>>> (like data comming from i.e draining task) without the risk of buffer
>>
>> coming
>>
>> Please proof-read your commit messages (and use an editor which 
>> spell-checks for you), typos and misleading information don't exactly 
>> boost trust in the suggested patch, regardless of its merits.
> 
> 
> Sorry for typos. Should I correct them and resend again or correct here 
> as we discuss?


better send a v5 when we've clarified what the 'irregular' wording 
refers to.