Re: [PATCH 4/7] ALSA: x86: hdmi: Add audio support for BYT and CHT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@xxxxxxx]
> Sent: Wednesday, December 14, 2016 6:26 PM
> To: Anand, Jerome <jerome.anand@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx;
> ville.syrjala@xxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; pierre-
> louis.bossart@xxxxxxxxxxxxxxx; Ughreja, Rakesh A
> <rakesh.a.ughreja@xxxxxxxxx>
> Subject: Re: [PATCH 4/7] ALSA: x86: hdmi: Add audio support for BYT and CHT
> 
> On Mon, 12 Dec 2016 19:10:40 +0100,
> Jerome Anand wrote:
> >
> > Hdmi audio driver based on the child platform device created by gfx
> > driver is implemented.
> > This audio driver is derived from legacy intel hdmi audio driver.
> >
> > The interfaces for interaction between gfx and audio are updated and
> > the driver implementation updated to derive interrupts in its own
> > address space based on irq chip framework
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jerome Anand <jerome.anand@xxxxxxxxx>
> > ---
> >  sound/x86/Makefile               |    2 +
> >  sound/x86/intel_hdmi_audio.c     | 1907
> ++++++++++++++++++++++++++++++++++++++
> >  sound/x86/intel_hdmi_audio.h     |  201 ++++
> >  sound/x86/intel_hdmi_audio_if.c  |  551 +++++++++++
> >  sound/x86/intel_hdmi_lpe_audio.c |   16 +-
> >  5 files changed, 2671 insertions(+), 6 deletions(-)  create mode
> > 100644 sound/x86/intel_hdmi_audio.c  create mode 100644
> > sound/x86/intel_hdmi_audio.h  create mode 100644
> > sound/x86/intel_hdmi_audio_if.c
> >
> > diff --git a/sound/x86/Makefile b/sound/x86/Makefile index
> > 78b2ae1..bc074d0 100644
> > --- a/sound/x86/Makefile
> > +++ b/sound/x86/Makefile
> > @@ -3,6 +3,8 @@ DRIVER_NAME := hdmi_lpe_audio  ccflags-y +=
> > -Idrivers/gpu/drm/i915
> >
> >  $(DRIVER_NAME)-objs += \
> > +	intel_hdmi_audio.o \
> > +	intel_hdmi_audio_if.o \
> >  	intel_hdmi_lpe_audio.o
> >
> >  obj-$(CONFIG_HDMI_LPE_AUDIO) += $(DRIVER_NAME).o diff --git
> > a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c new
> file
> > mode 100644 index 0000000..461b7d7
> > --- /dev/null
> > +++ b/sound/x86/intel_hdmi_audio.c
> > @@ -0,0 +1,1907 @@
> > +/*
> > + *   intel_hdmi_audio.c - Intel HDMI audio driver
> > + *
> > + *  Copyright (C) 2016 Intel Corp
> > + *  Authors:	Sailaja Bandarupalli <sailaja.bandarupalli@xxxxxxxxx>
> > + *		Ramesh Babu K V	<ramesh.babu@xxxxxxxxx>
> > + *		Vaibhav Agarwal <vaibhav.agarwal@xxxxxxxxx>
> > + *		Jerome Anand <jerome.anand@xxxxxxxxx>
> > + *
> >
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~
> > +~~~~~
> > + *
> > + *  This program is free software; you can redistribute it and/or
> > +modify
> > + *  it under the terms of the GNU General Public License as published
> > +by
> > + *  the Free Software Foundation; version 2 of the License.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > +but
> > + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU
> > + *  General Public License for more details.
> > + *
> > + *
> >
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~
> > +~~~~~
> > + * ALSA driver for Intel HDMI audio
> > + */
> > +
> > +#define pr_fmt(fmt)	"had: " fmt
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +#include <linux/module.h>
> > +#include <linux/acpi.h>
> > +#include <asm/cacheflush.h>
> > +#include <sound/pcm.h>
> > +#include <sound/core.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/initval.h>
> > +#include <sound/control.h>
> > +#include <sound/initval.h>
> > +#include "intel_hdmi_audio.h"
> > +
> > +static DEFINE_MUTEX(had_mutex);
> > +
> > +/*standard module options for ALSA. This module supports only one
> > +card*/ static int hdmi_card_index = SNDRV_DEFAULT_IDX1; static char
> > +*hdmi_card_id = SNDRV_DEFAULT_STR1; static struct snd_intelhad
> > +*had_data;
> > +
> > +module_param(hdmi_card_index, int, 0444);
> 
> Use module_param_named(index, hdmi_card_index, int, 0444); Ditto for id.
> 

OK

> > +MODULE_PARM_DESC(hdmi_card_index,
> > +		"Index value for INTEL Intel HDMI Audio controller.");
> > +module_param(hdmi_card_id, charp, 0444);

OK

> > +MODULE_PARM_DESC(hdmi_card_id,
> > +		"ID string for INTEL Intel HDMI Audio controller.");
> > +
> > +/*
> > + * ELD SA bits in the CEA Speaker Allocation data block */ static int
> > +eld_speaker_allocation_bits[] = {
> > +	[0] = FL | FR,
> > +	[1] = LFE,
> > +	[2] = FC,
> > +	[3] = RL | RR,
> > +	[4] = RC,
> > +	[5] = FLC | FRC,
> > +	[6] = RLC | RRC,
> > +	/* the following are not defined in ELD yet */
> > +	[7] = 0,
> > +};
> > +
> > +/*
> > + * This is an ordered list!
> > + *
> > + * The preceding ones have better chances to be selected by
> > + * hdmi_channel_allocation().
> > + */
> > +static struct cea_channel_speaker_allocation channel_allocations[] = {
> > +/*                        channel:   7     6    5    4    3     2    1    0  */
> > +{ .ca_index = 0x00,  .speakers = {   0,    0,   0,   0,   0,    0,  FR,  FL } },
> > +				/* 2.1 */
> > +{ .ca_index = 0x01,  .speakers = {   0,    0,   0,   0,   0,  LFE,  FR,  FL } },
> > +				/* Dolby Surround */
> > +{ .ca_index = 0x02,  .speakers = {   0,    0,   0,   0,  FC,    0,  FR,  FL } },
> > +				/* surround40 */
> > +{ .ca_index = 0x08,  .speakers = {   0,    0,  RR,  RL,   0,    0,  FR,  FL } },
> > +				/* surround41 */
> > +{ .ca_index = 0x09,  .speakers = {   0,    0,  RR,  RL,   0,  LFE,  FR,  FL } },
> > +				/* surround50 */
> > +{ .ca_index = 0x0a,  .speakers = {   0,    0,  RR,  RL,  FC,    0,  FR,  FL } },
> > +				/* surround51 */
> > +{ .ca_index = 0x0b,  .speakers = {   0,    0,  RR,  RL,  FC,  LFE,  FR,  FL } },
> > +				/* 6.1 */
> > +{ .ca_index = 0x0f,  .speakers = {   0,   RC,  RR,  RL,  FC,  LFE,  FR,  FL } },
> > +				/* surround71 */
> > +{ .ca_index = 0x13,  .speakers = { RRC,  RLC,  RR,  RL,  FC,  LFE,
> > +FR,  FL } },
> > +
> > +{ .ca_index = 0x03,  .speakers = {   0,    0,   0,   0,  FC,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x04,  .speakers = {   0,    0,   0,  RC,   0,    0,  FR,  FL } },
> > +{ .ca_index = 0x05,  .speakers = {   0,    0,   0,  RC,   0,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x06,  .speakers = {   0,    0,   0,  RC,  FC,    0,  FR,  FL } },
> > +{ .ca_index = 0x07,  .speakers = {   0,    0,   0,  RC,  FC,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x0c,  .speakers = {   0,   RC,  RR,  RL,   0,    0,  FR,  FL } },
> > +{ .ca_index = 0x0d,  .speakers = {   0,   RC,  RR,  RL,   0,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x0e,  .speakers = {   0,   RC,  RR,  RL,  FC,    0,  FR,  FL } },
> > +{ .ca_index = 0x10,  .speakers = { RRC,  RLC,  RR,  RL,   0,    0,  FR,  FL } },
> > +{ .ca_index = 0x11,  .speakers = { RRC,  RLC,  RR,  RL,   0,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x12,  .speakers = { RRC,  RLC,  RR,  RL,  FC,    0,  FR,  FL } },
> > +{ .ca_index = 0x14,  .speakers = { FRC,  FLC,   0,   0,   0,    0,  FR,  FL } },
> > +{ .ca_index = 0x15,  .speakers = { FRC,  FLC,   0,   0,   0,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x16,  .speakers = { FRC,  FLC,   0,   0,  FC,    0,  FR,  FL } },
> > +{ .ca_index = 0x17,  .speakers = { FRC,  FLC,   0,   0,  FC,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x18,  .speakers = { FRC,  FLC,   0,  RC,   0,    0,  FR,  FL } },
> > +{ .ca_index = 0x19,  .speakers = { FRC,  FLC,   0,  RC,   0,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x1a,  .speakers = { FRC,  FLC,   0,  RC,  FC,    0,  FR,  FL } },
> > +{ .ca_index = 0x1b,  .speakers = { FRC,  FLC,   0,  RC,  FC,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x1c,  .speakers = { FRC,  FLC,  RR,  RL,   0,    0,  FR,  FL } },
> > +{ .ca_index = 0x1d,  .speakers = { FRC,  FLC,  RR,  RL,   0,  LFE,  FR,  FL } },
> > +{ .ca_index = 0x1e,  .speakers = { FRC,  FLC,  RR,  RL,  FC,    0,  FR,  FL } },
> > +{ .ca_index = 0x1f,  .speakers = { FRC,  FLC,  RR,  RL,  FC,  LFE,
> > +FR,  FL } }, };
> > +
> > +static struct channel_map_table map_tables[] = {
> > +	{ SNDRV_CHMAP_FL,       0x00,   FL },
> > +	{ SNDRV_CHMAP_FR,       0x01,   FR },
> > +	{ SNDRV_CHMAP_RL,       0x04,   RL },
> > +	{ SNDRV_CHMAP_RR,       0x05,   RR },
> > +	{ SNDRV_CHMAP_LFE,      0x02,   LFE },
> > +	{ SNDRV_CHMAP_FC,       0x03,   FC },
> > +	{ SNDRV_CHMAP_RLC,      0x06,   RLC },
> > +	{ SNDRV_CHMAP_RRC,      0x07,   RRC },
> > +	{} /* terminator */
> > +};
> 
> We really have the same stuff in multiple places...
> A serious cleanup will be needed later, but it's OK for now to make the code
> self-contained.
> 
> 

OK - I'll send a cleanup patch after these are accepted.

> > +/* hardware capability structure */
> > +static const struct snd_pcm_hardware snd_intel_hadstream = {
> > +	.info =	(SNDRV_PCM_INFO_INTERLEAVED |
> > +		SNDRV_PCM_INFO_DOUBLE |
> > +		SNDRV_PCM_INFO_MMAP|
> > +		SNDRV_PCM_INFO_MMAP_VALID |
> > +		SNDRV_PCM_INFO_BATCH),
> > +	.formats = (SNDRV_PCM_FMTBIT_S24 |
> > +		SNDRV_PCM_FMTBIT_U24),
> 
> Are only these formats available?  I see the code dealing with 16bit samples...

We support only 24 bit samples. Changes related to 16 can be removed.

> Also, does it support unsigned format?
> 

Yes its supported

> (snip)
> > +static int snd_intelhad_pcm_trigger(struct snd_pcm_substream
> *substream,
> > +					int cmd)
> > +{
> > +	int caps, retval = 0;
> > +	unsigned long flag_irq;
> > +	struct snd_intelhad *intelhaddata;
> > +	struct had_stream_pvt *stream;
> > +	struct had_pvt_data *had_stream;
> > +
> > +	pr_debug("snd_intelhad_pcm_trigger called\n");
> > +
> > +	intelhaddata = snd_pcm_substream_chip(substream);
> > +	stream = substream->runtime->private_data;
> > +	had_stream = intelhaddata->private_data;
> > +
> > +	switch (cmd) {
> > +	case SNDRV_PCM_TRIGGER_START:
> > +		pr_debug("Trigger Start\n");
> > +
> > +		/* Disable local INTRs till register prgmng is done */
> > +		if (had_get_hwstate(intelhaddata)) {
> > +			pr_err("_START: HDMI cable plugged-out\n");
> > +			retval = -ENODEV;
> > +			break;
> > +		}
> > +		stream->stream_status = STREAM_RUNNING;
> > +
> > +		spin_lock_irqsave(&intelhaddata->had_spinlock, flag_irq);
> 
> The trigger callback is always atomic and already spin-irqlocked.
> 

OK

> > +static int snd_intelhad_pcm_prepare(struct snd_pcm_substream
> > +*substream) {
> > +	int retval;
> > +	u32 disp_samp_freq, n_param;
> > +	struct snd_intelhad *intelhaddata;
> > +	struct snd_pcm_runtime *runtime;
> > +	struct had_pvt_data *had_stream;
> > +
> > +	pr_debug("snd_intelhad_pcm_prepare called\n");
> > +
> > +	intelhaddata = snd_pcm_substream_chip(substream);
> > +	runtime = substream->runtime;
> > +	had_stream = intelhaddata->private_data;
> > +
> > +	if (had_get_hwstate(intelhaddata)) {
> > +		pr_err("%s: HDMI cable plugged-out\n", __func__);
> > +		snd_pcm_stop(substream,
> SNDRV_PCM_STATE_DISCONNECTED);
> 
> I wonder whether this works well with this driver.
> SNDRV_PCM_STATE_DISCONNECTED is for the hot-unplug.  It's fine to use it,
> but then PulseAudio assumes that the whole card got unplugged, and
> removes the device entry, thus there is no way back even if you replug,
> unless you really reload the driver.
> 

OK - snd_pcm_stop() must not be done. Else, the device needs to be rebooted for PB

> 
> thanks,
> 
> Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux