[PATCH v2 1/2] ASoC: SOF: add flag for position update ipc

[PATCH v2 1/2] ASoC: SOF: add flag for position update ipc

[Keyon Jie]

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

In some cases, FW might need use the host_period_bytes even no position
update ipc reqiured from driver, here add another flag for position update,
and preserve host_period_bytes for FW to use.

This might require corresponding FW change and ABI alignment.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
Signed-off-by: Keyon Jie <yang.jie@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..44acfa62fa69 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_period_irq; /* 1 means period IRQ mode OFF */
 
-	uint32_t reserved[2];
+	uint16_t reserved[3];
 	uint16_t chmap[SOF_IPC_MAX_CHANNELS];	/**< channel map - SOF_CHMAP_ */
 } __packed;
 
-- 
2.20.1

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

[Keyon Jie]

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

This patch prevents the reset of host period bytes.
The parameter has been used to keep information about
completion of period copy. Right now we keep this
information in period_irq.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
Signed-off-by: Keyon Jie <yang.jie@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..c7022346aba0 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_period_irq flag for ipc params */
 	if (hda && hda->no_ipc_position)
-		ipc_params->host_period_bytes = 0;
+		ipc_params->no_period_irq = 1;
 
 	ipc_params->stream_tag = hstream->stream_tag;
 
-- 
2.20.1

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

[Ranjani Sridharan]

On Wed, 2019-07-03 at 23:10 +0800, Keyon Jie wrote:
> From: Marcin Rajwa <marcin.rajwa@linux.intel.com>
> 
> This patch prevents the reset of host period bytes.
> The parameter has been used to keep information about
> completion of period copy. Right now we keep this
> information in period_irq.
> 
> 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>

> ---
>  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..c7022346aba0 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_period_irq flag for ipc params */
>  	if (hda && hda->no_ipc_position)
> -		ipc_params->host_period_bytes = 0;
> +		ipc_params->no_period_irq = 1;
>  
>  	ipc_params->stream_tag = hstream->stream_tag;
>  

Re: [PATCH v2 1/2] ASoC: SOF: add flag for position update ipc

[Kai Vehmanen]

Hi,

patch looks good, but commit message could be improved.

On Wed, 3 Jul 2019, Keyon Jie wrote:

> In some cases, FW might need use the host_period_bytes even no position
> update ipc reqiured from driver, here add another flag for position update,
> and preserve host_period_bytes for FW to use.

Speculation on what FW might do is not really needed. The 
host_period_bytes field has been overloaded with multiple 
semantics and this patch clears that, right. How about:

""
Remove the special case semantics of 'host_period_bytes==0'.
Add a new field 'no_period_irq' to signal whether period elapsed
IPC should be sent and use 'host_period_bytes' only for
period size.
""

> This might require corresponding FW change and ABI alignment.

This is not helpful -- we know this _is_ a minor ABI change
and needs to be aligned with FW.

Br, Kai

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

[Kai Vehmanen]

Hi,

On Wed, 3 Jul 2019, Keyon Jie wrote:

> From: Marcin Rajwa <marcin.rajwa@linux.intel.com>
> 
> This patch prevents the reset of host period bytes.
> The parameter has been used to keep information about
> completion of period copy. Right now we keep this
> information in period_irq.
> 
> Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

looks good, for this patch:

Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Re: [PATCH v2 1/2] ASoC: SOF: add flag for position update ipc

[Keyon Jie]

On 2019/7/4 下午6:03, Kai Vehmanen wrote:
> Hi,
> 
> patch looks good, but commit message could be improved.
> 
> On Wed, 3 Jul 2019, Keyon Jie wrote:
> 
>> In some cases, FW might need use the host_period_bytes even no position
>> update ipc reqiured from driver, here add another flag for position update,
>> and preserve host_period_bytes for FW to use.
> 
> Speculation on what FW might do is not really needed. The
> host_period_bytes field has been overloaded with multiple
> semantics and this patch clears that, right. How about:

Well, to me it is flavor choice, Ranjani suggested to illustrate the use 
case where the FW will use this host_period_bytes, and I agreed this 
will help people to understand why we need this change.

> 
> ""
> Remove the special case semantics of 'host_period_bytes==0'.
> Add a new field 'no_period_irq' to signal whether period elapsed
> IPC should be sent and use 'host_period_bytes' only for
> period size.
> ""
> 
>> This might require corresponding FW change and ABI alignment.
> 
> This is not helpful -- we know this _is_ a minor ABI change
> and needs to be aligned with FW.

It is minor change, but the FW change is still required, otherwise, we 
will get extra position update IPCs which may confuse the driver, please 
refer to here:
https://github.com/thesofproject/sof/pull/1592

Thanks,
~Keyon

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

Re: [PATCH v2 1/2] ASoC: SOF: add flag for position update ipc

[Kai Vehmanen]

Hi,

On Thu, 4 Jul 2019, Keyon Jie wrote:

> Well, to me it is flavor choice, Ranjani suggested to illustrate the use 
> case where the FW will use this host_period_bytes, and I agreed this 
> will help people to understand why we need this change.

hmm, ok. So maybe add "Allows FW to use 'host_period_bytes' field
for its original purpose" to my proposed wording..?

>> This is not helpful -- we know this _is_ a minor ABI change
>> and needs to be aligned with FW.
> It is minor change, but the FW change is still required, otherwise, we 
> will get extra position update IPCs which may confuse the driver, please 
[...]
> https://github.com/thesofproject/sof/pull/1592

Ack, but we know this already so best to put the accurate 
description in the commit message. The "might require FW change"
is a bit scary statement in a patch touching ABI structs. ;)

Re: [PATCH v2 1/2] ASoC: SOF: add flag for position update ipc

[Pierre-Louis Bossart]

On 7/3/19 10:10 AM, Keyon Jie wrote:
> From: Marcin Rajwa <marcin.rajwa@linux.intel.com>
> 
> In some cases, FW might need use the host_period_bytes even no position
> update ipc reqiured from driver, here add another flag for position update,
> and preserve host_period_bytes for FW to use.

please fix the commit message, e.g. with the suggested edit below

In some cases, FW might need to use the host_period_bytes field to fetch 
data over DMA but the driver does not need any position information 
returned over the IPC channel by the firmware. The current IPC 
definition prevents this capability, so add new field.

> 
> This might require corresponding FW change and ABI alignment.

remove this statement, this is already handled in backwards compatible mode.

> 
> Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
> Signed-off-by: Keyon Jie <yang.jie@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..44acfa62fa69 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_period_irq; /* 1 means period IRQ mode OFF */

I'd like this field to be renamed as 'no_position_update'. This really 
has nothing to do with no period_irq in general, even when you do use 
the no_irq mode you can still retrieve the position information from the 
HDaudio DMA registers.

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

Re: [PATCH v2 1/2] ASoC: SOF: add flag for position update ipc

[Keyon Jie]

On 2019/7/17 下午11:48, Pierre-Louis Bossart wrote:
> 
> 
> On 7/3/19 10:10 AM, Keyon Jie wrote:
>> From: Marcin Rajwa <marcin.rajwa@linux.intel.com>
>>
>> In some cases, FW might need use the host_period_bytes even no position
>> update ipc reqiured from driver, here add another flag for position 
>> update,
>> and preserve host_period_bytes for FW to use.
> 
> please fix the commit message, e.g. with the suggested edit below
> 
> In some cases, FW might need to use the host_period_bytes field to fetch 
> data over DMA but the driver does not need any position information 
> returned over the IPC channel by the firmware. The current IPC 
> definition prevents this capability, so add new field.

Good, thanks for the detail suggestion.

> 
>>
>> This might require corresponding FW change and ABI alignment.
> 
> remove this statement, this is already handled in backwards compatible 
> mode.

OK.

> 
>>
>> Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
>> Signed-off-by: Keyon Jie <yang.jie@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..44acfa62fa69 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_period_irq; /* 1 means period IRQ mode OFF */
> 
> I'd like this field to be renamed as 'no_position_update'. This really 
> has nothing to do with no period_irq in general, even when you do use 
> the no_irq mode you can still retrieve the position information from the 
> HDaudio DMA registers.

Agree, that's actually my original version, will change in next version, 
thanks.

Thanks,
~Keyon

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