| Message ID | 20190506095815.24578-3-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: hdmi-codec: fixes and improvements | expand |
On Mon, May 06, 2019 at 11:58:13AM +0200, Jerome Brunet wrote: > Remove current_substream pointer and replace the exclusive locking > mechanism with a simple variable and some atomic operations. The advantage of mutexes is that they are very simple and clear to reason about. It is therefore unclear that this conversion to use atomic variables is an improvement, they're generally more complex to reason about and fragile.
On Wed, 2019-05-08 at 16:00 +0900, Mark Brown wrote: > On Mon, May 06, 2019 at 11:58:13AM +0200, Jerome Brunet wrote: > > > Remove current_substream pointer and replace the exclusive locking > > mechanism with a simple variable and some atomic operations. > > The advantage of mutexes is that they are very simple and clear to > reason about. It is therefore unclear that this conversion to use > atomic variables is an improvement, they're generally more complex > to reason about and fragile. The point of this patch is not to remove the mutex in favor of atomic operations The point was to remove the current_substream pointer in favor of a simple 'busy' flag. After that, I ended up with a mutex protecting a flag and it seemed a bit overkill to me. Atomic op seemed like a good fit for this but I don't really care about that particular point. I can put back mutex to protect the flag if you prefer. Another solution would be to use the mutex as the 'busy' flag. Get the ownership of the hdmi using try_lock() in startup() and release it in shutdown() Would you have a preference Mark ?
On Wed, May 08, 2019 at 10:08:44AM +0200, Jerome Brunet wrote: > On Wed, 2019-05-08 at 16:00 +0900, Mark Brown wrote: > > The advantage of mutexes is that they are very simple and clear to > > reason about. It is therefore unclear that this conversion to use > > atomic variables is an improvement, they're generally more complex > > to reason about and fragile. > The point of this patch is not to remove the mutex in favor of atomic > operations Sure, but you mixed in a conversion to atomic operations as well. > I can put back mutex to protect the flag if you prefer. > Another solution would be to use the mutex as the 'busy' flag. > Get the ownership of the hdmi using try_lock() in startup() and > release it in shutdown() > Would you have a preference Mark ? Probably using a mutex for the flag is clearer. I'd rather keep things as simple as absolutely possible to avoid people making mistakes in future.
On Wed, 2019-05-08 at 17:57 +0900, Mark Brown wrote: > > Another solution would be to use the mutex as the 'busy' flag. > > Get the ownership of the hdmi using try_lock() in startup() and > > release it in shutdown() > > > Would you have a preference Mark ? > > Probably using a mutex for the flag is clearer. I'd rather keep things > as simple as absolutely possible to avoid people making mistakes in > future. Hi Mark, I received a notification that you applied this patch. Just to confirm, you expect a follow up patch to re-introduce the mutex, right ? Thanks Jerome
On Thu, May 09, 2019 at 10:11:48AM +0200, Jerome Brunet wrote: > On Wed, 2019-05-08 at 17:57 +0900, Mark Brown wrote: > > Probably using a mutex for the flag is clearer. I'd rather keep things > > as simple as absolutely possible to avoid people making mistakes in > > future. > I received a notification that you applied this patch. > Just to confirm, you expect a follow up patch to re-introduce the mutex, right ? Right.
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index eb31d7eddcbf..4d32f93f6be6 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -280,11 +280,10 @@ struct hdmi_codec_priv { struct hdmi_codec_pdata hcd; struct snd_soc_dai_driver *daidrv; struct hdmi_codec_daifmt daifmt[2]; - struct mutex current_stream_lock; - struct snd_pcm_substream *current_stream; uint8_t eld[MAX_ELD_BYTES]; struct snd_pcm_chmap *chmap_info; unsigned int chmap_idx; + unsigned long busy; }; static const struct snd_soc_dapm_widget hdmi_widgets[] = { @@ -392,42 +391,22 @@ static int hdmi_codec_chmap_ctl_get(struct snd_kcontrol *kcontrol, return 0; } -static int hdmi_codec_new_stream(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) -{ - struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); - int ret = 0; - - mutex_lock(&hcp->current_stream_lock); - if (!hcp->current_stream) { - hcp->current_stream = substream; - } else if (hcp->current_stream != substream) { - dev_err(dai->dev, "Only one simultaneous stream supported!\n"); - ret = -EINVAL; - } - mutex_unlock(&hcp->current_stream_lock); - - return ret; -} - static int hdmi_codec_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); int ret = 0; - ret = hdmi_codec_new_stream(substream, dai); - if (ret) - return ret; + ret = test_and_set_bit(0, &hcp->busy); + if (ret) { + dev_err(dai->dev, "Only one simultaneous stream supported!\n"); + return -EINVAL; + } if (hcp->hcd.ops->audio_startup) { ret = hcp->hcd.ops->audio_startup(dai->dev->parent, hcp->hcd.data); - if (ret) { - mutex_lock(&hcp->current_stream_lock); - hcp->current_stream = NULL; - mutex_unlock(&hcp->current_stream_lock); - return ret; - } + if (ret) + goto err; } if (hcp->hcd.ops->get_eld) { @@ -437,17 +416,18 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream, if (!ret) { ret = snd_pcm_hw_constraint_eld(substream->runtime, hcp->eld); - if (ret) { - mutex_lock(&hcp->current_stream_lock); - hcp->current_stream = NULL; - mutex_unlock(&hcp->current_stream_lock); - return ret; - } + if (ret) + goto err; } /* Select chmap supported */ hdmi_codec_eld_chmap(hcp); } return 0; + +err: + /* Release the exclusive lock on error */ + clear_bit(0, &hcp->busy); + return ret; } static void hdmi_codec_shutdown(struct snd_pcm_substream *substream, @@ -455,14 +435,10 @@ static void hdmi_codec_shutdown(struct snd_pcm_substream *substream, { struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); - WARN_ON(hcp->current_stream != substream); - hcp->chmap_idx = HDMI_CODEC_CHMAP_IDX_UNKNOWN; hcp->hcd.ops->audio_shutdown(dai->dev->parent, hcp->hcd.data); - mutex_lock(&hcp->current_stream_lock); - hcp->current_stream = NULL; - mutex_unlock(&hcp->current_stream_lock); + clear_bit(0, &hcp->busy); } static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, @@ -761,8 +737,6 @@ static int hdmi_codec_probe(struct platform_device *pdev) return -ENOMEM; hcp->hcd = *hcd; - mutex_init(&hcp->current_stream_lock); - hcp->daidrv = devm_kcalloc(dev, dai_count, sizeof(*hcp->daidrv), GFP_KERNEL); if (!hcp->daidrv)
If the hdmi-codec is on a codec-to-codec link, the substream pointer it receives is completely made up by snd_soc_dai_link_event(). The pointer will be different between startup() and shutdown(). The hdmi-codec complains when this happens even if it is not really a problem. The current_substream pointer is not used for anything useful apart from getting the exclusive ownership of the device. Remove current_substream pointer and replace the exclusive locking mechanism with a simple variable and some atomic operations. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- sound/soc/codecs/hdmi-codec.c | 58 ++++++++++------------------------- 1 file changed, 16 insertions(+), 42 deletions(-)