[alsa-devel] [PATCH] ASoC: sigmadsp: safeload should be used from 4 bytes
From: Danny Smith dannys@axis.com
Fixed range in safeload conditional to allow safeload to be used from 4 bytes.
Signed-off-by: Danny Smith dannys@axis.com --- sound/soc/codecs/sigmadsp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c index d53680ac78e4..d63d58350cd8 100644 --- a/sound/soc/codecs/sigmadsp.c +++ b/sound/soc/codecs/sigmadsp.c @@ -117,7 +117,7 @@ static int sigmadsp_ctrl_write(struct sigmadsp *sigmadsp, struct sigmadsp_control *ctrl, void *data) { /* safeload loads up to 20 bytes in a atomic operation */ - if (ctrl->num_bytes > 4 && ctrl->num_bytes <= 20 && sigmadsp->ops && + if (ctrl->num_bytes >= 4 && ctrl->num_bytes <= 20 && sigmadsp->ops && sigmadsp->ops->safeload) return sigmadsp->ops->safeload(sigmadsp, ctrl->addr, data, ctrl->num_bytes);
On 08/22/2018 08:11 AM, Robert Rosengren wrote:
From: Danny Smith dannys@axis.com
Fixed range in safeload conditional to allow safeload to be used from 4 bytes.
Hi,
Thanks for the patch. The reason why 4 bytes is excluded is that up to 4 bytes can be updated atomically with a single register write. But I could see that if the firmware reads the same parameter multiple times during the same run you could get inconsistent results.
Can you explain a bit more why you need this?
If we want to allow safeload for 4 byte parameters the same reasoning applies for parameters with less than 4 bytes as well and so the check should be removed completely.
- Lars
Signed-off-by: Danny Smith dannys@axis.com
sound/soc/codecs/sigmadsp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c index d53680ac78e4..d63d58350cd8 100644 --- a/sound/soc/codecs/sigmadsp.c +++ b/sound/soc/codecs/sigmadsp.c @@ -117,7 +117,7 @@ static int sigmadsp_ctrl_write(struct sigmadsp *sigmadsp, struct sigmadsp_control *ctrl, void *data) { /* safeload loads up to 20 bytes in a atomic operation */
- if (ctrl->num_bytes > 4 && ctrl->num_bytes <= 20 && sigmadsp->ops &&
- if (ctrl->num_bytes >= 4 && ctrl->num_bytes <= 20 && sigmadsp->ops && sigmadsp->ops->safeload) return sigmadsp->ops->safeload(sigmadsp, ctrl->addr, data, ctrl->num_bytes);
On 08/22/2018 08:34 AM, Lars-Peter Clausen wrote:
On 08/22/2018 08:11 AM, Robert Rosengren wrote:
From: Danny Smith dannys@axis.com
Fixed range in safeload conditional to allow safeload to be used from 4 bytes.
Hi,
Thanks for the patch. The reason why 4 bytes is excluded is that up to 4 bytes can be updated atomically with a single register write. But I could see that if the firmware reads the same parameter multiple times during the same run you could get inconsistent results.
Can you explain a bit more why you need this?
If we want to allow safeload for 4 byte parameters the same reasoning applies for parameters with less than 4 bytes as well and so the check should be removed completely.
- Lars
Hi,
Thanks for the feedback. It is correct that up to 4 bytes can be updated atomically but does that also guarantee that the data is safeloaded, i.e. updated when the parameter is not in use? Maybe updating up to 4 byte parameters without safeload does not cause audio glitches?
Regards, Danny
Signed-off-by: Danny Smith dannys@axis.com
sound/soc/codecs/sigmadsp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c index d53680ac78e4..d63d58350cd8 100644 --- a/sound/soc/codecs/sigmadsp.c +++ b/sound/soc/codecs/sigmadsp.c @@ -117,7 +117,7 @@ static int sigmadsp_ctrl_write(struct sigmadsp *sigmadsp, struct sigmadsp_control *ctrl, void *data) { /* safeload loads up to 20 bytes in a atomic operation */
- if (ctrl->num_bytes > 4 && ctrl->num_bytes <= 20 && sigmadsp->ops &&
- if (ctrl->num_bytes >= 4 && ctrl->num_bytes <= 20 && sigmadsp->ops && sigmadsp->ops->safeload) return sigmadsp->ops->safeload(sigmadsp, ctrl->addr, data, ctrl->num_bytes);
On 08/22/2018 09:53 AM, Danny Smith wrote:
On 08/22/2018 08:34 AM, Lars-Peter Clausen wrote:
On 08/22/2018 08:11 AM, Robert Rosengren wrote:
From: Danny Smith dannys@axis.com
Fixed range in safeload conditional to allow safeload to be used from 4 bytes.
Hi,
Thanks for the patch. The reason why 4 bytes is excluded is that up to 4 bytes can be updated atomically with a single register write. But I could see that if the firmware reads the same parameter multiple times during the same run you could get inconsistent results.
Can you explain a bit more why you need this?
If we want to allow safeload for 4 byte parameters the same reasoning applies for parameters with less than 4 bytes as well and so the check should be removed completely.
- Lars
Hi,
Thanks for the feedback. It is correct that up to 4 bytes can be updated atomically but does that also guarantee that the data is safeloaded, i.e. updated when the parameter is not in use? Maybe updating up to 4 byte parameters without safeload does not cause audio glitches?
The datasheet says:
""" To update parameters in real time while avoiding pop and click noises on the output, the ADAU1761 uses a software safeload mechanism. The software safeload mechanism enables the SigmaDSP core to load new parameters into RAM while guaranteeing that the parameters are not in use. This prevents an undesirable condition where an instruction could execute with a mix of old and new parameters. """
The way I understand the last sentence is that there is no issue when loading only 4 bytes or less since there is no risk of mixing old and new data.
I don't mind changing this, I was just curious if you had seen any issues with a 4 byte parameter update.
- Lars
On 08/22/2018 03:05 PM, Lars-Peter Clausen wrote:
On 08/22/2018 09:53 AM, Danny Smith wrote:
On 08/22/2018 08:34 AM, Lars-Peter Clausen wrote:
On 08/22/2018 08:11 AM, Robert Rosengren wrote:
From: Danny Smith dannys@axis.com
Fixed range in safeload conditional to allow safeload to be used from 4 bytes.
Hi,
Thanks for the patch. The reason why 4 bytes is excluded is that up to 4 bytes can be updated atomically with a single register write. But I could see that if the firmware reads the same parameter multiple times during the same run you could get inconsistent results.
Can you explain a bit more why you need this?
If we want to allow safeload for 4 byte parameters the same reasoning applies for parameters with less than 4 bytes as well and so the check should be removed completely.
- Lars
Hi,
Thanks for the feedback. It is correct that up to 4 bytes can be updated atomically but does that also guarantee that the data is safeloaded, i.e. updated when the parameter is not in use? Maybe updating up to 4 byte parameters without safeload does not cause audio glitches?
The datasheet says:
""" To update parameters in real time while avoiding pop and click noises on the output, the ADAU1761 uses a software safeload mechanism. The software safeload mechanism enables the SigmaDSP core to load new parameters into RAM while guaranteeing that the parameters are not in use. This prevents an undesirable condition where an instruction could execute with a mix of old and new parameters. """
The way I understand the last sentence is that there is no issue when loading only 4 bytes or less since there is no risk of mixing old and new data.
I don't mind changing this, I was just curious if you had seen any issues with a 4 byte parameter update.
- Lars
I have not seen any issues doing a 4byte parameter update without safeload but have not performed any extensive testing on the subject. The reason for the patch was that I noticed that sigmastudio uses safeload when doing 4byte writes but I cannot say if it is necessary or not.
Regards, Danny
On 08/22/2018 03:40 PM, Danny Smith wrote:
On 08/22/2018 03:05 PM, Lars-Peter Clausen wrote:
On 08/22/2018 09:53 AM, Danny Smith wrote:
On 08/22/2018 08:34 AM, Lars-Peter Clausen wrote:
On 08/22/2018 08:11 AM, Robert Rosengren wrote:
From: Danny Smith dannys@axis.com
Fixed range in safeload conditional to allow safeload to be used from 4 bytes.
Hi,
Thanks for the patch. The reason why 4 bytes is excluded is that up to 4 bytes can be updated atomically with a single register write. But I could see that if the firmware reads the same parameter multiple times during the same run you could get inconsistent results.
Can you explain a bit more why you need this?
If we want to allow safeload for 4 byte parameters the same reasoning applies for parameters with less than 4 bytes as well and so the check should be removed completely.
- Lars
Hi,
Thanks for the feedback. It is correct that up to 4 bytes can be updated atomically but does that also guarantee that the data is safeloaded, i.e. updated when the parameter is not in use? Maybe updating up to 4 byte parameters without safeload does not cause audio glitches?
The datasheet says:
""" To update parameters in real time while avoiding pop and click noises on the output, the ADAU1761 uses a software safeload mechanism. The software safeload mechanism enables the SigmaDSP core to load new parameters into RAM while guaranteeing that the parameters are not in use. This prevents an undesirable condition where an instruction could execute with a mix of old and new parameters. """
The way I understand the last sentence is that there is no issue when loading only 4 bytes or less since there is no risk of mixing old and new data.
I don't mind changing this, I was just curious if you had seen any issues with a 4 byte parameter update.
- Lars
I have not seen any issues doing a 4byte parameter update without safeload but have not performed any extensive testing on the subject. The reason for the patch was that I noticed that sigmastudio uses safeload when doing 4byte writes but I cannot say if it is necessary or not.
Ok, then lets change the driver, but drop the test completely so that anything up to 20 bytes uses safeload.
participants (3)
-
Danny Smith -
Lars-Peter Clausen -
Robert Rosengren