Re: [PATCH 11/11] ALSA: x86: Register multiple PCM devices for the LPE audio card

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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 */

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel