| Message ID | 20170425202730.1384-12-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/25/2017 03:27 PM, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Now that everything is in place let's register a PCM device for > each pipe of the display engine. This will make it possible to > actually output audio to multiple displays at the same time. And > it avoids modesets on unrelated displays from clobbering up the > ELD and whatnot for the display currently doing the playback. > > The alternative would be to have a PCM device per port, but per-pipe > is easier since the hardware actually works that way. Very nice. I just tested on a CHT Zotac box which has two connectors (1 HDMI and 1 DP), and I get sound concurrently on both, with hdmi being listed as device 2 and DP as device 0. I thought there were hardware restrictions but you proved me wrong. Kudos. The only point that I find weird is that the jacks are reported as 'on' on the 3 pipes, is there a way to tie them to an actual cable being used? [plb@ZOTAC ~]$ amixer -Dhw:0 controls | grep Jack numid=5,iface=CARD,name='HDMI/DP,pcm=0 Jack' numid=10,iface=CARD,name='HDMI/DP,pcm=1 Jack' numid=15,iface=CARD,name='HDMI/DP,pcm=2 Jack' [plb@ZOTAC ~]$ amixer -Dhw:0 cget numid=5 numid=5,iface=CARD,name='HDMI/DP,pcm=0 Jack' ; type=BOOLEAN,access=r-------,values=1 : values=on [plb@ZOTAC ~]$ amixer -Dhw:0 cset numid=5 off amixer: Control hw:0 element write error: Operation not permitted [plb@ZOTAC ~]$ amixer -Dhw:0 cget numid=10 numid=10,iface=CARD,name='HDMI/DP,pcm=1 Jack' ; type=BOOLEAN,access=r-------,values=1 : values=on [plb@ZOTAC ~]$ amixer -Dhw:0 cget numid=15 numid=15,iface=CARD,name='HDMI/DP,pcm=2 Jack' ; type=BOOLEAN,access=r-------,values=1 : values=on The ELD controls do show a null set of values for device 1, maybe the jack value should be set in accordance with the ELD validity? Also I am wondering if the display number could be used for the PCM device number, or as a hint in the device description to help the user know which PCM device to use. Anyway thanks for this patchset, nicely done. > > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_lpe_audio.c | 14 ++++----- > include/drm/intel_lpe_audio.h | 6 ++-- > sound/x86/intel_hdmi_audio.c | 53 +++++++++++++++------------------- > sound/x86/intel_hdmi_audio.h | 3 +- > 4 files changed, 34 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c > index a593fdf73171..270aa3e3f0e2 100644 > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c > @@ -111,6 +111,7 @@ lpe_audio_platdev_create(struct drm_i915_private *dev_priv) > pinfo.size_data = sizeof(*pdata); > pinfo.dma_mask = DMA_BIT_MASK(32); > > + pdata->num_pipes = INTEL_INFO(dev_priv)->num_pipes; > spin_lock_init(&pdata->lpe_audio_slock); > > platdev = platform_device_register_full(&pinfo); > @@ -318,7 +319,7 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, > enum pipe pipe, enum port port, > const void *eld, int ls_clock, bool dp_output) > { > - unsigned long irq_flags; > + unsigned long irqflags; > struct intel_hdmi_lpe_audio_pdata *pdata; > struct intel_hdmi_lpe_audio_pipe_pdata *ppdata; > u32 audio_enable; > @@ -327,14 +328,12 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, > return; > > pdata = dev_get_platdata(&dev_priv->lpe_audio.platdev->dev); > - ppdata = &pdata->pipe; > + ppdata = &pdata->pipe[pipe]; > > - spin_lock_irqsave(&pdata->lpe_audio_slock, irq_flags); > + spin_lock_irqsave(&pdata->lpe_audio_slock, irqflags); > > audio_enable = I915_READ(VLV_AUD_PORT_EN_DBG(port)); > > - pdata->pipe_id = pipe; > - > if (eld != NULL) { > memcpy(ppdata->eld, eld, HDMI_MAX_ELD_BYTES); > ppdata->port = port; > @@ -356,8 +355,7 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, > } > > if (pdata->notify_audio_lpe) > - pdata->notify_audio_lpe(dev_priv->lpe_audio.platdev); > + pdata->notify_audio_lpe(dev_priv->lpe_audio.platdev, pipe); > > - spin_unlock_irqrestore(&pdata->lpe_audio_slock, > - irq_flags); > + spin_unlock_irqrestore(&pdata->lpe_audio_slock, irqflags); > } > diff --git a/include/drm/intel_lpe_audio.h b/include/drm/intel_lpe_audio.h > index 26e569ad8683..b391b2822140 100644 > --- a/include/drm/intel_lpe_audio.h > +++ b/include/drm/intel_lpe_audio.h > @@ -39,10 +39,10 @@ struct intel_hdmi_lpe_audio_pipe_pdata { > }; > > struct intel_hdmi_lpe_audio_pdata { > - struct intel_hdmi_lpe_audio_pipe_pdata pipe; > - int pipe_id; > + struct intel_hdmi_lpe_audio_pipe_pdata pipe[3]; > + int num_pipes; > > - void (*notify_audio_lpe)(struct platform_device *pdev); > + void (*notify_audio_lpe)(struct platform_device *pdev, int pipe); > spinlock_t lpe_audio_slock; > }; > > diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c > index 5e2149fe5218..e5863a6d3aa9 100644 > --- a/sound/x86/intel_hdmi_audio.c > +++ b/sound/x86/intel_hdmi_audio.c > @@ -195,12 +195,12 @@ 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->card_ctx->mmio_start + ctx->had_config_offset + reg); > + return ioread32(ctx->mmio_start + reg); > } > > static void had_write_register_raw(struct snd_intelhad *ctx, u32 reg, u32 val) > { > - iowrite32(val, ctx->card_ctx->mmio_start + ctx->had_config_offset + reg); > + iowrite32(val, ctx->mmio_start + reg); > } > > static void had_read_register(struct snd_intelhad *ctx, u32 reg, u32 *val) > @@ -1551,16 +1551,12 @@ static irqreturn_t display_pipe_interrupt_handler(int irq, void *dev_id) > /* > * monitor plug/unplug notification from i915; just kick off the work > */ > -static void notify_audio_lpe(struct platform_device *pdev) > +static void notify_audio_lpe(struct platform_device *pdev, int pipe) > { > struct snd_intelhad_card *card_ctx = platform_get_drvdata(pdev); > - int pipe; > - > - for_each_pipe(card_ctx, pipe) { > - struct snd_intelhad *ctx = &card_ctx->pcm_ctx[pipe]; > + struct snd_intelhad *ctx = &card_ctx->pcm_ctx[pipe]; > > - schedule_work(&ctx->hdmi_audio_wq); > - } > + schedule_work(&ctx->hdmi_audio_wq); > } > > /* the work to handle monitor hot plug/unplug */ > @@ -1569,7 +1565,7 @@ static void had_audio_wq(struct work_struct *work) > struct snd_intelhad *ctx = > container_of(work, struct snd_intelhad, hdmi_audio_wq); > struct intel_hdmi_lpe_audio_pdata *pdata = ctx->dev->platform_data; > - struct intel_hdmi_lpe_audio_pipe_pdata *ppdata = &pdata->pipe; > + struct intel_hdmi_lpe_audio_pipe_pdata *ppdata = &pdata->pipe[ctx->pipe]; > > pm_runtime_get_sync(ctx->dev); > mutex_lock(&ctx->mutex); > @@ -1582,22 +1578,6 @@ static void had_audio_wq(struct work_struct *work) > dev_dbg(ctx->dev, "%s: HAD_NOTIFY_ELD : port = %d, tmds = %d\n", > __func__, ppdata->port, ppdata->ls_clock); > > - switch (pdata->pipe_id) { > - case 0: > - ctx->had_config_offset = AUDIO_HDMI_CONFIG_A; > - break; > - case 1: > - ctx->had_config_offset = AUDIO_HDMI_CONFIG_B; > - break; > - case 2: > - ctx->had_config_offset = AUDIO_HDMI_CONFIG_C; > - break; > - default: > - dev_dbg(ctx->dev, "Invalid pipe %d\n", > - pdata->pipe_id); > - break; > - } > - > memcpy(ctx->eld, ppdata->eld, sizeof(ctx->eld)); > > ctx->dp_output = ppdata->dp_output; > @@ -1794,7 +1774,7 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) > > init_channel_allocations(); > > - card_ctx->num_pipes = 1; > + card_ctx->num_pipes = pdata->num_pipes; > > for_each_pipe(card_ctx, pipe) { > struct snd_intelhad *ctx = &card_ctx->pcm_ctx[pipe]; > @@ -1806,9 +1786,24 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) > > INIT_WORK(&ctx->hdmi_audio_wq, had_audio_wq); > > - ctx->had_config_offset = AUDIO_HDMI_CONFIG_A; > + switch (pipe) { > + case 0: > + ctx->mmio_start = > + card_ctx->mmio_start + AUDIO_HDMI_CONFIG_A; > + break; > + case 1: > + ctx->mmio_start = > + card_ctx->mmio_start + AUDIO_HDMI_CONFIG_B; > + break; > + case 2: > + ctx->mmio_start = > + card_ctx->mmio_start + AUDIO_HDMI_CONFIG_C; > + break; > + default: > + break; > + } > > - ret = snd_pcm_new(card, INTEL_HAD, PCM_INDEX, MAX_PB_STREAMS, > + ret = snd_pcm_new(card, INTEL_HAD, pipe, MAX_PB_STREAMS, > MAX_CAP_STREAMS, &pcm); > if (ret) > goto err; > diff --git a/sound/x86/intel_hdmi_audio.h b/sound/x86/intel_hdmi_audio.h > index a209096b03df..ab0de5d525f4 100644 > --- a/sound/x86/intel_hdmi_audio.h > +++ b/sound/x86/intel_hdmi_audio.h > @@ -32,7 +32,6 @@ > > #include "intel_hdmi_lpe_audio.h" > > -#define PCM_INDEX 0 > #define MAX_PB_STREAMS 1 > #define MAX_CAP_STREAMS 0 > #define BYTES_PER_WORD 0x4 > @@ -124,7 +123,7 @@ struct snd_intelhad { > unsigned int period_bytes; /* PCM period size in bytes */ > > /* internal stuff */ > - unsigned int had_config_offset; > + void __iomem *mmio_start; > union aud_cfg aud_config; /* AUD_CONFIG reg value cache */ > struct work_struct hdmi_audio_wq; > struct mutex mutex; /* for protecting chmap and eld */
On Wed, 26 Apr 2017 03:58:57 +0200, Pierre-Louis Bossart wrote: > > On 04/25/2017 03:27 PM, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Now that everything is in place let's register a PCM device for > > each pipe of the display engine. This will make it possible to > > actually output audio to multiple displays at the same time. And > > it avoids modesets on unrelated displays from clobbering up the > > ELD and whatnot for the display currently doing the playback. > > > > The alternative would be to have a PCM device per port, but per-pipe > > is easier since the hardware actually works that way. > Very nice. I just tested on a CHT Zotac box which has two connectors > (1 HDMI and 1 DP), and I get sound concurrently on both, with hdmi > being listed as device 2 and DP as device 0. > I thought there were hardware restrictions but you proved me wrong. Kudos. > > The only point that I find weird is that the jacks are reported as > 'on' on the 3 pipes, is there a way to tie them to an actual cable > being used? The pdata check was changed to check port=-1 as the monitor off in the patch 6. Maybe the initialization is missing? Takashi > > [plb@ZOTAC ~]$ amixer -Dhw:0 controls | grep Jack > numid=5,iface=CARD,name='HDMI/DP,pcm=0 Jack' > numid=10,iface=CARD,name='HDMI/DP,pcm=1 Jack' > numid=15,iface=CARD,name='HDMI/DP,pcm=2 Jack' > [plb@ZOTAC ~]$ amixer -Dhw:0 cget numid=5 > numid=5,iface=CARD,name='HDMI/DP,pcm=0 Jack' > ; type=BOOLEAN,access=r-------,values=1 > : values=on > [plb@ZOTAC ~]$ amixer -Dhw:0 cset numid=5 off > amixer: Control hw:0 element write error: Operation not permitted > > [plb@ZOTAC ~]$ amixer -Dhw:0 cget numid=10 > numid=10,iface=CARD,name='HDMI/DP,pcm=1 Jack' > ; type=BOOLEAN,access=r-------,values=1 > : values=on > [plb@ZOTAC ~]$ amixer -Dhw:0 cget numid=15 > numid=15,iface=CARD,name='HDMI/DP,pcm=2 Jack' > ; type=BOOLEAN,access=r-------,values=1 > : values=on > > The ELD controls do show a null set of values for device 1, maybe the > jack value should be set in accordance with the ELD validity? > Also I am wondering if the display number could be used for the PCM > device number, or as a hint in the device description to help the user > know which PCM device to use. > > Anyway thanks for this patchset, nicely done. > > > > > Cc: Takashi Iwai <tiwai@suse.de> > > Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_lpe_audio.c | 14 ++++----- > > include/drm/intel_lpe_audio.h | 6 ++-- > > sound/x86/intel_hdmi_audio.c | 53 +++++++++++++++------------------- > > sound/x86/intel_hdmi_audio.h | 3 +- > > 4 files changed, 34 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c > > index a593fdf73171..270aa3e3f0e2 100644 > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c > > @@ -111,6 +111,7 @@ lpe_audio_platdev_create(struct drm_i915_private *dev_priv) > > pinfo.size_data = sizeof(*pdata); > > pinfo.dma_mask = DMA_BIT_MASK(32); > > + pdata->num_pipes = INTEL_INFO(dev_priv)->num_pipes; > > spin_lock_init(&pdata->lpe_audio_slock); > > platdev = platform_device_register_full(&pinfo); > > @@ -318,7 +319,7 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, > > enum pipe pipe, enum port port, > > const void *eld, int ls_clock, bool dp_output) > > { > > - unsigned long irq_flags; > > + unsigned long irqflags; > > struct intel_hdmi_lpe_audio_pdata *pdata; > > struct intel_hdmi_lpe_audio_pipe_pdata *ppdata; > > u32 audio_enable; > > @@ -327,14 +328,12 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, > > return; > > pdata = dev_get_platdata(&dev_priv->lpe_audio.platdev->dev); > > - ppdata = &pdata->pipe; > > + ppdata = &pdata->pipe[pipe]; > > - spin_lock_irqsave(&pdata->lpe_audio_slock, irq_flags); > > + spin_lock_irqsave(&pdata->lpe_audio_slock, irqflags); > > audio_enable = I915_READ(VLV_AUD_PORT_EN_DBG(port)); > > - pdata->pipe_id = pipe; > > - > > if (eld != NULL) { > > memcpy(ppdata->eld, eld, HDMI_MAX_ELD_BYTES); > > ppdata->port = port; > > @@ -356,8 +355,7 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, > > } > > if (pdata->notify_audio_lpe) > > - pdata->notify_audio_lpe(dev_priv->lpe_audio.platdev); > > + pdata->notify_audio_lpe(dev_priv->lpe_audio.platdev, pipe); > > - spin_unlock_irqrestore(&pdata->lpe_audio_slock, > > - irq_flags); > > + spin_unlock_irqrestore(&pdata->lpe_audio_slock, irqflags); > > } > > diff --git a/include/drm/intel_lpe_audio.h b/include/drm/intel_lpe_audio.h > > index 26e569ad8683..b391b2822140 100644 > > --- a/include/drm/intel_lpe_audio.h > > +++ b/include/drm/intel_lpe_audio.h > > @@ -39,10 +39,10 @@ struct intel_hdmi_lpe_audio_pipe_pdata { > > }; > > struct intel_hdmi_lpe_audio_pdata { > > - struct intel_hdmi_lpe_audio_pipe_pdata pipe; > > - int pipe_id; > > + struct intel_hdmi_lpe_audio_pipe_pdata pipe[3]; > > + int num_pipes; > > - void (*notify_audio_lpe)(struct platform_device *pdev); > > + void (*notify_audio_lpe)(struct platform_device *pdev, int pipe); > > spinlock_t lpe_audio_slock; > > }; > > diff --git a/sound/x86/intel_hdmi_audio.c > > b/sound/x86/intel_hdmi_audio.c > > index 5e2149fe5218..e5863a6d3aa9 100644 > > --- a/sound/x86/intel_hdmi_audio.c > > +++ b/sound/x86/intel_hdmi_audio.c > > @@ -195,12 +195,12 @@ 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->card_ctx->mmio_start + ctx->had_config_offset + reg); > > + return ioread32(ctx->mmio_start + reg); > > } > > static void had_write_register_raw(struct snd_intelhad *ctx, u32 > > reg, u32 val) > > { > > - iowrite32(val, ctx->card_ctx->mmio_start + ctx->had_config_offset + reg); > > + iowrite32(val, ctx->mmio_start + reg); > > } > > static void had_read_register(struct snd_intelhad *ctx, u32 reg, > > u32 *val) > > @@ -1551,16 +1551,12 @@ static irqreturn_t display_pipe_interrupt_handler(int irq, void *dev_id) > > /* > > * monitor plug/unplug notification from i915; just kick off the work > > */ > > -static void notify_audio_lpe(struct platform_device *pdev) > > +static void notify_audio_lpe(struct platform_device *pdev, int pipe) > > { > > struct snd_intelhad_card *card_ctx = platform_get_drvdata(pdev); > > - int pipe; > > - > > - for_each_pipe(card_ctx, pipe) { > > - struct snd_intelhad *ctx = &card_ctx->pcm_ctx[pipe]; > > + struct snd_intelhad *ctx = &card_ctx->pcm_ctx[pipe]; > > - schedule_work(&ctx->hdmi_audio_wq); > > - } > > + schedule_work(&ctx->hdmi_audio_wq); > > } > > /* the work to handle monitor hot plug/unplug */ > > @@ -1569,7 +1565,7 @@ static void had_audio_wq(struct work_struct *work) > > struct snd_intelhad *ctx = > > container_of(work, struct snd_intelhad, hdmi_audio_wq); > > struct intel_hdmi_lpe_audio_pdata *pdata = ctx->dev->platform_data; > > - struct intel_hdmi_lpe_audio_pipe_pdata *ppdata = &pdata->pipe; > > + struct intel_hdmi_lpe_audio_pipe_pdata *ppdata = &pdata->pipe[ctx->pipe]; > > pm_runtime_get_sync(ctx->dev); > > mutex_lock(&ctx->mutex); > > @@ -1582,22 +1578,6 @@ static void had_audio_wq(struct work_struct *work) > > dev_dbg(ctx->dev, "%s: HAD_NOTIFY_ELD : port = %d, tmds = %d\n", > > __func__, ppdata->port, ppdata->ls_clock); > > - switch (pdata->pipe_id) { > > - case 0: > > - ctx->had_config_offset = AUDIO_HDMI_CONFIG_A; > > - break; > > - case 1: > > - ctx->had_config_offset = AUDIO_HDMI_CONFIG_B; > > - break; > > - case 2: > > - ctx->had_config_offset = AUDIO_HDMI_CONFIG_C; > > - break; > > - default: > > - dev_dbg(ctx->dev, "Invalid pipe %d\n", > > - pdata->pipe_id); > > - break; > > - } > > - > > memcpy(ctx->eld, ppdata->eld, sizeof(ctx->eld)); > > ctx->dp_output = ppdata->dp_output; > > @@ -1794,7 +1774,7 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) > > init_channel_allocations(); > > - card_ctx->num_pipes = 1; > > + card_ctx->num_pipes = pdata->num_pipes; > > for_each_pipe(card_ctx, pipe) { > > struct snd_intelhad *ctx = &card_ctx->pcm_ctx[pipe]; > > @@ -1806,9 +1786,24 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) > > INIT_WORK(&ctx->hdmi_audio_wq, had_audio_wq); > > - ctx->had_config_offset = AUDIO_HDMI_CONFIG_A; > > + switch (pipe) { > > + case 0: > > + ctx->mmio_start = > > + card_ctx->mmio_start + AUDIO_HDMI_CONFIG_A; > > + break; > > + case 1: > > + ctx->mmio_start = > > + card_ctx->mmio_start + AUDIO_HDMI_CONFIG_B; > > + break; > > + case 2: > > + ctx->mmio_start = > > + card_ctx->mmio_start + AUDIO_HDMI_CONFIG_C; > > + break; > > + default: > > + break; > > + } > > - ret = snd_pcm_new(card, INTEL_HAD, PCM_INDEX, > > MAX_PB_STREAMS, > > + ret = snd_pcm_new(card, INTEL_HAD, pipe, MAX_PB_STREAMS, > > MAX_CAP_STREAMS, &pcm); > > if (ret) > > goto err; > > diff --git a/sound/x86/intel_hdmi_audio.h b/sound/x86/intel_hdmi_audio.h > > index a209096b03df..ab0de5d525f4 100644 > > --- a/sound/x86/intel_hdmi_audio.h > > +++ b/sound/x86/intel_hdmi_audio.h > > @@ -32,7 +32,6 @@ > > #include "intel_hdmi_lpe_audio.h" > > -#define PCM_INDEX 0 > > #define MAX_PB_STREAMS 1 > > #define MAX_CAP_STREAMS 0 > > #define BYTES_PER_WORD 0x4 > > @@ -124,7 +123,7 @@ struct snd_intelhad { > > unsigned int period_bytes; /* PCM period size in bytes */ > > /* internal stuff */ > > - unsigned int had_config_offset; > > + void __iomem *mmio_start; > > union aud_cfg aud_config; /* AUD_CONFIG reg value cache */ > > struct work_struct hdmi_audio_wq; > > struct mutex mutex; /* for protecting chmap and eld */ >
On Wed, 26 Apr 2017 09:04:46 +0200, Takashi Iwai wrote: > > On Wed, 26 Apr 2017 03:58:57 +0200, > Pierre-Louis Bossart wrote: > > > > On 04/25/2017 03:27 PM, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Now that everything is in place let's register a PCM device for > > > each pipe of the display engine. This will make it possible to > > > actually output audio to multiple displays at the same time. And > > > it avoids modesets on unrelated displays from clobbering up the > > > ELD and whatnot for the display currently doing the playback. > > > > > > The alternative would be to have a PCM device per port, but per-pipe > > > is easier since the hardware actually works that way. > > Very nice. I just tested on a CHT Zotac box which has two connectors > > (1 HDMI and 1 DP), and I get sound concurrently on both, with hdmi > > being listed as device 2 and DP as device 0. > > I thought there were hardware restrictions but you proved me wrong. Kudos. > > > > The only point that I find weird is that the jacks are reported as > > 'on' on the 3 pipes, is there a way to tie them to an actual cable > > being used? > > The pdata check was changed to check port=-1 as the monitor off in the > patch 6. Maybe the initialization is missing? I guess the problem is that the hotplug wq is called at the initialization to retrieve the pdata for all pipes. It's called with uninitialized port=0, so all flags are on at init. And it implies the potential problem: the pdata contains the information only for a single pipe. Maybe it should keep the status/ELD for all three pipes. Takashi
On Wed, Apr 26, 2017 at 09:19:21AM +0200, Takashi Iwai wrote: > On Wed, 26 Apr 2017 09:04:46 +0200, > Takashi Iwai wrote: > > > > On Wed, 26 Apr 2017 03:58:57 +0200, > > Pierre-Louis Bossart wrote: > > > > > > On 04/25/2017 03:27 PM, ville.syrjala@linux.intel.com wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Now that everything is in place let's register a PCM device for > > > > each pipe of the display engine. This will make it possible to > > > > actually output audio to multiple displays at the same time. And > > > > it avoids modesets on unrelated displays from clobbering up the > > > > ELD and whatnot for the display currently doing the playback. > > > > > > > > The alternative would be to have a PCM device per port, but per-pipe > > > > is easier since the hardware actually works that way. > > > Very nice. I just tested on a CHT Zotac box which has two connectors > > > (1 HDMI and 1 DP), and I get sound concurrently on both, with hdmi > > > being listed as device 2 and DP as device 0. > > > I thought there were hardware restrictions but you proved me wrong. Kudos. > > > > > > The only point that I find weird is that the jacks are reported as > > > 'on' on the 3 pipes, is there a way to tie them to an actual cable > > > being used? > > > > The pdata check was changed to check port=-1 as the monitor off in the > > patch 6. Maybe the initialization is missing? > > I guess the problem is that the hotplug wq is called at the > initialization to retrieve the pdata for all pipes. It's called with > uninitialized port=0, so all flags are on at init. Indeed. That will need to be fixed. > > And it implies the potential problem: the pdata contains the > information only for a single pipe. Maybe it should keep the > status/ELD for all three pipes. I already did that. That being said, the entire notify vs. wq is pretty racy. So I was thinking that maybe we could just eliminate the wq and do whatever is needed directly from the notify hook. And then we could eliminate the (ab)use of pdata to transfer the ELD and whatnot between the drivers. I guess the main complication is the driver load case. I didn't really think that one through so far.
On Wed, 26 Apr 2017 15:49:06 +0200, Ville Syrjälä wrote: > > On Wed, Apr 26, 2017 at 09:19:21AM +0200, Takashi Iwai wrote: > > On Wed, 26 Apr 2017 09:04:46 +0200, > > Takashi Iwai wrote: > > > > > > On Wed, 26 Apr 2017 03:58:57 +0200, > > > Pierre-Louis Bossart wrote: > > > > > > > > On 04/25/2017 03:27 PM, ville.syrjala@linux.intel.com wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > Now that everything is in place let's register a PCM device for > > > > > each pipe of the display engine. This will make it possible to > > > > > actually output audio to multiple displays at the same time. And > > > > > it avoids modesets on unrelated displays from clobbering up the > > > > > ELD and whatnot for the display currently doing the playback. > > > > > > > > > > The alternative would be to have a PCM device per port, but per-pipe > > > > > is easier since the hardware actually works that way. > > > > Very nice. I just tested on a CHT Zotac box which has two connectors > > > > (1 HDMI and 1 DP), and I get sound concurrently on both, with hdmi > > > > being listed as device 2 and DP as device 0. > > > > I thought there were hardware restrictions but you proved me wrong. Kudos. > > > > > > > > The only point that I find weird is that the jacks are reported as > > > > 'on' on the 3 pipes, is there a way to tie them to an actual cable > > > > being used? > > > > > > The pdata check was changed to check port=-1 as the monitor off in the > > > patch 6. Maybe the initialization is missing? > > > > I guess the problem is that the hotplug wq is called at the > > initialization to retrieve the pdata for all pipes. It's called with > > uninitialized port=0, so all flags are on at init. > > Indeed. That will need to be fixed. > > > > > And it implies the potential problem: the pdata contains the > > information only for a single pipe. Maybe it should keep the > > status/ELD for all three pipes. > > I already did that. Oh then it' fine, I just didn't find it. > That being said, the entire notify vs. wq is pretty racy. So I was > thinking that maybe we could just eliminate the wq and do whatever is > needed directly from the notify hook. And then we could eliminate the > (ab)use of pdata to transfer the ELD and whatnot between the drivers. > I guess the main complication is the driver load case. I didn't > really think that one through so far. One way would be to implement the get_eld() to call at the probe time. HD-audio calls such one (provided via component) at probe and resume time to sync with the actual h/w state. For LPE audio, it's even easier, as we may call i915 exported function directly. thanks, Takashi
diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c index a593fdf73171..270aa3e3f0e2 100644 --- a/drivers/gpu/drm/i915/intel_lpe_audio.c +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c @@ -111,6 +111,7 @@ lpe_audio_platdev_create(struct drm_i915_private *dev_priv) pinfo.size_data = sizeof(*pdata); pinfo.dma_mask = DMA_BIT_MASK(32); + pdata->num_pipes = INTEL_INFO(dev_priv)->num_pipes; spin_lock_init(&pdata->lpe_audio_slock); platdev = platform_device_register_full(&pinfo); @@ -318,7 +319,7 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, enum pipe pipe, enum port port, const void *eld, int ls_clock, bool dp_output) { - unsigned long irq_flags; + unsigned long irqflags; struct intel_hdmi_lpe_audio_pdata *pdata; struct intel_hdmi_lpe_audio_pipe_pdata *ppdata; u32 audio_enable; @@ -327,14 +328,12 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, return; pdata = dev_get_platdata(&dev_priv->lpe_audio.platdev->dev); - ppdata = &pdata->pipe; + ppdata = &pdata->pipe[pipe]; - spin_lock_irqsave(&pdata->lpe_audio_slock, irq_flags); + spin_lock_irqsave(&pdata->lpe_audio_slock, irqflags); audio_enable = I915_READ(VLV_AUD_PORT_EN_DBG(port)); - pdata->pipe_id = pipe; - if (eld != NULL) { memcpy(ppdata->eld, eld, HDMI_MAX_ELD_BYTES); ppdata->port = port; @@ -356,8 +355,7 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, } if (pdata->notify_audio_lpe) - pdata->notify_audio_lpe(dev_priv->lpe_audio.platdev); + pdata->notify_audio_lpe(dev_priv->lpe_audio.platdev, pipe); - spin_unlock_irqrestore(&pdata->lpe_audio_slock, - irq_flags); + spin_unlock_irqrestore(&pdata->lpe_audio_slock, irqflags); } diff --git a/include/drm/intel_lpe_audio.h b/include/drm/intel_lpe_audio.h index 26e569ad8683..b391b2822140 100644 --- a/include/drm/intel_lpe_audio.h +++ b/include/drm/intel_lpe_audio.h @@ -39,10 +39,10 @@ struct intel_hdmi_lpe_audio_pipe_pdata { }; struct intel_hdmi_lpe_audio_pdata { - struct intel_hdmi_lpe_audio_pipe_pdata pipe; - int pipe_id; + struct intel_hdmi_lpe_audio_pipe_pdata pipe[3]; + int num_pipes; - void (*notify_audio_lpe)(struct platform_device *pdev); + void (*notify_audio_lpe)(struct platform_device *pdev, int pipe); spinlock_t lpe_audio_slock; }; diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 5e2149fe5218..e5863a6d3aa9 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -195,12 +195,12 @@ 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->card_ctx->mmio_start + ctx->had_config_offset + reg); + return ioread32(ctx->mmio_start + reg); } static void had_write_register_raw(struct snd_intelhad *ctx, u32 reg, u32 val) { - iowrite32(val, ctx->card_ctx->mmio_start + ctx->had_config_offset + reg); + iowrite32(val, ctx->mmio_start + reg); } static void had_read_register(struct snd_intelhad *ctx, u32 reg, u32 *val) @@ -1551,16 +1551,12 @@ static irqreturn_t display_pipe_interrupt_handler(int irq, void *dev_id) /* * monitor plug/unplug notification from i915; just kick off the work */ -static void notify_audio_lpe(struct platform_device *pdev) +static void notify_audio_lpe(struct platform_device *pdev, int pipe) { struct snd_intelhad_card *card_ctx = platform_get_drvdata(pdev); - int pipe; - - for_each_pipe(card_ctx, pipe) { - struct snd_intelhad *ctx = &card_ctx->pcm_ctx[pipe]; + struct snd_intelhad *ctx = &card_ctx->pcm_ctx[pipe]; - schedule_work(&ctx->hdmi_audio_wq); - } + schedule_work(&ctx->hdmi_audio_wq); } /* the work to handle monitor hot plug/unplug */ @@ -1569,7 +1565,7 @@ static void had_audio_wq(struct work_struct *work) struct snd_intelhad *ctx = container_of(work, struct snd_intelhad, hdmi_audio_wq); struct intel_hdmi_lpe_audio_pdata *pdata = ctx->dev->platform_data; - struct intel_hdmi_lpe_audio_pipe_pdata *ppdata = &pdata->pipe; + struct intel_hdmi_lpe_audio_pipe_pdata *ppdata = &pdata->pipe[ctx->pipe]; pm_runtime_get_sync(ctx->dev); mutex_lock(&ctx->mutex); @@ -1582,22 +1578,6 @@ static void had_audio_wq(struct work_struct *work) dev_dbg(ctx->dev, "%s: HAD_NOTIFY_ELD : port = %d, tmds = %d\n", __func__, ppdata->port, ppdata->ls_clock); - switch (pdata->pipe_id) { - case 0: - ctx->had_config_offset = AUDIO_HDMI_CONFIG_A; - break; - case 1: - ctx->had_config_offset = AUDIO_HDMI_CONFIG_B; - break; - case 2: - ctx->had_config_offset = AUDIO_HDMI_CONFIG_C; - break; - default: - dev_dbg(ctx->dev, "Invalid pipe %d\n", - pdata->pipe_id); - break; - } - memcpy(ctx->eld, ppdata->eld, sizeof(ctx->eld)); ctx->dp_output = ppdata->dp_output; @@ -1794,7 +1774,7 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) init_channel_allocations(); - card_ctx->num_pipes = 1; + card_ctx->num_pipes = pdata->num_pipes; for_each_pipe(card_ctx, pipe) { struct snd_intelhad *ctx = &card_ctx->pcm_ctx[pipe]; @@ -1806,9 +1786,24 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) INIT_WORK(&ctx->hdmi_audio_wq, had_audio_wq); - ctx->had_config_offset = AUDIO_HDMI_CONFIG_A; + switch (pipe) { + case 0: + ctx->mmio_start = + card_ctx->mmio_start + AUDIO_HDMI_CONFIG_A; + break; + case 1: + ctx->mmio_start = + card_ctx->mmio_start + AUDIO_HDMI_CONFIG_B; + break; + case 2: + ctx->mmio_start = + card_ctx->mmio_start + AUDIO_HDMI_CONFIG_C; + break; + default: + break; + } - ret = snd_pcm_new(card, INTEL_HAD, PCM_INDEX, MAX_PB_STREAMS, + ret = snd_pcm_new(card, INTEL_HAD, pipe, MAX_PB_STREAMS, MAX_CAP_STREAMS, &pcm); if (ret) goto err; diff --git a/sound/x86/intel_hdmi_audio.h b/sound/x86/intel_hdmi_audio.h index a209096b03df..ab0de5d525f4 100644 --- a/sound/x86/intel_hdmi_audio.h +++ b/sound/x86/intel_hdmi_audio.h @@ -32,7 +32,6 @@ #include "intel_hdmi_lpe_audio.h" -#define PCM_INDEX 0 #define MAX_PB_STREAMS 1 #define MAX_CAP_STREAMS 0 #define BYTES_PER_WORD 0x4 @@ -124,7 +123,7 @@ struct snd_intelhad { unsigned int period_bytes; /* PCM period size in bytes */ /* internal stuff */ - unsigned int had_config_offset; + void __iomem *mmio_start; union aud_cfg aud_config; /* AUD_CONFIG reg value cache */ struct work_struct hdmi_audio_wq; struct mutex mutex; /* for protecting chmap and eld */