[alsa-devel] [PATCH 0/8] Yet more fixes for LPE HDMI audio
Hi,
here is yet another round of LPE HDMI audio driver updates. After reading the report from Pierre, I tested the monitor connection and xrandr, and confirmed that it sucks. Here is an attempt to improve the handling.
The patches are found in topic/intel-lpe-audio branch as usual, too.
Takashi
===
Takashi Iwai (8): ALSA: x86: Implement jack control ALSA: x86: Use snd_pcm_stop_xrun() for connection / disconnection paths ALSA: x86: Fix memory leak in had_build_channel_allocation_map() ALSA: x86: Don't return an error from chmap ctl at disconnected ALSA: x86: Avoid register accesses during disconnection ALSA: x86: Stop the stream when buffer is processed after disconnection ALSA: x86: Minor code rearrangement ALSA: x86: Don't bail out from PCM ops when disconnected
sound/x86/intel_hdmi_audio.c | 118 +++++++++++++++++++++---------------------- sound/x86/intel_hdmi_audio.h | 1 + 2 files changed, 59 insertions(+), 60 deletions(-)
This patch implements a jack interface for notifying HDMI/DP connection. PA listens to this, so it can handle the monitor connection more gracefully.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 23 +++++++++++++++++++++++ sound/x86/intel_hdmi_audio.h | 1 + 2 files changed, 24 insertions(+)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 9889cdf3ccf4..a30ca03e49ae 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -37,6 +37,7 @@ #include <sound/pcm_params.h> #include <sound/initval.h> #include <sound/control.h> +#include <sound/jack.h> #include <drm/drm_edid.h> #include <drm/intel_lpe_audio.h> #include "intel_hdmi_audio.h" @@ -1382,6 +1383,8 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) }
had_build_channel_allocation_map(intelhaddata); + + snd_jack_report(intelhaddata->jack, SND_JACK_AVOUT); }
/* process hot unplug, called from wq with mutex locked */ @@ -1414,6 +1417,7 @@ static void had_process_hot_unplug(struct snd_intelhad *intelhaddata) snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP);
out: + snd_jack_report(intelhaddata->jack, 0); if (substream) had_substream_put(intelhaddata); kfree(intelhaddata->chmap->chmap); @@ -1609,6 +1613,21 @@ static void had_audio_wq(struct work_struct *work) }
/* + * Jack interface + */ +static int had_create_jack(struct snd_intelhad *ctx) +{ + int err; + + err = snd_jack_new(ctx->card, "HDMI/DP", SND_JACK_AVOUT, &ctx->jack, + true, false); + if (err < 0) + return err; + ctx->jack->private_data = ctx; + return 0; +} + +/* * PM callbacks */
@@ -1780,6 +1799,10 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) if (ret < 0) goto err;
+ ret = had_create_jack(ctx); + if (ret < 0) + goto err; + ret = snd_card_register(card); if (ret) goto err; diff --git a/sound/x86/intel_hdmi_audio.h b/sound/x86/intel_hdmi_audio.h index d6ba90fd011d..2d3e389f76b3 100644 --- a/sound/x86/intel_hdmi_audio.h +++ b/sound/x86/intel_hdmi_audio.h @@ -130,6 +130,7 @@ struct snd_intelhad { struct work_struct hdmi_audio_wq; struct mutex mutex; /* for protecting chmap and eld */ bool need_reset; + struct snd_jack *jack; };
#endif /* _INTEL_HDMI_AUDIO_ */
This seems more friendly to user-space, as it's notified at least as an error, instead of forcibly moving the PCM state to SETUP out of sudden.
Moreover, snd_pcm_stop() needs an extra PCM spinlock I forgot, while snd_pcm_stop_xrun() takes the spinlock by itself.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index a30ca03e49ae..a7343f2d2730 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -1378,7 +1378,7 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) dev_dbg(intelhaddata->dev, "Force to stop the active stream by disconnection\n"); /* Set runtime->state to hw_params done */ - snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP); + snd_pcm_stop_xrun(substream); had_substream_put(intelhaddata); }
@@ -1414,7 +1414,7 @@ static void had_process_hot_unplug(struct snd_intelhad *intelhaddata)
/* Report to above ALSA layer */ if (substream) - snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP); + snd_pcm_stop_xrun(substream);
out: snd_jack_report(intelhaddata->jack, 0);
The previously allocated chmap has to be released before setting the new one.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index a7343f2d2730..5f2445389716 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -444,11 +444,12 @@ static void had_build_channel_allocation_map(struct snd_intelhad *intelhaddata) u8 eld_high, eld_high_mask = 0xF0; u8 high_msb;
+ kfree(intelhaddata->chmap->chmap); + intelhaddata->chmap->chmap = NULL; + chmap = kzalloc(sizeof(*chmap), GFP_KERNEL); - if (!chmap) { - intelhaddata->chmap->chmap = NULL; + if (!chmap) return; - }
dev_dbg(intelhaddata->dev, "eld speaker = %x\n", intelhaddata->eld[DRM_ELD_SPEAKER]); @@ -493,10 +494,8 @@ static void had_build_channel_allocation_map(struct snd_intelhad *intelhaddata) break; } } - if (i >= ARRAY_SIZE(channel_allocations)) { - intelhaddata->chmap->chmap = NULL; + if (i >= ARRAY_SIZE(channel_allocations)) kfree(chmap); - } }
/*
It's not wise to return an error at info/get callback when disconnected, which happens at any time. The chmap ctl is supposed to fill zero for such a case, instead.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 5f2445389716..71f01204a590 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -504,11 +504,6 @@ static void had_build_channel_allocation_map(struct snd_intelhad *intelhaddata) static int had_chmap_ctl_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo) { - struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol); - struct snd_intelhad *intelhaddata = info->private_data; - - if (!intelhaddata->connected) - return -ENODEV; uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; uinfo->count = HAD_MAX_CHANNEL; uinfo->value.integer.min = 0; @@ -524,13 +519,12 @@ static int had_chmap_ctl_get(struct snd_kcontrol *kcontrol, int i; const struct snd_pcm_chmap_elem *chmap;
- if (!intelhaddata->connected) - return -ENODEV; - + memset(ucontrol->value.integer.value, 0, + sizeof(long) * HAD_MAX_CHANNEL); mutex_lock(&intelhaddata->mutex); if (!intelhaddata->chmap->chmap) { mutex_unlock(&intelhaddata->mutex); - return -ENODATA; + return 0; }
chmap = intelhaddata->chmap->chmap;
It seems that accessing registers during disconnection often leads to the GPU pipe error. The original driver had a similar check in the past, but it was lost through refactoring. Now put a connection check in the register access functions.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 71f01204a590..97362233c326 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -192,12 +192,16 @@ static void had_substream_put(struct snd_intelhad *intelhaddata) /* Register access functions */ static void had_read_register(struct snd_intelhad *ctx, u32 reg, u32 *val) { - *val = ioread32(ctx->mmio_start + ctx->had_config_offset + reg); + if (!ctx->connected) + *val = 0; + else + *val = ioread32(ctx->mmio_start + ctx->had_config_offset + reg); }
static void had_write_register(struct snd_intelhad *ctx, u32 reg, u32 val) { - iowrite32(val, ctx->mmio_start + ctx->had_config_offset + reg); + if (ctx->connected) + iowrite32(val, ctx->mmio_start + ctx->had_config_offset + reg); }
/* @@ -229,6 +233,8 @@ static void had_ack_irqs(struct snd_intelhad *ctx) { u32 status_reg;
+ if (!ctx->connected) + return; had_read_register(ctx, AUD_HDMI_STATUS, &status_reg); status_reg |= HDMI_AUDIO_BUFFER_DONE | HDMI_AUDIO_UNDERRUN; had_write_register(ctx, AUD_HDMI_STATUS, status_reg); @@ -998,7 +1004,7 @@ static void wait_clear_underrun_bit(struct snd_intelhad *intelhaddata) */ static void had_do_reset(struct snd_intelhad *intelhaddata) { - if (!intelhaddata->need_reset) + if (!intelhaddata->need_reset || !intelhaddata->connected) return;
/* Reset buffer pointers */
On Wed, 15 Feb 2017 22:29:33 +0100, Takashi Iwai wrote:
It seems that accessing registers during disconnection often leads to the GPU pipe error. The original driver had a similar check in the past, but it was lost through refactoring. Now put a connection check in the register access functions.
One thing that came to my mind later is about an interrupt that is triggered after the disconnection. Although this shouldn't happen, it's still better to clear the IRQ if it really happens.
Below is a revised patch to cover it. topic/intel-lpe-audio branch was updated accordingly, too.
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ALSA: x86: Avoid register accesses during disconnection
It seems that accessing registers during disconnection often leads to the GPU pipe error. The original driver had a similar check in the past, but it was lost through refactoring. Now put a connection check in the register access functions.
One exception is the irq handler: it still needs to access the raw register even while disconnected, because it has to read and write to ACK the irq mask. Although the irq shouldn't be raised while disconnected (the stream should have been disabled), let's make it safer for now.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 71f01204a590..8d365718c692 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -190,14 +190,28 @@ static void had_substream_put(struct snd_intelhad *intelhaddata) }
/* Register access functions */ +static u32 had_read_register_raw(struct snd_intelhad *ctx, u32 reg) +{ + return ioread32(ctx->mmio_start + ctx->had_config_offset + reg); +} + +static void had_write_register_raw(struct snd_intelhad *ctx, u32 reg, u32 val) +{ + iowrite32(val, ctx->mmio_start + ctx->had_config_offset + reg); +} + static void had_read_register(struct snd_intelhad *ctx, u32 reg, u32 *val) { - *val = ioread32(ctx->mmio_start + ctx->had_config_offset + reg); + if (!ctx->connected) + *val = 0; + else + *val = had_read_register_raw(ctx, reg); }
static void had_write_register(struct snd_intelhad *ctx, u32 reg, u32 val) { - iowrite32(val, ctx->mmio_start + ctx->had_config_offset + reg); + if (ctx->connected) + had_write_register_raw(ctx, reg, val); }
/* @@ -229,6 +243,8 @@ static void had_ack_irqs(struct snd_intelhad *ctx) { u32 status_reg;
+ if (!ctx->connected) + return; had_read_register(ctx, AUD_HDMI_STATUS, &status_reg); status_reg |= HDMI_AUDIO_BUFFER_DONE | HDMI_AUDIO_UNDERRUN; had_write_register(ctx, AUD_HDMI_STATUS, status_reg); @@ -998,7 +1014,7 @@ static void wait_clear_underrun_bit(struct snd_intelhad *intelhaddata) */ static void had_do_reset(struct snd_intelhad *intelhaddata) { - if (!intelhaddata->need_reset) + if (!intelhaddata->need_reset || !intelhaddata->connected) return;
/* Reset buffer pointers */ @@ -1526,18 +1542,20 @@ static const struct snd_kcontrol_new had_controls[] = { static irqreturn_t display_pipe_interrupt_handler(int irq, void *dev_id) { struct snd_intelhad *ctx = dev_id; - u32 audio_stat, audio_reg; + u32 audio_stat;
- audio_reg = AUD_HDMI_STATUS; - had_read_register(ctx, audio_reg, &audio_stat); + /* use raw register access to ack IRQs even while disconnected */ + audio_stat = had_read_register_raw(ctx, AUD_HDMI_STATUS);
if (audio_stat & HDMI_AUDIO_UNDERRUN) { - had_write_register(ctx, audio_reg, HDMI_AUDIO_UNDERRUN); + had_write_register_raw(ctx, AUD_HDMI_STATUS, + HDMI_AUDIO_UNDERRUN); had_process_buffer_underrun(ctx); }
if (audio_stat & HDMI_AUDIO_BUFFER_DONE) { - had_write_register(ctx, audio_reg, HDMI_AUDIO_BUFFER_DONE); + had_write_register_raw(ctx, AUD_HDMI_STATUS, + HDMI_AUDIO_BUFFER_DONE); had_process_buffer_done(ctx); }
This shouldn't happen, but just to be sure...
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 97362233c326..2ed0a34bc19e 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -961,19 +961,22 @@ static void had_process_buffer_done(struct snd_intelhad *intelhaddata) { struct snd_pcm_substream *substream;
- if (!intelhaddata->connected) - return; /* disconnected? - bail out */ - substream = had_substream_get(intelhaddata); if (!substream) return; /* no stream? - bail out */
+ if (!intelhaddata->connected) { + snd_pcm_stop_xrun(substream); + goto out; /* disconnected? - bail out */ + } + /* process or stop the stream */ if (had_process_ringbuf(substream, intelhaddata) < 0) snd_pcm_stop_xrun(substream); else snd_pcm_period_elapsed(substream);
+ out: had_substream_put(intelhaddata); }
Put the stuff in the right order; notification should be at the end of the action.
Also dropped a superfluous debug print and incorrect comments.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 2ed0a34bc19e..8b6e1c728798 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -1374,18 +1374,15 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) __func__, __LINE__); spin_unlock_irq(&intelhaddata->had_spinlock);
- /* Safety check */ + had_build_channel_allocation_map(intelhaddata); + + /* Report to above ALSA layer */ substream = had_substream_get(intelhaddata); if (substream) { - dev_dbg(intelhaddata->dev, - "Force to stop the active stream by disconnection\n"); - /* Set runtime->state to hw_params done */ snd_pcm_stop_xrun(substream); had_substream_put(intelhaddata); }
- had_build_channel_allocation_map(intelhaddata); - snd_jack_report(intelhaddata->jack, SND_JACK_AVOUT); }
@@ -1394,14 +1391,11 @@ static void had_process_hot_unplug(struct snd_intelhad *intelhaddata) { struct snd_pcm_substream *substream;
- substream = had_substream_get(intelhaddata); - spin_lock_irq(&intelhaddata->had_spinlock); - if (!intelhaddata->connected) { dev_dbg(intelhaddata->dev, "Device already disconnected\n"); spin_unlock_irq(&intelhaddata->had_spinlock); - goto out; + return;
}
@@ -1414,16 +1408,17 @@ static void had_process_hot_unplug(struct snd_intelhad *intelhaddata) __func__, __LINE__); spin_unlock_irq(&intelhaddata->had_spinlock);
+ kfree(intelhaddata->chmap->chmap); + intelhaddata->chmap->chmap = NULL; + /* Report to above ALSA layer */ - if (substream) + substream = had_substream_get(intelhaddata); + if (substream) { snd_pcm_stop_xrun(substream); + had_substream_put(intelhaddata); + }
- out: snd_jack_report(intelhaddata->jack, 0); - if (substream) - had_substream_put(intelhaddata); - kfree(intelhaddata->chmap->chmap); - intelhaddata->chmap->chmap = NULL; }
/*
Currently the driver returns -ENODEV when the monitor is disconnected. But PA alsa module doesn't like this and it starts playing Juliet, kills itself as if it were a fatal tragedy.
Since we protect the whole read/write at disconnection, just allow the PCM accesses even during disconnection.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 22 ---------------------- 1 file changed, 22 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 8b6e1c728798..f8fa3197f6ef 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -1044,13 +1044,6 @@ static int had_pcm_open(struct snd_pcm_substream *substream)
pm_runtime_get_sync(intelhaddata->dev);
- if (!intelhaddata->connected) { - dev_dbg(intelhaddata->dev, "%s: HDMI cable plugged-out\n", - __func__); - retval = -ENODEV; - goto error; - } - /* set the runtime hw parameter with local snd_pcm_hardware struct */ runtime->hw = had_pcm_hardware;
@@ -1176,14 +1169,6 @@ static int had_pcm_trigger(struct snd_pcm_substream *substream, int cmd) case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME: - /* Disable local INTRs till register prgmng is done */ - if (!intelhaddata->connected) { - dev_dbg(intelhaddata->dev, - "_START: HDMI cable plugged-out\n"); - retval = -ENODEV; - break; - } - /* Enable Audio */ had_ack_irqs(intelhaddata); /* FIXME: do we need this? */ had_enable_audio(intelhaddata, true); @@ -1217,13 +1202,6 @@ static int had_pcm_prepare(struct snd_pcm_substream *substream) intelhaddata = snd_pcm_substream_chip(substream); runtime = substream->runtime;
- if (!intelhaddata->connected) { - dev_dbg(intelhaddata->dev, "%s: HDMI cable plugged-out\n", - __func__); - retval = -ENODEV; - goto prep_end; - } - dev_dbg(intelhaddata->dev, "period_size=%d\n", (int)frames_to_bytes(runtime, runtime->period_size)); dev_dbg(intelhaddata->dev, "periods=%d\n", runtime->periods);
participants (1)
-
Takashi Iwai